feat: auto-recover Reyden Thrift connections onto the kernel backend - #523
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
An unconfigured connection to a Reyden / Real-Time SQL warehouse defaults to
the Thrift backend, which the SQL Gateway proxy rejects with SQLSTATE KP001.
Detect that rejection (StatusError.sqlState === "KP001") in
ThriftBackend.openSession and transparently re-open the session on the
KernelBackend (SEA), remembering the warehouse in a process-wide cache keyed
by (host, warehouse_id) with a ~6h TTL so later connects skip the doomed
Thrift attempt. Only the default path auto-recovers; an explicit backend
choice (routed upstream in the client) is unaffected. On a double failure the
kernel error is surfaced with the original Thrift rejection preserved as its
cause.
Also re-throw the original error unchanged on the non-recovery paths:
StatusError implements Error but does not extend it, so the previous
`error instanceof Error ? error : new Error(String(error))` normalization
wrapped every StatusError into Error("[object Object]"), losing its sqlState
and the double-failure cause.
Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
- ThriftBackend: track the KernelBackend(s) created for Reyden (KP001) fallback and close them in close(), so the process-global log-bridge onLevelChange listener installed by connect() is released instead of leaking on every recovery. Add a createKernelBackend() seam so tests can inject a fake without the native binding, plus a test that close() releases the fallback backend. - ReydenWarehouseCache test: add a TTL-expiry test (sinon fake timers) covering the 6h boundary and opportunistic eviction on access. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
9c318f4 to
34412e1
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
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.
- ThriftBackend: reuse a single fallback KernelBackend across all Reyden (KP001) fallback sessions on the connection instead of constructing one per openSession. connectionOptions are fixed after connect, so it is created + connected once (lazily, memoized; the attempt is cleared on connect failure so a later open can retry) and released in close() — this stops per-session accumulation of backends and process-global log-bridge listeners. - On the double-failure path, only set the kernel error's `cause` when it is absent, so a cause the kernel error may already carry is not clobbered. - Test now opens two fallback sessions and asserts a single KernelBackend is created and connected once, reused for both, and closed once on close(). Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
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).
| this.name = 'Status Error'; | ||
| this.message = status.errorMessage || ''; | ||
| this.code = status.errorCode || -1; | ||
| this.sqlState = status.sqlState; |
There was a problem hiding this comment.
seems concerning that node driver never looks into sql state before 😬
| /** | ||
| * Mark a warehouse as being Reyden (KP001 rejection detected). | ||
| */ | ||
| public markReyden(host: string, warehouseId: string): void { |
There was a problem hiding this comment.
is there any reason why we don't sweep for expired keys here like Python and Go?
markReyden only added the new entry, so the per-key lazy eviction in isKnownReyden never reclaimed an entry that was marked and then never looked up again — it would persist for the life of the process. Sweep expired entries when marking a warehouse; markReyden runs only on an actual Thrift rejection, so the sweep is near-free and bounds the cache to warehouses seen within the TTL window. Adds a fake-timer test. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the auto-recovery flow is well-structured and thoroughly unit-tested; the sqlState capture, KP001 detection, cache keying/TTL, and useKernel guardrail all check out against the surrounding code. One low-severity lifecycle edge: close() can miss a fallback KernelBackend whose connect is still in flight, leaking its process-global log-bridge listener.
| // Release the process-global log-bridge listener held by the Reyden-fallback KernelBackend. | ||
| // DBSQLClient owns the rest of the connection lifecycle and clears its own state | ||
| // (connectionProvider, authProvider, thrift client) after this returns. | ||
| if (this.fallbackKernelBackend) { |
There was a problem hiding this comment.
🔵 Low — close() releases the fallback backend only when this.fallbackKernelBackend is already assigned. But getFallbackKernelBackend() sets this.fallbackKernelBackend inside the async IIFE, after await kernelBackend.connect(...) resolves. If close() runs while a fallback connect is still in flight (e.g. openSession() triggered recovery and hasn't resolved when the client is torn down), this.fallbackKernelBackend is still undefined, so close() skips it. The pending connect then completes, installs the process-global kernel log-bridge listener, and assigns this.fallbackKernelBackend on a backend nobody will ever close() — leaking that listener for the life of the process.
Consider awaiting/closing the in-flight attempt as well, e.g. if (this.fallbackKernelBackendConnect) { const kb = await this.fallbackKernelBackendConnect.catch(() => undefined); await kb?.close(); } and then clearing both fields. Narrow window (requires close() racing an unresolved fallback connect), hence low severity.
close() released the fallback backend only via this.fallbackKernelBackend, which getFallbackKernelBackend assigns after connect() resolves. A close() that ran while a fallback connect was still in flight found the field unset and skipped it; the pending connect then resolved, installed the process- global log-bridge listener, and assigned the field on a backend nobody would ever close — leaking that listener for the process lifetime. Await the in-flight connect promise in close() instead of only the resolved field, closing whatever it produces (whether already connected or still in flight). Adds a test that calls close() during an unresolved fallback connect. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Solid, well-tested port of the Reyden KP001 auto-recovery — the guardrail (ThriftBackend only on the default path), StatusError.sqlState wiring, connect(options) signature, and the close() lifecycle for an in-flight fallback connect all check out against the surrounding code. Two low-severity notes: a write-only fallbackKernelBackend dead field (with a harmless stale-write-after-close), and that the fallback also fires for an explicit useKernel: false, which slightly contradicts the described guardrail.
After close() switched to awaiting the in-flight connect, the fallbackKernelBackend field became write-only: assigned in the connect IIFE and cleared in close(), but read nowhere. It also left a confusing stale write — a close() racing an in-flight connect nulled the field before the IIFE re-assigned it onto an already-closed backend. Remove the field; the fallbackKernelBackendConnect promise (whose resolved value is the backend) is the single source of truth. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Solid, well-tested port of the Reyden Thrift auto-recovery feature — the StatusError.sqlState plumbing, KP001 detection, single-fallback semantics, cause-chaining, memoized fallback-backend reuse, and the close()/in-flight-connect race are all handled carefully and covered by unit tests. The instanceof Error normalization fix for StatusError is correct and verified against Status.assert. Two low-severity notes only: a dead tri-state in the cache and a pinning/no-retry behavior worth confirming as intentional.
markReyden was the only writer and only ever stored isReyden: true, so the CacheEntry.isReyden field and isKnownReyden's boolean | undefined return type carried an unreachable "known not Reyden" state. Drop the field and return a plain boolean: an unexpired entry means Reyden, its absence means not known. Callers already used the result in a boolean context. Update the tests that asserted the old undefined return to expect false. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Solid, well-structured port of the Reyden Thrift→SEA auto-recovery feature with thorough mocked unit coverage (KP001 detection, cache pre-check/TTL/sweep, single-fallback-backend reuse, and the close()-races-connect leak case). StatusError.sqlState wiring is correct (TStatus declares the field), the instanceof StatusError re-throw correctly avoids the implements Error normalization trap, and the memoized fallback-backend lifecycle/close semantics look right. One low-severity note inline about the double-failure cause guard discarding the KP001 when the kernel error already has a cause. Minor: the feature ships unit-tests-only (acknowledged in the PR) — an e2e path isn't practical without a real Reyden warehouse, so that's informational, not blocking.
2 participants
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 theKernelBackend(SEA), so no connection-parameter change is needed.StatusErrornow carriessqlState;ThriftBackend.openSessiontreatssqlState === 'KP001'as the Reyden marker (matched on the SQLSTATE only).openSessionre-opens once viaKernelBackend. On a double failure the kernel error is surfaced with the original Thrift rejection preserved as itscause.ReydenWarehouseCachekeyed 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.)ThriftBackendis only reached on the default path, so it never overrides an explicituseKernel.Incidental bug fix
The non-recovery paths now re-throw the original error unchanged.
StatusErrorimplements Errorbut does notextendsit, sonew StatusError(...) instanceof Errorisfalseat runtime; the recovery catch'serror instanceof Error ? error : new Error(String(error))normalization wrapped everyStatusErrorintoError("[object Object]"), losing itssqlStateon the non-Reyden path and corrupting the double-failurecause.How is this tested?
ReydenThriftRecoveryOrchestration.test.tsdrives the realThriftBackend.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-failurecausepreservation.ReydenThriftRecovery.test.tscovers the cache, warehouse-ID extraction, and SQLSTATE capture. Full unit suite green (1287 passing); type-check, eslint, prettier clean.Notes
optionalDependencies(pinned0.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.2.0.0section 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.