fix(drivers): stop a 2s deadline failing healthy DuckDB stores, and make a broken client say so

.github/workflows/ci.yml

@@ -60,6 +60,21 @@ jobs:
               - 'packages/opencode/test/altimate/drivers-mongodb-e2e.test.ts'
               - 'packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts'
               - 'packages/opencode/test/altimate/connections.test.ts'
+              # Run by the "DuckDB store-open E2E" step in the driver-e2e job.
+              # Without these, a PR touching only these files skips that job,
+              # and the main TypeScript job runs them with ALTIMATE_DUCKDB_E2E
+              # unset, which skips every test in them — no execution anywhere.
+              - 'packages/opencode/test/altimate/duckdb-open-e2e.test.ts'
+              - 'packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts'
+              - 'packages/opencode/test/altimate/duckdb-lock-helper.ts'
+              - 'packages/opencode/src/altimate/tools/warehouse-test.ts'
+              - 'packages/drivers/test/**'
+              # These govern whether the native DuckDB binding is fetched at all
+              # (`trustedDependencies`, and the pinned version). A change to them
+              # can break every real-DuckDB test while touching no test file.
+              - 'package.json'
+              - 'bun.lock'
+              - 'packages/drivers/package.json'
             dbt-tools:
               - 'packages/dbt-tools/**'
             installer:
@@ -286,6 +301,14 @@ jobs:
       - name: Install dependencies
         run: bun install
 
+      # `packages/drivers` declares no scripts and no job had it as a working
+      # directory, so its 141 unit tests — including the driver's lock, timeout
+      # and read-only regressions — never ran in CI at all. The main TypeScript
+      # job runs `bun test` from `packages/opencode` only.
+      - name: Run drivers unit suite
+        run: bun test --timeout 60000
+        working-directory: packages/drivers
+
       - name: Run local driver E2E (DuckDB, SQLite, PostgreSQL)
         run: bun test test/altimate/drivers-e2e.test.ts
         working-directory: packages/opencode
@@ -294,6 +317,24 @@ jobs:
           TEST_PG_PORT: "15432"
           TEST_PG_PASSWORD: testpass123
 
+      # Needs a dedicated process: four files in test/altimate install a
+      # top-level mock.module("@altimateai/drivers/duckdb", …), and Bun
+      # evaluates every test file's top level before running any test, so in a
+      # whole-directory run these would silently exercise a fake. With
+      # ALTIMATE_DUCKDB_E2E=1 a missing or mocked driver fails the step rather
+      # than skipping it.
+      #
+      # `--timeout` is explicit because invoking `bun test` directly does not use
+      # the package's `test` script, so it would otherwise take the CLI default
+      # of 5000ms — shorter than both the driver's 30s default open budget and
+      # the lock helper's 30s readiness budget, which would reimpose exactly the
+      # kind of too-short outer deadline this PR removes.
+      - name: Run DuckDB store-open E2E (real store, no mocks)
+        run: bun test --timeout 90000 test/altimate/duckdb-open-e2e.test.ts test/altimate/warehouse-test-duckdb-e2e.test.ts
+        working-directory: packages/opencode
+        env:
+          ALTIMATE_DUCKDB_E2E: "1"
+
       - name: Run Docker driver E2E (MySQL, SQL Server, Redshift)
         run: bun test test/altimate/drivers-docker-e2e.test.ts
         working-directory: packages/opencode

package.json

