[SEA-NodeJS] Address #416 review: stable op.id (telemetry), runAsync contract, doc + signal/coverage gaps

Review findings (databricks-sql-nodejs#416):

- F2 (HIGH): revert the sync-path op.id mutation. `op.id` flipped from a client
  UUID to the server statement_id after `result()` resolved, but the facade keys
  telemetry start/complete on it (DBSQLOperation → MetricsAggregator), so the
  flip split the records across two keys and dropped the summary. `id` is now
  stable for the operation's lifetime; the resolved server statement_id is
  surfaced via a debug log for server/kernel correlation instead. Test updated:
  asserts id is stable AND the server id is logged.

- F1 (HIGH): `runAsync` is the SEA sync/async toggle but was JSDoc-@deprecated,
  and a comment falsely claimed it "mirrors Thrift's runAsync distinction"
  (Thrift hardcodes runAsync:true and never reads the option). Replaced the
  @deprecated tag with the cross-backend contract (Thrift: no-op; kernel:
  selects sync-default vs async) and corrected the in-code comment.

- Doc: SeaSessionBackend class comment still said metadata "defers to M1 —
  throws"; metadata is fully implemented. Rewritten to list the implemented
  surface.

- F3 (MED): ThriftSessionBackend now debug-logs when rowLimit / statementConf
  (kernel-only options) are set on the Thrift path, instead of dropping silently.

- F4 (MED): added the missing coverage using the previously-dead fakes —
  sync-path Failed/SQL-error envelope (`resultError`), submit-time error mapping
  on both paths (`throwOnExecute`), and queryTags-vs-statementConf.query_tags
  collision precedence.

- F5 (MED): the query-timeout best-effort cancel now warn-logs a failed cancel
  (mirrors the fetch-error cleanup) so a still-running server statement is
  diagnosable.

- F10 (LOW): hoisted `Object.values(OperationState)` to a module const off the
  100ms async poll loop.

Validated: tsc/eslint/prettier clean; 243 SEA / 1162 full unit tests pass; live
smoke confirms op.id is stable across fetch on both paths and both return
correct data.

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

Author: msrathore-db

lib/contracts/IDBSQLSession.ts

@@ -12,7 +12,18 @@ export type ExecuteStatementOptions = {
    */
   queryTimeout?: number | bigint | Int64;
   /**
-   * @deprecated This option is no longer supported and will be removed in future releases
+   * Selects the execution lifecycle. The only observable effect is WHEN
+   * `executeStatement` resolves; the result data, schema, and error classes are
+   * identical regardless.
+   *
+   * - **Thrift backend:** no-op. The Thrift path always submits asynchronously
+   *   (`runAsync: true` on the wire) and polls during fetch; this option is not
+   *   read.
+   * - **Kernel backend (`useSEA`):** selects the kernel execution path —
+   *   `false`/unset (default) runs the blocking direct-results path (faster,
+   *   cancellable mid-compute); `true` submits and polls (returns a pending
+   *   handle before completion). Default is sync, matching the python
+   *   connector's `cursor.execute()`.
    */
   runAsync?: boolean;
   maxRows?: number | bigint | Int64 | null;

lib/sea/SeaOperationBackend.ts

@@ -101,11 +101,15 @@ function delay(ms: number): Promise<void> {
  * with the enum; `Canceled` (one-L spelling) is mapped defensively, and any
  * unrecognised value collapses to `Unknown`.
  */
+// Hoisted out of the (hot, 100ms) async poll loop — `Object.values` would
+// otherwise allocate a fresh array on every status tick.
+const OPERATION_STATE_VALUES = Object.values(OperationState) as string[];
+
 function statusStringToOperationState(state: string): OperationState {
   if (state === 'Canceled') {
     return OperationState.Cancelled;
   }
-  if ((Object.values(OperationState) as string[]).includes(state)) {
+  if (OPERATION_STATE_VALUES.includes(state)) {
     return state as OperationState;
   }
   return OperationState.Unknown;
@@ -157,10 +161,6 @@ export default class SeaOperationBackend implements IOperationBackend {
   // Undefined on the async / metadata paths.
   private readonly cancellableExecution?: SeaNativeCancellableExecution;
 
-  // Sync path: the server statement id captured from the resolved `Statement`
-  // once `result()` settles. Undefined until then; surfaced via `id`.
-  private resolvedStatementId?: string;
-
   // Metadata path: terminal statement. Also the resolved fetch handle on the
   // sync-execute path once `cancellableExecution.result()` settles.
   private blockingStatement?: SeaOperationStatement;
@@ -235,14 +235,15 @@ export default class SeaOperationBackend implements IOperationBackend {
   }
 
   public get id(): string {
-    // On the sync (cancellable) path the server statement id isn't known at
-    // construction — it's published mid-`result()` once the initial execute
-    // round-trip returns. Surface it once available (preferring the id captured
-    // from the resolved `Statement`, then the live canceller slot) so a
-    // cancelled/closed sync operation is traceable by the same id the server and
-    // kernel logs key on, matching the async path (which has it from `submit`).
-    // Falls back to the construction-time id (a client UUID) until then.
-    return this.resolvedStatementId ?? this.cancellableExecution?.statementId ?? this._id;
+    // STABLE for the operation's lifetime. The facade keys telemetry start/
+    // complete on this value (DBSQLOperation → MetricsAggregator), so it must
+    // NOT mutate — a sync op's server statement_id isn't known until `result()`
+    // resolves (mid-execute), and flipping `id` then would split the start/
+    // complete records across two keys and silently drop the summary. The
+    // resolved server statement_id is instead surfaced via a debug log (see
+    // `getFetchHandle`) for server/kernel log correlation. On the async path
+    // `_id` already IS the server id (available at submit).
+    return this._id;
   }
 
   public hasResultSet(): boolean {
@@ -482,9 +483,20 @@ export default class SeaOperationBackend implements IOperationBackend {
       // Still Pending/Running — enforce the client-side timeout before sleeping.
       if (deadline !== undefined && Date.now() >= deadline) {
         // Best-effort server-side cancel so the statement doesn't keep running
-        // after we stop waiting; never mask the timeout with a cancel failure.
+        // after we stop waiting; never mask the timeout with a cancel failure,
+        // but warn-log a failed cancel so a still-running server statement is
+        // diagnosable (mirrors the fetch-error cleanup path).
         // eslint-disable-next-line no-await-in-loop
-        await this.cancel().catch(() => undefined);
+        await this.cancel().catch((cancelErr) => {
+          const cause = cancelErr instanceof Error ? cancelErr.message : String(cancelErr);
+          this.context
+            .getLogger()
+            .log(
+              LogLevel.warn,
+              `SEA query-timeout cleanup: cancel() failed for operation ${this._id}; the server-side ` +
+                `statement may keep running until the session is closed. Cause: ${cause}`,
+            );
+        });
         throw new OperationStateError(OperationStateErrorCode.Timeout);
       }
 
@@ -555,9 +567,15 @@ export default class SeaOperationBackend implements IOperationBackend {
           .result()
           .then((stmt) => {
             this.blockingStatement = stmt as unknown as SeaOperationStatement;
-            // Capture the now-known server statement id (stable on the resolved
-            // Statement) so `id` reports it for the rest of the lifecycle.
-            this.resolvedStatementId = this.blockingStatement.statementId ?? this.resolvedStatementId;
+            // Log the now-known server statement id (NOT surfaced via `id`,
+            // which must stay stable for telemetry correlation) so a sync op is
+            // correlatable to server/kernel logs by its client operation id.
+            const serverId = this.blockingStatement.statementId;
+            if (serverId && serverId !== this._id) {
+              this.context
+                .getLogger()
+                .log(LogLevel.debug, `SEA operation ${this._id} resolved to server statement_id ${serverId}`);
+            }
             return stmt as unknown as SeaFetchHandle;
           })
           .catch((err) => {

lib/sea/SeaSessionBackend.ts

@@ -52,12 +52,12 @@ export interface SeaSessionBackendOptions {
 /**
  * SEA-backed implementation of `ISessionBackend`.
  *
- * **M0 scope:** `executeStatement` + `close`. Metadata methods
- * (`getCatalogs`, `getSchemas`, etc.) defer to M1 — they throw a clear
- * `HiveDriverError` so consumers using SEA against metadata APIs get an
- * actionable message instead of silently falling back. The Thrift
- * backend continues to handle the metadata path by default (callers
- * opt into SEA via `ConnectionOptions.useSEA`).
+ * **Scope:** `executeStatement` (sync + async), `close`, `getInfo`, and the
+ * full metadata surface (`getCatalogs`, `getSchemas`, `getTables`,
+ * `getColumns`, `getFunctions`, `getTableTypes`, `getTypeInfo`,
+ * `getPrimaryKeys`, `getCrossReference`) — each forwards to the kernel's napi
+ * metadata calls (see `runMetadata`). The Thrift backend remains the default;
+ * callers opt into the kernel path via `ConnectionOptions.useSEA`.
  *
  * **Session config flow:** catalog / schema / sessionConf are applied
  * once at session creation (kernel `Session::builder().defaults()` +
@@ -155,10 +155,13 @@ export default class SeaSessionBackend implements ISessionBackend {
       );
     }
 
-    // `runAsync` selects the kernel execution path, exactly mirroring the
-    // Thrift backend's `runAsync` distinction (the only observable difference is
-    // WHEN `executeStatement` resolves — the public API, result shape, schema,
-    // and error classes are identical on both paths):
+    // `runAsync` selects the kernel execution path. NOTE: this is a SEA/kernel-
+    // specific use of the option — the Thrift backend hardcodes `runAsync: true`
+    // on the wire and never reads `options.runAsync`, so the field is a no-op
+    // there. The only observable difference between the two SEA paths is WHEN
+    // `executeStatement` resolves; the public API, result shape, schema, and
+    // error classes are identical on both (and to Thrift). See the option's
+    // JSDoc in `IDBSQLSession` for the cross-backend contract.
     //
     //   - DEFAULT (`runAsync` false/undefined) — SYNC. Route through
     //     `executeStatementCancellable`: the kernel blocks on `execute()`

lib/thrift-backend/ThriftSessionBackend.ts

@@ -170,6 +170,19 @@ export default class ThriftSessionBackend implements ISessionBackend {
     const driver = await this.context.getDriver();
     const clientConfig = this.context.getConfig();
 
+    // `rowLimit` / `statementConf` are kernel-backend (SEA) options with no
+    // Thrift wire equivalent. Surface a debug breadcrumb rather than dropping
+    // them silently, so a caller that set them on the Thrift path has signal.
+    if (options.rowLimit !== undefined || options.statementConf !== undefined) {
+      this.context
+        .getLogger()
+        .log(
+          LogLevel.debug,
+          'ThriftSessionBackend.executeStatement: rowLimit / statementConf are kernel-backend (useSEA) ' +
+            'options and are ignored on the Thrift path.',
+        );
+    }
+
     const request = new TExecuteStatementReq({
       sessionHandle: this.sessionHandle,
       statement,

tests/unit/sea/execution.test.ts

@@ -326,16 +326,16 @@ function makeBinding(connection: SeaConnection): SeaNativeBinding & {
   return Object.assign(binding, { openSessionStub });
 }
 
-function makeContext(): IClientContext {
-  const logger: IDBSQLLogger = {
+function makeContext(logger?: IDBSQLLogger): IClientContext {
+  const log: IDBSQLLogger = logger ?? {
     log(_level: LogLevel, _message: string): void {
       // no-op
     },
   };
   const config = {} as ClientConfig;
   return {
     getConfig: () => config,
-    getLogger: () => logger,
+    getLogger: () => log,
     getConnectionProvider: async () => {
       throw new Error('not used by SEA backend');
     },
@@ -659,6 +659,37 @@ describe('SeaSessionBackend', () => {
     expect(conf?.query_tags).to.contain('team:x');
   });
 
+  it('queryTags wins over a query_tags key in statementConf (precedence on collision)', async () => {
+    const connection = new FakeNativeConnection();
+    const session = makeSession(connection);
+    await session.executeStatement('SELECT 1', {
+      statementConf: { query_tags: 'manual-raw-value' },
+      queryTags: { team: 'x' },
+    });
+    const conf = (connection.lastOptions as { statementConf?: Record<string, string> }).statementConf;
+    // The structured `queryTags` option overwrites a raw `query_tags` conf key —
+    // a single, predictable wire value rather than two competing ones.
+    expect(conf?.query_tags).to.contain('team:x').and.to.not.equal('manual-raw-value');
+  });
+
+  it('maps a submit-time kernel error via logAndMapError on both paths', async () => {
+    const envelope = `__databricks_error__:${JSON.stringify({ code: 'SqlError', message: 'SUBMIT_BOOM' })}`;
+    for (const opts of [{}, { runAsync: true }]) {
+      const connection = new FakeNativeConnection();
+      connection.throwOnExecute = new Error(envelope); // fails executeStatementCancellable / submitStatement
+      const session = makeSession(connection);
+      let thrown: unknown;
+      try {
+        // eslint-disable-next-line no-await-in-loop
+        await session.executeStatement('SELECT 1', opts);
+      } catch (err) {
+        thrown = err;
+      }
+      expect(thrown, `path ${JSON.stringify(opts)}`).to.be.instanceOf(HiveDriverError);
+      expect((thrown as Error).message).to.match(/SUBMIT_BOOM/);
+    }
+  });
+
   // Genuinely unsupported on SEA — rejected (rather than silently ignored) so
   // a caller/agent gets signal instead of a no-op. queryTags / queryTimeout /
   // rowLimit are NOT here — they are forwarded (asserted above).
@@ -994,18 +1025,42 @@ describe('SeaOperationBackend — sync (executeStatementCancellable) path', () =
     ).to.throw(HiveDriverError, /exactly one/);
   });
 
-  it('surfaces the resolved server statement id as op.id once the sync execute completes', async () => {
-    const op = makeSyncOp(new FakeCancellableExecution());
-    // Before result() resolves, the server id is not yet known (real
-    // `CancellableExecution.statementId` is null pre-round-trip), so op.id is the
-    // construction-time fallback — a client UUID, NOT the server statement id.
+  it('keeps op.id stable across the sync execute and logs the resolved server statement id', async () => {
+    // op.id MUST stay stable (the facade keys telemetry start/complete on it —
+    // a mid-flight flip to the server id would split the records and drop the
+    // summary). The server statement_id is surfaced via a debug log instead.
+    const logs: Array<{ level: LogLevel; message: string }> = [];
+    const logger: IDBSQLLogger = { log: (level, message) => logs.push({ level, message }) };
+    const op = new SeaOperationBackend({
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      cancellableExecution: new FakeCancellableExecution() as any,
+      context: makeContext(logger),
+    });
     const idBefore = op.id;
     expect(idBefore).to.be.a('string').and.have.length.greaterThan(0);
-    expect(idBefore).to.not.equal('01ef-fake-statement-id');
-    // Driving the blocking execute to terminal publishes the server id; op.id
-    // then reports it (parity with the async path, traceable in server logs).
     await op.waitUntilReady();
-    expect(op.id).to.equal('01ef-fake-statement-id');
+    // Stable: driving result() to terminal does NOT mutate the id.
+    expect(op.id).to.equal(idBefore);
+    // But the now-known server statement_id is logged for correlation.
+    expect(logs.some((l) => l.message.includes('01ef-fake-statement-id'))).to.equal(true);
+  });
+
+  it('surfaces the kernel SQL-error envelope when a sync result() rejects (Failed)', async () => {
+    const exec = new FakeCancellableExecution();
+    // The kernel rejects result() with a sentinel-framed structured error;
+    // decodeNapiKernelError turns it into a typed HiveDriverError (sync path).
+    exec.resultError = new Error(
+      `__databricks_error__:${JSON.stringify({ code: 'SqlError', message: 'TABLE_OR_VIEW_NOT_FOUND' })}`,
+    );
+    const op = makeSyncOp(exec);
+    let thrown: unknown;
+    try {
+      await op.waitUntilReady();
+    } catch (err) {
+      thrown = err;
+    }
+    expect(thrown).to.be.instanceOf(HiveDriverError);
+    expect((thrown as Error).message).to.match(/TABLE_OR_VIEW_NOT_FOUND/);
   });
 
   it('status() reports Running before result() and Succeeded after', async () => {