PR #1198 - AltimateAI/altimate-code

Issue for this PR

Type of change

Bug fix
New feature
Refactor / code improvement
Documentation

What does this PR do?

The bug. warehouse_test failed on local DuckDB stores with Timed out opening DuckDB database "<path>". Seven calls across six separate compiled-binary processes, durations 2061 / 2002 / 2005 / 2005 / 2004 / 2003 / 2004 ms — six of seven within 5ms of exactly 2000. A fixed deadline firing, not contention, which scatters. The stores were fine: in the same processes moments later, python3 -c "import duckdb" opened the same 1.3MB files and queried them.

packages/drivers/src/duckdb.ts rejected the open after a hard-coded 2000ms with no way to raise it. That is 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 (fs, dns, crypto), not just DuckDB's own work. On a loaded machine a healthy open crosses it, and the driver then rejects and closes the handle that was about to succeed, leaving the caller no way to recover. The budget now defaults to 30s and is settable per connection (open_timeout_ms) or by ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS.

A second, live path to the identical symptom. The "open callback fired synchronously" sentinel was undefined — which is exactly what a success callback invoked with no arguments passes. Such a callback was stored in pendingOpen, the replay guard if (pendingOpen !== undefined) then read it as "has not fired yet", the promise never settled, and the open failed on the deadline. That produces this exact string, deterministically, at exactly 2000ms. The sentinel is now a symbol the callback cannot supply, and a zero-argument success is normalised to null.

The expensive half: the failure was silent. warehouse_test reported Connection 'x': FAILED for a driver that will not load and for a wrong password alike, so an infrastructure fault read as a configuration fault — or, to a model or anyone reading a transcript, as the task simply not working. Registry.test() now returns error_category and infrastructure, and the tool renders local-client faults (driver_missing, driver_open_timeout, store_locked) as an unmistakable INFRASTRUCTURE FAILURE with infrastructure: true in metadata. categorizeConnectionError already existed for telemetry; this reuses it and adds the two categories the generic timeout / not found rules were swallowing.

Read-only and lock detection (cherry-picked, not rewritten). The first commit is 9d05c9f37a verbatim, from a branch it had been sitting on alone. It fixes two things:

config.readonly was read nowhere in duckdb.ts — across packages/drivers/src/ the only reader is sqlite.ts:15. A caller declaring a read-only connection still got a read-write open and took the exclusive lock.
The read-only retry was unreachable, not merely narrow. onOpen rejected with Error("DUCKDB_LOCKED") only when msg.includes("locked") || includes("SQLITE_BUSY") || includes("DUCKDB_LOCKED") matched, and the outer gate was err.message === "DUCKDB_LOCKED". DuckDB's real message is Could not set lock on file …: Conflicting lock is held, which contains "lock" but never "locked", so the matcher returned false and the retry path could never execute on a genuine lock collision.

On top of that commit I dropped the DUCKDB_LOCKED normalisation inside onOpen and made wrapDuckDBError append rather than replace, because DuckDB's own text names the PID and executable holding the lock and that is the only way to find the other process. One assertion in driver-security.test.ts was updated to match the new wording.

A separate defect found while reproducing. duckdb was missing from trustedDependencies, so bun install never ran its node-pre-gyp install lifecycle script and lib/binding/duckdb.node was never fetched. On such a tree every DuckDB call fails with "driver not installed" even though the package is present. drivers-e2e.test.ts already carries a comment working around this exact symptom. I am calling it out separately because it is not the cause of the field failure above — the affected traces contain zero occurrences of not installed, npm install, binding, MODULE_NOT_FOUND or Cannot find module.

Correction — this deadline was not the whole cause of the field failure. The paragraph above says the affected traces contain no resolution-failure strings, and that is still true of those traces. But a later investigation found the field failure was coupled, not single-cause: driver resolution fell back to an on-demand install, eight of those ran concurrently and corrupted the shared dependency tree, and every load after that failed against the wreckage. The 2000ms deadline is a real defect and it did fire, but it was one link in that chain rather than the sole cause, and fixing it alone would not have made that run pass. I am leaving the timing evidence above as recorded, and correcting the conclusion drawn from it.

