[SEA-NodeJS] Address #416 P1 review round 2: cancel-intent, statement-leak, PEM order, TLS warn, Thrift signal

databricks-sql-nodejs#416 (gopalldb P1):

- P1.1: `seaCancel` no longer rolls `isCancelled` back to false when the kernel
  cancel RPC fails. The caller asked to cancel, so the op stays cancelled
  (subsequent fetches fail fast, poll-loop observers stay consistent); the RPC
  failure is still surfaced via the rethrow. Rolling back silently resurrected a
  cancelled op while the server statement might still run. Test asserts the flag
  stays set on failure.

- P1.5: the async poll loop now best-effort `close()`s the kernel statement on
  every server-driven terminal error (Failed / Cancelled / Closed / Unknown /
  Timeout) before throwing — previously it leaked the statement handle until
  session close (only `fetchChunk` cleaned up). Warn-logs a close failure.

- P1.3: `customCaCert` PEM check is now an ordered regex
  (`BEGIN…END` block) instead of two independent substring checks, so a blob
  containing both markers out of order (e.g. a proxy-intercept page) is rejected.

- P1.4: warn when `customCaCert` is set together with
  `checkServerCertificate: false` — verification is fully off so the custom CA
  is unused; the combo is still honoured but no longer silently masks it.

- P1.6: the Thrift `rowLimit`/`statementConf` ignored-option signal is now a
  WARN (was debug) — these materially change results (e.g. `rowLimit` not
  capping), so a caller on the Thrift path gets a visible warning.

P1.2 (race in-flight `status()` against a cancel signal) deferred: bounded by the
HTTP transport timeout today; the proper fix needs a cancel-signal promise +
napi `AbortSignal`, which the binding doesn't yet expose — tracked as a
follow-up.

Validated: tsc/eslint/prettier clean; 243 SEA / 1162 full unit tests pass; live
smoke confirms both modes execute correctly.

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

Author: msrathore-db

lib/sea/SeaAuth.ts

@@ -195,16 +195,15 @@ export function buildSeaTlsOptions(options: ConnectionOptions): SeaTlsOptions {
 
   if (customCaCert !== undefined) {
     if (typeof customCaCert === 'string') {
-      // Light PEM sanity check — require both the BEGIN and END markers so a
-      // truncated/headerless cert is rejected here rather than surfacing as an
-      // opaque kernel TLS error. Full parsing is deferred to the kernel.
-      if (
-        !customCaCert.includes('-----BEGIN CERTIFICATE-----') ||
-        !customCaCert.includes('-----END CERTIFICATE-----')
-      ) {
+      // Light PEM sanity check — require a well-ordered BEGIN…END block so a
+      // truncated/headerless cert (or a stray page that merely contains both
+      // literals out of order, e.g. a proxy-intercept page) is rejected here
+      // rather than surfacing as an opaque kernel TLS error. Ordered match, not
+      // two independent substring checks. Full parsing is deferred to the kernel.
+      if (!/-----BEGIN CERTIFICATE-----[\s\S]+?-----END CERTIFICATE-----/.test(customCaCert)) {
         throw new HiveDriverError(
           'SEA backend: `customCaCert` string does not look like a PEM certificate ' +
-            "(missing the '-----BEGIN CERTIFICATE-----' / '-----END CERTIFICATE-----' markers). " +
+            "(expected a '-----BEGIN CERTIFICATE-----' … '-----END CERTIFICATE-----' block). " +
             'Pass PEM text or a Buffer of PEM bytes.',
         );
       }

lib/sea/SeaBackend.ts

@@ -16,6 +16,8 @@ import IBackend from '../contracts/IBackend';
 import ISessionBackend from '../contracts/ISessionBackend';
 import IClientContext from '../contracts/IClientContext';
 import { ConnectionOptions, OpenSessionRequest } from '../contracts/IDBSQLClient';
+import { InternalConnectionOptions } from '../contracts/InternalConnectionOptions';
+import { LogLevel } from '../contracts/IDBSQLLogger';
 import HiveDriverError from '../errors/HiveDriverError';
 import { getSeaNative, SeaNativeBinding, SeaConnection } from './SeaNativeLoader';
 import { decodeNapiKernelError } from './SeaErrorMapping';
@@ -78,6 +80,22 @@ export default class SeaBackend implements IBackend {
     // Any non-PAT mode (or a missing/empty token) throws here, before
     // we ever touch the native binding.
     this.nativeOptions = buildSeaConnectionOptions(options);
+
+    // Warn on the insecure combo: a `customCaCert` paired with
+    // `checkServerCertificate: false` is almost always a mistake — verification
+    // is fully off, so the custom trust anchor is never used. The combo is
+    // still honoured (kernel contract), but a secure-looking `customCaCert`
+    // shouldn't silently mask disabled verification.
+    const tlsOpts = options as ConnectionOptions & InternalConnectionOptions;
+    if (tlsOpts.checkServerCertificate === false && tlsOpts.customCaCert !== undefined) {
+      this.context
+        .getLogger()
+        .log(
+          LogLevel.warn,
+          'SEA: `customCaCert` is set but `checkServerCertificate: false` disables certificate ' +
+            'verification entirely — the custom CA is not used. Set `checkServerCertificate: true` to use it.',
+        );
+    }
   }
 
   public async openSession(request: OpenSessionRequest): Promise<ISessionBackend> {

lib/sea/SeaOperationBackend.ts

@@ -468,15 +468,28 @@ export default class SeaOperationBackend implements IOperationBackend {
         case OperationState.Failed:
           // `status()` collapses Failed to the variant name only; the real
           // SQL-error envelope (sql_state / error_code / query_id) rides on
-          // `awaitResult()`'s rejection — drive it to surface the typed error.
-          // eslint-disable-next-line no-await-in-loop
-          await this.throwAsyncError();
+          // `awaitResult()`'s rejection — drive it to surface the typed error,
+          // then best-effort close the leaked statement before it propagates.
+          try {
+            // eslint-disable-next-line no-await-in-loop
+            await this.throwAsyncError();
+          } catch (failErr) {
+            // eslint-disable-next-line no-await-in-loop
+            await this.bestEffortClose();
+            throw failErr;
+          }
           break;
         case OperationState.Cancelled:
+          // eslint-disable-next-line no-await-in-loop
+          await this.bestEffortClose();
           throw new OperationStateError(OperationStateErrorCode.Canceled);
         case OperationState.Closed:
+          // eslint-disable-next-line no-await-in-loop
+          await this.bestEffortClose();
           throw new OperationStateError(OperationStateErrorCode.Closed);
         default:
+          // eslint-disable-next-line no-await-in-loop
+          await this.bestEffortClose();
           throw new OperationStateError(OperationStateErrorCode.Unknown);
       }
 
@@ -497,6 +510,10 @@ export default class SeaOperationBackend implements IOperationBackend {
                 `statement may keep running until the session is closed. Cause: ${cause}`,
             );
         });
+        // Release the statement handle too (cancel stops the work; close frees
+        // the handle) — best-effort, mirroring the other terminal branches.
+        // eslint-disable-next-line no-await-in-loop
+        await this.bestEffortClose();
         throw new OperationStateError(OperationStateErrorCode.Timeout);
       }
 
@@ -549,6 +566,26 @@ export default class SeaOperationBackend implements IOperationBackend {
     throw new HiveDriverError(`SEA operation ${this._id} reported Failed but produced a result.`);
   }
 
+  /**
+   * Best-effort close of the kernel statement when the poll loop ends on a
+   * server-driven terminal error (Failed/Cancelled/Closed/Unknown/Timeout).
+   * Without it the kernel-side statement handle leaks until session close (the
+   * poll loop, unlike `fetchChunk`, otherwise just throws). Never masks the
+   * original error; warn-logs a close failure so the leak is diagnosable.
+   */
+  private async bestEffortClose(): Promise<void> {
+    await seaClose(this.lifecycle, this.lifecycleHandle, this.context, this._id).catch((closeErr) => {
+      const cause = closeErr instanceof Error ? closeErr.message : String(closeErr);
+      this.context
+        .getLogger()
+        .log(
+          LogLevel.warn,
+          `SEA poll-loop cleanup: close() failed for operation ${this._id}; the server-side ` +
+            `statement may leak until the session is closed. Cause: ${cause}`,
+        );
+    });
+  }
+
   /**
    * Resolve (and memoise) the fetch handle: `awaitResult()`'s `AsyncResultHandle`
    * on the query path, or the already-terminal `Statement` on the metadata path.

lib/sea/SeaOperationLifecycle.ts

@@ -131,12 +131,17 @@ export async function seaCancel(
 
   context.getLogger().log(LogLevel.debug, `Cancelling SEA operation with id: ${operationId}`);
 
+  // Set the intent BEFORE the RPC and keep it set even if the cancel RPC
+  // fails: the caller asked to cancel, so the operation must stay cancelled
+  // (subsequent fetches fail fast, and any poll-loop observer that already saw
+  // the flag stays consistent). The RPC failure is surfaced via the rethrow —
+  // we do NOT roll the flag back, which would silently resurrect a cancelled
+  // operation while the kernel-side statement may still be running.
   state.isCancelled = true;
 
   try {
     await statement.cancel();
   } catch (err) {
-    state.isCancelled = false;
     rethrowKernelError(err);
   }
 

lib/thrift-backend/ThriftSessionBackend.ts

@@ -177,9 +177,10 @@ export default class ThriftSessionBackend implements ISessionBackend {
       this.context
         .getLogger()
         .log(
-          LogLevel.debug,
+          LogLevel.warn,
           'ThriftSessionBackend.executeStatement: rowLimit / statementConf are kernel-backend (useSEA) ' +
-            'options and are ignored on the Thrift path.',
+            'options with no Thrift wire equivalent — they are IGNORED on the Thrift path (e.g. rowLimit ' +
+            'will not cap the result set). Use the kernel backend (useSEA) to honour them.',
         );
     }
 

tests/unit/sea/operation-lifecycle.test.ts

@@ -166,6 +166,10 @@ describe('SeaOperationLifecycle (helpers)', () => {
       }
       expect(thrown).to.be.instanceOf(HiveDriverError);
       expect((thrown as Error).message).to.contain('statement already closed');
+      // The caller asked to cancel: a failed cancel RPC must NOT roll the intent
+      // back (doing so would silently resurrect a cancelled op while the
+      // server-side statement may still be running).
+      expect(state.isCancelled).to.equal(true);
     });
 
     it('logs a debug message tagged with the operation id', async () => {