feat: auto-recover Reyden Thrift connections onto the kernel backend

[rahuls-db]

Description

Ports the Reyden Thrift auto-recovery feature (already in the Python driver, databricks/databricks-sql-python#948; Go port databricks/databricks-sql-go#479) to the Node.js driver.

An unconfigured connection to a Reyden / Real-Time SQL warehouse defaults to the Thrift backend, which the SQL Gateway proxy rejects with SQLSTATE KP001. This change detects that rejection and transparently re-opens the session on the KernelBackend (SEA), so no connection-parameter change is needed.

• Detection: StatusError now carries sqlState; ThriftBackend.openSession treats sqlState === 'KP001' as the Reyden marker (matched on the SQLSTATE only).
• Recovery: on KP001, openSession re-opens once via KernelBackend. On a double failure the kernel error is surfaced with the original Thrift rejection preserved as its cause.
• Pinning: a process-wide ReydenWarehouseCache keyed by (host_lowercased, warehouse_id), ~6h TTL, with opportunistic eviction; a pre-check skips the Thrift round-trip for a known-Reyden warehouse. (Node is single-threaded, so no locking.)
• Guardrail: the explicit-backend choice is honored upstream in the client — ThriftBackend is only reached on the default path, so it never overrides an explicit useKernel.

Incidental bug fix

The non-recovery paths now re-throw the original error unchanged. StatusError implements Error but does not extends it, so new StatusError(...) instanceof Error is false at runtime; the recovery catch's error instanceof Error ? error : new Error(String(error)) normalization wrapped every StatusError into Error("[object Object]"), losing its sqlState on the non-Reyden path and corrupting the double-failure cause.

How is this tested?

• Unit tests

ReydenThriftRecoveryOrchestration.test.ts drives the real ThriftBackend.openSession (stubbing only the two leaf I/O methods): reactive recovery onto the kernel, cache pre-check skipping Thrift, cache marking, non-KP001 pass-through, and double-failure cause preservation. ReydenThriftRecovery.test.ts covers the cache, warehouse-ID extraction, and SQLSTATE capture. Full unit suite green (1287 passing); type-check, eslint, prettier clean.

Notes

• Kernel availability: the kernel binding ships as per-platform optionalDependencies (pinned 0.2.0). If the platform's binding isn't installed, the fallback surfaces the load error — the same kernel-availability constraint the Python driver has with its optional [kernel] extra.
• CHANGELOG: deferred to maintainers — the in-flight 2.0.0 section is a themed breaking-security release, so the target release for this feature is a maintainer call.

Related

Design: "Simplifying Reyden Onboarding on Drivers" (Option A).

---

This PR was created with GitHub MCP.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested feature port. One medium concern: the fallback KernelBackend is never close()d, so its process-global log-bridge onLevelChange listener leaks on every Reyden recovery (F1). Also a minor test-coverage gap on the cache TTL-expiry path (F2). The StatusError re-throw fix and sqlState capture look correct, and backend selection confirms ThriftBackend is only reached on the default path.

[peco-review-bot]

🟡 Medium — The fallback KernelBackend created here is never close()d, leaking a process-global log-bridge listener.

kernelBackend.connect(this.connectionOptions) calls installKernelLogBridge(...), which — because DBSQLLogger implements onLevelChange (lib/DBSQLLogger.ts:37) — registers a logger.onLevelChange(...) listener and returns an unsubscribe function stored on the kernel backend's kernelLogUnsubscribe. That unsubscribe is only ever invoked by KernelBackend.close().

But this kernelBackend local is discarded once openSession returns. DBSQLClient tracks only the outer ThriftBackend as this.backend, and ThriftBackend.close() is a documented no-op — so the kernel backend's close() never runs. The onLevelChange listener (and the process-global kernel tracing sink retarget) therefore persists for the process lifetime. Every Reyden fallback session (each openSession call on a KP001 / known-Reyden warehouse constructs a fresh KernelBackend and connects again) adds another listener that is never removed.

Consider holding the fallback KernelBackend on the ThriftBackend instance and calling its close() from ThriftBackend.close(), so the bridge unsubscribe fires when the client is closed.

[peco-review-bot]

🔵 Low — The TTL-expiry branch (isExpired → opportunistic eviction returning undefined) is never exercised by any test. ReydenThriftRecovery.test.ts covers marking, host case-insensitivity, isolation, size, and clear, but not expiry — so a regression in the Date.now() - entry.timestamp > TTL_MS comparison or the delete-on-access eviction would pass CI silently. Since Date.now() isn't injectable here, testing this would need a clock stub (e.g. sinon fake timers) or a seam to override the timestamp. Low severity, but the 6h TTL is a core part of the cache contract described in the file header.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested port of the Reyden Thrift→SEA auto-recovery. Detection/recovery/cache logic and the StatusError instanceof bug fix all look correct. One Medium around per-session accumulation of fallback KernelBackends, plus a Low about blind cause overwrite.

[peco-review-bot]

🟡 Medium — A single ThriftBackend instance is created once per client (DBSQLClient sets this.backend at connect and reuses it for every openSession — see DBSQLClient.ts:852). Every fallback openSession here constructs a new KernelBackend, calls connect() on it (which installs a process-global kernel log-bridge listener), and pushes it into fallbackKernelBackends. These are only released in ThriftBackend.close().

So a long-lived client that opens N sessions against a known-Reyden warehouse accumulates N KernelBackend instances and performs N installKernelLogBridge registrations, all held for the lifetime of the client. Because the pre-check path (isKnownReyden → openSessionWithKernelBackend) also runs per openSession, this is the common steady-state, not an edge case. The bridge is last-writer-wins so only one is functionally active, but the references/listeners still pile up.

Consider caching/reusing a single fallback KernelBackend per ThriftBackend (keyed by connectionOptions, which are fixed after connect) instead of one per openSession, so repeated session opens don't accumulate backends and listeners.

[peco-review-bot]

🔵 Low — On the double-failure path the kernel error's cause is assigned unconditionally, overwriting any cause the decoded kernel error may already carry (kernel errors flow through decodeNapiKernelError). If a kernel error ever chains its own underlying cause, that chain is silently discarded in favor of the Thrift KP001 error. Low impact today since decoded kernel errors don't appear to set cause, but a guard (only set when cause is absent, or nest the existing one) would be safer and future-proof.

[peco-review-bot]

Verdict: 1 Low

Looks good — the connect(options) change matches the IBackend contract, sqlState is a real TStatus field, and the KP001 detection / cache / single-reuse fallback logic is coherent and well-tested. One low-severity resource-lifecycle edge case: a close() that races an in-flight fallback kernel connect can orphan a KernelBackend (and its process-global log-bridge listener).

[peco-review-bot]

🔵 Low — close() only tears down the fallback kernel backend when this.fallbackKernelBackend is already set. If close() runs while a fallback connect() is still in flight (i.e. an openSession KP001 recovery is racing a close()), fallbackKernelBackend is still undefined, so close() is a no-op. When the pending fallbackKernelBackendConnect promise later resolves, it assigns this.fallbackKernelBackend = kernelBackend — an orphaned KernelBackend that installed a process-global log-bridge listener in connect() and is now never closed. This is exactly the listener leak the reuse logic is meant to prevent, just moved to the close-during-open window. Consider awaiting/guarding the in-flight fallbackKernelBackendConnect in close() (e.g. await it, then close whatever it produced, and set a closed flag so a late-resolving connect closes itself). Narrow race, hence low, but the connector is long-lived and listeners are process-global.