diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8826a5579e..619343a1cf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,6 +60,21 @@ jobs: - 'packages/opencode/test/altimate/drivers-mongodb-e2e.test.ts' - 'packages/opencode/test/altimate/drivers-clickhouse-e2e.test.ts' - 'packages/opencode/test/altimate/connections.test.ts' + # Run by the "DuckDB store-open E2E" step in the driver-e2e job. + # Without these, a PR touching only these files skips that job, + # and the main TypeScript job runs them with ALTIMATE_DUCKDB_E2E + # unset, which skips every test in them — no execution anywhere. + - 'packages/opencode/test/altimate/duckdb-open-e2e.test.ts' + - 'packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts' + - 'packages/opencode/test/altimate/duckdb-lock-helper.ts' + - 'packages/opencode/src/altimate/tools/warehouse-test.ts' + - 'packages/drivers/test/**' + # These govern whether the native DuckDB binding is fetched at all + # (`trustedDependencies`, and the pinned version). A change to them + # can break every real-DuckDB test while touching no test file. + - 'package.json' + - 'bun.lock' + - 'packages/drivers/package.json' dbt-tools: - 'packages/dbt-tools/**' installer: @@ -286,6 +301,14 @@ jobs: - name: Install dependencies run: bun install + # `packages/drivers` declares no scripts and no job had it as a working + # directory, so its 141 unit tests — including the driver's lock, timeout + # and read-only regressions — never ran in CI at all. The main TypeScript + # job runs `bun test` from `packages/opencode` only. + - name: Run drivers unit suite + run: bun test --timeout 60000 + working-directory: packages/drivers + - name: Run local driver E2E (DuckDB, SQLite, PostgreSQL) run: bun test test/altimate/drivers-e2e.test.ts working-directory: packages/opencode @@ -294,6 +317,24 @@ jobs: TEST_PG_PORT: "15432" TEST_PG_PASSWORD: testpass123 + # Needs a dedicated process: four files in test/altimate install a + # top-level mock.module("@altimateai/drivers/duckdb", …), and Bun + # evaluates every test file's top level before running any test, so in a + # whole-directory run these would silently exercise a fake. With + # ALTIMATE_DUCKDB_E2E=1 a missing or mocked driver fails the step rather + # than skipping it. + # + # `--timeout` is explicit because invoking `bun test` directly does not use + # the package's `test` script, so it would otherwise take the CLI default + # of 5000ms — shorter than both the driver's 30s default open budget and + # the lock helper's 30s readiness budget, which would reimpose exactly the + # kind of too-short outer deadline this PR removes. + - name: Run DuckDB store-open E2E (real store, no mocks) + run: bun test --timeout 90000 test/altimate/duckdb-open-e2e.test.ts test/altimate/warehouse-test-duckdb-e2e.test.ts + working-directory: packages/opencode + env: + ALTIMATE_DUCKDB_E2E: "1" + - name: Run Docker driver E2E (MySQL, SQL Server, Redshift) run: bun test test/altimate/drivers-docker-e2e.test.ts working-directory: packages/opencode 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/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 867840d0a4..8fe7dfb2e2 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -4,6 +4,51 @@ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +// altimate_change start — configurable, generous open budget +/** + * How long to wait for DuckDB to finish opening a store before giving up. + * + * This is a liveness guard, not a performance budget. Opening a store is + * dispatched to the libuv threadpool, so the wait covers queueing behind every + * other threadpool user in the process (fs, dns, crypto), not just DuckDB's own + * work. A busy agent process can therefore push a healthy open well past a + * second, and the previous hard-coded 2s ceiling turned that into an + * unrecoverable failure on a store that was fine. + */ +const DEFAULT_OPEN_TIMEOUT_MS = 30_000 + +/** + * Largest delay `setTimeout` represents. A larger value overflows the timer's + * 32-bit signed delay and is clamped to 1ms, which would turn a deliberately + * huge budget into an immediate deadline — the exact failure this file exists + * to remove. Clamp instead, so an over-large budget still behaves like a very + * long one. + */ +const MAX_TIMER_MS = 2_147_483_647 + +/** Read a positive, finite millisecond budget, or `undefined` if unusable. */ +function positiveMs(value: unknown): number | undefined { + const n = typeof value === "number" ? value : Number(value) + return Number.isFinite(n) && n > 0 ? Math.min(n, MAX_TIMER_MS) : undefined +} + +/** + * Where the budget came from, which decides how a caller should read a + * deadline failure: one the connection set is that connection's own doing and + * is fixed by changing it, while the default or a machine-wide env var firing + * says something about the machine instead. + */ +type TimeoutSource = "connection" | "env" | "default" + +function resolveOpenTimeoutMs(config: ConnectionConfig): { ms: number; source: TimeoutSource } { + const fromConfig = positiveMs(config.open_timeout_ms) + if (fromConfig !== undefined) return { ms: fromConfig, source: "connection" } + const fromEnv = positiveMs(globalThis.process?.env?.["ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS"]) + if (fromEnv !== undefined) return { ms: fromEnv, source: "env" } + return { ms: DEFAULT_OPEN_TIMEOUT_MS, source: "default" } +} +// altimate_change end + export async function connect(config: ConnectionConfig): Promise { let duckdb: any try { @@ -14,17 +59,42 @@ export async function connect(config: ConnectionConfig): Promise { } const dbPath = (config.path as string) ?? ":memory:" + // altimate_change start — configurable open budget + const { ms: openTimeoutMs, source: openTimeoutSource } = resolveOpenTimeoutMs(config) + // altimate_change end let db: any let connection: any // altimate_change start — improve DuckDB error messages + // Real DuckDB lock failures read "Could not set lock on file ... Conflicting + // lock is held", which contains "lock" but never "locked". Matching only + // "locked"/"DUCKDB_LOCKED" therefore missed every genuine lock collision, so + // the read-only retry never fired and concurrent readers just failed. + function isLockError(err: unknown): boolean { + const msg = (err instanceof Error ? err.message : String(err)).toLowerCase() + return ( + msg.includes("locked") || + // Both halves, not either. "could not set lock" on its own also covers + // non-contention failures — an unsupported filesystem lock, a permissions + // problem — and matching it alone would wrap those as "locked by another + // process", fabricating a wrapper that Registry.categorizeConnectionError + // then trusts as a recoverable `store_locked`. That would send the reader + // hunting for a process to close while hiding the real filesystem fault. + // Registry's raw matcher already requires both; these must agree. + (msg.includes("could not set lock") && msg.includes("conflicting lock")) || + msg.includes("sqlite_busy") + ) + } + function wrapDuckDBError(err: Error): Error { - const msg = err.message || String(err) - if (msg.toLowerCase().includes("locked") || msg.includes("SQLITE_BUSY") || msg.includes("DUCKDB_LOCKED")) { + if (isLockError(err)) { + // Keep DuckDB's own text: it names the PID and executable holding the + // conflicting lock, which is the only way to find the other process. return new Error( `Database "${dbPath}" is locked by another process. ` + - `DuckDB does not support concurrent write access. ` + - `Close other connections to this file and try again.`, + `DuckDB takes an exclusive file lock, so a store already open ` + + `read-write elsewhere cannot be opened again — not even read-only. ` + + `Close the other connection and try again.\n${err.message || String(err)}`, ) } return err @@ -58,10 +128,15 @@ export async function connect(config: ConnectionConfig): Promise { 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 { @@ -70,9 +145,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) { @@ -81,15 +159,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) } @@ -97,22 +172,54 @@ 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}"`)) + // Nothing can reach this handle once the promise rejects. If the + // callback arrives later it closes the handle itself (see the + // `resolved` branch in onOpen); if it never arrives, that branch + // never runs, so close here too. Both paths are idempotent + // because closeQuietly swallows a double close. + closeQuietly() + reject( + new Error( + `DuckDB store "${dbPath}" did not finish opening within ${openTimeoutMs}ms. ` + + `This is a client-side deadline in the DuckDB driver, not a fault in the store ` + + `— the open may simply be queued behind other work in this process. ` + + (openTimeoutSource === "connection" + ? // Named so callers can tell a self-inflicted deadline from one + // they did not choose. Registry.categorizeConnectionError keys + // off this phrase to report it as configuration rather than as + // a broken client. + `This deadline was set on this connection as open_timeout_ms=${openTimeoutMs}; ` + + `raise or remove it.` + : `Raise the budget with this connection's open_timeout_ms (which takes ` + + `priority) or ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS if the machine is loaded.`), + ), + ) } - }, 2000) - if (pendingOpen !== undefined) onOpen(pendingOpen) + }, openTimeoutMs) + if (pendingOpen !== NOT_FIRED) onOpen(pendingOpen) }) + // altimate_change start — honour an explicit read-only connection. + // DuckDB takes an EXCLUSIVE file lock when opened read-write, so N + // concurrent readers of one .duckdb file leave N-1 of them failing to + // connect at all. Opening READ_ONLY up front is the only way several + // processes can share a file, and it is what a caller that declared + // `readonly` asked for. Relying on the lock-error retry below is not + // equivalent: it is best-effort string matching, and it wastes a full + // open attempt per connection. + const wantReadOnly = config.readonly === true && dbPath !== ":memory:" try { - db = await tryConnect() + db = await tryConnect(wantReadOnly ? "READ_ONLY" : undefined) } catch (err: any) { - if (err.message === "DUCKDB_LOCKED" && dbPath !== ":memory:") { + // altimate_change end + if (isLockError(err) && !wantReadOnly && dbPath !== ":memory:") { // Retry in read-only mode — allows concurrent reads try { db = await tryConnect("READ_ONLY") @@ -121,6 +228,17 @@ export async function connect(config: ConnectionConfig): Promise { 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 6a0f8d8c5d..7f33e0a24e 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() @@ -133,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 @@ -179,6 +189,139 @@ 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 + // 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 + // 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 8be3bfc672..12ba2ba4d0 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -282,6 +282,30 @@ 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")) { + // 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("could not set lock") && msg.includes("conflicting lock")) + ) + 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 +314,31 @@ export function categorizeConnectionError(e: unknown): string { return "other" } +// altimate_change start — distinguish infrastructure faults from config faults +/** + * 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 + // --------------------------------------------------------------------------- // Public API // --------------------------------------------------------------------------- @@ -405,7 +454,15 @@ 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 + recoverable?: boolean +}> { try { const connector = await get(name) const config = configs.get(name) @@ -422,7 +479,16 @@ 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), + recoverable: isRecoverableFailure(category), + } + // altimate_change end } } @@ -516,9 +582,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() @@ -527,6 +600,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 ad90c7631e..f69f72e712 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -329,6 +329,23 @@ 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 + /** + * 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 } // --- Warehouse Management --- diff --git a/packages/opencode/src/altimate/tools/warehouse-test.ts b/packages/opencode/src/altimate/tools/warehouse-test.ts index e05e45b3d8..aa1d7d3ab3 100644 --- a/packages/opencode/src/altimate/tools/warehouse-test.ts +++ b/packages/opencode/src/altimate/tools/warehouse-test.ts @@ -20,9 +20,62 @@ 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. + // 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`, + 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/connections.test.ts b/packages/opencode/test/altimate/connections.test.ts index 5b52972eee..8224274560 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,73 @@ 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("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") + }) + + 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..0fc9f25705 --- /dev/null +++ b/packages/opencode/test/altimate/duckdb-lock-helper.ts @@ -0,0 +1,106 @@ +import { fileURLToPath } from "node:url" +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 +} + +// 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 { + const scriptPath = path.join(scratchDir, "hold-duckdb-lock.ts") + fs.writeFileSync( + scriptPath, + [ + `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. + ` 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", + ) + + 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)"}`) + })() + + // 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 { + 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 new file mode 100644 index 0000000000..2ea99f8199 --- /dev/null +++ b/packages/opencode/test/altimate/duckdb-open-e2e.test.ts @@ -0,0 +1,296 @@ +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" +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 +// 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 = "" + // 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 = "" + +// 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 { + 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") + 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") + 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. 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 " + + "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("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 }) + // 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(rows.length).toBe(1) + // And the right answer, which requires actually computing it. + expect(String(rows[0][0])).toBe(LIVENESS_MD5) + }) + + 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") + }) + + 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 () => { + 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 }) + 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 + } + }) + + 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("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(lockedPath, dir) + try { + 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 + // 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(lockedPath, dir) + try { + 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") + } 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(lockedPath, dir) + await lock.release() + const c = await connect({ type: "duckdb", path: lockedPath }) + await c.connect() + // `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() + }) + + 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..c277225677 --- /dev/null +++ b/packages/opencode/test/altimate/warehouse-test-duckdb-e2e.test.ts @@ -0,0 +1,213 @@ +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 { holdWriteLock } from "./duckdb-lock-helper" +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 = "" +// 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 + +async function warehouseTest(name: string) { + return Dispatcher.call("warehouse.test", { name }) +} + +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-")) + 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") + 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) { + 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.", + ) + } + }) + + // 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(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 }) + }) + + 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++) { + await Registry.closeAll() + 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. 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.error_category).toBe("config_error") + expect(result.infrastructure).toBe(false) + }) + + 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("a store locked by another process is reported as a recoverable lock", async () => { + const lock = await holdWriteLock(lockedPath, dir) + try { + await Registry.closeAll() + Registry.reset() + 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 + // 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(lockedPath, dir) + try { + await Registry.closeAll() + Registry.reset() + 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) + // 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: [] } + + Registry.reset() + Registry.setConfigs({ local: { type: "duckdb", path: storePath } }) + const ok = await tool.execute({ name: "local" }, ctx) + expect(ok.title).toContain("OK") + + Registry.reset() + 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. + 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") + }) +})