Skip to content

[SEA-NodeJS] Kernel-parity batch: mTLS + custom headers/UA + retry/backoff + operation-status fields#420

Merged
msrathore-db merged 2 commits into
mainfrom
msrathore/sea-mtls-headers-useragent
Jun 8, 2026
Merged

[SEA-NodeJS] Kernel-parity batch: mTLS + custom headers/UA + retry/backoff + operation-status fields#420
msrathore-db merged 2 commits into
mainfrom
msrathore/sea-mtls-headers-useragent

Conversation

@msrathore-db

@msrathore-db msrathore-db commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

What

Consolidated PR landing every kernel-supported capability that was missing on the Node SEA path — the items the kernel core (and the Python connector) already support but the Node driver did not yet wire. Folds in the previously-separate #427 and #422.

Features

  1. mTLS + custom HTTP headers + user-agent entry (orig. this PR) — clientCertPem/clientKeyPem, customHeaders, userAgentEntry → kernel TlsConfig / HttpConfig.custom_headers. Kernel: ✅ · Python: ✅ #819/#823.
  2. Retry/backoff tuning (was [SEA-NodeJS] Forward retry/backoff tuning to the kernel on the SEA path #427) — buildSeaRetryOptions maps the driver's ClientConfig retry knobs (ms) → kernel retry{Min,Max}WaitSecs / retryMaxAttempts / retryOverallTimeoutSecs (whole secs), merged in SeaBackend.connect(). Unset knobs are omitted (kernel default stands; no NaN across the FFI). Kernel: ✅ · Python: ✅ #820.
  3. Operation-status fields (was [SEA-NodeJS] Surface operation-status fields (displayMessage/diagnosticInfo/errorDetailsJson, numModifiedRows) on getOperationStatus #422) — numModifiedRows / displayMessage / diagnosticInfo / errorDetailsJson surfaced through getOperationStatus instead of a flat Succeeded (the M1 item in SeaOperationLifecycle). Kernel: ✅ · Python: ✅ #825.

Plus the napi package-name alignment to @databricks/databricks-sql-kernel-* (kernel #131/#135).

numModifiedRows — full Thrift parity for SEA DML (sync and async)

Feature 3 wired the accessor, but it was empty in practice: Thrift reports numModifiedRows on the operation status; SEA puts no count on its status envelope — it returns the count as the DML result set (a single row of num_affected_rows / num_inserted_rows / num_updated_rows / num_deleted_rows). So the kernel accessor read status.num_modified_rows, which is always null against current SEA warehouses.

The kernel (databricks-sql-kernel #144) now derives it from that result set, and this PR surfaces it on both execution paths:

  • Sync (runAsync:false, the default): kernel derives the count off the terminal ExecutedStatement's result via a non-consuming peek (ResultStream::peek_first_batch), so the count row still flows to fetchAll() — byte-identical to Thrift.
  • Async (runAsync:true): the count rides on the terminal GetStatement poll the async path already makes; the kernel derives it there (no extra fetch, gated on the manifest signature so SELECT streaming is untouched) and exposes the accessors on the napi AsyncStatement. readRichStatusFields reads off the async handle (extracted into a shared readStatusFieldsFrom), and status() surfaces them on the Succeeded state.

Detection is strict (every result column must be a num_*_rows counter), so a SELECT … AS num_affected_rows is not mistaken for a DML count.

Review-comment fixes + failing unit test (this round)

  • Failing unit test: ArrowResultConverter.test.ts used BigInt literals (123n), which don't compile at the tsconfig ES2018 target the unit-test job typechecks against (TS2737) → replaced with BigInt('123').
  • C1 — parameterized the kernel-statement fakes + added sync-DML and async propagation tests asserting non-null numModifiedRows/displayMessage/diagnosticInfo/errorDetailsJson flow through op.status() (previously the fakes returned all-null, so propagation was untested).
  • C2 — added synthesizeThriftStatus tests: Int64 re-boxing of numModifiedRows (incl. 0 vs absent), null → undefined mapping, string passthrough.
  • C3buildSeaHttpOptions now rejects CR/LF/NUL in customHeaders names/values (HTTP header injection) with a clear HiveDriverError, before the FFI hop — instead of the kernel's opaque connect-time error.
  • C4 — simplified a dead conjunct in readRichStatusFields (the constructor already guarantees handle-kind exclusivity).
  • C5 — memoized the rich-status read so re-status()-ing a terminal operation reuses it instead of re-hitting the FFI accessors.

Verification

  • SEA + Thrift unit suites: green; tsc clean (ES2018).
  • Live (pecotesting http_path2): SEA INSERT/UPDATE/DELETE/MERGE report numModifiedRows 3/2/1/2 on both the sync and async paths — matching Thrift exactly — with fetchAll() byte-identical across both backends and SELECT carrying no count. CR/LF/NUL custom headers rejected early with a clear error.

⚠️ Depends on kernel #144

KERNEL_REV is pinned to #144's branch HEAD (75bfa52), which carries the retry kwargs + PK/FK metadata reshape + DNS fast-fail and the DML numModifiedRows derivation (sync + async). Re-pin to #144's squash-merge SHA before merge (orphan-SHA risk otherwise). The napi contract (native/sea/index.d.ts) is committed in sync with that SHA.

Supersedes

This pull request and its description were written by Isaac.

@msrathore-db msrathore-db force-pushed the msrathore/sea-mtls-headers-useragent branch from 68dbb3d to 1607ce0 Compare June 5, 2026 01:11
@msrathore-db msrathore-db force-pushed the msrathore/sea-mtls-headers-useragent branch from 1607ce0 to b0cf092 Compare June 5, 2026 01:31
@msrathore-db msrathore-db force-pushed the msrathore/sea-mtls-headers-useragent branch from b0cf092 to cdcf766 Compare June 5, 2026 01:40
@msrathore-db msrathore-db force-pushed the msrathore/sea-mtls-headers-useragent branch from cdcf766 to 84924c9 Compare June 5, 2026 01:56
@msrathore-db msrathore-db force-pushed the msrathore/sea-mtls-headers-useragent branch from 84924c9 to cfe3b3f Compare June 5, 2026 08:41
@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@msrathore-db msrathore-db changed the title [SEA-NodeJS] Kernel backend: mTLS, custom HTTP headers & User-Agent [SEA-NodeJS] Kernel-parity batch: mTLS + custom headers/UA + retry/backoff + operation-status fields Jun 6, 2026
@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

Comment thread lib/sea/SeaOperationBackend.ts Outdated
* status-field read must not turn a successful operation's status query into
* a throw. The fields are best-effort metadata, not the operation outcome.
*/
private async readRichStatusFields(): Promise<SeaRichStatusFields> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH CONFIDENCE — flagged by test + devil's-advocate reviewers]

readRichStatusFields() is the read/aggregate path for the headline feature (numModifiedRows/displayMessage/diagnosticInfo/errorDetailsJson). But the kernel-statement fakes that back it return null for every accessor — tests/unit/sea/execution.test.ts:63-76 and tests/unit/sea/SeaOperationBackend.test.ts:83-97 (the latter isn't even in this PR). So this method only ever runs returning {null, null, null, null}, and no test sets e.g. numModifiedRows → 42 and asserts it surfaces on op.status() / getOperationStatus().

Pass-but-prove-nothing today: the DML numModifiedRows reaching the public surface (the core feature claim), the readOrNull catch branch, and the older-binding all-null degrade branch (typeof handle.numModifiedRows !== 'function').

Suggested fix: parameterize the kernel-statement fakes to return non-null values and assert they propagate through op.status().


Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated — real coverage gap, now addressed. tests/unit/sea/execution.test.ts drives the kernel-statement fakes with configurable non-null rich values plus a richReads counter, and asserts propagation end-to-end: "surfaces rich-status fields (numModifiedRows etc.) through op.status() once Succeeded" (async path), "surfaces … and memoizes the read" (sync path), "reports all-null rich-status for a SELECT", and "does not read rich-status fields before the async statement is terminal". So numModifiedRows → N reaching op.status(), the readOrNull catch, and the all-null degrade branch are now asserted. Resolved.

This reply was written by Isaac.

numModifiedRows:
status.numModifiedRows === undefined || status.numModifiedRows === null
? undefined
: new Int64(status.numModifiedRows),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MODERATE CONFIDENCE — flagged by test + devil's-advocate reviewers]

synthesizeThriftStatus now maps the four new fields, re-boxing numModifiedRows as a node-int64 Int64 and collapsing null → undefined. tests/unit/thrift-backend/wireSynthesis.test.ts has zero references to any of these fields (not in this PR, not updated). The Int64 re-box and null-coercion are exactly the kind of line that regresses silently — a revert to a bare number, or null instead of undefined, would pass every test.

The Int64 type choice itself is correct (the Thrift wire type is I64). The gap is purely test coverage.

Suggested fix: add wireSynthesis cases asserting numModifiedRows: 5 → Int64(5), null → undefined, and passthrough for the three string fields.


Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated — the Int64 re-box / null-coercion were untested; now covered. tests/unit/thrift-backend/wireSynthesis.test.ts has a rich status fields block asserting numModifiedRows: 5 → Int64(5), 0 → Int64(0) (real zero-row DML, not absent), null → undefined, absent → undefined, the three string fields pass through, and null strings → undefined. A revert to a bare number or null would now fail. Resolved.

This reply was written by Isaac.

Comment thread lib/sea/SeaAuth.ts
tls.customCaCert = customCaCert;
} else {
throw new HiveDriverError('SEA backend: `customCaCert` must be a PEM string or a Buffer.');
headers.push({ name, value });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW CONFIDENCE — flagged by security reviewer]

customHeaders (name + value) is forwarded verbatim into the napi header list, which the kernel now sends on every SEA request. The only filtering is dropping reserved names (authorization / x-databricks-org-id); header names and values are not checked for CR/LF/NUL. If an app builds customHeaders from semi-trusted input, a \r\n-bearing value could enable header/request smuggling depending on how the kernel's HTTP client serializes headers.

Medium, not High: the same map is already trusted on the existing OOB telemetry/feature-flag path, and the kernel's hyper/reqwest encoder typically rejects invalid header bytes. But the JS layer is the right place to reject obviously-malformed names/values now that they feed the primary data plane — mirroring the PEM sanity-checks already in this file.

Suggested fix: validate header name (token chars) and value (no CR/LF/NUL) in buildSeaHttpOptions and throw HiveDriverError on violation; at minimum confirm the kernel rejects invalid header bytes.


Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated and already handled in SeaAuth.ts: buildSeaHttpOptions calls validateHeaderToken on both the header name and value — FORBIDDEN_HEADER_CHARS = /[\r\n\0]/ — throwing a clear HiveDriverError before the FFI hop. That rejects exactly the CR/LF/NUL request-splitting/header-injection vector you flagged, mirroring the PEM sanity checks. (The kernel reqwest encoder also rejects these, but opaquely at connect time; this surfaces an actionable error at the point the header is set.) Resolved.

This reply was written by Isaac.

Comment thread lib/sea/SeaOperationBackend.ts Outdated

// The async path never produces a terminal sync `Statement`, so there is
// nothing to read these off of.
if (this.asyncStatement && !this.cancellableExecution) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH CONFIDENCE — flagged by language + architecture + maintainability reviewers]

The constructor enforces that exactly one of asyncStatement / statement / cancellableExecution is set, so this.asyncStatement being truthy already implies !this.cancellableExecution. The second conjunct here is therefore dead — if (this.asyncStatement) is equivalent and reads cleaner against the mutual-exclusivity invariant the rest of the file relies on.

Non-blocking nit. Either simplify to if (this.asyncStatement) or add a one-word comment that the second half is belt-and-suspenders.


Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — done. Simplified to if (this.asyncStatement); the && !this.cancellableExecution conjunct was redundant given the constructor`s mutual-exclusivity invariant. Resolved.

This reply was written by Isaac.

}
// The blocking `result()` has resolved a terminal `Statement` — surface
// its rich status fields alongside the Succeeded state.
return { state: OperationState.Succeeded, hasResultSet: true, ...(await this.readRichStatusFields()) };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW CONFIDENCE — flagged by performance reviewer]

The public status() API re-runs readRichStatusFields() on every call for a SEA sync/metadata operation that has already completed. A consumer polling status() in their own loop against a finished operation pays 4 FFI accessor reads per call instead of 0. These are local FFI reads of the kernel's already-cached terminal response (no network RPC), parallelized, errors swallowed to null — so it's bounded and minor.

Optional: memoize the result once the statement is terminal to eliminate the repeat reads. Not required to merge.


Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed. readRichStatusFields() is memoized via richStatusFieldsPromise (computed once, then the cached promise is returned), and it is only invoked on a terminal state — the async path calls it solely when state === Succeeded, the sync path only after getFetchHandle() has resolved the terminal statement. So a consumer polling status() on a finished op pays the 4 parallel FFI reads once and 0 thereafter; the richReads counter in the sync test asserts the re-read does not happen. Resolved.

This reply was written by Isaac.

@vikrantpuppala

Copy link
Copy Markdown
Collaborator

Code Review Squad — Review

Score: 81/100 — MODERATE RISK

No correctness blockers in a large, well-engineered SEA kernel-parity batch (mTLS, custom headers/UA, retry/backoff, operation-status fields). The one concentrated concern — raised independently by the test reviewer and the devil's advocate — is that the headline operation-status feature (numModifiedRows/displayMessage/diagnosticInfo/errorDetailsJson) is only ever exercised with all-null stub values, so the read → map → surface path is unverified end-to-end. mTLS, headers/UA, and retry/backoff are well covered and clean; the remaining items are doc-accuracy gaps and nits. All 9 reviewers delivered (full coverage).


Medium Findings

F3 — runAsync:true path silently omits rich status fields — undocumented parity asymmetry

[LOW CONFIDENCE — flagged by devil's-advocate reviewer]

The PR sells the operation-status fields as "at parity with Thrift," but that holds only on the sync and metadata paths. runAsync defaults to false (SeaSessionBackend.ts:181), so the common DML case routes sync and readRichStatusFields() runs — the default works. But on an explicit runAsync: true path, status() and the waitUntilReadyAsync Succeeded callback return { state, hasResultSet } with no rich fields (the diff's own comments admit "they stay undefined here"). Result is a three-way mismatch: SEA-sync has the fields, SEA-async doesn't, Thrift always does.

This is a kernel-binding limitation (the accessors only exist on the sync Statement), not trivially fixable in this PR — but it should be documented as a known asymmetry rather than presented as full parity.

F4 — Public customHeaders doc not updated for new SEA primary-transport behavior

[MODERATE CONFIDENCE — flagged by security + agent-compat reviewers]

lib/contracts/IDBSQLClient.ts documents customHeaders as applying to "driver-owned out-of-band requests (telemetry POSTs and feature-flag GETs). Not applied to the primary Thrift transport." After this PR, on the SEA backend these headers are sent on the primary data-plane request to the warehouse (minus reserved auth/org-id). The doc is silent on SEA and would lead a reader to wrongly conclude customHeaders never reaches query requests.

Suggested fix: add one sentence, e.g. "On the SEA backend these are additionally sent on the primary request transport (reserved auth/org-id headers are stripped)."

Low Findings

Low findings (2) — click to expand

F6 — Stale DBSQLOperation.ts doc comment contradicts shipped behavior

[LOW CONFIDENCE — flagged by devil's-advocate reviewer]

The doc comment on DBSQLOperation.status() (lib/DBSQLOperation.ts:176) still reads "...with Thrift-only fields (taskStatus, numModifiedRows, etc.) left undefined" on non-Thrift backends. This PR specifically makes numModifiedRows/displayMessage/diagnosticInfo/errorDetailsJson populated on SEA, so the comment now directly misleads. Update it.

F7 — Confirm KERNEL_REV pin actually contains kernel #141 before merge

[LOW CONFIDENCE — flagged by devil's-advocate reviewer]

TypeScript compiles against the committed native/sea/index.d.ts (which declares the new accessors), but the real native binary is built from KERNEL_REV at CI time. The runtime guard typeof handle.numModifiedRows !== 'function' degrades gracefully to all-null if the pinned kernel predates the accessors — which means if KERNEL_REV (fcc459b) isn't actually pointing at a kernel containing #141, the whole rich-fields feature silently returns null with green unit tests; only a live-warehouse E2E catches it. The PR description itself notes KERNEL_REV "needs re-pinning before merge." Confirm the pin contains #141 before merging.

Separately: the PR has an outstanding DCO sign-off bot flag (process, not code) — addressable with a sign-off rebase.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

@gopalldb

gopalldb commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Findings (3 final, all MAJOR bugs):

📄 lib/DBSQLParameter.ts
⚠️ [MAJOR] Line 140 | bug | logical_claude_agent [1/3 Claude flagged]
DATE parameter yields wrong calendar date in non-UTC timezones.
this.value.toISOString().slice(0, 10) converts the Date to UTC before
extracting the date portion. When a user in a positive-offset timezone (e.g.
UTC+10) creates new Date(2024, 2, 14) intending March 14, the internal UTC
representation is 2024-03-13T14:00:00Z, so toISOString().slice(0, 10) yields
"2024-03-13" — off by one day. The same off-by-one occurs in the other
direction for negative-offset timezones near midnight. This affects any
DBSQLParameterType.DATE binding where the Date was constructed with local-time
semantics rather than a UTC midnight literal.
💡 Suggested Fix:
Use local-time accessors (getFullYear, getMonth, getDate) to extract the
calendar date the user intended, rather than converting through UTC. This
matches how users typically reason about DATE parameters — as a local calendar
date, not a UTC instant.
```suggestion
stringValue: isDateType
? ${this.value.getFullYear()}-${String(this.value.getMonth() + 1).padStart(2, '0')}-${String(this.value.getDate()).padStart(2, '0')}
: this.value.toISOString(),

📄 lib/sea/SeaOperationBackend.ts
⚠️ [MAJOR] Line 430 | bug | logical_claude_agent [1/3 Claude flagged]
status() on the sync (cancellableExecution) path can block indefinitely
when called during execution. Once getFetchHandle() has been called (e.g.,
by a concurrent getResultSlicer() or waitUntilReadyCancellable()),
this.fetchHandlePromise becomes truthy. The status() method then falls
through to the Succeeded branch (line 435), which calls await this.readRichStatusFields(). That in turn calls await this.getFetchHandle()
(line 521 of computeRichStatusFields), which awaits the still-pending
result(). This means a consumer polling status() to observe progress on the
sync path will block until the entire execution completes — defeating the
purpose of a status poll. The check !this.fetchHandlePromise conflates 'the
promise exists' with 'the promise has resolved', but only the latter implies the
operation is truly Succeeded.
💡 Suggested Fix:
Track whether the blocking result() has actually resolved (e.g., check
this.blockingStatement !== undefined for the cancellableExecution path, since
it's only assigned after result() resolves) rather than using the existence of
fetchHandlePromise as a proxy for completion.
if (!this.blockingStatement) {
return { state: OperationState.Running, hasResultSet: true };
}

⚠️  [MAJOR]  Line 615 | bug | logical_claude_agent  [1/3 Claude flagged]
Async path progress callback omits rich status fields on terminal Succeeded

tick. On the sync (runAsync:false) path, waitUntilReadyCancellable (line
711-713) reads readRichStatusFields() and merges
numModifiedRows/displayMessage/etc. into the progress callback's status
object. On the async path, waitUntilReadyAsync fires the callback at line 615
with only { state, hasResultSet: true } — even when state is Succeeded —
so the terminal tick never carries numModifiedRows. Consumers wiring a
progress callback for DML row counts (e.g. a progress UI) will see the count on
the sync path but not on the async path, breaking the documented parity between
the two execution modes.

…ckoff + operation-status fields

Squashed rebase of the SEA kernel-parity batch onto current main
(replaces the prior multi-commit branch, which had diverged with
unsigned commits and a merge-commit history that conflicted with the
since-merged #424/#426). Net feature set unchanged:

- mTLS (clientCertPem/clientKeyPem) + custom HTTP headers + User-Agent
  entry on the SEA path, with PEM and header-token (CR/LF/NUL)
  validation in SeaAuth.
- Retry/backoff tuning forwarded to the kernel (omitting unset knobs).
- Operation-status fields surfaced through op.status(): numModifiedRows,
  displayMessage, diagnosticInfo, errorDetailsJson (async + sync paths),
  memoized once terminal; Thrift wire synthesis maps the same fields.
- preserveBigNumericPrecision connection option (Thrift + SEA) and
  DATE/large-number parameter type-inference fix.

Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…rettier

Review findings (GopalDB / code-review agent), each validated:

1. DBSQLParameter DATE off-by-one in non-UTC timezones — `toISOString()`
   converts to UTC before slicing, so a local-constructed Date shifts a
   day in offset zones. Use local calendar accessors
   (getFullYear/getMonth/getDate). Validated end-to-end on a live SEA
   warehouse under TZ=Australia/Sydney: a `new Date(2024,2,14)` now
   round-trips as 2024-03-14 (was 2024-03-13). Regression test sabotages
   toISOString so it guards in any timezone (incl. the UTC CI runner).

2. SeaOperationBackend.status() (sync path) could block indefinitely —
   it gated Succeeded on `fetchHandlePromise` *existing*, but that is
   assigned synchronously when getFetchHandle() is first called while
   result() may still be pending; status() would then report Succeeded
   early AND await the pending result() via readRichStatusFields(). Gate
   on `blockingStatement` (set only after result() resolves) instead.

3. Async-path progress callback omitted the rich-status fields on the
   terminal Succeeded tick (the sync path includes them) — parity break
   for DML numModifiedRows. Merge the rich fields into the Succeeded
   callback in waitUntilReadyAsync.

Also:
- KERNEL_REV -> 9c2e2378 (kernel main, #144 merged). Binding contract
  (native/sea/index.d.ts) already matched the bumped surface.
- prettier --write (lint fix) + new regression tests for findings 1-3.

Co-authored-by: Isaac
Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
@msrathore-db msrathore-db force-pushed the msrathore/sea-mtls-headers-useragent branch from 0fcd03e to 980ac66 Compare June 8, 2026 16:31
@msrathore-db

Copy link
Copy Markdown
Contributor Author

Thanks @gopalldb — all three findings validated and fixed in 980ac66.

1. DATE off-by-one (DBSQLParameter.ts) — CONFIRMED, fixed.
toISOString().slice(0, 10) converts to UTC first, shifting the day in non-UTC zones. Switched to local calendar accessors (getFullYear/getMonth/getDate). Validated end-to-end on a live SEA warehouse under TZ=Australia/Sydney: binding new Date(2024, 2, 14) (internally 2024-03-13T13:00Z) now round-trips as 2024-03-14 — previously it would have sent 2024-03-13. Added a regression test that sabotages toISOString so it guards in any timezone (incl. the UTC CI runner, where the two formulations would otherwise agree).

2. status() sync-path can block indefinitely (SeaOperationBackend.ts:430) — CONFIRMED, fixed.
The gate used fetchHandlePromise existence as a completion proxy, but it's assigned synchronously when getFetchHandle() is first called (by a concurrent fetch / waitUntilReady) while result() is still pending — so status() reported Succeeded early and then awaited the pending result() via readRichStatusFields(). Now gated on blockingStatement, which is only set inside the resolve handler after result() completes. Added a regression test: with a parked result(), status() returns Running promptly (raced against a timeout) rather than blocking.

3. Async progress callback omits rich fields on the terminal tick (SeaOperationBackend.ts:615) — CONFIRMED, fixed.
waitUntilReadyAsync fired { state, hasResultSet } even on Succeeded, while the sync waitUntilReadyCancellable merges numModifiedRows/etc. Now the async Succeeded tick carries the rich fields too (read is memoised and doesn't force result materialisation). Added a regression test asserting the Succeeded callback carries numModifiedRows while non-terminal ticks do not.

Also in this push: KERNEL_REV bumped to kernel main 9c2e2378 (#144 merged), the branch rebased onto current main (squashed — the prior history had unsigned commits + merge commits that conflicted with the since-merged #424/#426), DCO sign-off on all commits, and prettier --write (fixes the failing lint).

This reply was written by Isaac.

@msrathore-db msrathore-db added this pull request to the merge queue Jun 8, 2026
Merged via the queue into main with commit 4804f1e Jun 8, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants