Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
msrathore-db merged 6 commits into from
Jun 8, 2026
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb749d6
[SEA-NodeJS] Sync execute via directResults (executeStatementDirect)
msrathore-db Jun 6, 2026
c1d66dc
[SEA-NodeJS] Address review: test the AsyncStatement arm; type-guard;…
msrathore-db Jun 7, 2026
d29e675
[SEA-NodeJS] Regenerate napi .d.ts JSDoc to match corrected kernel di…
msrathore-db Jun 7, 2026
8f1b268
[SEA-NodeJS] Make queryTimeout a no-op on SEA (stop abusing wait_time…
msrathore-db Jun 7, 2026
11f0576
[SEA-NodeJS] Remove the dead queryTimeout deadline plumbing (kernel f…
msrathore-db Jun 7, 2026
82e549b
style: prettier-format SeaOperationBackend + execution.test after que…
msrathore-db Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions lib/sea/SeaSessionBackend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import InfoValue from '../dto/InfoValue';
import HiveDriverError from '../errors/HiveDriverError';
import ParameterError from '../errors/ParameterError';
import { LogLevel } from '../contracts/IDBSQLLogger';
import { SeaConnection, SeaNativeExecuteOptions, SeaStatement } from './SeaNativeLoader';
import { SeaConnection, SeaNativeExecuteOptions, SeaStatement, SeaNativeAsyncStatement } from './SeaNativeLoader';
import { decodeNapiKernelError } from './SeaErrorMapping';
import { numberToInt64 } from '../thrift-backend/ThriftSessionBackend';
import SeaOperationBackend from './SeaOperationBackend';
Expand Down Expand Up @@ -183,24 +183,34 @@ export default class SeaSessionBackend implements ISessionBackend {
options.queryTimeout !== undefined ? numberToInt64(options.queryTimeout).toNumber() : undefined;

if (!runAsync) {

@msrathore-db msrathore-db Jun 7, 2026

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.

🟡 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.)

// Sync path: forward `queryTimeoutSecs` to the napi options — the kernel
// `execute()` honours it (server statement timeout).
// DEFAULT — directResults (the Thrift/JDBC model). The kernel's
// `executeStatementDirect` runs ExecuteStatement with a bounded server
// inline wait and returns WITHOUT polling past it:
// - a terminal `Statement` (result inline) for a fast query, or
// - an `AsyncStatement` (a poll/cancel handle) for a slow one.
// Either way the handle is tied to a server-owned statement, so a long
// query stays cancellable via `op.cancel()` and `close()` is a clean
// release (no client-side drive-to-terminal). Fire-and-forget DDL/DML
// commits because the server runs it inline during the POST.
const execOptions = this.buildExecuteOptions(options, queryTimeoutSecs);
let cancellableExecution;
let direct;
try {
cancellableExecution =
direct =
execOptions === undefined
? await this.connection.executeStatementCancellable(statement)
: await this.connection.executeStatementCancellable(statement, execOptions);
? await this.connection.executeStatementDirect(statement)
: await this.connection.executeStatementDirect(statement, execOptions);
} catch (err) {
throw this.logAndMapError('executeStatement', err);
}
// Feature-detect the arm: only `AsyncStatement` (the Running arm) exposes
// `awaitResult`; the terminal `Statement` (Completed arm) does not.
const asAsync = direct as unknown as SeaNativeAsyncStatement;

@msrathore-db msrathore-db Jun 7, 2026

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.

🟡 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;.

if (direct !== null && typeof asAsync.awaitResult === 'function') {

@msrathore-db msrathore-db Jun 7, 2026

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.

🟡 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.

return new SeaOperationBackend({ asyncStatement: asAsync, context: this.context, queryTimeoutSecs });
}
return new SeaOperationBackend({
cancellableExecution: cancellableExecution!,
statement: direct as unknown as SeaStatement,
context: this.context,
// The kernel honours `queryTimeoutSecs` on the sync `execute` path, so
// it is forwarded via the napi options (see `buildExecuteOptions`); the
// backend also keeps it as a deadline guard for parity with async.
queryTimeoutSecs,
});
}
Expand Down
16 changes: 16 additions & 0 deletions native/sea/index.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions tests/unit/sea/execution.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,26 @@ class FakeNativeConnection implements SeaConnection {
return this.lastCancellableExecution;
}

// directResults (`runAsync: false`, the DEFAULT) query path: records sql +
// options and returns either a terminal `Statement` (Completed arm) or — when
// `directReturnsRunning` is set — a pending `AsyncStatement` (Running arm),
// the two arms `SeaSessionBackend.executeStatement` feature-detects via
// `awaitResult`.
public directReturnsRunning = false;

@msrathore-db msrathore-db Jun 7, 2026

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.

🔴 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.


public async executeStatementDirect(sql: string, options?: unknown): Promise<any> {
if (this.throwOnExecute) {
throw this.throwOnExecute;
}
this.lastSql = sql;
this.lastOptions = options;
if (this.directReturnsRunning) {
this.lastAsyncStatement = new FakeAsyncStatement(this.submitStatusValue);
return this.lastAsyncStatement;
}
return this.statementToReturn;
}

// Async-submit path: records sql + per-statement options (for forwarding
// assertions) and returns a pending AsyncStatement.
public async submitStatement(sql: string, options?: unknown): Promise<any> {
Expand Down
Loading