From 7c8f0983a36a8f031e94aae9f0a93574bd5437c0 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 29 Aug 2026 17:38:59 -0700 Subject: [PATCH 1/7] fix(drivers): let DuckDB stores be opened read-only, and detect real lock errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DuckDB takes an EXCLUSIVE file lock when opened read-write, so N concurrent readers of one .duckdb file leave N-1 unable to connect at all. Two bugs made that unrecoverable: - `config.readonly` was ignored entirely. The driver read only `config.path`, so a caller that declared a read-only connection still got a read-write open and took the exclusive lock. - The read-only retry was gated on `err.message === "DUCKDB_LOCKED"`, an exact match against a normaliser that only looked for "locked"/"SQLITE_BUSY". Real DuckDB lock failures read "Could not set lock on file ... Conflicting lock is held" — which contains "lock" but never "locked" — so the retry never fired on an actual lock collision. Honour `readonly` up front, and match the lock messages DuckDB really emits. Found by a benchmark pilot: 8 of 12 scored datasets use DuckDB, and under concurrency the agent could not open them, so it spent its turn budget running `npm install duckdb` trying to repair the environment instead of doing the task. 140 driver tests pass. (cherry picked from commit 9d05c9f37adebf503fdef50d5d46c258e5951e7b) --- packages/drivers/src/duckdb.ts | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index 867840d0a4..bcaa2c6ce7 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -18,9 +18,23 @@ export async function connect(config: ConnectionConfig): Promise { 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") || + msg.includes("conflicting lock") || + msg.includes("could not set lock") || + msg.includes("sqlite_busy") || + msg.includes("duckdb_locked") + ) + } + 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)) { return new Error( `Database "${dbPath}" is locked by another process. ` + `DuckDB does not support concurrent write access. ` + @@ -109,10 +123,20 @@ export async function connect(config: ConnectionConfig): Promise { if (pendingOpen !== undefined) 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") From e9466daadda5db36ed4bb3bac1e43ee781caaee8 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 29 Aug 2026 19:47:32 -0700 Subject: [PATCH 2/7] fix(drivers): stop a 2s deadline failing healthy DuckDB stores, and say so when the client is broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `warehouse_test` failed on local DuckDB stores with `Timed out opening DuckDB database ""` at 2002-2061ms, six of seven within 5ms of exactly 2000. Python opened the same 1.3MB store moments later and queried it fine, so the store was healthy and the driver's own deadline was the failure. The deadline was a hard-coded 2000ms with no way to raise it, and it was not a performance budget: DuckDB dispatches the open to the libuv threadpool, so the wait covers queueing behind every other threadpool user in the process. On a loaded machine a healthy open crosses it, and the driver then rejects and closes the handle that was about to succeed. - Default the budget to 30s, overridable per connection (`open_timeout_ms`) or by `ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS`. - Fix a live path to the same symptom: the "callback fired synchronously" sentinel was `undefined`, which is exactly what a success callback invoked with no arguments passes, so such a callback was recorded and never replayed and the open hung until the deadline. The sentinel is now a symbol. - Say what actually happened. The old text named the store and read as "this file is broken", which sent investigators after the file for hours. Also, when the client is broken, stop it looking like a bad connection. `warehouse_test` reported "Connection 'x': FAILED" for a driver that will not load and for a wrong password alike. It now classifies local-client faults (`driver_missing`, `driver_open_timeout`, `store_locked`) and renders them as an unmistakable INFRASTRUCTURE FAILURE with `infrastructure: true` in metadata, so neither a model nor a reader of a transcript can score broken infrastructure as a task failure. Two more real faults found on the way: - `duckdb` was missing from `trustedDependencies`, so `bun install` never ran its `node-pre-gyp install` and `lib/binding/duckdb.node` was never fetched. Every DuckDB call on such a tree fails with "driver not installed" even though the package is present. Verified both directions in a scratch install. - `wrapDuckDBError` replaced DuckDB's lock message with a friendly summary, discarding the one actionable part — DuckDB names the PID and executable holding the conflicting lock. It now appends rather than replaces, and `onOpen` no longer flattens the error to a bare `DUCKDB_LOCKED`. Tests open a real store on disk; a mock is what missed this. They need their own `bun test` process, because 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 a whole-directory run silently exercises a fake. Under `ALTIMATE_DUCKDB_E2E=1` a missing or mocked driver fails the run rather than skipping it, and CI gets a dedicated step. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .github/workflows/ci.yml | 12 ++ package.json | 1 + packages/drivers/src/duckdb.ts | 89 +++++++--- packages/drivers/test/driver-security.test.ts | 5 +- .../altimate/native/connections/registry.ts | 33 +++- .../opencode/src/altimate/native/types.ts | 11 ++ .../src/altimate/tools/warehouse-test.ts | 28 ++- .../test/altimate/duckdb-open-e2e.test.ts | 163 ++++++++++++++++++ .../warehouse-test-duckdb-e2e.test.ts | 122 +++++++++++++ 9 files changed, 439 insertions(+), 25 deletions(-) create mode 100644 packages/opencode/test/altimate/duckdb-open-e2e.test.ts create mode 100644 packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8826a5579e..62e9193b29 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -294,6 +294,18 @@ 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. + - name: Run DuckDB store-open E2E (real store, no mocks) + run: bun test 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 diff --git a/package.json b/package.json index 74609917a4..18b54a34ee 100644 --- a/package.json +++ b/package.json @@ -126,6 +126,7 @@ "printWidth": 120 }, "trustedDependencies": [ + "duckdb", "esbuild", "node-pty", "protobufjs", diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index bcaa2c6ce7..e2646e162a 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -4,6 +4,34 @@ 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 + +/** 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 ? n : undefined +} + +function resolveOpenTimeoutMs(config: ConnectionConfig): number { + return ( + positiveMs(config.open_timeout_ms) ?? + positiveMs(globalThis.process?.env?.["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"]) ?? + DEFAULT_OPEN_TIMEOUT_MS + ) +} +// altimate_change end + export async function connect(config: ConnectionConfig): Promise { let duckdb: any try { @@ -14,6 +42,9 @@ export async function connect(config: ConnectionConfig): Promise { } const dbPath = (config.path as string) ?? ":memory:" + // altimate_change start — configurable open budget + const openTimeoutMs = resolveOpenTimeoutMs(config) + // altimate_change end let db: any let connection: any @@ -35,10 +66,13 @@ export async function connect(config: ConnectionConfig): Promise { function wrapDuckDBError(err: Error): Error { 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 @@ -72,10 +106,15 @@ export async function connect(config: ConnectionConfig): Promise { let timeout: ReturnType | 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 { @@ -84,9 +123,12 @@ export async function connect(config: ConnectionConfig): Promise { // 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) { @@ -95,15 +137,12 @@ export async function connect(config: ConnectionConfig): Promise { } 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) } @@ -111,16 +150,24 @@ export async function connect(config: ConnectionConfig): Promise { 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}"`)) + 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. ` + + `Raise it with 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. diff --git a/packages/drivers/test/driver-security.test.ts b/packages/drivers/test/driver-security.test.ts index 6a0f8d8c5d..34e34e45d6 100644 --- a/packages/drivers/test/driver-security.test.ts +++ b/packages/drivers/test/driver-security.test.ts @@ -54,7 +54,10 @@ describe("DuckDB driver", () => { expect.unreachable("Should have thrown") } catch (e: any) { expect(e.message).toContain("locked by another process") - expect(e.message).toContain("does not support concurrent write access") + expect(e.message).toContain("exclusive file lock") + // DuckDB's own text names the PID holding the lock, so it must survive + // being wrapped rather than being replaced by the friendly summary. + expect(e.message).toContain("SQLITE_BUSY: database is locked") } await connector.close() diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 8be3bfc672..366215e1ef 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -282,6 +282,12 @@ export function detectAuthMethod(config: ConnectionConfig | null | undefined): s export function categorizeConnectionError(e: unknown): string { const msg = String(e).toLowerCase() if (msg.includes("not installed") || msg.includes("cannot find module")) return "driver_missing" + // altimate_change start — categories for local-client faults + // Checked before the generic "timeout"/"not found" rules below, which are + // about the remote warehouse and would otherwise swallow these. + if (msg.includes("did not finish opening")) return "driver_open_timeout" + if (msg.includes("locked by another process")) return "store_locked" + // altimate_change end if (msg.includes("password") || msg.includes("authentication") || msg.includes("unauthorized") || msg.includes("jwt")) return "auth_failed" if (msg.includes("timeout") || msg.includes("timed out")) return "timeout" @@ -290,6 +296,19 @@ export function categorizeConnectionError(e: unknown): string { return "other" } +// altimate_change start — distinguish infrastructure faults from config faults +/** + * Categories where the local client is broken, not the connection's config and + * not the remote warehouse. A caller cannot fix these by correcting + * credentials, and a harness must not score them as a task failure. + */ +const INFRASTRUCTURE_CATEGORIES = new Set(["driver_missing", "driver_open_timeout", "store_locked"]) + +export function isInfrastructureFailure(category: string): boolean { + return INFRASTRUCTURE_CATEGORIES.has(category) +} +// altimate_change end + // --------------------------------------------------------------------------- // Public API // --------------------------------------------------------------------------- @@ -405,7 +424,9 @@ export function list(): { warehouses: WarehouseInfo[] } { } /** Test a connection by running a simple query. */ -export async function test(name: string): Promise<{ connected: boolean; error?: string }> { +export async function test( + name: string, +): Promise<{ connected: boolean; error?: string; error_category?: string; infrastructure?: boolean }> { try { const connector = await get(name) const config = configs.get(name) @@ -422,7 +443,15 @@ export async function test(name: string): Promise<{ connected: boolean; error?: } return { connected: true } } catch (e) { - return { connected: false, error: String(e) } + // altimate_change start — report *why* the test failed, not just that it did + const category = categorizeConnectionError(e) + return { + connected: false, + error: String(e), + error_category: category, + infrastructure: isInfrastructureFailure(category), + } + // altimate_change end } } diff --git a/packages/opencode/src/altimate/native/types.ts b/packages/opencode/src/altimate/native/types.ts index ad90c7631e..e96c5ccc5c 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -329,6 +329,17 @@ export interface WarehouseTestParams { export interface WarehouseTestResult { connected: boolean error?: string + // altimate_change start — distinguish infrastructure faults from config faults + /** Coarse cause, from `categorizeConnectionError` (e.g. `driver_missing`, `auth_failed`). */ + error_category?: string + /** + * True when the failure is in the local client — a driver that will not load, + * an open that never completed — rather than in the connection's + * configuration or the remote warehouse. These are not the caller's fault and + * are not fixable by editing the connection. + */ + infrastructure?: boolean + // altimate_change end } // --- Warehouse Management --- diff --git a/packages/opencode/src/altimate/tools/warehouse-test.ts b/packages/opencode/src/altimate/tools/warehouse-test.ts index e05e45b3d8..17999d9552 100644 --- a/packages/opencode/src/altimate/tools/warehouse-test.ts +++ b/packages/opencode/src/altimate/tools/warehouse-test.ts @@ -20,9 +20,35 @@ export const WarehouseTestTool = Tool.define("warehouse_test", { } } + // altimate_change start — never let a broken client look like a bad connection + // A driver that will not load, or an open that never completed, is a + // fault in this machine's install. Reporting it with the same wording as + // a wrong password invites both the model and anyone reading the + // transcript to treat broken infrastructure as a task or config failure. + if (result.infrastructure) { + return { + title: `Connection '${args.name}': INFRASTRUCTURE FAILURE`, + metadata: { + connected: false, + error: result.error, + error_category: result.error_category, + infrastructure: true, + }, + output: + `INFRASTRUCTURE FAILURE — the warehouse client on this machine is broken. ` + + `This is NOT a problem with the connection's configuration, and NOT something ` + + `to work around by trying a different query or a different tool.\n` + + `Category: ${result.error_category}\n` + + `Error: ${result.error ?? "Unknown error"}\n\n` + + `Stop and report this rather than continuing — results produced after this ` + + `point did not come from warehouse '${args.name}'.`, + } + } + // altimate_change end + return { title: `Connection '${args.name}': FAILED`, - metadata: { connected: false }, + metadata: { connected: false, error: result.error, error_category: result.error_category }, output: `Failed to connect to warehouse '${args.name}'.\nError: ${result.error ?? "Unknown error"}`, } } catch (e) { diff --git a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts new file mode 100644 index 0000000000..3d89c496ce --- /dev/null +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -0,0 +1,163 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" + +import { connect } from "../../../drivers/src/duckdb" + +// These tests open a real DuckDB store on disk. They are pointless against a +// mock: every bug they cover lives in the native open callback or in the +// driver's own deadline, neither of which a fake exposes. +// +// They need their own `bun test` process. Four files in this directory 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 the driver is already replaced by a fake no matter how the files sort and +// regardless of which specifier this file imports. `mock.restore()` does not +// undo a module mock. Hence the gate below, and the dedicated CI step. +// +// The gate is opt-IN so the shared suite stays green, but when it is on a +// missing or faked driver is a hard failure, never a skip. A whole e2e file +// that quietly reports success while skipping is the same class of defect this +// PR exists to fix. +const RUN = process.env["ALTIMATE_DUCKDB_E2E"] === "1" +const ddbTest = RUN ? test : test.skip + +let dir = "" +let storePath = "" + +/** Run `fn` and return the error message it threw, or "" if it succeeded. */ +async function messageFrom(fn: () => Promise): Promise { + try { + await fn() + return "" + } catch (e) { + return e instanceof Error ? e.message : String(e) + } +} + +describe("DuckDB driver: opening a real store", () => { + beforeAll(async () => { + if (!RUN) return + dir = fs.mkdtempSync(path.join(os.tmpdir(), "duckdb-open-")) + storePath = path.join(dir, "warehouse.duckdb") + const c = await connect({ type: "duckdb", path: storePath }) + await c.connect() + await c.execute("CREATE TABLE t AS SELECT 1 AS a, 'x' AS b") + const probe = await c.execute("SELECT count(*) AS n FROM t") + await c.close() + // Refuse to run against anything but the real thing. A fake driver would + // pass most assertions below while proving nothing. + if (Number(probe.rows?.[0]?.[0]) !== 1) { + throw new Error( + "ALTIMATE_DUCKDB_E2E=1 but the DuckDB driver is not the real one — " + + "either the native binding is missing, or another test file has replaced " + + "the module with a mock. Run this file in its own `bun test` process.", + ) + } + }) + + afterAll(() => { + if (dir) fs.rmSync(dir, { recursive: true, force: true }) + }) + + ddbTest("opens the same store repeatedly without flaking", async () => { + // The failure this replaces was reported as 7 of 7 calls failing, so a + // single green open proves nothing. Repeat enough to catch an + // intermittent regression. + for (let i = 0; i < 20; i++) { + const c = await connect({ type: "duckdb", path: storePath }) + await c.connect() + const r = await c.execute("SELECT count(*) AS n FROM t") + expect(Number(r.rows[0][0])).toBe(1) + await c.close() + } + }) + + ddbTest("opens the same store from many connectors at once", async () => { + const results = await Promise.all( + Array.from({ length: 8 }, async () => { + const c = await connect({ type: "duckdb", path: storePath }) + await c.connect() + const r = await c.execute("SELECT count(*) AS n FROM t") + await c.close() + return Number(r.rows[0][0]) + }), + ) + expect(results).toEqual(Array(8).fill(1)) + }) + + ddbTest("honours config.readonly by actually opening READ_ONLY", async () => { + const c = await connect({ type: "duckdb", path: storePath, readonly: true }) + await c.connect() + // Reads work. + const r = await c.execute("SELECT count(*) AS n FROM t") + expect(Number(r.rows[0][0])).toBe(1) + // Writes do not — which is what proves READ_ONLY reached DuckDB rather + // than `readonly` being silently dropped, as it was before. + expect(await messageFrom(() => c.execute("CREATE TABLE writeme (x INTEGER)"))).not.toBe("") + await c.close() + }) + + ddbTest("a store that opens fine is not failed by the open deadline", async () => { + // Same file, same process, two budgets. An unreachably small budget must + // fail; the default must succeed. That is the whole mechanism behind the + // field failure: the deadline fired, not the store. + const tooShort = await connect({ type: "duckdb", path: storePath, open_timeout_ms: 1 }) + expect(await messageFrom(() => tooShort.connect())).toContain("did not finish opening within 1ms") + + const normal = await connect({ type: "duckdb", path: storePath }) + await normal.connect() + const r = await normal.execute("SELECT count(*) AS n FROM t") + expect(Number(r.rows[0][0])).toBe(1) + await normal.close() + }) + + ddbTest("the open deadline names itself as a client-side deadline", async () => { + const c = await connect({ type: "duckdb", path: storePath, open_timeout_ms: 1 }) + const message = await messageFrom(() => c.connect()) + // The old message was `Timed out opening DuckDB database ""`, which + // reads as "the store is broken" and sent investigators after the file. + expect(message).toContain("not a fault in the store") + expect(message).toContain("ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS") + }) + + ddbTest("the open deadline is tunable by environment variable", async () => { + const prev = process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] + process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = "1" + try { + const c = await connect({ type: "duckdb", path: storePath }) + expect(await messageFrom(() => c.connect())).toContain("did not finish opening within 1ms") + } finally { + if (prev === undefined) delete process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] + else process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = prev + } + }) + + ddbTest("an unusable timeout value falls back to the default budget", async () => { + for (const bad of [0, -1, Number.NaN, "abc"]) { + const c = await connect({ type: "duckdb", path: storePath, open_timeout_ms: bad }) + await c.connect() + await c.close() + } + }) + + ddbTest("reports a lock conflict as a lock conflict, keeping DuckDB's detail", async () => { + // The verbatim message DuckDB emits when another process holds the file. + // It contains "lock" but never "locked", which is why the driver's old + // `.includes("locked")` check never matched a real collision. + const real = + 'IO Error: Could not set lock on file "/tmp/warehouse.duckdb": Conflicting lock is held in ' + + "/usr/bin/node (PID 65001) by user someone. See also https://duckdb.org/docs/stable/connect/concurrency" + expect(real.toLowerCase().includes("locked")).toBe(false) + + // Drive it through the driver by pointing at a directory, which fails the + // open for an unrelated reason, to confirm non-lock errors pass through + // untouched rather than being mislabelled as a lock. + const c = await connect({ type: "duckdb", path: dir }) + const message = await messageFrom(() => c.connect()) + expect(message).not.toBe("") + expect(message).not.toContain("is locked by another process") + expect(message).not.toContain("did not finish opening") + }) +}) diff --git a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts new file mode 100644 index 0000000000..c8c76815e4 --- /dev/null +++ b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts @@ -0,0 +1,122 @@ +import { afterAll, afterEach, beforeAll, describe, expect, test } from "bun:test" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" + +import { Dispatcher } from "../../src/altimate/native" +import * as Registry from "../../src/altimate/native/connections/registry" +import { WarehouseTestTool } from "../../src/altimate/tools/warehouse-test" +import { initTool } from "./tool-fixture" + +// End-to-end coverage for the `warehouse_test` tool against a real DuckDB file +// on disk, through the same `Dispatcher.call("warehouse.test", …)` the tool +// itself makes. A mock connector would not have caught the bug this replaces — +// it lived in the native open path — so nothing here is faked. +// +// Needs its own `bun test` process, and refuses to run against a fake. See the +// long note in `duckdb-open-e2e.test.ts` for why. +const RUN = process.env["ALTIMATE_DUCKDB_E2E"] === "1" +const ddbTest = RUN ? test : test.skip + +let dir = "" +let storePath = "" + +async function warehouseTest(name: string) { + return Dispatcher.call("warehouse.test", { name }) +} + +describe("warehouse.test against a real DuckDB store", () => { + beforeAll(async () => { + process.env["ALTIMATE_TELEMETRY_DISABLED"] = "true" + if (!RUN) return + dir = fs.mkdtempSync(path.join(os.tmpdir(), "warehouse-test-")) + storePath = path.join(dir, "warehouse.duckdb") + Registry.reset() + Registry.setConfigs({ seed: { type: "duckdb", path: storePath } }) + const c = await Registry.get("seed") + await c.execute("CREATE TABLE t AS SELECT 1 AS a") + const probe = await c.execute("SELECT count(*) AS n FROM t") + Registry.reset() + // Refuse to run against anything but the real driver — see above. + if (Number(probe.rows?.[0]?.[0]) !== 1) { + throw new Error( + "ALTIMATE_DUCKDB_E2E=1 but the DuckDB driver is not the real one — " + + "either the native binding is missing, or another test file has replaced " + + "the module with a mock. Run this file in its own `bun test` process.", + ) + } + }) + + afterEach(() => { + Registry.reset() + }) + + afterAll(() => { + delete process.env["ALTIMATE_TELEMETRY_DISABLED"] + Registry.reset() + if (dir) fs.rmSync(dir, { recursive: true, force: true }) + }) + + ddbTest("connects on every one of 10 consecutive fresh registries", async () => { + // The failure this replaces was 7 of 7 calls failing, and a fix that is + // merely usually right would recreate exactly that contamination. Each + // iteration resets the registry so nothing is served from cache. + for (let i = 0; i < 10; i++) { + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const result = await warehouseTest("local") + expect(result.connected).toBe(true) + expect(result.error).toBeUndefined() + } + }) + + ddbTest("reports a broken client as an infrastructure failure, not a failed connection", async () => { + Registry.reset() + // An unreachably small open budget stands in for any local fault that + // stops the store opening. The point of the assertion is the *shape* of + // the report: a caller must be able to tell this apart from a bad + // password without parsing prose. + Registry.setConfigs({ local: { type: "duckdb", path: storePath, open_timeout_ms: 1 } }) + const result = await warehouseTest("local") + expect(result.connected).toBe(false) + expect(result.infrastructure).toBe(true) + expect(result.error_category).toBe("driver_open_timeout") + }) + + ddbTest("does not mark a genuine configuration mistake as infrastructure", async () => { + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const result = await warehouseTest("nonexistent") + expect(result.connected).toBe(false) + expect(result.infrastructure).toBe(false) + }) + + ddbTest("a healthy store is unaffected by the deadline that failed the previous case", async () => { + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const result = await warehouseTest("local") + expect(result.connected).toBe(true) + }) + + ddbTest("the warehouse_test tool renders infrastructure faults unmistakably", async () => { + const tool = await initTool(WarehouseTestTool) + const ctx = { sessionID: "s", messageID: "m", agent: "build", abort: new AbortController().signal, messages: [] } + + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const ok = await tool.execute({ name: "local" }, ctx) + expect(ok.title).toContain("OK") + + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath, open_timeout_ms: 1 } }) + const broken = await tool.execute({ name: "local" }, ctx) + // The whole point: a reader of this transcript — human or model — must not + // be able to mistake a broken client for the model getting something + // wrong, or for a connection that needs its credentials fixed. + expect(broken.title).toContain("INFRASTRUCTURE FAILURE") + expect(broken.output).toContain("INFRASTRUCTURE FAILURE") + expect(broken.output).toContain("NOT a problem with the connection's configuration") + expect(broken.metadata.infrastructure).toBe(true) + expect(broken.metadata.error_category).toBe("driver_open_timeout") + }) +}) From e0d5decd1011bda9f0b5435baf9c15dbe0640bd7 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 00:50:10 -0700 Subject: [PATCH 3/7] fix(drivers): classify a locked DuckDB store, and stop treating a lock as a broken install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on #1198. Fifteen threads, four of them the same defect. **A lock on an explicitly read-only open was reported as an unclassified failure.** `connect()` only wrapped a lock error on the retry path, and an explicit `config.readonly` open never reaches that path — correctly, since DuckDB's file lock is exclusive against read-only opens too, so retrying `READ_ONLY` when we already asked for `READ_ONLY` would just repeat the same failure. But the `else` rethrew DuckDB's raw text, and `categorizeConnectionError` matches only the wrapper's `locked by another process` wording, so a plain lock collision came out as `other` — the same shape as a wrong password. Now wrapped (still not retried). `:memory:` is excluded for the same reason the retry excludes it: no other process can hold it, so the wrapper's text would be false. **`categorizeConnectionError` now also matches DuckDB's raw text** — `conflicting lock` and `could not set lock` — so a lock that reaches it unwrapped from any other path is still classified. **A locked store is no longer rendered as a broken client.** It stays `infrastructure` (nothing about the connection's config is wrong, and a harness must not score it as a task failure) but is now also `recoverable`, and `warehouse_test` renders it as `STORE LOCKED` with the remedy the driver's own message already gives — close the conflicting connection and retry — instead of the stop-and-report copy meant for an install that will never work. **Also fixed** - `setTimeout` clamps a delay past 2^31-1 to about 1ms, so a deliberately huge `open_timeout_ms` became an immediate deadline — the exact inversion this change exists to prevent. Clamped before it reaches the timer. - The deadline message named only `ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS`, which `config.open_timeout_ms` overrides. It now names both, and which one wins. - A deadline that fires because the open callback never arrives left the native handle open. When the callback does arrive late, `onOpen` closes it; when it never arrives that branch never runs, so the timeout closes it too. Both paths are idempotent. - Dropped a dead `msg.includes("duckdb_locked")` clause: `msg` is lowercased, so `locked` already matches it. - The `drivers` CI path filter did not list the two DuckDB E2E files its own step runs, so a PR touching only those files got no execution anywhere — the driver job skipped, and the main job runs them with the gate unset. - `Registry.reset()` is synchronous and `close()` is not, so resetting leaked a native handle per connector. Added `Registry.closeAll()`, used by `reload()` and by the E2E suite. - The E2E suite deleted `ALTIMATE_TELEMETRY_DISABLED` on teardown instead of restoring what it found. - `bun.lock` did not record the `trustedDependencies` addition. **Tests** - `driver-security.test.ts`: an explicitly read-only open that hits a lock is wrapped, and attempted exactly once with `READ_ONLY`. Fails without the fix. - `connections.test.ts`: eight cases pinning the classification — DuckDB's verbatim lock text (which contains `lock` but never `locked`), the wrapper's text, the open deadline, and which categories are infrastructure vs recoverable. These run in the normal suite, ungated. - `duckdb-lock-helper.ts`: holds a write lock from a separate OS process. In-process is not a test — DuckDB's lock is per-process. - E2E: read-only and read-write opens against a foreign lock both report a lock and keep DuckDB's PID detail; the store is usable once the lock is released (which guards the helper itself); an over-large budget does not become 1ms; `warehouse_test` reports `store_locked` + recoverable and renders `STORE LOCKED` rather than stop-and-report. Gates: typecheck 13/13 (it caught a real error in the new helper, so it does cover `packages/opencode/test`); markers ok against `origin/main`; drivers 141 pass 0 fail; `test/altimate` 4222 pass 0 fail; gated DuckDB E2E 19 pass 0 fail. Lint is 5863 warnings / 1 error: the error is the pre-existing `consistent-return` in `packages/http-recorder/test/record-replay.test.ts`, and the warning count is +1 from an un-awaited `mock.module` in the new test, which is how every other `mock.module` in that file is written. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .github/workflows/ci.yml | 8 ++ bun.lock | 1 + packages/drivers/src/duckdb.ts | 34 ++++++- packages/drivers/test/driver-security.test.ts | 52 ++++++++++ .../altimate/native/connections/registry.ts | 55 +++++++++-- .../opencode/src/altimate/native/types.ts | 6 ++ .../src/altimate/tools/warehouse-test.ts | 27 ++++++ .../test/altimate/connections.test.ts | 58 +++++++++++- .../test/altimate/duckdb-lock-helper.ts | 94 +++++++++++++++++++ .../test/altimate/duckdb-open-e2e.test.ts | 58 ++++++++++++ .../warehouse-test-duckdb-e2e.test.ts | 62 +++++++++++- 11 files changed, 440 insertions(+), 15 deletions(-) create mode 100644 packages/opencode/test/altimate/duckdb-lock-helper.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62e9193b29..e24440595d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,14 @@ 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' dbt-tools: - 'packages/dbt-tools/**' installer: diff --git a/bun.lock b/bun.lock index 75cd20c40e..00991dd350 100644 --- a/bun.lock +++ b/bun.lock @@ -490,6 +490,7 @@ "tree-sitter-powershell", "protobufjs", "web-tree-sitter", + "duckdb", "tree-sitter-bash", ], "patchedDependencies": { diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index e2646e162a..c376c3f3a1 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -17,10 +17,19 @@ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, Sche */ 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 ? n : undefined + return Number.isFinite(n) && n > 0 ? Math.min(n, MAX_TIMER_MS) : undefined } function resolveOpenTimeoutMs(config: ConnectionConfig): number { @@ -59,8 +68,7 @@ export async function connect(config: ConnectionConfig): Promise { msg.includes("locked") || msg.includes("conflicting lock") || msg.includes("could not set lock") || - msg.includes("sqlite_busy") || - msg.includes("duckdb_locked") + msg.includes("sqlite_busy") ) } @@ -157,12 +165,19 @@ export async function connect(config: ConnectionConfig): Promise { timeout = setTimeout(() => { if (!resolved) { resolved = true + // 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. ` + - `Raise it with ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS if the machine is loaded.`, + `Raise the budget with this connection's open_timeout_ms (which takes ` + + `priority) or ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS if the machine is loaded.`, ), ) } @@ -192,6 +207,17 @@ export async function connect(config: ConnectionConfig): Promise { 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 } diff --git a/packages/drivers/test/driver-security.test.ts b/packages/drivers/test/driver-security.test.ts index 34e34e45d6..0d80a8d93c 100644 --- a/packages/drivers/test/driver-security.test.ts +++ b/packages/drivers/test/driver-security.test.ts @@ -136,6 +136,13 @@ describe("DuckDB driver", () => { }) }) + // Verbatim DuckDB output for a cross-process lock collision. It contains + // "lock" but never "locked", which is why the driver's original + // `.includes("locked")` check never matched a real collision. + const REAL_DUCKDB_LOCK_ERROR = + 'IO Error: Could not set lock on file "/tmp/test.duckdb": Conflicting lock is held in ' + + "/usr/bin/node (PID 65001) by user someone." + describe("connect retry with READ_ONLY", () => { test("retries with READ_ONLY when file DB is locked on initial connect", async () => { let connectAttempts = 0 @@ -182,6 +189,51 @@ describe("DuckDB driver", () => { await connector.close() }) + test("wraps a lock error on an explicitly read-only open, and does not retry it", async () => { + // A caller that set `readonly` already gets READ_ONLY on the first open, + // so there is nothing for the retry to try differently — DuckDB's file + // lock is exclusive against read-only opens too. The failure must still + // be *labelled* a lock, though: consumers classify it by the wrapper's + // "locked by another process" wording, and this branch used to rethrow + // DuckDB's raw text, which contains "lock" but never "locked". + let attempts = 0 + const accessModes: Array = [] + mock.module("duckdb", () => ({ + default: { + Database: class { + constructor(_path: string, optsOrCb: any, cb?: (err: Error | null) => void) { + const opts = typeof optsOrCb === "function" ? undefined : optsOrCb + accessModes.push(opts?.access_mode) + attempts++ + setTimeout(() => openCallback(optsOrCb, cb)(new Error(REAL_DUCKDB_LOCK_ERROR)), 0) + } + connect() { + return {} + } + close(cb: any) { + if (cb) cb(null) + } + }, + }, + })) + + const { connect } = await import("../src/duckdb") + const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", readonly: true }) + + try { + await connector.connect() + expect.unreachable("Should have thrown") + } catch (e: any) { + expect(e.message).toContain("locked by another process") + // DuckDB's own text survives: it names the PID holding the lock. + expect(e.message).toContain("Conflicting lock is held") + } + + // Exactly one attempt, and it asked for READ_ONLY up front. + expect(attempts).toBe(1) + expect(accessModes).toEqual(["READ_ONLY"]) + }) + test("does not retry in-memory DB on lock error", async () => { mock.module("duckdb", () => ({ default: { diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 366215e1ef..3461d1e3b4 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -286,7 +286,16 @@ export function categorizeConnectionError(e: unknown): string { // Checked before the generic "timeout"/"not found" rules below, which are // about the remote warehouse and would otherwise swallow these. if (msg.includes("did not finish opening")) return "driver_open_timeout" - if (msg.includes("locked by another process")) return "store_locked" + // "locked by another process" is the DuckDB driver's own wrapper. The other + // two are DuckDB's raw text ("Could not set lock on file …: Conflicting lock + // is held"), matched here as well so a lock that reaches this function + // unwrapped is still classified rather than falling through to "other". + if ( + msg.includes("locked by another process") || + msg.includes("conflicting lock") || + msg.includes("could not set lock") + ) + return "store_locked" // altimate_change end if (msg.includes("password") || msg.includes("authentication") || msg.includes("unauthorized") || msg.includes("jwt")) return "auth_failed" @@ -298,15 +307,27 @@ export function categorizeConnectionError(e: unknown): string { // altimate_change start — distinguish infrastructure faults from config faults /** - * Categories where the local client is broken, not the connection's config and - * not the remote warehouse. A caller cannot fix these by correcting + * Categories where the fault is in this machine, not the connection's config + * and not the remote warehouse. A caller cannot fix these by correcting * credentials, and a harness must not score them as a task failure. */ const INFRASTRUCTURE_CATEGORIES = new Set(["driver_missing", "driver_open_timeout", "store_locked"]) +/** + * The subset of the above that clears on its own once another process lets go. + * Still infrastructure — nothing about the connection's config is wrong — but + * the right response is to close the conflicting connection and retry, not to + * stop and report a broken install. + */ +const RECOVERABLE_CATEGORIES = new Set(["store_locked"]) + export function isInfrastructureFailure(category: string): boolean { return INFRASTRUCTURE_CATEGORIES.has(category) } + +export function isRecoverableFailure(category: string): boolean { + return RECOVERABLE_CATEGORIES.has(category) +} // altimate_change end // --------------------------------------------------------------------------- @@ -426,7 +447,13 @@ export function list(): { warehouses: WarehouseInfo[] } { /** Test a connection by running a simple query. */ export async function test( name: string, -): Promise<{ connected: boolean; error?: string; error_category?: string; infrastructure?: boolean }> { +): Promise<{ + connected: boolean + error?: string + error_category?: string + infrastructure?: boolean + recoverable?: boolean +}> { try { const connector = await get(name) const config = configs.get(name) @@ -450,6 +477,7 @@ export async function test( error: String(e), error_category: category, infrastructure: isInfrastructureFailure(category), + recoverable: isRecoverableFailure(category), } // altimate_change end } @@ -545,9 +573,16 @@ export async function remove(name: string): Promise<{ success: boolean; error?: } } -/** Reload all configs and clear cached connectors. */ -export async function reload(): Promise { - // Close all cached connectors +// altimate_change start — a way to release native handles without reloading +/** + * Close every cached connector and drop it from the cache. + * + * `reset()` cannot do this: it is synchronous, and `close()` is not. Callers + * that create real connectors (tests, and `reload()` below) must await this, + * otherwise the native handle behind each connector stays open — on Windows + * that also blocks deleting the file underneath it. + */ +export async function closeAll(): Promise { for (const [, connector] of connectors) { try { await connector.close() @@ -556,6 +591,12 @@ export async function reload(): Promise { } } connectors.clear() +} +// altimate_change end + +/** Reload all configs and clear cached connectors. */ +export async function reload(): Promise { + await closeAll() loaded = false load() } diff --git a/packages/opencode/src/altimate/native/types.ts b/packages/opencode/src/altimate/native/types.ts index e96c5ccc5c..f69f72e712 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -339,6 +339,12 @@ export interface WarehouseTestResult { * are not fixable by editing the connection. */ infrastructure?: boolean + /** + * True when the failure clears on its own once another process lets go — a + * store locked by another writer. Still `infrastructure`, but the response is + * to close the conflicting connection and retry, not to stop and report. + */ + recoverable?: boolean // altimate_change end } diff --git a/packages/opencode/src/altimate/tools/warehouse-test.ts b/packages/opencode/src/altimate/tools/warehouse-test.ts index 17999d9552..aa1d7d3ab3 100644 --- a/packages/opencode/src/altimate/tools/warehouse-test.ts +++ b/packages/opencode/src/altimate/tools/warehouse-test.ts @@ -25,6 +25,33 @@ export const WarehouseTestTool = Tool.define("warehouse_test", { // fault in this machine's install. Reporting it with the same wording as // a wrong password invites both the model and anyone reading the // transcript to treat broken infrastructure as a task or config failure. + // A locked store is infrastructure too — nothing about the connection's + // config is wrong — but it is the one category that clears itself once + // the other process lets go, and the driver's own message names the PID + // holding the lock. Telling the model to stop and report would be wrong + // advice for the one fault it can actually act on. + if (result.recoverable) { + return { + title: `Connection '${args.name}': STORE LOCKED`, + metadata: { + connected: false, + error: result.error, + error_category: result.error_category, + infrastructure: true, + recoverable: true, + }, + output: + `STORE LOCKED — another process holds this store open, so it could not be ` + + `opened here. This is NOT a problem with the connection's configuration and ` + + `NOT a credentials failure.\n` + + `Category: ${result.error_category}\n` + + `Error: ${result.error ?? "Unknown error"}\n\n` + + `The error above names the process holding the lock. Close that connection ` + + `and try again. Do not work around this by querying something else — until ` + + `the lock clears, no result here comes from warehouse '${args.name}'.`, + } + } + if (result.infrastructure) { return { title: `Connection '${args.name}': INFRASTRUCTURE FAILURE`, diff --git a/packages/opencode/test/altimate/connections.test.ts b/packages/opencode/test/altimate/connections.test.ts index 5b52972eee..d3e030eecf 100644 --- a/packages/opencode/test/altimate/connections.test.ts +++ b/packages/opencode/test/altimate/connections.test.ts @@ -10,7 +10,12 @@ afterAll(() => { delete process.env.ALTIMATE_TELEMETRY_DISABLED }) // --------------------------------------------------------------------------- import * as Registry from "../../src/altimate/native/connections/registry" -import { detectAuthMethod } from "../../src/altimate/native/connections/registry" +import { + categorizeConnectionError, + detectAuthMethod, + isInfrastructureFailure, + isRecoverableFailure, +} from "../../src/altimate/native/connections/registry" import * as CredentialStore from "../../src/altimate/native/connections/credential-store" import { parseDbtProfiles, dbtConnectionsToConfigs } from "../../src/altimate/native/connections/dbt-profiles" import { discoverContainers, containerToConfig } from "../../src/altimate/native/connections/docker-discovery" @@ -20,6 +25,57 @@ import { registerAll } from "../../src/altimate/native/connections/register" // ConnectionRegistry // --------------------------------------------------------------------------- +// altimate_change start — classification of local-client faults +describe("categorizeConnectionError: local-client faults", () => { + // Verbatim DuckDB output. It contains "lock" but never "locked", which is + // why matching only the driver's own wrapper wording missed it. + const RAW_DUCKDB_LOCK = + 'IO Error: Could not set lock on file "/tmp/warehouse.duckdb": Conflicting lock is held in ' + + "/usr/bin/node (PID 65001) by user someone. See also https://duckdb.org/docs/stable/connect/concurrency" + + test("the raw DuckDB lock message never contains the substring 'locked'", () => { + expect(RAW_DUCKDB_LOCK.toLowerCase().includes("locked")).toBe(false) + }) + + test("classifies the raw DuckDB lock message as store_locked", () => { + expect(categorizeConnectionError(new Error(RAW_DUCKDB_LOCK))).toBe("store_locked") + }) + + test("classifies the driver's wrapped lock message as store_locked", () => { + const wrapped = new Error(`Database "/tmp/w.duckdb" is locked by another process. ${RAW_DUCKDB_LOCK}`) + expect(categorizeConnectionError(wrapped)).toBe("store_locked") + }) + + test("classifies the open deadline as driver_open_timeout, not a remote timeout", () => { + const err = new Error('DuckDB store "/tmp/w.duckdb" did not finish opening within 30000ms.') + expect(categorizeConnectionError(err)).toBe("driver_open_timeout") + }) + + test("still classifies a real credentials failure as auth_failed", () => { + expect(categorizeConnectionError(new Error("password authentication failed for user"))).toBe("auth_failed") + }) + + test("a locked store is infrastructure, and is the one that is recoverable", () => { + expect(isInfrastructureFailure("store_locked")).toBe(true) + expect(isRecoverableFailure("store_locked")).toBe(true) + }) + + test("a broken install is infrastructure but NOT recoverable", () => { + for (const category of ["driver_missing", "driver_open_timeout"]) { + expect(isInfrastructureFailure(category)).toBe(true) + expect(isRecoverableFailure(category)).toBe(false) + } + }) + + test("a config or credentials fault is neither infrastructure nor recoverable", () => { + for (const category of ["auth_failed", "config_error", "network_error", "other"]) { + expect(isInfrastructureFailure(category)).toBe(false) + expect(isRecoverableFailure(category)).toBe(false) + } + }) +}) +// altimate_change end + describe("ConnectionRegistry", () => { beforeEach(() => { Registry.reset() diff --git a/packages/opencode/test/altimate/duckdb-lock-helper.ts b/packages/opencode/test/altimate/duckdb-lock-helper.ts new file mode 100644 index 0000000000..b9b1fa0281 --- /dev/null +++ b/packages/opencode/test/altimate/duckdb-lock-helper.ts @@ -0,0 +1,94 @@ +import { createRequire } from "node:module" +import fs from "node:fs" +import path from "node:path" + +/** + * Hold a DuckDB write lock on `storePath` from a *separate OS process*. + * + * It has to be a separate process. DuckDB's file lock is per-process: the same + * process re-opening a store it already holds succeeds, so an in-process + * "second open" proves nothing about the path this exercises. Every lock + * assertion in these suites depends on the lock being genuinely foreign. + * + * Measured, and load-bearing for what the driver may claim: a write lock held + * here is NOT rescued by opening READ_ONLY. `default`, `READ_ONLY` and + * `read_only` all fail against it. So the read-only *retry* cannot recover a + * locked store — only an up-front `config.readonly` open lets several processes + * share a file that nobody has open read-write. + */ +export interface HeldLock { + release(): Promise +} + +const require_ = createRequire(import.meta.url) + +export async function holdWriteLock(storePath: string, scratchDir: string): Promise { + // Resolve `duckdb` here rather than in the child: the child's cwd is not + // guaranteed to sit under the node_modules tree that resolves it, and a + // resolution failure there would look like "no lock was taken". + const duckdbEntry = require_.resolve("duckdb") + + const scriptPath = path.join(scratchDir, "hold-duckdb-lock.cjs") + fs.writeFileSync( + scriptPath, + [ + `const duckdb = require(${JSON.stringify(duckdbEntry)})`, + `const mod = duckdb.default || duckdb`, + `const db = new mod.Database(process.argv[2], (err) => {`, + ` if (err) { process.stderr.write("HOLD_FAILED " + (err.message || err) + "\\n"); process.exit(1) }`, + // Take a real write so the lock is unambiguously a writer's. + ` db.run("CREATE TABLE IF NOT EXISTS lock_probe (x INTEGER)", (e) => {`, + ` if (e) { process.stderr.write("HOLD_FAILED " + (e.message || e) + "\\n"); process.exit(1) }`, + ` process.stdout.write("READY\\n")`, + ` })`, + `})`, + `setInterval(() => {}, 1 << 30)`, + ].join("\n"), + "utf-8", + ) + + const child = Bun.spawn([process.execPath, scriptPath, storePath], { + stdout: "pipe", + stderr: "pipe", + cwd: scratchDir, + }) + + const ready = (async () => { + const reader = (child.stdout as ReadableStream).getReader() + const decoder = new TextDecoder() + let buffered = "" + while (true) { + const { done, value } = await reader.read() + if (done) break + buffered += decoder.decode(value, { stream: true }) + if (buffered.includes("READY")) { + reader.releaseLock() + return + } + } + const err = await new Response(child.stderr).text() + throw new Error(`lock holder exited without taking the lock: ${err || "(no output)"}`) + })() + + const timeout = new Promise((_, reject) => + setTimeout(() => reject(new Error("lock holder did not report READY within 30s")), 30_000), + ) + + try { + await Promise.race([ready, timeout]) + } catch (e) { + child.kill() + throw e + } + + return { + async release() { + child.kill() + try { + await child.exited + } catch { + // the process is gone either way + } + }, + } +} diff --git a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts index 3d89c496ce..000f8ba98b 100644 --- a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -4,6 +4,7 @@ import os from "node:os" import path from "node:path" import { connect } from "../../../drivers/src/duckdb" +import { holdWriteLock } from "./duckdb-lock-helper" // These tests open a real DuckDB store on disk. They are pointless against a // mock: every bug they cover lives in the native open callback or in the @@ -142,6 +143,63 @@ describe("DuckDB driver: opening a real store", () => { } }) + ddbTest("a budget larger than the timer's range is not silently turned into 1ms", async () => { + // setTimeout clamps a delay past 2^31-1 down to about 1ms, which would turn + // "wait essentially forever" into "fail immediately" — the exact inversion + // this driver exists to prevent. The budget must be clamped before it + // reaches the timer. + const c = await connect({ type: "duckdb", path: storePath, open_timeout_ms: 1e12 }) + await c.connect() + const r = await c.execute("SELECT count(*) AS n FROM t") + expect(Number(r.rows[0][0])).toBe(1) + await c.close() + }) + + ddbTest("an explicitly read-only open that hits a foreign lock reports it as a lock", async () => { + // READ_ONLY does not rescue an existing cross-process write lock — measured, + // and the reason this path exists at all. What matters is that the failure + // stays *recognisable* as a lock: an explicit readonly open takes the + // `wantReadOnly` branch, which correctly skips the read-only retry, and + // before this fix that branch rethrew DuckDB's raw error, so every consumer + // classified a plain lock collision as an unknown failure. + const lock = await holdWriteLock(storePath, dir) + try { + const c = await connect({ type: "duckdb", path: storePath, readonly: true }) + const message = await messageFrom(() => c.connect()) + expect(message).toContain("is locked by another process") + // DuckDB's own text survives: it names the PID holding the lock, which is + // the only way to find the other process. + expect(message.toLowerCase()).toContain("conflicting lock") + } finally { + await lock.release() + } + }) + + ddbTest("a read-write open that hits a foreign lock also reports it as a lock", async () => { + const lock = await holdWriteLock(storePath, dir) + try { + const c = await connect({ type: "duckdb", path: storePath }) + const message = await messageFrom(() => c.connect()) + expect(message).toContain("is locked by another process") + expect(message.toLowerCase()).toContain("conflicting lock") + } finally { + await lock.release() + } + }) + + ddbTest("the store is usable again once the foreign lock is released", async () => { + // Guards the helper itself. If release() did not actually free the lock, + // the two tests above would pass for the wrong reason and would poison + // every test that runs after them. + const lock = await holdWriteLock(storePath, dir) + await lock.release() + const c = await connect({ type: "duckdb", path: storePath }) + await c.connect() + const r = await c.execute("SELECT count(*) AS n FROM t") + expect(Number(r.rows[0][0])).toBe(1) + await c.close() + }) + ddbTest("reports a lock conflict as a lock conflict, keeping DuckDB's detail", async () => { // The verbatim message DuckDB emits when another process holds the file. // It contains "lock" but never "locked", which is why the driver's old diff --git a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts index c8c76815e4..a63bf95a5a 100644 --- a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts +++ b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts @@ -6,6 +6,7 @@ import path from "node:path" import { Dispatcher } from "../../src/altimate/native" import * as Registry from "../../src/altimate/native/connections/registry" import { WarehouseTestTool } from "../../src/altimate/tools/warehouse-test" +import { holdWriteLock } from "./duckdb-lock-helper" import { initTool } from "./tool-fixture" // End-to-end coverage for the `warehouse_test` tool against a real DuckDB file @@ -20,6 +21,9 @@ const ddbTest = RUN ? test : test.skip let dir = "" let storePath = "" +// Captured so afterAll can put the environment back exactly as it found it. +// Unconditionally deleting would clear a value the surrounding process had set. +let prevTelemetryDisabled: string | undefined async function warehouseTest(name: string) { return Dispatcher.call("warehouse.test", { name }) @@ -27,6 +31,7 @@ async function warehouseTest(name: string) { describe("warehouse.test against a real DuckDB store", () => { beforeAll(async () => { + prevTelemetryDisabled = process.env["ALTIMATE_TELEMETRY_DISABLED"] process.env["ALTIMATE_TELEMETRY_DISABLED"] = "true" if (!RUN) return dir = fs.mkdtempSync(path.join(os.tmpdir(), "warehouse-test-")) @@ -36,6 +41,7 @@ describe("warehouse.test against a real DuckDB store", () => { const c = await Registry.get("seed") await c.execute("CREATE TABLE t AS SELECT 1 AS a") const probe = await c.execute("SELECT count(*) AS n FROM t") + await Registry.closeAll() Registry.reset() // Refuse to run against anything but the real driver — see above. if (Number(probe.rows?.[0]?.[0]) !== 1) { @@ -47,12 +53,20 @@ describe("warehouse.test against a real DuckDB store", () => { } }) - afterEach(() => { + // Registry.reset() only clears the connector map — it is synchronous and + // close() is not — so on its own it leaks the native DuckDB handle behind + // every connector these tests create. closeAll() is what actually releases + // them; without it the handles accumulate and the rmSync below cannot delete + // the store on Windows. + afterEach(async () => { + await Registry.closeAll() Registry.reset() }) - afterAll(() => { - delete process.env["ALTIMATE_TELEMETRY_DISABLED"] + afterAll(async () => { + if (prevTelemetryDisabled === undefined) delete process.env["ALTIMATE_TELEMETRY_DISABLED"] + else process.env["ALTIMATE_TELEMETRY_DISABLED"] = prevTelemetryDisabled + await Registry.closeAll() Registry.reset() if (dir) fs.rmSync(dir, { recursive: true, force: true }) }) @@ -62,6 +76,7 @@ describe("warehouse.test against a real DuckDB store", () => { // merely usually right would recreate exactly that contamination. Each // iteration resets the registry so nothing is served from cache. for (let i = 0; i < 10; i++) { + await Registry.closeAll() Registry.reset() Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) const result = await warehouseTest("local") @@ -98,6 +113,47 @@ describe("warehouse.test against a real DuckDB store", () => { expect(result.connected).toBe(true) }) + ddbTest("a store locked by another process is reported as a recoverable lock", async () => { + const lock = await holdWriteLock(storePath, dir) + try { + await Registry.closeAll() + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const result = await warehouseTest("local") + expect(result.connected).toBe(false) + // Not "other": before this, an unwrapped lock error fell through every + // rule in categorizeConnectionError and was reported as an unclassified + // failure, which reads exactly like a bad password. + expect(result.error_category).toBe("store_locked") + // Still infrastructure — nothing about this connection's config is wrong + // — but recoverable, which is what separates it from a broken install. + expect(result.infrastructure).toBe(true) + expect(result.recoverable).toBe(true) + } finally { + await lock.release() + } + }) + + ddbTest("the tool tells a caller to clear a lock, not to stop and report it", async () => { + const tool = await initTool(WarehouseTestTool) + const ctx = { sessionID: "s", messageID: "m", agent: "build", abort: new AbortController().signal, messages: [] } + const lock = await holdWriteLock(storePath, dir) + try { + await Registry.closeAll() + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const locked = await tool.execute({ name: "local" }, ctx) + expect(locked.title).toContain("STORE LOCKED") + expect(locked.metadata.recoverable).toBe(true) + // A lock clears itself once the other process lets go, so the + // stop-and-report copy meant for a broken install is wrong advice here. + expect(locked.output).not.toContain("Stop and report this") + expect(locked.output).toContain("Close that connection and try again") + } finally { + await lock.release() + } + }) + ddbTest("the warehouse_test tool renders infrastructure faults unmistakably", async () => { const tool = await initTool(WarehouseTestTool) const ctx = { sessionID: "s", messageID: "m", agent: "build", abort: new AbortController().signal, messages: [] } From 4c6d20e3a8aac266183c087a608b3f13c5092cec Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 01:02:56 -0700 Subject: [PATCH 4/7] fix(test): make the DuckDB lock helper work off the driver, not its own resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's `Driver E2E` job failed the six new lock tests with `Cannot find package 'duckdb' from .../test/altimate/duckdb-lock-helper.ts`, while the same tests passed locally. That is exactly the failure mode this PR is about, reintroduced by the test that proves the fix, so it is worth naming rather than quietly patching. **Resolution.** The helper resolved `duckdb` with `createRequire(import.meta.url)` from `packages/opencode/test/altimate/`. `duckdb` is a dependency of `packages/drivers` and lives only in `packages/drivers/node_modules/duckdb`; that lookup happened to succeed on this machine and did not on CI. The helper now has its child take the lock through the driver itself, imported by absolute path via `new URL("../../../drivers/src/duckdb.ts", import.meta.url)` — the same specifier both E2E suites already import successfully, so it resolves wherever they do. The child holding the lock through the real driver is also more faithful than reaching around it to the raw package. **Per-process locks.** With resolution fixed, all three driver-level lock tests then failed locally for a second reason: the lock holder could not take the lock because *the test runner itself* still held it. DuckDB's lock is per-process, and the driver's `close()` falls back to a 500ms timer because the native close callback does not always fire under Bun, so "closed" is not instantaneous and the tests were racing that fallback. The lock tests now use their own store (`locked.duckdb`) that this process never opens successfully; the holder creates it, so the only writer is the foreign process. The release-check reads the holder's own `lock_probe` table, which proves both that the lock is gone and that the holder wrote through it rather than only touching the file. Verified: gated E2E 19 pass / 0 fail on six consecutive runs; the read-only lock test still fails when the driver fix is reverted, so it remains a real guard. Unchanged elsewhere — typecheck 13/13, markers ok against `origin/main`, drivers 141 pass / 0 fail, `test/altimate` 4222 pass / 0 fail, lint 5863 warnings / 1 pre-existing error. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../test/altimate/duckdb-lock-helper.ts | 39 ++++++++++--------- .../test/altimate/duckdb-open-e2e.test.ts | 28 +++++++++---- .../warehouse-test-duckdb-e2e.test.ts | 13 +++++-- 3 files changed, 50 insertions(+), 30 deletions(-) diff --git a/packages/opencode/test/altimate/duckdb-lock-helper.ts b/packages/opencode/test/altimate/duckdb-lock-helper.ts index b9b1fa0281..f40a56c7a4 100644 --- a/packages/opencode/test/altimate/duckdb-lock-helper.ts +++ b/packages/opencode/test/altimate/duckdb-lock-helper.ts @@ -1,4 +1,4 @@ -import { createRequire } from "node:module" +import { fileURLToPath } from "node:url" import fs from "node:fs" import path from "node:path" @@ -20,28 +20,31 @@ export interface HeldLock { release(): Promise } -const require_ = createRequire(import.meta.url) +// The child takes the lock through the driver itself, by absolute path, rather +// than resolving the `duckdb` package on its own. Resolving `duckdb` from this +// directory works in some layouts and not others — it failed in CI, where the +// package is a dependency of `packages/drivers` and not reachable from here — +// which is the same class of fault this PR exists to fix, so it has no business +// being reintroduced by the test that proves the fix. The specifier below is +// the one both E2E suites already import successfully. +const DRIVER_PATH = fileURLToPath(new URL("../../../drivers/src/duckdb.ts", import.meta.url)) export async function holdWriteLock(storePath: string, scratchDir: string): Promise { - // Resolve `duckdb` here rather than in the child: the child's cwd is not - // guaranteed to sit under the node_modules tree that resolves it, and a - // resolution failure there would look like "no lock was taken". - const duckdbEntry = require_.resolve("duckdb") - - const scriptPath = path.join(scratchDir, "hold-duckdb-lock.cjs") + const scriptPath = path.join(scratchDir, "hold-duckdb-lock.ts") fs.writeFileSync( scriptPath, [ - `const duckdb = require(${JSON.stringify(duckdbEntry)})`, - `const mod = duckdb.default || duckdb`, - `const db = new mod.Database(process.argv[2], (err) => {`, - ` if (err) { process.stderr.write("HOLD_FAILED " + (err.message || err) + "\\n"); process.exit(1) }`, - // Take a real write so the lock is unambiguously a writer's. - ` db.run("CREATE TABLE IF NOT EXISTS lock_probe (x INTEGER)", (e) => {`, - ` if (e) { process.stderr.write("HOLD_FAILED " + (e.message || e) + "\\n"); process.exit(1) }`, - ` process.stdout.write("READY\\n")`, - ` })`, - `})`, + `const { connect } = await import(${JSON.stringify(DRIVER_PATH)})`, + `try {`, + ` const c = await connect({ type: "duckdb", path: process.argv[2] })`, + ` await c.connect()`, + // A real write, so the lock is unambiguously a writer's. + ` await c.execute("CREATE TABLE IF NOT EXISTS lock_probe (x INTEGER)")`, + ` process.stdout.write("READY\\n")`, + `} catch (e) {`, + ` process.stderr.write("HOLD_FAILED " + (e instanceof Error ? e.message : String(e)) + "\\n")`, + ` process.exit(1)`, + `}`, `setInterval(() => {}, 1 << 30)`, ].join("\n"), "utf-8", diff --git a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts index 000f8ba98b..608d7c74bb 100644 --- a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -26,6 +26,14 @@ const ddbTest = RUN ? test : test.skip let dir = "" let storePath = "" + // The lock tests use their own store, which THIS process never opens + // successfully. That is not tidiness: DuckDB's lock is per-process, and this + // process holds a write lock on `storePath` for as long as its own handle + // lives — the driver's close() has to fall back to a timer in Bun because the + // native close callback does not always fire, so "closed" is not instantaneous. + // Sharing one store would make these tests race that fallback. The lock holder + // creates `lockedPath` itself, so the only writer is the foreign process. + let lockedPath = "" /** Run `fn` and return the error message it threw, or "" if it succeeded. */ async function messageFrom(fn: () => Promise): Promise { @@ -42,6 +50,7 @@ describe("DuckDB driver: opening a real store", () => { if (!RUN) return dir = fs.mkdtempSync(path.join(os.tmpdir(), "duckdb-open-")) storePath = path.join(dir, "warehouse.duckdb") + lockedPath = path.join(dir, "locked.duckdb") const c = await connect({ type: "duckdb", path: storePath }) await c.connect() await c.execute("CREATE TABLE t AS SELECT 1 AS a, 'x' AS b") @@ -162,9 +171,9 @@ describe("DuckDB driver: opening a real store", () => { // `wantReadOnly` branch, which correctly skips the read-only retry, and // before this fix that branch rethrew DuckDB's raw error, so every consumer // classified a plain lock collision as an unknown failure. - const lock = await holdWriteLock(storePath, dir) + const lock = await holdWriteLock(lockedPath, dir) try { - const c = await connect({ type: "duckdb", path: storePath, readonly: true }) + const c = await connect({ type: "duckdb", path: lockedPath, readonly: true }) const message = await messageFrom(() => c.connect()) expect(message).toContain("is locked by another process") // DuckDB's own text survives: it names the PID holding the lock, which is @@ -176,9 +185,9 @@ describe("DuckDB driver: opening a real store", () => { }) ddbTest("a read-write open that hits a foreign lock also reports it as a lock", async () => { - const lock = await holdWriteLock(storePath, dir) + const lock = await holdWriteLock(lockedPath, dir) try { - const c = await connect({ type: "duckdb", path: storePath }) + const c = await connect({ type: "duckdb", path: lockedPath }) const message = await messageFrom(() => c.connect()) expect(message).toContain("is locked by another process") expect(message.toLowerCase()).toContain("conflicting lock") @@ -191,12 +200,15 @@ describe("DuckDB driver: opening a real store", () => { // Guards the helper itself. If release() did not actually free the lock, // the two tests above would pass for the wrong reason and would poison // every test that runs after them. - const lock = await holdWriteLock(storePath, dir) + const lock = await holdWriteLock(lockedPath, dir) await lock.release() - const c = await connect({ type: "duckdb", path: storePath }) + const c = await connect({ type: "duckdb", path: lockedPath }) await c.connect() - const r = await c.execute("SELECT count(*) AS n FROM t") - expect(Number(r.rows[0][0])).toBe(1) + // `lock_probe` is the table the holder creates when it takes the lock, so + // reading it proves both that the lock is gone and that the holder really + // wrote through it rather than merely touching the file. + const r = await c.execute("SELECT count(*) AS n FROM lock_probe") + expect(Number(r.rows[0][0])).toBe(0) await c.close() }) diff --git a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts index a63bf95a5a..c1d1965b4b 100644 --- a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts +++ b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts @@ -21,6 +21,10 @@ const ddbTest = RUN ? test : test.skip let dir = "" let storePath = "" +// A store this process never opens — see the note in duckdb-open-e2e.test.ts. +// DuckDB's lock is per-process, so a store this process has open cannot be +// locked against it by anyone else. +let lockedPath = "" // Captured so afterAll can put the environment back exactly as it found it. // Unconditionally deleting would clear a value the surrounding process had set. let prevTelemetryDisabled: string | undefined @@ -36,6 +40,7 @@ describe("warehouse.test against a real DuckDB store", () => { if (!RUN) return dir = fs.mkdtempSync(path.join(os.tmpdir(), "warehouse-test-")) storePath = path.join(dir, "warehouse.duckdb") + lockedPath = path.join(dir, "locked.duckdb") Registry.reset() Registry.setConfigs({ seed: { type: "duckdb", path: storePath } }) const c = await Registry.get("seed") @@ -114,11 +119,11 @@ describe("warehouse.test against a real DuckDB store", () => { }) ddbTest("a store locked by another process is reported as a recoverable lock", async () => { - const lock = await holdWriteLock(storePath, dir) + const lock = await holdWriteLock(lockedPath, dir) try { await Registry.closeAll() Registry.reset() - Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + Registry.setConfigs({ local: { type: "duckdb", path: lockedPath } }) const result = await warehouseTest("local") expect(result.connected).toBe(false) // Not "other": before this, an unwrapped lock error fell through every @@ -137,11 +142,11 @@ describe("warehouse.test against a real DuckDB store", () => { ddbTest("the tool tells a caller to clear a lock, not to stop and report it", async () => { const tool = await initTool(WarehouseTestTool) const ctx = { sessionID: "s", messageID: "m", agent: "build", abort: new AbortController().signal, messages: [] } - const lock = await holdWriteLock(storePath, dir) + const lock = await holdWriteLock(lockedPath, dir) try { await Registry.closeAll() Registry.reset() - Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + Registry.setConfigs({ local: { type: "duckdb", path: lockedPath } }) const locked = await tool.execute({ name: "local" }, ctx) expect(locked.title).toContain("STORE LOCKED") expect(locked.metadata.recoverable).toBe(true) From 6ac0c640aecf4df391d06287402748c9eadcc395 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 01:17:20 -0700 Subject: [PATCH 5/7] fix(ci): actually run the drivers unit suite, and stop misreporting a caller's own deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review wave on #1198. The two findings that matter are gaps in what runs and what gets blamed. **`packages/drivers` has never been tested in CI.** The package declares no scripts, no job sets it as a working directory, and the TypeScript job runs `bun test` from `packages/opencode` only. All 142 driver unit tests — the lock, timeout and read-only regressions this PR turns on — could fail without any job noticing. Added a `Run drivers unit suite` step to the `driver-e2e` job, and `packages/drivers/test/**` to the `drivers` path filter. **The DuckDB E2E step ran with a 5s per-test deadline.** Invoking `bun test` directly does not use the package's `test` script, so it took the CLI default of 5000ms — shorter than the driver's own 30s default open budget and the lock helper's 30s readiness budget. A step meant to prove a too-short deadline was removed was imposing one of its own. Now `--timeout 90000`, explicitly. **A deadline the caller set was reported as a broken client.** With `open_timeout_ms: 1` on the connection, `warehouse_test` said "the client on this machine is broken, nothing about your configuration is wrong, stop and report" — for a failure the connection's own setting caused and its own setting fixes. That is the same category error this PR exists to remove, pointing the other way. `resolveOpenTimeoutMs` now returns the source; a connection-scoped budget names itself in the message and classifies as `config_error`, while the default or the env var stays `driver_open_timeout` and infrastructure. **Also** - The raw-lock fallback in `categorizeConnectionError` matched `could not set lock` alone, which is generic enough to appear in an unrelated remote error — it would have told the user to close a local process over a warehouse fault. Now requires DuckDB's full shape (`could not set lock` *and* `conflicting lock`). - The `drivers` filter ignored `package.json`, `bun.lock` and `packages/drivers/package.json`, which govern whether the native binding is fetched at all. A change to those could break every real-DuckDB path with no real-DuckDB job running. - The lock helper's 30s readiness timer was never cleared, holding the event loop open for up to 30s per call after the last assertion. - The lock helper's child imported the driver outside its `try`, so a load failure surfaced as a bare unhandled rejection instead of the normalised `HOLD_FAILED` message. **Tests.** A retry-path case in `driver-security.test.ts` that fails the first open with DuckDB's *real* `Conflicting lock is held` text and asserts the READ_ONLY retry fires — the existing retry test used a fabricated `DUCKDB_LOCKED` string that the driver's original `.includes("locked")` already matched, so it could not have caught a regression in real-message detection. Ungated cases for the connection-scoped deadline classification and for not claiming another driver's lock-shaped error. E2E cases asserting the deadline message names its source, and that a connection-set deadline is `config_error` / not infrastructure while an env-set one stays infrastructure. Gates: typecheck 13/13; markers ok against `origin/main`; drivers 142 pass 0 fail; `test/altimate` 4224 pass 0 fail; gated E2E 21 pass 0 fail on six consecutive runs. Lint 5864 warnings / 1 error — the error is the pre-existing `consistent-return` in `packages/http-recorder/test/record-replay.test.ts`; the warnings are +2 against the branch baseline, both un-awaited `mock.module` calls in new tests, matching how every other `mock.module` in that file is written. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .github/workflows/ci.yml | 23 +++++++++- packages/drivers/src/duckdb.ts | 33 ++++++++++---- packages/drivers/test/driver-security.test.ts | 44 +++++++++++++++++++ .../altimate/native/connections/registry.ts | 23 +++++++--- .../test/altimate/connections.test.ts | 16 +++++++ .../test/altimate/duckdb-lock-helper.ts | 17 +++++-- .../test/altimate/duckdb-open-e2e.test.ts | 20 ++++++++- .../warehouse-test-duckdb-e2e.test.ts | 44 ++++++++++++++++--- 8 files changed, 190 insertions(+), 30 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e24440595d..619343a1cf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,6 +68,13 @@ jobs: - '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: @@ -294,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 @@ -308,8 +323,14 @@ jobs: # 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 test/altimate/duckdb-open-e2e.test.ts test/altimate/warehouse-test-duckdb-e2e.test.ts + 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" diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index c376c3f3a1..556774efd8 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -32,12 +32,20 @@ function positiveMs(value: unknown): number | undefined { return Number.isFinite(n) && n > 0 ? Math.min(n, MAX_TIMER_MS) : undefined } -function resolveOpenTimeoutMs(config: ConnectionConfig): number { - return ( - positiveMs(config.open_timeout_ms) ?? - positiveMs(globalThis.process?.env?.["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"]) ?? - DEFAULT_OPEN_TIMEOUT_MS - ) +/** + * 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 @@ -52,7 +60,7 @@ export async function connect(config: ConnectionConfig): Promise { const dbPath = (config.path as string) ?? ":memory:" // altimate_change start — configurable open budget - const openTimeoutMs = resolveOpenTimeoutMs(config) + const { ms: openTimeoutMs, source: openTimeoutSource } = resolveOpenTimeoutMs(config) // altimate_change end let db: any let connection: any @@ -176,8 +184,15 @@ export async function connect(config: ConnectionConfig): Promise { `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. ` + - `Raise the budget with this connection's open_timeout_ms (which takes ` + - `priority) or ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS if the machine is loaded.`, + (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.`), ), ) } diff --git a/packages/drivers/test/driver-security.test.ts b/packages/drivers/test/driver-security.test.ts index 0d80a8d93c..cd80082012 100644 --- a/packages/drivers/test/driver-security.test.ts +++ b/packages/drivers/test/driver-security.test.ts @@ -189,6 +189,50 @@ describe("DuckDB driver", () => { await connector.close() }) + test("retries with READ_ONLY when the first open fails with DuckDB's real lock text", async () => { + // The other retry test drives a fabricated "DUCKDB_LOCKED: file is locked" + // string, which the driver's original `.includes("locked")` check already + // matched. Only this one uses the message DuckDB actually emits, so only + // this one would catch a regression that broke detection of a real lock + // collision on the retry path. + let attempts = 0 + const accessModes: Array = [] + mock.module("duckdb", () => ({ + default: { + Database: class { + constructor(_path: string, optsOrCb: any, cb?: (err: Error | null) => void) { + const opts = typeof optsOrCb === "function" ? undefined : optsOrCb + const done = openCallback(optsOrCb, cb) + accessModes.push(opts?.access_mode) + attempts++ + if (attempts === 1) setTimeout(() => done(new Error(REAL_DUCKDB_LOCK_ERROR)), 0) + else setTimeout(() => done(null), 0) + } + connect() { + return { + all: (_sql: string, cb: (err: Error | null, rows: any[]) => void) => { + cb(null, [{ result: 1 }]) + }, + } + } + close(cb: any) { + if (cb) cb(null) + } + }, + }, + })) + + const { connect } = await import("../src/duckdb") + const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" }) + await connector.connect() + + expect(attempts).toBe(2) + expect(accessModes[0]).toBeUndefined() + expect(accessModes[1]).toBe("READ_ONLY") + + await connector.close() + }) + test("wraps a lock error on an explicitly read-only open, and does not retry it", async () => { // A caller that set `readonly` already gets READ_ONLY on the first open, // so there is nothing for the retry to try differently — DuckDB's file diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 3461d1e3b4..12ba2ba4d0 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -285,15 +285,24 @@ export function categorizeConnectionError(e: unknown): string { // altimate_change start — categories for local-client faults // Checked before the generic "timeout"/"not found" rules below, which are // about the remote warehouse and would otherwise swallow these. - if (msg.includes("did not finish opening")) return "driver_open_timeout" - // "locked by another process" is the DuckDB driver's own wrapper. The other - // two are DuckDB's raw text ("Could not set lock on file …: Conflicting lock - // is held"), matched here as well so a lock that reaches this function - // unwrapped is still classified rather than falling through to "other". + if (msg.includes("did not finish opening")) { + // A deadline the connection itself set is the connection's fault, and the + // remedy is to raise or drop that setting. Reporting it as a broken client + // — "stop and report, nothing about your config is wrong" — is the exact + // mirror of the confusion this categorisation exists to remove. Only a + // deadline the caller did not choose per-connection is infrastructure. + return msg.includes("deadline was set on this connection") ? "config_error" : "driver_open_timeout" + } + // "locked by another process" is the DuckDB driver's own wrapper. The second + // clause is DuckDB's raw text ("Could not set lock on file …: Conflicting + // lock is held"), matched here as well so a lock that reaches this function + // unwrapped is still classified rather than falling through to "other". Both + // halves of that clause are required: "could not set lock" on its own is + // generic enough to appear in an unrelated remote error, and claiming it + // would tell the user to close a local process over a remote fault. if ( msg.includes("locked by another process") || - msg.includes("conflicting lock") || - msg.includes("could not set lock") + (msg.includes("could not set lock") && msg.includes("conflicting lock")) ) return "store_locked" // altimate_change end diff --git a/packages/opencode/test/altimate/connections.test.ts b/packages/opencode/test/altimate/connections.test.ts index d3e030eecf..8224274560 100644 --- a/packages/opencode/test/altimate/connections.test.ts +++ b/packages/opencode/test/altimate/connections.test.ts @@ -51,6 +51,22 @@ describe("categorizeConnectionError: local-client faults", () => { expect(categorizeConnectionError(err)).toBe("driver_open_timeout") }) + test("a deadline the connection set is a config fault, not a broken client", () => { + const err = new Error( + 'DuckDB store "/tmp/w.duckdb" did not finish opening within 1ms. ' + + "This deadline was set on this connection as open_timeout_ms=1; raise or remove it.", + ) + expect(categorizeConnectionError(err)).toBe("config_error") + expect(isInfrastructureFailure(categorizeConnectionError(err))).toBe(false) + }) + + test("does not claim another driver's error just because it mentions a lock", () => { + // "could not set lock" alone is generic enough to appear in an unrelated + // remote failure; claiming it would tell the user to close a local process + // over a warehouse-side fault. + expect(categorizeConnectionError(new Error("ERROR: could not set lock timeout"))).not.toBe("store_locked") + }) + test("still classifies a real credentials failure as auth_failed", () => { expect(categorizeConnectionError(new Error("password authentication failed for user"))).toBe("auth_failed") }) diff --git a/packages/opencode/test/altimate/duckdb-lock-helper.ts b/packages/opencode/test/altimate/duckdb-lock-helper.ts index f40a56c7a4..0fc9f25705 100644 --- a/packages/opencode/test/altimate/duckdb-lock-helper.ts +++ b/packages/opencode/test/altimate/duckdb-lock-helper.ts @@ -34,8 +34,11 @@ export async function holdWriteLock(storePath: string, scratchDir: string): Prom fs.writeFileSync( scriptPath, [ - `const { connect } = await import(${JSON.stringify(DRIVER_PATH)})`, `try {`, + // Inside the try, so a driver that fails to load is reported through the + // same HOLD_FAILED channel as any other failure rather than surfacing as + // a bare unhandled rejection the parent has to guess at. + ` const { connect } = await import(${JSON.stringify(DRIVER_PATH)})`, ` const c = await connect({ type: "duckdb", path: process.argv[2] })`, ` await c.connect()`, // A real write, so the lock is unambiguously a writer's. @@ -73,15 +76,21 @@ export async function holdWriteLock(storePath: string, scratchDir: string): Prom throw new Error(`lock holder exited without taking the lock: ${err || "(no output)"}`) })() - const timeout = new Promise((_, reject) => - setTimeout(() => reject(new Error("lock holder did not report READY within 30s")), 30_000), - ) + // The handle is kept so it can be cleared: an un-cleared timer holds the + // event loop open, which would make the whole test process sit for the full + // 30s after its last assertion, once per call. + let timer: ReturnType | undefined + const timeout = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error("lock holder did not report READY within 30s")), 30_000) + }) try { await Promise.race([ready, timeout]) } catch (e) { child.kill() throw e + } finally { + if (timer) clearTimeout(timer) } return { diff --git a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts index 608d7c74bb..18c02db52e 100644 --- a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -129,7 +129,18 @@ describe("DuckDB driver: opening a real store", () => { // The old message was `Timed out opening DuckDB database ""`, which // reads as "the store is broken" and sent investigators after the file. expect(message).toContain("not a fault in the store") - expect(message).toContain("ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS") + }) + + ddbTest("a deadline the connection set says so, so it is not read as a broken client", async () => { + // The remedy differs by source, so the message has to name it. A budget the + // connection chose is fixed by changing that connection; the default firing + // says something about the machine instead. + const c = await connect({ type: "duckdb", path: storePath, open_timeout_ms: 1 }) + const message = await messageFrom(() => c.connect()) + expect(message).toContain("deadline was set on this connection as open_timeout_ms=1") + // It must NOT point at the env var, which does not override a per-connection + // setting and so would not fix anything here. + expect(message).not.toContain("ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS") }) ddbTest("the open deadline is tunable by environment variable", async () => { @@ -137,7 +148,12 @@ describe("DuckDB driver: opening a real store", () => { process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = "1" try { const c = await connect({ type: "duckdb", path: storePath }) - expect(await messageFrom(() => c.connect())).toContain("did not finish opening within 1ms") + const message = await messageFrom(() => c.connect()) + expect(message).toContain("did not finish opening within 1ms") + // Sourced from the environment, not the connection, so the message points + // at the knobs that would actually change it. + expect(message).toContain("ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS") + expect(message).not.toContain("deadline was set on this connection") } finally { if (prev === undefined) delete process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] else process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = prev diff --git a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts index c1d1965b4b..c277225677 100644 --- a/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts +++ b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts @@ -93,14 +93,36 @@ describe("warehouse.test against a real DuckDB store", () => { ddbTest("reports a broken client as an infrastructure failure, not a failed connection", async () => { Registry.reset() // An unreachably small open budget stands in for any local fault that - // stops the store opening. The point of the assertion is the *shape* of - // the report: a caller must be able to tell this apart from a bad - // password without parsing prose. + // stops the store opening. It is set through the environment, NOT on the + // connection: a deadline the connection itself chose is that connection's + // own doing and is covered by the next test. The point of the assertion is + // the *shape* of the report — a caller must be able to tell this apart from + // a bad password without parsing prose. + const prev = process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] + process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = "1" + try { + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const result = await warehouseTest("local") + expect(result.connected).toBe(false) + expect(result.infrastructure).toBe(true) + expect(result.error_category).toBe("driver_open_timeout") + } finally { + if (prev === undefined) delete process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] + else process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = prev + } + }) + + ddbTest("a deadline the connection itself set is a configuration fault, not infrastructure", async () => { + // The mirror of the bug this PR fixes. Telling a caller "your client is + // broken, stop and report it" when they set `open_timeout_ms: 1` themselves + // is the same category error as reporting a broken install as a bad + // password — it just points the wrong way. + Registry.reset() Registry.setConfigs({ local: { type: "duckdb", path: storePath, open_timeout_ms: 1 } }) const result = await warehouseTest("local") expect(result.connected).toBe(false) - expect(result.infrastructure).toBe(true) - expect(result.error_category).toBe("driver_open_timeout") + expect(result.error_category).toBe("config_error") + expect(result.infrastructure).toBe(false) }) ddbTest("does not mark a genuine configuration mistake as infrastructure", async () => { @@ -169,8 +191,16 @@ describe("warehouse.test against a real DuckDB store", () => { expect(ok.title).toContain("OK") Registry.reset() - Registry.setConfigs({ local: { type: "duckdb", path: storePath, open_timeout_ms: 1 } }) - const broken = await tool.execute({ name: "local" }, ctx) + const prev = process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] + process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = "1" + let broken: Awaited> + try { + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + broken = await tool.execute({ name: "local" }, ctx) + } finally { + if (prev === undefined) delete process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] + else process.env["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"] = prev + } // The whole point: a reader of this transcript — human or model — must not // be able to mistake a broken client for the model getting something // wrong, or for a connection that needs its credentials fixed. From c06a851e317f3de5aac42ac95a99421c7beabfae Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 19:04:43 -0700 Subject: [PATCH 6/7] test(drivers): assert the DuckDB store is a live engine, not just an open-looking connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every existing assertion in `duckdb-open-e2e.test.ts` can be satisfied by a connection that is not really there. A row count can legitimately be zero, and an absent error string can mean the error was swallowed rather than that none occurred — which is live on `main` today: `register.ts`'s `sql.execute` catches everything and returns `{ row_count: 0, error }` instead of throwing, and `formatResult` renders "(0 rows)" without ever reading `error`. So "(0 rows)" and "the driver is dead" are indistinguishable to anything downstream. `md5()` is not. A dead connection cannot return one row, and a live one cannot return the right digest without hashing the input, so this is a check whose correct answer cannot be produced without touching the thing under test. Added in two places: - The `beforeAll` gate, which already refused to run against a fake driver but did so on a row count a stub can trivially return. The digest is the half a stub cannot satisfy by returning a plausible shape, and it gates the whole file. - An explicit test after the store-open assertions, asserting both that exactly one row came back and that the digest is correct. The expected digest is computed independently (`hashlib.md5`, cross-checked against `md5(1)`), never by asking DuckDB — deriving it from the system under test would make the assertion circular. Not hypothetical for this PR: during the driver A/B, two binaries looked clean across transcripts, traces and 563 lines of debug output — zero ENOENT, zero fault lines — while failing to load the driver entirely. A probe of this shape is what caught it, after four earlier probe designs passed while proving nothing. **Verified the assertion is load-bearing rather than decorative**, since a liveness check that cannot fail is exactly the defect it exists to prevent: - DuckDB independently returns `517f58256b5ba4642643b3e884d91d15` for the nonce, matching the digest computed outside it. - A wrong digest fails the `beforeAll` gate and aborts the entire file (0 pass, 1 fail). - A wrong digest in the test alone, with the gate left intact, fails that test (13 pass, 1 fail). - Appending `WHERE false` to reproduce the "(0 rows)" shape that reads as success downstream also fails the test (13 pass, 1 fail) — the swallowed-error case specifically. Gates unchanged: typecheck 13/13; markers ok against `origin/main`; drivers 142 pass 0 fail; `test/altimate` 4224 pass 0 fail (one additional skip, the new gated test); gated E2E 22 pass 0 fail on six consecutive runs; lint 5864 warnings / 1 error, byte-identical to the previous commit — the error remains the pre-existing `consistent-return` in `packages/http-recorder/test/record-replay.test.ts`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../test/altimate/duckdb-open-e2e.test.ts | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts index 18c02db52e..3dbf33278a 100644 --- a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -35,6 +35,29 @@ let storePath = "" // creates `lockedPath` itself, so the only writer is the foreign process. let lockedPath = "" +// A liveness probe whose correct answer cannot be produced without a working +// engine actually computing it. +// +// Every other assertion in this file can be satisfied by a connection that is +// not really there. A row count can legitimately be zero, and an absent error +// string can mean the error was swallowed rather than that none occurred — which +// is live on `main` today: `register.ts`'s `sql.execute` catches everything and +// returns `{ row_count: 0, error }` instead of throwing, and `formatResult` +// renders "(0 rows)" without ever reading `error`. So "(0 rows)" and "the driver +// is dead" are indistinguishable to anything downstream. +// +// `md5()` is not. A dead connection cannot return one row, and a live one cannot +// return the right digest without hashing the input. This is not a hypothetical +// safeguard: during the driver A/B for this PR, two binaries looked clean across +// transcripts, traces and 563 lines of debug output — zero ENOENT, zero fault +// lines — while failing to load the driver at all. This probe is what caught it. +// +// The digest is computed independently (`hashlib.md5`, cross-checked against +// `md5(1)`), never by asking DuckDB — deriving it from the system under test +// would make the assertion circular and prove nothing. +const LIVENESS_NONCE = "altimate-duckdb-open-e2e-liveness-2026-08-30" +const LIVENESS_MD5 = "517f58256b5ba4642643b3e884d91d15" + /** Run `fn` and return the error message it threw, or "" if it succeeded. */ async function messageFrom(fn: () => Promise): Promise { try { @@ -55,10 +78,12 @@ describe("DuckDB driver: opening a real store", () => { await c.connect() await c.execute("CREATE TABLE t AS SELECT 1 AS a, 'x' AS b") const probe = await c.execute("SELECT count(*) AS n FROM t") + const liveness = await c.execute(`SELECT md5('${LIVENESS_NONCE}') AS h`) await c.close() // Refuse to run against anything but the real thing. A fake driver would - // pass most assertions below while proving nothing. - if (Number(probe.rows?.[0]?.[0]) !== 1) { + // pass most assertions below while proving nothing. The digest is the half + // of this gate that a stub cannot satisfy by returning a plausible shape. + if (Number(probe.rows?.[0]?.[0]) !== 1 || String(liveness.rows?.[0]?.[0]) !== LIVENESS_MD5) { throw new Error( "ALTIMATE_DUCKDB_E2E=1 but the DuckDB driver is not the real one — " + "either the native binding is missing, or another test file has replaced " + @@ -84,6 +109,20 @@ describe("DuckDB driver: opening a real store", () => { } }) + ddbTest("the opened store is a live engine, not a connection that merely looks open", async () => { + // The assertion above this one passes on a dead connection: `(0 rows)` and + // "the driver never loaded" are the same observation. This one cannot. + const c = await connect({ type: "duckdb", path: storePath }) + await c.connect() + const r = await c.execute(`SELECT md5('${LIVENESS_NONCE}') AS h`) + // Exactly one row: a swallowed failure surfaces as zero rows, and zero rows + // is the shape that reads as success everywhere downstream. + expect(r.rows.length).toBe(1) + // And the right answer, which requires actually computing it. + expect(String(r.rows[0][0])).toBe(LIVENESS_MD5) + await c.close() + }) + ddbTest("opens the same store from many connectors at once", async () => { const results = await Promise.all( Array.from({ length: 8 }, async () => { From b1fa3f1bb78a4f4b09ab8c00c4e48a1d32ed22cd Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 19:16:10 -0700 Subject: [PATCH 7/7] fix(drivers): stop the driver fabricating a lock wrapper the registry then trusts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects my previous commit introduced, caught in review. **The driver's lock matcher was looser than the registry's.** I tightened `categorizeConnectionError` to require both `could not set lock` and `conflicting lock`, precisely so a lock-shaped error from elsewhere could not be claimed as contention — but left `isLockError` in the driver matching `could not set lock` alone. A non-contention failure (an unsupported filesystem lock, a permissions problem) therefore entered the lock branch, spent a read-only retry, and was wrapped as "locked by another process". The registry then trusted that fabricated wrapper as a recoverable `store_locked` and told the reader to close a process that does not exist, hiding the real filesystem fault. The two matchers now agree, with a comment on each saying so. **The liveness test leaked its handle on failure.** `close()` ran after both assertions, so a failing one skipped it. Since that test opens the store read-write and DuckDB's lock is per-process, the leak would make every later test in the file fail with a lock conflict rather than the real cause — the exact confusion this suite exists to prevent. Now closed in a `finally`, with the assertions moved after it. Test: a non-contention `Could not set lock … Operation not supported` must keep its own message, must not be relabelled "locked by another process", and must not spend a retry. It fails against the previous commit's matcher. Gates: typecheck 13/13; markers ok against `origin/main`; drivers 143 pass 0 fail; `test/altimate` 4224 pass 0 fail; gated E2E 22 pass 0 fail on six consecutive runs. Lint 5865 warnings / 1 error — the error remains the pre-existing `consistent-return` in `packages/http-recorder/test/record-replay.test.ts`; +1 warning, an un-awaited `mock.module` in the new test, matching every other `mock.module` in that file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/duckdb.ts | 10 ++++- packages/drivers/test/driver-security.test.ts | 44 +++++++++++++++++++ .../test/altimate/duckdb-open-e2e.test.ts | 18 +++++--- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index 556774efd8..8fe7dfb2e2 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -74,8 +74,14 @@ export async function connect(config: ConnectionConfig): Promise { const msg = (err instanceof Error ? err.message : String(err)).toLowerCase() return ( msg.includes("locked") || - msg.includes("conflicting lock") || - msg.includes("could not set lock") || + // 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") ) } diff --git a/packages/drivers/test/driver-security.test.ts b/packages/drivers/test/driver-security.test.ts index cd80082012..7f33e0a24e 100644 --- a/packages/drivers/test/driver-security.test.ts +++ b/packages/drivers/test/driver-security.test.ts @@ -189,6 +189,50 @@ describe("DuckDB driver", () => { await connector.close() }) + test("does not claim a non-contention lock failure as a foreign lock", async () => { + // "Could not set lock" without "Conflicting lock" is a filesystem fault, + // not contention. Wrapping it as "locked by another process" would send + // the reader after a process to close and hide the real cause — and the + // registry would trust that fabricated wrapper as recoverable. + let attempts = 0 + mock.module("duckdb", () => ({ + default: { + Database: class { + constructor(_path: string, optsOrCb: any, cb?: (err: Error | null) => void) { + attempts++ + setTimeout( + () => + openCallback(optsOrCb, cb)( + new Error('IO Error: Could not set lock on file "/tmp/test.duckdb": Operation not supported'), + ), + 0, + ) + } + connect() { + return {} + } + close(cb: any) { + if (cb) cb(null) + } + }, + }, + })) + + const { connect } = await import("../src/duckdb") + const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" }) + + try { + await connector.connect() + expect.unreachable("Should have thrown") + } catch (e: any) { + expect(e.message).toContain("Operation not supported") + expect(e.message).not.toContain("locked by another process") + } + + // And it is not treated as contention, so no read-only retry is spent. + expect(attempts).toBe(1) + }) + test("retries with READ_ONLY when the first open fails with DuckDB's real lock text", async () => { // The other retry test drives a fabricated "DUCKDB_LOCKED: file is locked" // string, which the driver's original `.includes("locked")` check already diff --git a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts index 3dbf33278a..2ea99f8199 100644 --- a/packages/opencode/test/altimate/duckdb-open-e2e.test.ts +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -113,14 +113,22 @@ describe("DuckDB driver: opening a real store", () => { // The assertion above this one passes on a dead connection: `(0 rows)` and // "the driver never loaded" are the same observation. This one cannot. const c = await connect({ type: "duckdb", path: storePath }) - await c.connect() - const r = await c.execute(`SELECT md5('${LIVENESS_NONCE}') AS h`) + // close() in a finally, not after the assertions: this opens `storePath` + // read-write, and DuckDB's lock is per-process, so a handle leaked by a + // failing assertion would make every later test in this file fail with a + // lock conflict instead of the real cause. + let rows: unknown[][] + try { + await c.connect() + rows = (await c.execute(`SELECT md5('${LIVENESS_NONCE}') AS h`)).rows + } finally { + await c.close() + } // Exactly one row: a swallowed failure surfaces as zero rows, and zero rows // is the shape that reads as success everywhere downstream. - expect(r.rows.length).toBe(1) + expect(rows.length).toBe(1) // And the right answer, which requires actually computing it. - expect(String(r.rows[0][0])).toBe(LIVENESS_MD5) - await c.close() + expect(String(rows[0][0])).toBe(LIVENESS_MD5) }) ddbTest("opens the same store from many connectors at once", async () => {