@@ -126,6 +126,7 @@
     "printWidth": 120
   },
   "trustedDependencies": [
+    "duckdb",
     "esbuild",
     "node-pty",
     "protobufjs",

packages/drivers/src/duckdb.ts

@@ -4,6 +4,51 @@
 
 import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types"
 
+// altimate_change start — configurable, generous open budget
+/**
+ * How long to wait for DuckDB to finish opening a store before giving up.
+ *
+ * This is a liveness guard, not a performance budget. Opening a store is
+ * dispatched to the libuv threadpool, so the wait covers queueing behind every
+ * other threadpool user in the process (fs, dns, crypto), not just DuckDB's own
+ * work. A busy agent process can therefore push a healthy open well past a
+ * second, and the previous hard-coded 2s ceiling turned that into an
+ * unrecoverable failure on a store that was fine.
+ */
+const DEFAULT_OPEN_TIMEOUT_MS = 30_000
+
+/**
+ * Largest delay `setTimeout` represents. A larger value overflows the timer's
+ * 32-bit signed delay and is clamped to 1ms, which would turn a deliberately
+ * huge budget into an immediate deadline — the exact failure this file exists
+ * to remove. Clamp instead, so an over-large budget still behaves like a very
+ * long one.
+ */
+const MAX_TIMER_MS = 2_147_483_647
+
+/** Read a positive, finite millisecond budget, or `undefined` if unusable. */
+function positiveMs(value: unknown): number | undefined {
+  const n = typeof value === "number" ? value : Number(value)
+  return Number.isFinite(n) && n > 0 ? Math.min(n, MAX_TIMER_MS) : undefined
+}
+
+/**
+ * Where the budget came from, which decides how a caller should read a
+ * deadline failure: one the connection set is that connection's own doing and
+ * is fixed by changing it, while the default or a machine-wide env var firing
+ * says something about the machine instead.
+ */
+type TimeoutSource = "connection" | "env" | "default"
+
+function resolveOpenTimeoutMs(config: ConnectionConfig): { ms: number; source: TimeoutSource } {
+  const fromConfig = positiveMs(config.open_timeout_ms)
+  if (fromConfig !== undefined) return { ms: fromConfig, source: "connection" }
+  const fromEnv = positiveMs(globalThis.process?.env?.["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"])
+  if (fromEnv !== undefined) return { ms: fromEnv, source: "env" }
+  return { ms: DEFAULT_OPEN_TIMEOUT_MS, source: "default" }
+}
+// altimate_change end
+
 export async function connect(config: ConnectionConfig): Promise<Connector> {
   let duckdb: any
   try {
@@ -14,17 +59,42 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
   }
 
   const dbPath = (config.path as string) ?? ":memory:"
+  // altimate_change start — configurable open budget
+  const { ms: openTimeoutMs, source: openTimeoutSource } = resolveOpenTimeoutMs(config)
+  // altimate_change end
   let db: any
   let connection: any
 
   // altimate_change start — improve DuckDB error messages
+  // Real DuckDB lock failures read "Could not set lock on file ... Conflicting
+  // lock is held", which contains "lock" but never "locked". Matching only
+  // "locked"/"DUCKDB_LOCKED" therefore missed every genuine lock collision, so
+  // the read-only retry never fired and concurrent readers just failed.
+  function isLockError(err: unknown): boolean {
+    const msg = (err instanceof Error ? err.message : String(err)).toLowerCase()
+    return (
+      msg.includes("locked") ||
+      // Both halves, not either. "could not set lock" on its own also covers
+      // non-contention failures — an unsupported filesystem lock, a permissions
+      // problem — and matching it alone would wrap those as "locked by another
+      // process", fabricating a wrapper that Registry.categorizeConnectionError
+      // then trusts as a recoverable `store_locked`. That would send the reader
+      // hunting for a process to close while hiding the real filesystem fault.
+      // Registry's raw matcher already requires both; these must agree.
+      (msg.includes("could not set lock") && msg.includes("conflicting lock")) ||
+      msg.includes("sqlite_busy")
+    )
+  }
+
   function wrapDuckDBError(err: Error): Error {
-    const msg = err.message || String(err)
-    if (msg.toLowerCase().includes("locked") || msg.includes("SQLITE_BUSY") || msg.includes("DUCKDB_LOCKED")) {
+    if (isLockError(err)) {
+      // Keep DuckDB's own text: it names the PID and executable holding the
+      // conflicting lock, which is the only way to find the other process.
       return new Error(
         `Database "${dbPath}" is locked by another process. ` +
-        `DuckDB does not support concurrent write access. ` +
-        `Close other connections to this file and try again.`,
+        `DuckDB takes an exclusive file lock, so a store already open ` +
+        `read-write elsewhere cannot be opened again — not even read-only. ` +
+        `Close the other connection and try again.\n${err.message || String(err)}`,
       )
     }
     return err
@@ -58,10 +128,15 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
           let timeout: ReturnType<typeof setTimeout> | undefined
           let instance: any
           // Sentinel for an open callback that fired synchronously (before
-          // `instance` was assigned): `undefined` = not yet fired, `null` =
-          // fired with success, `Error` = fired with failure. Replayed once
-          // `instance` exists.
-          let pendingOpen: Error | null | undefined
+          // `instance` was assigned), replayed once `instance` exists.
+          //
+          // This MUST be a value the callback can never supply. It used to be
+          // `undefined`, which is exactly what a success callback invoked with
+          // no arguments passes — so such a callback was recorded and then
+          // never replayed, the promise never settled, and the open failed on
+          // the deadline below with a timeout message that named nothing.
+          const NOT_FIRED = Symbol("duckdb-open-not-fired")
+          let pendingOpen: Error | null | typeof NOT_FIRED = NOT_FIRED
           const opts = accessMode ? { access_mode: accessMode } : undefined
           const closeQuietly = () => {
             try {
@@ -70,9 +145,12 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
               // best-effort cleanup of a half-open handle
             }
           }
-          const onOpen = (err: Error | null) => {
+          const onOpen = (err?: Error | null) => {
+            // Normalise a zero-argument success callback to `null` so it is
+            // never confused with "has not fired yet".
+            const outcome = err ?? null
             if (!instance) {
-              pendingOpen = err
+              pendingOpen = outcome
               return
             }
             if (resolved) {
@@ -81,38 +159,67 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
             }
             resolved = true
             if (timeout) clearTimeout(timeout)
-            if (err) {
+            if (outcome) {
               // Open failed — release the half-open handle so it doesn't leak.
+              // Reject with DuckDB's own error: callers classify it with
+              // isLockError(), and its text names the conflicting process.
               closeQuietly()
-              const msg = err.message || String(err)
-              if (msg.toLowerCase().includes("locked") || msg.includes("SQLITE_BUSY") || msg.includes("DUCKDB_LOCKED")) {
-                reject(new Error("DUCKDB_LOCKED"))
-              } else {
-                reject(err)
-              }
+              reject(outcome)
             } else {
               resolve(instance)
             }
           }
           instance = opts
             ? new duckdb.Database(dbPath, opts, onOpen)
             : new duckdb.Database(dbPath, onOpen)
-          // Bun: native callback may not fire; fall back after 2s. Arm the timer
-          // BEFORE replaying a synchronous callback so a sync resolve/reject can
-          // actually clear it (otherwise it lingers ~2s and delays process exit).
+          // Liveness guard against an open callback that never fires. Arm the
+          // timer BEFORE replaying a synchronous callback so a sync
+          // resolve/reject can actually clear it (otherwise it lingers and
+          // delays process exit).
           timeout = setTimeout(() => {
             if (!resolved) {
               resolved = true
-              reject(new Error(`Timed out opening DuckDB database "${dbPath}"`))
+              // Nothing can reach this handle once the promise rejects. If the
+              // callback arrives later it closes the handle itself (see the
+              // `resolved` branch in onOpen); if it never arrives, that branch
+              // never runs, so close here too. Both paths are idempotent
+              // because closeQuietly swallows a double close.
+              closeQuietly()
+              reject(
+                new Error(
+                  `DuckDB store "${dbPath}" did not finish opening within ${openTimeoutMs}ms. ` +
+                  `This is a client-side deadline in the DuckDB driver, not a fault in the store ` +
+                  `— the open may simply be queued behind other work in this process. ` +
+                  (openTimeoutSource === "connection"
+                    ? // Named so callers can tell a self-inflicted deadline from one
+                      // they did not choose. Registry.categorizeConnectionError keys
+                      // off this phrase to report it as configuration rather than as
+                      // a broken client.
+                      `This deadline was set on this connection as open_timeout_ms=${openTimeoutMs}; ` +
+                      `raise or remove it.`
+                    : `Raise the budget with this connection's open_timeout_ms (which takes ` +
+                      `priority) or ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS if the machine is loaded.`),
+                ),
+              )
             }
-          }, 2000)
-          if (pendingOpen !== undefined) onOpen(pendingOpen)
+          }, openTimeoutMs)
+          if (pendingOpen !== NOT_FIRED) onOpen(pendingOpen)
         })
 
+      // altimate_change start — honour an explicit read-only connection.
+      // DuckDB takes an EXCLUSIVE file lock when opened read-write, so N
+      // concurrent readers of one .duckdb file leave N-1 of them failing to
+      // connect at all. Opening READ_ONLY up front is the only way several
+      // processes can share a file, and it is what a caller that declared
+      // `readonly` asked for. Relying on the lock-error retry below is not
+      // equivalent: it is best-effort string matching, and it wastes a full
+      // open attempt per connection.
+      const wantReadOnly = config.readonly === true && dbPath !== ":memory:"
       try {
-        db = await tryConnect()
+        db = await tryConnect(wantReadOnly ? "READ_ONLY" : undefined)
       } catch (err: any) {
-        if (err.message === "DUCKDB_LOCKED" && dbPath !== ":memory:") {
+        // altimate_change end
+        if (isLockError(err) && !wantReadOnly && dbPath !== ":memory:") {
           // Retry in read-only mode — allows concurrent reads
           try {
             db = await tryConnect("READ_ONLY")
@@ -121,6 +228,17 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
               retryErr instanceof Error ? retryErr : new Error(String(retryErr)),
             )
           }
+        } else if (isLockError(err) && dbPath !== ":memory:") {
+          // An explicit read-only open is NOT rescued by the retry above — and
+          // must not be: DuckDB's file lock is exclusive against read-only
+          // opens too, so re-opening READ_ONLY when we already asked for
+          // READ_ONLY would only repeat the same failure. It still has to be
+          // wrapped, because categorizeConnectionError matches the wrapper's
+          // "locked by another process" wording; the raw DuckDB text would be
+          // reported as an unclassified failure. An in-memory store is excluded
+          // for the same reason the retry excludes it: no other process can
+          // hold it, so the wrapper's text would be false.
+          throw wrapDuckDBError(err instanceof Error ? err : new Error(String(err)))
         } else {
           throw err
         }