Not this PR. #1122 fixes driver resolution (a bare await import("duckdb") resolving against bunfs in a compiled binary). The code is disjoint — that PR touches the file's head, this touches wrapDuckDBError and the connect() body, and 9d05c9f37a cherry-picks onto current main with no conflict — but per the correction above the two failures are links in one chain, not unrelated events.

How did you verify your code works?

Field evidence, added after review: this PR's absence causes measured data loss under concurrency.

A benchmark rig launches concurrent trials that share a DuckDB store by construction. Two binaries, same store, same concurrency of 8, differing only in whether this PR is present:

binary	canary returned	lock conflicts	driver load failures
without this PR	1 / 8	7	0
with this PR	8 / 8	0	0

IO Error: Could not set lock on file … Conflicting lock is held … (PID 23384)

Seven of eight concurrent readers could not open a store they had every right to read, because config.readonly was dropped on the floor and the read-only retry was gated on a string that a real DuckDB lock error never contains. The affected surface is 8 of 12 datasets and 40 of 54 queries; a sweep run without this PR would have completed and produced a plausible, fully-traced number from trials that mostly read nothing.

Rig-side causes were eliminated before this comparison was drawn: no WAL on the store, an explicit CHECKPOINT changed nothing, and four concurrent READ_ONLY opens through the module itself all succeeded.

This is independent of the unit tests below — it is a controlled A/B in the environment the code actually runs in, and it is the strongest reason to merge.

Against a real DuckDB store through the real Dispatcher.call("warehouse.test", …) path — the same call the tool makes. A mock is what missed this bug in the first place.

Reproduced first, from two directions, before changing anything:

Condition	Result on `main`
Write lock held by a second process	0/7 — `IO Error: Could not set lock on file … Conflicting lock is held in … (PID 65001)`
`lib/binding/duckdb.node` absent	0/7 — `DuckDB driver not installed. Run: npm install duckdb`

Then the mechanism, in one process on one file: with open_timeout_ms: 1 the driver fails; at the default it succeeds and returns the row. Same store, same process — the deadline was the failure, not the store, which is what the field evidence showed.

Ruling out the resolution bug as the cause of the timeout. I compiled a probe with the production compile options from script/build.ts (external: ["duckdb"], autoloadPackageJson: true) and ran it from a cwd with no node_modules: resolution failure surfaces as DuckDB driver not installed in 0-2ms, never as a timeout. From a cwd where duckdb resolves, the same compiled binary opened the store and queried it. So #1122's failure and this one are genuinely different events.

Repeat runs, because a fix that is merely usually right would recreate the contamination this replaces:

ALTIMATE_DUCKDB_E2E=1 bun test on the two e2e files, 25 consecutive runs, 25/25 fully green (325 test executions).
Compiled binary, 30 fresh processes × 7 warehouse.test calls = 210/210 connected, 0 failures.

Concurrency, measured across separate OS processes with the real package (independently reproduced by a colleague):

