[SEA-NodeJS] Sync execute via directResults (executeStatementDirect): fix CREATE, drop close-drives, keep cancel

[msrathore-db]

What

The default sync path (runAsync: false) now calls the kernel's additive Connection.executeStatementDirect (directResults) instead of executeStatementCancellable. The session feature-detects the arm via awaitResult:

• fast query → a terminal Statement (result inline) → operation backend's statement arm;
• slow query → an AsyncStatement (poll/cancel handle) → asyncStatement arm.

Why Node needs directResults (and Python doesn't)

Mid-run cancellation needs a handle to the running statement during the run. Where that handle can come from depends on the driver's concurrency model:

• Node is single-threaded (one event loop). The only cancel path is op.cancel(), and op is the return value of executeStatement. A blocking execute() that drives the query to completion would only resolve op after the query finishes — leaving nothing to cancel mid-run. (The event loop isn't frozen — await yields — but op simply doesn't exist as a value until the call resolves.) directResults makes executeStatement return the handle right after the ~10s server inline wait, while the statement is still running, so op exists during the run and a later event-loop turn (a timeout, SIGINT, a UI "stop") can call op.cancel().
• Python doesn't need this. Its cancel lives on the cursor (cursor.cancel()), which exists before execute(), backed by a StatementCanceller registered up front. Because Python is multi-threaded, that canceller fires from another thread while the blocking execute() is still running. Nothing needs to be returned early — so Python use_kernel keeps the plain blocking execute() with no change.

(First-window caveat: the statement id isn't known until the inline-wait POST returns, so the first ~10s is uncancellable on every backend, Thrift included.)

The three fixes (a server-owned handle)

1. CREATE / fire-and-forget commits (fast DDL) — server runs it inline during the POST.
2. No close-drives — close() is a clean release (~120ms on a forever-query), never a drive-to-terminal.
3. Mid-run cancel kept — op.cancel() stops a long query (~150–300ms).

Slow-DDL behavior — identical to Thrift (verified side-by-side)

For a statement longer than the inline wait, executeStatement returns a RUNNING handle (still executing server-side). Outcome, same on both backends:

after executeStatement(slow CREATE)	commits?
await op.fetchAll() / op.finished()	✅ yes
pause / leave open (no close)	✅ yes — server finishes it autonomously
await op.close() with no prior fetch/finished	❌ no — close() cancels the still-running statement

So fire-and-forget commits for fast DDL; slow DDL must be awaited (fetchAll/finished) or it's cancelled on close() — exactly as Thrift behaves today.

Error timing

Query errors surface eagerly at executeStatement — matching Python on both backends (Thrift and use_kernel also raise at execute; verified 2×2). Node-Thrift is the lone outlier that defers the error to fetch. Eager is the majority, fail-fast behavior — no change needed.

Dependency

Requires databricks/databricks-sql-kernel#140 (additive execute_direct). The kernel's execute() is unchanged, so Python use_kernel needs no change. Supersedes #425.

Testing

• CI green: unit-test (Node 14/16/18/20), e2e-test, lint, coverage. SEA unit suite 260 passing; eslint clean.
• e2e (pecotesting): fast CREATE fire-and-forget commits; slow CREATE await/pause commit, close-without-await cancels (Thrift parity); 100k read; error-at-execute; mid-run cancel; close() cheap on an abandoned long query.

This pull request and its description were written by Isaac.

[msrathore-db]

🔴 F1 — The AsyncStatement (slow-query) arm is completely untested (High confidence — flagged by all 5 reviewers; verified)

This PR adds directReturnsRunning and an executeStatementDirect fake that can return a FakeAsyncStatement — but no test ever sets directReturnsRunning = true, and no new it(...) case was added. The flag is only referenced at its declaration (here), its comment, and the if inside the mock body.

As a result, the branch this entire PR exists to add — SeaSessionBackend.executeStatement detecting awaitResult and constructing new SeaOperationBackend({ asyncStatement }) — has zero coverage. Every existing sync-path test silently flows through the terminal {statement} arm, so the added mock plumbing is currently dead code. A regression that inverted the feature-detect, dropped the awaitResult check, or broke the async handoff would pass CI.

Suggested fix: add ≥1 test that sets connection.directReturnsRunning = true, calls the sync-default executeStatement, and asserts the returned op drives the polling arm (status()/awaitResult()) and that op.cancel() reaches the async statement's cancel() mid-run — the stated purpose of the PR. Pair it with an assertion contrasting the terminal arm.

[msrathore-db]

🟡 F3 — Stale block comment contradicts the new code (Moderate confidence — 2 reviewers; verified)

This new inline comment correctly describes directResults, but the runAsync selector comment block just above (lines ~166–173) still says the DEFAULT path "Route[s] through executeStatementCancellable: the kernel blocks on execute() … The blocking drive runs in the operation backend's result()" — exactly the behavior this PR removes. The two comments in the same method now contradict each other.

Fix: update the 166–173 block to describe directResults. (There's also a trivially stale comment at execution.test.ts:699 still referencing executeStatementCancellable.)

[msrathore-db]

🟡 F4 — as unknown as double-casts are avoidable; a user-defined type guard is safer (Moderate confidence — language + architecture)

direct is genuinely typed Statement | AsyncStatement (the real union, not any), so direct as unknown as … here and on the statement arm launder away type information the compiler already has. A user-defined type guard narrows both arms cast-free and compiles clean under strict mode:

function isAsyncStatement(x: SeaStatement | SeaNativeAsyncStatement): x is SeaNativeAsyncStatement {
  return typeof (x as SeaNativeAsyncStatement).awaitResult === 'function';
}

Then: if (isAsyncStatement(direct)) { return new SeaOperationBackend({ asyncStatement: direct, ... }); } with no casts. User-defined guards are already idiomatic in this repo (lib/result/utils.ts isString). Optional but recommended; pair with let direct: SeaStatement | SeaNativeAsyncStatement;.

[msrathore-db]

🟡 F5 — Inconsistent null/undefined defensive handling (Moderate confidence — security, devils-advocate, language; verified)

The kernel contract (Promise<Statement | AsyncStatement>) never returns null, so direct !== null is effectively dead. It's also inconsistent: it guards only the async-arm property read — a null (or undefined) direct falls through to the {statement} arm. Verified: SeaOperationBackend's providedCount counts statement !== undefined, so statement: null is accepted (count 1) and constructs a backend wrapping a dead handle; failure is then deferred to first fetch/close as an opaque TypeError rather than a mapped driver error. (undefined would instead throw at the property read here.) Only triggers on a kernel contract violation, so this is latent/defensive — not a live bug.

Fix (optional): reject a non-object direct up front via logAndMapError, or drop the dead null check; if keeping it, use direct != null to cover both null and undefined.

[msrathore-db]

Review Findings Summary

Out-of-line findings (no specific file:line)

🟠 F2 — "drop close-drives" is not delivered: the cancellableExecution machinery is orphaned, not removed (High confidence — 4 reviewers; verified)

Verified: executeStatementCancellable/cancellableExecution now has zero production producers in lib/ (the only mention in SeaSessionBackend.ts is a stale comment). But none of the machinery was deleted — Connection.executeStatementCancellable (native/sea/index.d.ts), the composite lifecycleHandle whose close() falls back to cancellableExecution.cancel() (the literal "close-drives" behavior, lib/sea/SeaOperationBackend.ts ~226–230), waitUntilReadyCancellable, the status() / getFetchHandle cancellable branches, plus the entire describe('… executeStatementCancellable path') test block + FakeCancellableExecution all remain live.

This is dead-but-tested code: a reader trusting the PR title will believe close-drives is gone, and the surviving test block misdirects coverage (it looks like sync-path coverage but tests a path production no longer reaches).

Fix: either remove the cancellableExecution plumbing + its napi method + its tests in this PR, or amend the PR description to say the old path is orphaned for later removal, not dropped. Add a tracking TODO at minimum.

---

🔵 F6 — Feature-detection by method-presence is a fragile API boundary (design-debt) (Low confidence — architecture raised; security + language verified it's correct today)

typeof asAsync.awaitResult === 'function' (SeaSessionBackend.ts:208) is correct today — verified AsyncStatement has awaitResult() and the terminal Statement does not. But it relies on Statement never gaining an awaitResult-shaped method, and the as unknown as cast means the compiler won't catch future drift. Since the kernel owns both sides of the union and kernel PR #140 is additive, a tagged discriminator (kind field / Either wrapper) would be drift-proof.

Fix (optional): request a tagged discriminator from the kernel team, or add a defensive contract comment in native/sea/index.d.ts warning that Statement must never gain an awaitResult member.

---

🔵 F7 — Loader doesn't validate executeStatementDirect at load time (Low confidence — architecture)

assertBindingShape in lib/sea/SeaNativeLoader.ts checks for CancellableExecution/AsyncStatement/AsyncResultHandle but not the new free method executeStatementDirect. A stale .node binary missing it passes load-time validation and fails at first sync execute with "X is not a function".

Fix (optional): add executeStatementDirect to the binding-shape assertion so a stale binary fails fast at load.

[msrathore-db]

Addressed all findings in c1d66dc (+ d29e675 regenerating the .d.ts JSDoc to match the corrected kernel docs).

F1 — AsyncStatement (Running) arm untested (FIXED). Added three tests driving the fake's directReturnsRunning: (a) a still-running query routes through the AsyncStatement arm and is driven via status()/waitUntilReady(); (b) op.cancel() reaches the running statement's cancel() — the branch this PR exists to add; (c) contrast — a fast query routes through the terminal Statement arm and cancel reaches it there. SEA suite now 263 passing (was 260).

F4 — avoidable as unknown as double-casts (FIXED). Since SeaStatement/SeaNativeAsyncStatement are the exact napi alias types, direct is already the precise union — replaced both casts with a cast-free user-defined type guard isSeaAsyncStatement (idiomatic, narrows both arms).

F5 — inconsistent null handling (FIXED). Replaced the dead/inconsistent direct !== null (which let a null fall through to the {statement} arm) with an up-front rejection of null/undefined via logAndMapError(HiveDriverError), so a kernel-contract violation surfaces as a mapped driver error, not a deferred opaque TypeError.

F3 — stale comments (FIXED). Updated the runAsync selector comment block (still described executeStatementCancellable + blocking result()) to directResults, and fixed the stale executeStatementCancellable comment in the test. Also documented the queryTimeout→CANCEL interaction.

eslint clean; SEA unit suite 263 passing.