feat(kernel): wire ABI-version check, mTLS, and CloudFetch toggle options#410
Draft
mani-mathur-arch wants to merge 31 commits into
Draft
feat(kernel): wire ABI-version check, mTLS, and CloudFetch toggle options#410mani-mathur-arch wants to merge 31 commits into
mani-mathur-arch wants to merge 31 commits into
Conversation
…initial namespace Close the four DAIS-scope rows the SEA-via-kernel backend previously rejected at connect time. All are Go-side only (no new kernel dependency): the OAuth setters are on merged kernel #162, metric-view is an existing session conf, and the namespace uses plain SQL. - Metric view: config.EffectiveSessionParams() derives the server conf (spark.sql.thriftserver.metadata.metricview.enabled) once, backend-neutrally, so both backends send the identical conf. The Thrift OpenSession special-case is removed (behaviour-preserving); the kernel forwards it via SessionConf. Reject dropped; reclassified forwarded. - Initial namespace: applied post-connect via USE CATALOG / USE SCHEMA (the OSS ODBC workaround) since the kernel C ABI has no catalog/schema setter. quoteIdent (untagged) backtick-quotes identifiers; a USE failure fails connect and closes the session. Reject dropped; reclassified forwarded. - OAuth M2M/U2M: the kernel drives its own OAuth flow from raw credentials (mirroring pyo3/napi and the Node/Python kernel bindings), read off cfg.Authenticator — the single source of truth (last-writer-wins, matching Thrift). The m2m/u2m authenticators expose auth.M2MCredentialsProvider / auth.U2MCredentialsProvider; resolveKernelAuth type-switches them and returns a *kernelAuth descriptor. KernelBackend.setAuth branches to set_auth_pat / set_auth_m2m / set_auth_u2m; U2M uses Go's cloud-inferred client id (kernel defaults for scopes/port). No new config fields. Verified: default CGO_ENABLED=0 suite + golangci-lint v2.12.2 clean; tagged databricks_kernel unit tests (auth-mode -> setter mapping, quoteIdent); live staging e2e for initial namespace (current_catalog/current_schema) and metric-view (session opens + queries; the conf is not SET-introspectable on either backend). M2M/U2M covered by unit tests (no staging service principal; U2M is interactive). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
… single-source auth, coverage + docs Remediation of the code-review pass on the DAIS gap-closure work. Verified against source; full default + tagged suites, golangci-lint v2.12.2, and live staging e2e all green. - Move the OAuth credential-provider interfaces (M2MCredentialsProvider / U2MCredentialsProvider) out of the public auth package into internal/backend/kernel, so the secret-reading capability is not part of the driver's public API. The unexported m2m/u2m authenticators satisfy them structurally. - Collapse the duplicate auth descriptor: validateKernelConfig/resolveKernelAuth now return kernel.Auth directly (its type is in an untagged file, so the default build builds it cgo-free); dropped dbsql.kernelAuth, kernelAuthMode, and toKernelAuth (which also removed a stale build-tag comment). - Route the initial-namespace failure-path session close through call() so a failed close is logged (via lastError's Warn), mirroring CloseSession. - Add an env-guarded live M2M e2e (TestKernelE2EM2M, skips without DATABRICKS_CLIENT_ID/_SECRET) and a last-writer-wins auth regression test; the resolveKernelAuth -> kernel.Auth path is table-tested for M2M/U2M. - Document: U2M is interactive (browser on cache-miss, blocks up to the kernel's ~120s callback timeout, connect ctx deadline not honored during that window, no C-ABI override — use PAT/M2M for headless); the U2M Scopes/RedirectPort fields are dormant-but-wired (no Go option feeds them yet); the metric-view e2e is a deliberate connect-smoke (routing asserted in TestEffectiveSessionParams). Custom M2M scopes remain unforwardable over the C ABI (no scopes arg on set_auth_m2m) — a kernel gap shared with ODBC, no authz impact (all-apis always requested); tracked in the kernel-gaps notes rather than worked around. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The kernel backend returns INTERVAL columns as native arrow duration (day-time) and month-interval (year-month) values, whereas the Thrift path receives them pre-formatted from the server (its native-interval config is off in prod, so it never scans a duration/month-interval array). Format them Go-side in the shared untagged arrowscan package to the same strings the Thrift path returns — "D HH:MM:SS.nnnnnnnnn" and "years-months", negatives signed — so a query's result is identical across backends. Replaces the fail-loud "intervals are not yet handled" default arm with the two type arms; golden-string unit tests (day/day-to-sec/seconds-unit/negative, year/year-month/months/negative) run in the default CGO_ENABLED=0 build. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Over Arrow the kernel delivers TIMESTAMP with a tz ("UTC") and TIMESTAMP_NTZ
with an empty tz, but — like the Thrift path — the driver ignores that field
and renders both via ToTime + .In(loc). The LTZ-vs-NTZ difference is carried
entirely by the instant the server sends, not by the client inspecting the
tz, so no arrowscan change is needed: the existing code already matches Thrift.
Verified live on both backends (America/New_York + Asia/Kolkata, including a
DST spring-forward literal, and nested/null shapes): kernel == Thrift
byte-for-byte for both types. Add an untagged parity case (TimeZone "UTC" vs
"") so a future "don't shift NTZ" change — which looks correct in isolation
but would diverge from Thrift, which shifts NTZ too — fails default CI, plus a
live e2e pinning the round-trip.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
VARIANT and GEOMETRY need no special rendering on the kernel path: verified
live on both backends, both arrive over Arrow as plain STRING columns — a
top-level VARIANT is its JSON text ({"a":1,"b":[2,3]}), a scalar VARIANT is
"42", and GEOMETRY is its WKT "POINT(1 2)". Nested inside a container the
variant/geometry element is a string leaf, rendered as a quoted, JSON-escaped
string (the variant's own JSON is escaped as text, NOT re-parsed) — identical
on both backends.
Add untagged parity cases: a top-level string equivalence (variant object /
scalar / geometry WKT) and nested string-leaf cases in an array, so the string
arm's handling of these types can't silently drift between backends. GEOGRAPHY
is intentionally excluded — not enabled on the benchmark warehouse and no
consumer has asked for it.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
A failed kernel query returned the raw *KernelError, so consumers doing errors.As(err, &DBExecutionError) — the way they inspect Thrift failures — didn't get SqlState()/QueryId()/IsRetryable() through the standard interface. kernelOp.ExecutionError now digs the sqlstate out of the underlying *KernelError and wraps the cause via NewExecutionErrorWithState, so kernel query failures surface with the same DBExecutionError shape as Thrift. Adds the neutral NewExecutionErrorWithState to the untagged internal/errors package (Thrift's NewExecutionError needs a TGetOperationStatusResp the kernel backend can't produce), unit-tested in the default CGO_ENABLED=0 build. Parity is type + SQLSTATE, not byte-identical text — kernel messages are richer (they carry the SQL error class + suggestions). Verified live: unknown table → 42P01, unknown column → 42703, byte-identical sqlstate to Thrift. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…al caveat Record what the kernel backend inherits unchanged above the backend seam — the database/sql connection pool (each conn wraps one kernel session), per-connection CREATE_SESSION / DELETE_SESSION telemetry (recorded unconditionally in connector.go, backend-agnostic), and the telemetry exporter's circuit breaker — and that result types render byte-for-byte with Thrift (scalars, exact DECIMAL, TIMESTAMP / TIMESTAMP_NTZ, INTERVAL, nested + VARIANT as JSON, GEOMETRY as WKT). Remove the now-stale "INTERVAL types are not yet handled by the kernel scanner" caveat (intervals render now), and narrow the telemetry caveat to what is actually missing: only EXECUTE_STATEMENT telemetry (gated on a per-statement query id the kernel C ABI doesn't yet surface) — CREATE_SESSION / DELETE_SESSION are unaffected. Add a live-verified connection-pool e2e (40 concurrent queries over pool cap 8) backing the inherited-pool claim. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The kernel backend rejected bound parameters at execute time. Now it binds them:
the driver's backend.Param{Name, Type, Value} maps 1:1 onto the kernel's
kernel_statement_bind_parameter (K1) — value already stringified, Type the
Databricks SQL type name, empty Name → positional, nil Value → SQL NULL ("VOID").
bindParams runs after set_sql (which clears any prior binds), using the existing
newCStr/newCStrOrNull helpers and the call() FFI-safety wrapper; a bind failure
closes the statement and surfaces via toStatementError.
Removes the fail-loud reject in Execute (and its now-unused errors import). The
old TestExecuteRejectsParams is repurposed as TestExecuteHandleLessOpContract
(the non-nil handle-less Operation contract, now driven by a nil-session failure
since params no longer reject). Live parity: 10 cases (positional/named, each
scalar type, NULL, multi-param, predicate) produce byte-identical output on the
kernel and Thrift backends.
Requires a kernel build carrying kernel_statement_bind_parameter; the KERNEL_REV
pin is bumped to the K1 merge SHA when it lands.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
kernelOp.StatementID() returned "", so the kernel backend emitted no EXECUTE_STATEMENT telemetry and QueryIdCallback fired with an empty id (connection.go gates both on a non-empty statement id). Wire StatementID() to the server query id via kernel_executed_statement_query_id (K1), captured at execute time into a cached field — the same lifetime discipline as affectedRows, since the C accessor returns a pointer borrowed from the exec handle and the op is closed (nulling exec) before StatementID() is read on some paths. C.GoString deep-copies out of the borrowed string. Live e2e: a registered QueryIdCallback fires with a non-empty server id after a kernel query. Updates doc.go — bound parameters (c6) and EXECUTE_STATEMENT telemetry are now supported; the remaining kernel-backend limitation is batch-boundary (not mid-fetch) read cancellation. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Bumps the kernel pin from the #163 canceller rev to the tip of the PuPr statement-surface branch (databricks-sql-kernel#165), which adds kernel_statement_bind_parameter and kernel_executed_statement_query_id — the two C-ABI symbols the bound-parameter (c6) and EXECUTE_STATEMENT-telemetry (c7) commits link against. Re-pin to the squash-merge SHA once #165 lands. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…ction, bind-mapping test Addresses the review pass on this PR (comments left on #399). Fixes the actionable set; a follow-up Isaac re-review then flagged that one of the requested changes (surfacing the kernel Retryable flag) was itself unsafe, so that one is intentionally NOT made — see below. - Interval negation overflow (High): formatDayTimeInterval / formatYearMonthInterval negated the full magnitude up front (v = -v). At math.MinInt64 (day-time µs) / math.MinInt32 (year-month) that wraps back negative, so every component came out negative AND a '-' was prepended — doubly-negated garbage — and both are representable Spark interval bounds. Now derive each component from the signed value and take its magnitude when formatting (abs64); widen year-month to int64 before negating. Adds MinInt64 (µs + ns) and MinInt32 regression cases. - Cancelled-execute skipped session-fatal eviction (Med): when execErr raced a ctx cancel, execute returned ctx.Err() without calling evictIfSessionFatal, leaving a dead conn marked valid in the pool. Hoist the evict above the ctx-cancelled branch so it fires on both paths (drained watcher first, so no race). Also wrap BOTH the ctx error and the kernel error with two %w verbs so errors.Is(context.DeadlineExceeded) still matches AND the *KernelError (sqlstate/queryId) stays reachable via errors.As instead of being dropped. - Bind mapping had no executing coverage (Med): the live Param-binding proof (TestKernelParamsVsThrift) needs a warehouse and only runs in the credentialed nightly job, so the positional/named + SQL-NULL/empty-string decision shipped untested at PR time. Extract that pure decision into an untagged paramBindArg (bindparams.go), consumed by the cgo bindParams, and unit-test it under CGO_ENABLED=0 (TestParamBindArg). Rename a comment's dead TestKernelE2EParams reference to the real tests. - Stale StatementID() comments (Low): both said StatementID() is "" on this backend, but this PR made it return the real server id on the success path. Scope the empty-id claim to the execute-error path. NOT changed (Isaac re-review, MAJOR): surfacing the kernel's Retryable flag on kernelOp.ExecutionError. That is exclusively the post-submission path, where a network/unavailable failure may have already committed a non-idempotent INSERT/UPDATE/MERGE — reporting IsRetryable()==true would invite an app to double-write, and it diverges from the Thrift path (always non-retryable here). This mirrors toStatementError refusing driver.ErrBadConn for the same reason. sqlState/queryId extraction is unchanged; TestExecutionErrorNeverRetryable pins that a Retryable KernelError still reports IsRetryable()==false. The connect-phase path (toConnError), where the retryable signal IS safe, is unaffected. Verified: default (CGO_ENABLED=0) + kernel-tagged suites pass, gofmt clean, Isaac review clean (0 final comments). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…rnel Follow-up to the review round: the unsupported-authenticator default case in resolveKernelAuth still returned a plain errors.New, while every other unsupported kernel option wraps ErrNotSupportedByKernel and doc.go advertises that errors.Is(err, ErrNotSupportedByKernel) detects any unsupported kernel feature. So this PR shipped a documented contract its own code broke for token-provider / external / federated auth. Wrap it with %w to honor the contract (same fix #403 makes one commit up the stack — matching its wording so the two converge cleanly on rebase). The empty-PAT case stays unwrapped: a missing token is misconfiguration to fix, not a feature the kernel can't honor. Tighten the "non-PAT/non-OAuth authenticator rejected" test to assert errors.Is(err, ErrNotSupportedByKernel) instead of only err != nil, so the contract is pinned rather than documented as an exception. Verified: default-build suite passes, go vet + gofmt clean. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…e skip
Expose the two kernel-only TLS knobs whose C-ABI setters already exist on
kernel main (no kernel change needed), via an experimental-option idiom:
- WithKernelTrustedCerts(pem) -> kernel_session_config_set_tls_trusted_certs,
adding a PEM CA bundle on top of the system roots. Required because the
kernel's rustls stack ignores SSL_CERT_FILE, so a custom CA (corporate
re-signing proxy / on-prem CA) must be handed over explicitly.
- WithKernelSkipHostnameVerify() -> set_tls_skip_hostname_verification,
skipping only the hostname check while keeping chain validation
(finer-grained than the blanket WithSkipTLSHostVerify).
The knobs live on a non-exported config.KernelExperimentalConfig off
config.Config (not UserConfig), so they stay off the stable DSN surface
(mirroring Node's InternalConnectionOptions / Python's underscore kwargs).
The Thrift path rejects a non-nil block loudly at connect rather than
silently ignoring it, so a caller who forgets WithUseKernel learns the
option had no effect. OpenSession forwards each to the kernel C ABI via a
byte-buffer helper (cBytes); a reflective guard
(TestKernelExperimentalFieldsClassified) keeps a new field from slipping
either path unclassified.
mTLS client cert/key (needs an absent kernel C-ABI setter, K5) and the
CloudFetch on/off toggle (K3) are deliberately out of scope here — they are
tracked separately (PECOBLR-3652 / PECOBLR-3653).
Closes PECOBLR-3651.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…ift-reject test Follow-up on the four review findings for the richer-TLS kernel options: - Add ErrRequiresKernelBackend sentinel (mirror of ErrNotSupportedByKernel) and wrap the Thrift-path rejection with %w, so callers detect it via errors.Is instead of message text. Documented in doc.go. - WithKernelTrustedCerts copies the PEM defensively (matching DeepCopy) so a caller mutating the slice between NewConnector and Connect can't change the trust store. - Add a MITM WARNING to WithKernelSkipHostnameVerify, consistent with WithSkipTLSHostVerify. - Add TestWithKernelOptionsRejectedOnThriftPath: end-to-end Connect() reject on the Thrift path, asserting on the sentinel. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…erage Non-blocking follow-ups: - doc.go: mention ErrRequiresKernelBackend in the main Errors section, symmetric with ErrNotSupportedByKernel. - Add TestWithKernelTrustedCertsCopiesPEM: mutating the caller's slice after the option must not change stored config (option-set counterpart to the DeepCopy aliasing test). - TestConfig_DeepCopy: set KernelExperimental in the all-values case so Config.DeepCopy's wiring of that field is exercised, and assert it's copied not aliased. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…relate Unify kernel-backend logging onto the driver's shared logger so the one knob that controls the rest of the driver — DATABRICKS_LOG_LEVEL / dbsql.SetLogLevel — governs it, and each binding line carries the structured connId/corrId/queryId fields that let it be correlated in a multi-connection process. Addresses vikrantpuppala's review on #393 (klog + the kernel Rust subscriber emitted unstructured lines straight to os.Stderr, uncorrelatable and gated on a separate DBSQL_KERNEL_DEBUG rather than the driver log level). Three parts: - Level: klog now emits through logger.Logger.Debug() instead of raw fmt.Fprintf(os.Stderr, ...). A cheap kernelDebugOff() front gate (GetLevel() > Debug) short-circuits before any formatting/allocation, so it stays a true no-op at the default Warn level — including during benchmarks. Replaces the old DBSQL_KERNEL_DEBUG bool + the caller-less KernelDebugEnabled. - Correlation: new klogCtx(ctx, ...) pulls connId/corrId/queryId off ctx via logger.WithContext (the exact idiom the Thrift/conn path uses); the conn layer already stuffs those IDs into ctx before calling the backend. Every hot-path site (execute, nextBatch, newKernelRows, Open/CloseSession) uses it; ctx-less sites degrade to klog. The WithContext allocation is behind the same up-front level gate, so it never runs below Debug. - Kernel Rust logs: initKernelLogging maps the driver level -> the kernel_init_logging level string (kernelLogLevel), so DATABRICKS_LOG_LEVEL drives the kernel's Rust logs too. DBSQL_KERNEL_DEBUG is kept as an advanced override (forces the subscriber on, defers to RUST_LOG for kernel verbosity). Known limitation (documented): the kernel's Rust lines are still plain text and do not yet carry the structured fields — that needs the kernel log-callback ABI (PECOBLR-3654 / K4). Only the Go binding lines are structured today. Tests: TestKernelLogLevel pins the level mapping; TestKernelLogNoAllocWhenOff uses testing.AllocsPerRun to prove klog/klogCtx allocate 0 times at Warn level (guards the hot-path zero-cost guarantee). Closes PECOBLR-3650. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Address the review comments on #401 — the Go binding logs now follow the live driver logger, but the Rust subscriber is process-wide/first-call-wins and stderr-only, so several "one unified knob" claims were stronger than the implementation. Fixes, all in the kernel backend (databricks_kernel tag; the default pure-Go build is unaffected): - doc.go: Rust logs are stderr-only and NOT routed through logger.SetLogOutput; Rust verbosity is fixed at the first kernel session (set the level before the first connection); "each line carries connId/corrId/queryId" softened to request-scoped lines where a ctx is in scope (bindParams / operation teardown stay uncorrelated). - cgo.go: kernelLogLevel maps FatalLevel/PanicLevel -> OFF (not ERROR), so the Rust subscriber is never louder than the driver the user configured — at driver=fatal the Go side suppresses even Error() lines. Tightened the initKernelLogging comment to state the first-session level sampling and the stderr-only sink, and dropped the inaccurate "only installed when the level is at or below the threshold" sentence. - kernel_test.go: TestKernelLogLevel updated for fatal/panic->OFF; new TestKernelLogCtxEmitsCorrelation captures logger output at debug level and asserts klogCtx emits the message + connId/corrId/queryId, then asserts silence at warn — the positive-behavior half the alloc test can't see. Verified: CGO_ENABLED=0 build+vet+test green; CGO_ENABLED=1 -tags databricks_kernel build+vet green, kernel package tests pass (incl. all three log tests). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Follow-up to the #401 review (finding 5, thread on kernel_test.go): the level- resolution logic initKernelLogging feeds to kernel_init_logging was documented but not tested. Extract the pure decision so it can be pinned by CI without cgo. - New untagged logging_level.go holds kernelLogLevel (moved out of cgo.go) and a new resolveKernelLogArg() (level string, useNULL bool): the NULL-on- DBSQL_KERNEL_DEBUG override vs the mapped driver level. Mirrors the existing errors_classify.go pattern (pure logic, no build tag) so its tests run under CGO_ENABLED=0. cgo.go's initKernelLogging is now a thin caller of it. - New untagged logging_level_test.go: TestKernelLogLevel (moved) + new TestResolveKernelLogArg, which pins the two invariants the PR justifies in prose — DBSQL_KERNEL_DEBUG (non-empty) yields useNULL=true so the kernel honors RUST_LOG, and otherwise the driver level is mapped in (incl. fatal→OFF). Empty value is treated as unset. - Drops the now-unused os import from cgo.go and zerolog from kernel_test.go. Not covered (documented, cost/value flips): that the string is actually handed to C.kernel_init_logging and the sync.Once first-call-wins — asserting those needs a function-pointer seam over the cgo call plus resetting a package global, to test one assignment + stdlib sync.Once. Verified: CGO_ENABLED=0 build+vet+test green (both new tests run here); CGO_ENABLED=1 -tags databricks_kernel build+vet green, all four log tests pass. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Add a scheduled (07:00 UTC) + manually dispatchable workflow that runs the
credential-gated end-to-end suites against a real test SQL warehouse. The
ordinary "Go" workflow injects no warehouse secrets, so every E2E test t.Skip()s
there and real-warehouse behaviour — large multi-page CloudFetch, S3 downloads,
drain-past-deadline, real auth — is otherwise not exercised in CI.
Two jobs against one test warehouse:
- thrift-e2e: pure-Go (CGO_ENABLED=0), the high-value CloudFetch drain +
exact-row-count scenarios.
- kernel-e2e: reuses the go.yml kernel-build machinery (JFrog/cargo proxy,
pinned Rust, kernel App-token clone, kernel-lib/cargo caches), then runs the
databricks_kernel-tagged E2E funcs plus the Thrift-vs-kernel parity funcs
(which need both the kernel lib and live credentials).
Both suites now read the same DATABRICKS_PECOTESTING_* credentials, so one secret
set drives the whole workflow — the kernel E2E / parity test helpers are updated
from the ad-hoc DATABRICKS_HOST/_HTTP_PATH/_TOKEN vars to the PECOTESTING set
(with the _TOKEN_PERSONAL fallback the Thrift suite already uses). Each job has a
fail-loud credential guard so a missing secret errors instead of silently
skipping into a misleading green. TestKernelE2EM2M is excluded (-skip) since it
needs a service-principal client id/secret the warehouse secret set doesn't
carry; the job comment documents how to enable it.
Alerting is GitHub's built-in failed-scheduled-run notification. Provisioning the
DATABRICKS_PECOTESTING_* secrets in the repository is the remaining step to make
the nightly run green.
Closes PECOBLR-3308.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…fires first The kernel E2E job had a 40-min job cap while `go test` alone was given -timeout 30m, leaving only ~10 min for checkout, toolchains, caches, and a potentially cold ~200-crate `make kernel-lib` Rust build. On a cache miss the build can exceed that, so GitHub SIGKILLs the whole job at 40 min before Go's timeout can emit its which-test-hung goroutine dump — losing the diagnostics. Split the budget explicitly: raise the job cap to 65 min and add a 25-min per-step timeout on `make kernel-lib`. Worst case is 25 (build) + 30 (go test) + setup < 65, so a wedged build is killed distinctly as a build failure and a hung test still hits Go's own -timeout first. Also moved the E2E step's descriptive comment down onto the Run step it documents. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Extend the driver-test dispatch so the Go integration suite can run against the SEA-via-kernel backend, selected by label — the analogue of the existing Thrift labels, not a copy. Adds one kernel-namespaced label: - integration-test-kernel → sea backend, passthrough (real warehouse) The label resolves both proxy_mode and a new go_mode (thrift vs sea), and go_mode rides in the repository_dispatch client_payload. databricks-driver-test reads it to decide whether to build the kernel static lib and run the tagged (databricks_kernel) leg; the Thrift labels send go_mode=thrift and are unchanged. The new label is added to the on-new-commit label-drop list and the skip-stub guidance. The kernel label is passthrough (not replay): the sea leg has no committed recordings to replay yet, so there is no sea replay label until those are captured. The required merge-queue gate stays Thrift-only (go_mode pinned to thrift): the kernel leg is previewable on demand but not a required gate until the SEA backend ships in a released driver. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…h rejection Two review-driven fixes on the kernel backend, both Go-only. CLOSE_STATEMENT telemetry: kernelRows.Close() now fires the OnClose callback, so kernel queries record close latency / statement success-or-failure the same way the Thrift path does (conn gates that recording on OnClose firing). Previously it never fired, so kernel traffic emitted no close telemetry — including failures — a production observability blind spot. Next() is split into a thin wrapper that records the first non-EOF error as iterationErr (io.EOF is normal drain), which Close() passes to the callback; closeErr is nil since the kernel teardown has no fallible close RPC. The callback is armed only after newKernelRows finishes constructing (mirroring the Thrift NewRows), so a schema-fetch/import failure's cleanup Close() does not record a falsely-successful CLOSE_STATEMENT. Auth rejection sentinel: the unsupported-authenticator branch of resolveKernelAuth wrapped a bare errors.New, so fallback logic could only substring-match. It now wraps ErrNotSupportedByKernel via %w so callers can errors.Is it, matching the other kernel config rejections. The missing-personal-access-token error stays a plain error (missing-required-config, not an unsupported feature — it must not signal "fall back to Thrift"). Tests: TestKernelRowsCloseFiresOnClose (success, iterationErr propagation, idempotent double-close, nil-callbacks, construction-failure-no-success-close); strengthened the non-PAT authenticator rejection to assert errors.Is(ErrNotSupportedByKernel). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…x Thrift comment Addresses two review comments on the CLOSE_STATEMENT telemetry change: - The "construction-failure Close must not fire a success OnClose" subtest was a no-op: `fired` was never assignable to true, so it duplicated the nil-callbacks case and asserted nothing. Replaced with a test that drives the real newKernelRows cleanup path — a nil result stream makes kernel_result_stream_get_schema return a defined InvalidArgument error (the kernel null-checks the handle, never UB), so newKernelRows takes its r.Close() cleanup branch and returns an error, and the supplied OnClose must not fire. Verified by mutation: arming the callback before the schema import makes the test fail as intended. - Reworded the newKernelRows comment that claimed the deferred callback assignment matches Thrift's NewRows "just before returning" — Thrift actually assigns closeCallback during construction. Dropped the inaccurate comparison and state the invariant directly. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Lint fix (the failing CI check): TestResolveKernelLogArg used unchecked os.Setenv/os.Unsetenv (5 errcheck violations). Switch to t.Setenv, which auto-restores and returns nothing; the "unset" cases become t.Setenv(key, "") since resolveKernelLogArg gates on os.Getenv != "" (empty == unset). Drops the now-unused os import and the manual save/restore. Comment/doc cleanup (no behavior change): - Trim doc.go's kernel section (~40% shorter): fold the repetitive logging and cancellation prose, drop redundant parentheticals; every user-actionable fact (supported options, U2M-blocks-on-browser, Rust-log caveats, TLS knobs) kept. - Remove references to other PRs, other language bindings, and internal review/milestone artifacts from code comments, keeping the technical rationale. Verified: default-build lint 0 issues, gofmt clean, default + kernel-tagged suites pass. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…2M scopes, INTERVAL test Five fixes from the /full-review pass, verified against source (and the kernel C header at KERNEL_REV) before acting: - Bound STRING params with an interior NUL are now rejected. The kernel bind ABI takes value as a NUL-terminated const char* with no length, so a NUL would silently truncate (Thrift sends it length-prefixed) — a parity-breaking silent divergence. Guard lives in the untagged bindparams.go (checkParamValue), so it is unit-tested under CGO_ENABLED=0. - Failed kernel queries now emit EXECUTE_STATEMENT telemetry. On the execute-error path there is no exec handle, so op.statementID was left "" and conn's StatementID()-gated telemetry never fired — error-rate dashboards undercounted kernel failures while the same Thrift failures were visible. The query id rides on the KernelError; statementIDFromError (untagged, unit-tested) pulls it out. - Custom M2M OAuth scopes are now rejected at connect instead of silently dropped. The kernel's set_auth_m2m carries no scopes and applies "all-apis" itself, so a custom set would silently downgrade (a least-privilege caller getting broader access). Widened M2MCredentialsProvider with M2MScopes(); resolveKernelAuth rejects a non-default set with ErrNotSupportedByKernel like every other unsupported option. - Added a live INTERVAL parity test (TestKernelE2EInterval): the kernel formats the native arrow duration/month-interval Go-side, Thrift receives the server's pre-formatted string, and the two scanned strings must match — giving doc.go's "byte-for-byte identical" claim a real ground-truth check on the sign/format edges the arrow-level parity suite structurally can't reach. - Reworded the nightly-e2e job-gate comment: the skip mechanism is the NIGHTLY_E2E_ENABLED variable, and the credential step hard-fails by design (it is not a skip-on-missing-secret gate). - Deduplicated the DATABRICKS_PECOTESTING_* credential read+skip block (4 sites) into a shared pecoTestingCreds(t) helper. Verified: default (CGO_ENABLED=0) + kernel-tagged suites both 24 pkg ok; gofmt clean; Isaac Review clean (0 crit/major/minor). Live against a real warehouse: INTERVAL + scalar/nested parity, params-vs-Thrift 10/10, full kernel e2e, and M2M all pass. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The execute() header still read "Executes SQL text only; bound parameters are rejected up front by Execute" — stale since params became supported (bindParams is called in the body). Reworded to state that params are bound and staging is the statement rejected up front. Comment-only. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Wire the kernel backend's two blocking cgo calls to the new cancellable C-ABI entry points via a ctxWatcher that bridges a Go context onto a kernel cancel token: - OpenSession -> kernel_session_open_cancellable - rows.go nextBatch -> kernel_result_stream_next_batch_cancellable Previously each blocked in an uninterruptible cgo call: a slow warehouse cold-start or a hung CloudFetch chunk ignored the caller's ctx deadline. The watcher fires the token on ctx.Done(), which drops the in-flight kernel request future (a real abort), and the call sites prefer the ctx error on cancellation while preserving the kernel error as cause (matching the execute path and the database/sql convention). A session-fatal error is evicted before the ctx-cancelled return so a failure racing a cancel still evicts the conn. A non-cancellable ctx (nil Done) yields a nil watcher -> NULL token -> the plain, unchanged path, so there is zero watcher overhead on the common case. Requires the kernel cancel-token symbols; bump KERNEL_REV to the merged kernel revision before this builds in CI (it links a locally-staged archive today). Tests: tagged ctxWatcher unit tests (fires on cancel/deadline, nil-safe on an uncancellable ctx, clean teardown without fire) exercising the real cgo ctx->token bridge in the build-and-test-kernel CI job. Default CGO_ENABLED=0 build unchanged. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Bumps the kernel pin from the placeholder statement-surface rev to the tip of the tier2-features branch (databricks-sql-kernel#167 @ 02c0a43), the head of the unmerged kernel PR stack (#165 -> #166 cancel-token -> #167 tier2). It's a linear superset carrying every new C-ABI symbol these driver branches link against: kernel_session_open_cancellable / kernel_result_stream_next_batch_cancellable (cancel token, #166) plus kernel_abi_version, kernel_session_close_blocking, set_tls_client_certificate, set_cloudfetch_enabled (#167). The kernel-lib build fetches the bare commit via its PR-head-ref fallback, so an unmerged SHA is buildable. Temporary: re-pin to the squash-merge SHA once the kernel stack lands. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…ions
Wire the four new kernel C-ABI symbols on the driver side, extending the
existing experimental-config surface:
- checkABIVersion(): a sync.Once handshake at the top of OpenSession compares
the linked library's kernel_abi_version() against the header's
DATABRICKS_KERNEL_ABI_VERSION and refuses to connect on a mismatch, so a
differently-built prebuilt .a can't be silently misread.
- WithKernelClientCertificate(cert, key): mTLS client identity forwarded via
the paired kernel_session_config_set_tls_client_certificate. Both PEM halves
are required together — validateKernelConfig rejects an unpaired credential
loudly so a lone key can't be silently dropped — and the key is never logged.
- WithKernelCloudFetch(enabled): a tri-state *bool (nil keeps the kernel
default on; set forwards kernel_session_config_set_cloudfetch_enabled).
Distinct from the plain-bool WithCloudFetch, whose unset state can't be told
from false.
All three are kernel-only and rejected loudly on the Thrift path (the connector
fails when KernelExperimental is non-nil). The reflective classification guard is
extended so a new experimental field can't ship unforwarded/unrejected.
CloseSession stays fire-and-forget: the C ABI now also offers
kernel_session_close_blocking, but adopting it would make close a blocking
network round-trip with no deadline honored, so swapping to it is grouped with
the cancellable-close follow-up.
Requires the kernel ABI-version / mTLS / CloudFetch symbols; bump KERNEL_REV to
the merged kernel revision before this builds in CI (it links a locally-staged
archive today).
Tests: experimental-field classification guard + option wiring + Thrift
rejection + DeepCopy (untagged); TestSetKernelTLS mTLS case, TestABIVersionMatches,
and mTLS-pairing validation (tagged / config). Default CGO_ENABLED=0 build
unchanged.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
force-pushed
the
mani/sea-kernel-tier2-features
branch
from
July 16, 2026 15:13
a067a22 to
c54f051
Compare
WithKernelClientCertificate marks the kernel experimental config as set, but an empty cert+key pair (e.g. from a failed PEM load) was indistinguishable from the option never being called: the old XOR validation accepted it and applyKernelTLS skipped the setter, so a caller who explicitly requested mTLS connected with no client identity. Add an explicit TLSClientCertConfigured marker (robust against nil-vs-empty), set it whenever the option is invoked, and tighten validateKernelConfig to reject any incomplete mTLS request (missing cert, missing key, or both empty). Covered by new option/validation/DeepCopy tests and the exhaustiveness guard.
The ABI-version handshake was only exercised on the happy path (TestABIVersionMatches, cgo build). The mismatch branch — the runtime hazard the check exists for, a driver header linked against a differently-built prebuilt .a — had no coverage because it lived behind the cgo symbols and the one-shot abiCheckOnce cache. Extract the pure got-vs-want verdict into compareABI (untagged) and have checkABIVersion delegate to it, then add TestCompareABI asserting the mismatch returns a non-nil error naming both versions and the remediation. Being untagged, the negative test runs under the default CGO_ENABLED=0 build with no kernel lib linked. Production behavior is unchanged.
mani-mathur-arch
force-pushed
the
mani/sea-kernel-ctx-cancel
branch
from
July 17, 2026 10:07
42af196 to
26cf09b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Wire the four new kernel C-ABI symbols on the driver side, extending the existing experimental-config surface:
checkABIVersion()— async.Oncehandshake at the top ofOpenSessioncompares the linked library'skernel_abi_version()against the header macro and refuses to connect on a mismatch.WithKernelClientCertificate(cert, key)— mTLS client identity via the paired setter. Both PEM halves required together (validateKernelConfigrejects an unpaired credential loudly); the key is never logged.WithKernelCloudFetch(enabled)— a tri-state*bool(nil keeps the kernel default on; set forwards the toggle). Distinct from the plain-boolWithCloudFetch.All three are kernel-only and rejected loudly on the Thrift path; the reflective classification guard is extended so a new experimental field can't ship unforwarded/unrejected.
CloseSessionstays fire-and-forget (adoptingkernel_session_close_blockingwould make close a blocking round-trip with no deadline — grouped with the cancellable-close follow-up).Closes PECOBLR-3648 / 3652 / 3653 (driver side).
Tests
DeepCopy(all underCGO_ENABLED=0); mTLS-pairing validation.TestSetKernelTLSmTLS marshalling case,TestABIVersionMatches. Live-verified on staging (WithKernelCloudFetch(true)connects;(false)reaches the server and hits the documentedCAN_CLOUD_DOWNLOADgate). Default build unchanged.Co-authored-by: Isaac