Experiment	Result
Write lock held by another process; probe `default` / `READ_ONLY` / `read_only`	all three fail
9 stores × 8 concurrent opens, READ_ONLY up front	9/9 stores, 72/72 opens
Same, opened read-write (`main`'s behaviour)	0/9 stores (per-store 4,5,4,3,4,4,4,4,4 of 8)

The read-only retry cannot rescue a lock already held read-write by another process — DuckDB's lock is exclusive against read-only opens too. It is the up-front config.readonly open that does the work. Quote the 9/9 only with that precondition.

trustedDependencies, both directions: a scratch bun install with trustedDependencies: ["duckdb"] produces lib/binding/duckdb.node; without it the directory is empty, which is the state a fresh worktree of this repo was in.

New tests fail on unmodified main — I swapped main's duckdb.ts in and reran: 4 of 8 fail there (the config.readonly test and the three deadline tests) and all 8 pass with this change.

About the test placement, because it is unusual and a reviewer will ask. Four files in test/altimate install a top-level mock.module("@altimateai/drivers/duckdb", …), Bun evaluates every test file's top level before running any test, and mock.restore() does not undo a module mock. So in a whole-directory run the real driver is already replaced no matter how files sort or which specifier the test imports — I confirmed order makes no difference. These files are therefore gated on ALTIMATE_DUCKDB_E2E=1 with a dedicated CI step, following the existing drivers-e2e.test.ts pattern. When the gate is on, a missing or mocked driver fails rather than skips: a green e2e file that quietly skipped everything is the same class of defect as the one this PR fixes.

Gate	Result (after the review round)
`bun run typecheck`	13/13 successful
`bun run script/upstream/analyze.ts --markers --base origin/main --strict`	ok — no upstream-shared files modified. Run against `origin/main`, not a bare `main`: a stale local `main` hides violations CI catches
`bun run lint`	5863 warnings, 1 error. The error is the known pre-existing `consistent-return` in `packages/http-recorder/test/record-replay.test.ts`. The warning count is +1 against this branch's previous commit, from an un-awaited `mock.module` in a new test — which is how every other `mock.module` in that file is written
`packages/drivers` full suite	141 pass, 0 fail
`packages/opencode` `test/altimate` (gate off)	4222 pass, 0 fail, 151 files
`ALTIMATE_DUCKDB_E2E=1`, the two e2e files	19 pass, 0 fail

Not verified by me:

bun run typecheck reporting 13/13 does not type-cover packages/drivers/src/duckdb.ts — that package declares no typecheck script, so it is not in the turbo task. Do not read 13/13 as coverage of the main change. It does cover packages/opencode, tests included: it caught a real type error in the new lock helper during the review round, which is the only direct evidence here of what the gate does and does not reach.
A green bun test --cwd packages/drivers proves nothing about module resolution. That package's own node_modules is reachable for the whole run, so the resolution failure in #1122 cannot appear in it by construction. Two separate investigations drew the wrong conclusion from exactly that command. The only evidence about resolution in this PR is the compiled-binary probe described above.
I could not reproduce the 2000ms timeout on an idle laptop, which is expected: the deadline fires under load. The mechanism is demonstrated by making the budget the variable rather than by recreating the field load, and by the field's own timing distribution.
Windows, and the 30s default under a genuinely pathological store (very large, or a large WAL to replay). 30s is a judgement call, not a measured figure.
The trustedDependencies addition makes bun install fetch a ~55MB prebuilt binary for everyone, including contributors who never touch DuckDB. I think that is the right trade because the alternative is a driver that silently does not work, but it is a cost and a reviewer may disagree.

Review round (commit e0d5decd10). 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 lock is exclusive against read-only opens too, so retrying READ_ONLY when we already asked for READ_ONLY only repeats the failure. 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.
The classifier now also matches DuckDB's raw text (conflicting lock, could not set lock), so a lock reaching it unwrapped from any other path is still classified.
A locked store is no longer rendered as a broken client. Two reviewers were right that INFRASTRUCTURE FAILURE — stop and report is wrong advice for a lock. It stays infrastructure (nothing about the config is wrong, and a harness must not score it as a task failure) but is now also recoverable, and the tool renders STORE LOCKED with the remedy the driver's own message already gives.
setTimeout clamps a delay past 2^31-1 to about 1ms, so a deliberately huge open_timeout_ms became an immediate deadline. 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 wins.
A deadline firing because the callback never arrives left the handle open. A late callback closes it via onOpen; when it never arrives the timeout closes it. Both 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 E2E files its own step runs, so a PR touching only those files got no execution anywhere.
Registry.reset() is synchronous and close() is not, so it leaked a native handle per connector. Added Registry.closeAll(), used by reload() and the E2E suite.
The E2E suite deleted ALTIMATE_TELEMETRY_DISABLED on teardown instead of restoring it.
bun.lock did not record the trustedDependencies addition.

Tests added. driver-security.test.ts pins the read-only wrap and that the open is attempted exactly once with READ_ONLY — it fails without the fix. connections.test.ts adds eight ungated cases pinning the classification, including DuckDB's verbatim lock text (which contains lock but never locked). A new duckdb-lock-helper.ts holds a write lock from a separate OS process, because DuckDB's lock is per-process and an in-process second open proves nothing; the E2E suite uses it to cover read-only and read-write opens against a foreign lock, that the store is usable once released (which guards the helper itself), and the STORE LOCKED rendering.

The review round's own tests hit this PR's bug, and CI caught it. The first push of the round added a helper that holds a DuckDB write lock from a separate OS process. It resolved duckdb with createRequire(import.meta.url) from packages/opencode/test/altimate/. That passed on my machine and failed on CI with Cannot find package 'duckdb' — duckdb is a dependency of packages/drivers and lives only in packages/drivers/node_modules. Same class of fault as the one this PR exists to fix, reintroduced by the test written to prove the fix. The helper now takes the lock through the driver, imported by the absolute path both E2E suites already resolve.

Fixing that surfaced a second one worth recording: with resolution working, all three driver-level lock tests failed locally because the test runner itself still held the write lock. 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. The lock tests now use their own store that this process never opens successfully. Six consecutive gated E2E runs, 19/19 each.

Second review wave (commit 6ac0c640ae). Seventeen threads; two of them found gaps that mattered more than anything in the first wave.

packages/drivers had never run 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 have failed with no job noticing. Added a Run drivers unit suite step to the driver-e2e job. Worth flagging: enabling it could surface pre-existing Linux failures a macOS run does not show.
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.
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 that connection's own setting caused. Same category error this PR removes, pointing the other way. resolveOpenTimeoutMs now returns the source; a connection-scoped budget names itself and classifies as config_error, while the default or env var stays driver_open_timeout and infrastructure.
The raw-lock fallback matched could not set lock alone, 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.
The drivers filter ignored package.json, bun.lock and packages/drivers/package.json, which govern whether the native binding is fetched at all.
The lock helper's 30s readiness timer was never cleared, and its child imported the driver outside its try so a load failure surfaced as a bare unhandled rejection.

Declined, with reasons on the threads: per-test tmpdir() scoping for the two E2E files (they run in a dedicated single-file Bun process, sequentially, and the lock tests specifically need a store this process never opens — DuckDB's lock is per-process); per-test mock.module teardown in driver-security.test.ts (every test in that block already installs its own mock, and isolating only the new ones would leave the file inconsistent); and generation-invalidating the connector cache in reload() (a real latent race, but unchanged by this PR and deserving its own change with its own concurrency tests).

Final gate results: typecheck 13/13; markers ok against origin/main; packages/drivers 142 pass / 0 fail; test/altimate 4224 pass / 0 fail; gated DuckDB 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 this branch's pre-review-round commit, both un-awaited mock.module calls in new tests, matching how every other mock.module in that file is written.

Liveness assertion (commit c06a851e31). Every other 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. That 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 downstream.

md5() is not: a dead connection cannot return one row, and a live one cannot return the right digest without hashing the input. The probe now gates the whole file (in the beforeAll that already refused to run against a fake, but did so on a row count a stub can trivially return) and is asserted explicitly in its own test. The expected digest is computed independently and cross-checked, never by asking DuckDB — deriving it from the system under test would make the check circular.

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 entirely; a probe of this shape is what caught it, after four earlier probe designs passed while proving nothing.

Verified load-bearing rather than decorative, since a liveness check that cannot fail is the very defect it exists to prevent: DuckDB independently returns the expected digest; a wrong digest aborts the whole file at the gate (0 pass, 1 fail); a wrong digest in the test alone, gate intact, fails that test (13 pass, 1 fail); and appending WHERE false to reproduce the (0 rows) shape fails it too — the swallowed-error case specifically.

Screenshots / recordings

Not a UI change.

Checklist

I have tested my changes locally
I have not included unrelated changes in this PR

[!NOTE]
Medium Risk
Changes native DuckDB connection behavior, agent-facing warehouse test messaging, and CI coverage for local stores—high impact for DuckDB users but scoped to drivers/connections rather than auth or remote warehouse protocols.

Overview
Replaces the DuckDB driver’s fixed 2s open deadline with a 30s default and tunable budgets (open_timeout_ms, ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS), fixes the sync open callback sentinel so zero-arg successes no longer hang until timeout, and improves lock detection (DuckDB’s real “conflicting lock” text), config.readonly opens, error wrapping, and cleanup when the callback never fires.

warehouse.test / registry now classify local-client failures (driver_open_timeout, store_locked, etc.), expose infrastructure / recoverable, and the warehouse_test tool distinguishes INFRASTRUCTURE FAILURE from STORE LOCKED (close-and-retry) vs ordinary config failures. Adds Registry.closeAll() for async connector teardown.

Install & CI: adds duckdb to trustedDependencies so the native binding installs; runs packages/drivers unit tests; runs gated real-DuckDB E2E in a dedicated step (ALTIMATE_DUCKDB_E2E=1, 90s timeout) and widens path filters so those tests and lock files still trigger the driver job.

^{Reviewed by Cursor Bugbot for commit b1fa3f1bb78a4f4b09ab8c00c4e48a1d32ed22cd. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by cubic

Fixes the DuckDB driver failing healthy local stores with a 2s open deadline, and makes broken clients report as infrastructure faults instead of failed connections. Closes #1197.

Bug fixes

Raises the open timeout from a hard-coded 2000ms to 30s, settable per connection (open_timeout_ms) or via ALTIMATE_DUCKDB_OPEN_TIMEOUT_MS; the old deadline fired under load because opens queue behind other libuv threadpool work.
Fixes a second path to the same timeout: a synchronous success callback was recorded as undefined (the "not fired" sentinel), so the open never settled and hit the deadline.
Honors config.readonly by opening READ_ONLY up front; lock detection now matches DuckDB's real message, which contains "lock" but never "locked", and requires both "could not set lock" and "conflicting lock" so non-contention failures keep their own message instead of being mislabeled as a lock.
Wraps lock errors on explicit read-only opens too (still never retried, :memory: excluded), keeping DuckDB's original text that names the PID and executable holding the conflicting lock.
Clamps over-large budgets so setTimeout can't turn them into an instant deadline, and the deadline now closes the native handle when the open callback never arrives.
Adds Registry.closeAll(); synchronous Registry.reset() leaked a native handle per connector.

Reporting and tests

Registry.test() now returns error_category, infrastructure, and recoverable; warehouse_test renders driver_missing/driver_open_timeout as INFRASTRUCTURE FAILURE and store_locked as STORE LOCKED with close-and-retry advice. A deadline the connection set itself classifies as config_error, and categorizeConnectionError matches DuckDB's raw lock text so an unwrapped lock no longer falls through to other.
Adds duckdb to trustedDependencies so bun install fetches the native binding; without it every DuckDB call fails with "driver not installed".
New e2e tests open a real store on disk, gated on ALTIMATE_DUCKDB_E2E=1 (gate-off skips, gate-on hard-fails on a missing or mocked driver), and include an md5 liveness probe that a dead or fake driver cannot satisfy. CI now also runs the packages/drivers unit suite (previously never ran) and a dedicated gated step with an explicit 90s timeout.

Summary by CodeRabbit

Bug Fixes
- Improved DuckDB connection handling for timeouts, read-only access, lock detection, and cleanup.
- Warehouse connection checks now distinguish infrastructure failures, locked stores, recoverable errors, and ordinary failures.
- Error messages now provide clearer failure categories and recovery guidance.
- Resolved issues that could leave connections open after failed operations.
Tests
- Added end-to-end coverage for real DuckDB stores, concurrent access, timeouts, read-only behavior, and lock recovery.
- Expanded validation of warehouse-test failure reporting and connection lifecycle management.