Skip to content

fix(db): install busy_timeout before the connection's first statement - #12394

Merged
diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.51from
pacocartones:fix/deflake-exclusive-connection-leases
Sep 2, 2026
Merged

diegosouzapw merged 2 commits into
cheaper-inference:release/v3.8.51from
pacocartones:fix/deflake-exclusive-connection-leases

Conversation

@pacocartones

Copy link
Copy Markdown
Contributor

Summary

  • getDbInstance() ran PRAGMA journal_mode = WAL as the connection's first statement, before PRAGMA busy_timeout = 2000, and openSqliteDatabase() passes no driver-level timeout. A process opening the database while another one is closing its WAL connection (checkpoint + WAL delete take an EXCLUSIVE file lock for a few hundred microseconds) therefore died with database is locked (errcode 5) at src/lib/db/core.ts instead of waiting. This is the failure behind the flaky cross-process contenders never both acquire the same connection in tests/unit/exclusive-connection-leases.test.ts, which failed on release/v3.8.51 runs 33525300898 and 33493797519 and on unrelated PR runs 33571057980 and 33576326489. The busy timeout is now installed before the first statement.
  • The corruption probe's isTransientProbeError matched /SQLITE_BUSY/ against error.message, but both drivers report the plain text database is locked (better-sqlite3 puts the code in .code, node:sqlite in .errcode). A transient lock during the probe therefore took the "corrupt database" path and renamed the file to storage.sqlite.probe-failed-… with Manual recovery required. The probe now recognises the drivers' real BUSY/PROTOCOL/IOERR signals (code SQLITE_BUSY|PROTOCOL|IOERR, errcode 5/10/15, or the message text) as transient.

Related Issues

Validation

  • Change type: Backend (SQLite open path)
  • Focused tests and category gates from the golden path: tests/unit/db-open-first-statement-busy-timeout.test.ts (new, deterministic), tests/unit/exclusive-connection-leases.test.ts (11/11, run 10 times sequentially: 10/10 green), tests/unit/probe-9541-repro.test.ts (6/6), npm run typecheck:core 0, npm run check:changelog-integrity OK
  • npm run lint — repository-wide eslint exit 0 (run with --pass-on-unpruned-suppressions; the literal command reports only pre-existing unused global suppressions on this base)
  • Reconciled with the current active release base release/v3.8.51; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR
  • SonarQube is temporarily opt-in while the private project has no quota; it is not a PR gate.

Mutation: with both fixes reverted the new test fails 10/10 (probe rename path); with only the probe fix it fails at core.ts getDbInstance with the identical stack CI shows; with both fixes it passes 10/10.

Tests Added Or Updated

  • tests/unit/db-open-first-statement-busy-timeout.test.ts (new): seeds a real database through getDbInstance(), holds the file's EXCLUSIVE lock from a node:sqlite connection (locking_mode = EXCLUSIVE plus one read keeps it until close), spawns a child that signals readiness and then calls getDbInstance(), releases the lock 1200 ms after the signal (longer than the probe's 500 ms retry, shorter than the 2000 ms busy timeout), and asserts exit code 0, no probe-failed rename, and busy_timeout = 2000 on the opened connection.

Coverage Notes

  • The reordered pragma block in src/lib/db/core.ts and the transient-error classification in src/lib/db/probeUtils.ts are both exercised by the new test; the existing lease and probe suites keep covering the surrounding behaviour.
  • Not changed here, noted for a follow-up: src/lib/db/adapters/runtimeRequire.ts builds createRequire from process.argv[1] || process.cwd() without a trailing slash, so processes without argv[1] (for example node -e workers) silently fall back from better-sqlite3 to node:sqlite; and the other openSqliteDatabase() call sites in core.ts also open without a driver-level timeout.

The "cross-process contenders never both acquire the same connection" test
(tests/unit/exclusive-connection-leases.test.ts) flaked in the Unit Tests
fast-path shard with `database is locked` thrown from getDbInstance() at
`PRAGMA journal_mode = WAL` (nodeSqliteShared.ts pragma). The root cause is
in the SQLite open path, not in the lease logic:

- getDbInstance() ran `journal_mode = WAL` (the connection's first
  statement, which takes a SHARED lock) before `busy_timeout = 2000`. A WAL
  connection that closes briefly holds the database file EXCLUSIVE while it
  checkpoints and deletes the WAL, so a process opening inside that window
  got SQLITE_BUSY with no busy handler installed. node:sqlite's DatabaseSync
  opens with busy timeout 0 (better-sqlite3 defaults to 5000ms), which is
  why only the eval children of that test, which fall back to node:sqlite,
  ever died: the faster contender exited while the slower one was opening.
- The corruption probe that runs first had the same exposure and a worse
  outcome: isTransientProbeError() matched /SQLITE_BUSY/ against the
  message, but both drivers report plain "database is locked" (better-sqlite3
  carries the code in `code`, node:sqlite in `errcode`), so a transient lock
  renamed the database to storage.sqlite.probe-failed-* and aborted with
  "Manual recovery required".

Set busy_timeout before the first statement and classify the drivers'
native BUSY/PROTOCOL/IOERR errors as transient in the probe.

Regression test: tests/unit/db-open-first-statement-busy-timeout.test.ts
seeds a real database, holds its file lock from another connection (WAL +
locking_mode=EXCLUSIVE keeps the EXCLUSIVE lock until close) and releases
it 1200ms after the child reaches getDbInstance(), so the open must wait it
out. It is deterministic: 10/10 failures before the fix (probe rename path),
failure at `journal_mode = WAL` in core.ts with only the probe fix applied,
10/10 passes after. exclusive-connection-leases.test.ts passes 10/10
sequential runs; typecheck:core, eslint (repo suppressions) and prettier
are clean.

Co-authored-by: Leon Marcos <leonaniagomez@gmail.com>
@diegosouzapw
diegosouzapw merged commit c8e2cb3 into cheaper-inference:release/v3.8.51 Sep 2, 2026
16 checks passed
diegosouzapw added a commit to backryun/OmniRoute that referenced this pull request Sep 2, 2026
…duplicate-pack-artifact

cheaper-inference#12394 landed the same probe/busy_timeout area first, so src/lib/db/core.ts takes the
tip's side. probeUtils.ts is the union of both: this PR's wider message regex (SQLite also
says "database table is locked", "database schema is locked" and "database is busy")
plus cheaper-inference#12394's driver code/errcode path — each covered a half the other missed, and the
PR's own ENOENT test caught it.
diegosouzapw pushed a commit that referenced this pull request Sep 2, 2026
…ntax, uc-image ids (#12423)

Three regressions inherited by every PR rebased onto release/v3.8.51, caught and documented with the exact failing output.

The one that mattered most: src/shared/providers/webSessionCredentials.ts did not parse. The UC merge (#11513) inserted the uc: entry inside maxai.storageKeys and lost the array's closing ], plus the entry's }, leaving `ERROR: Expected "]" but found ":"` at line 351. That module is imported by the provider API routes, bulk-web-session, autoCombo's virtualFactory, keepaliveThreshold and dashboard components, so the break was live on the tip and flooded unrelated catalog tests with transform failures. That was my conflict resolution, not the contributor's code — thank you for catching it and for tracing it to the root commit rather than patching around the symptom.

Also fixed: the duplicate bin/cli/utils/volatileEnvPath.mjs entry in PACK_ARTIFACT_REQUIRED_PATHS (findMissingArtifactPaths reported it twice), and UC image models made prefix-addressable without letting them claim historical bare model ids belonging to other providers.

Reconciled on merge: #12394 landed the busy_timeout/probe work first, so src/lib/db/core.ts takes the tip's side. probeUtils.ts is the union of both rather than either side — this PR's message regex is wider (SQLite also reports "database table is locked", "database schema is locked" and "database is busy"), while #12394 added the driver code/errcode path that keeps a transient lock from being classified as corruption and renaming the database away. Taking either alone would have dropped the other half; this PR's own ENOENT test is what surfaced it.

Verified: 76/76 across uc-image, probe-9541-repro, web-session-contract, pack-artifact-policy, bulk-web-session-import and exclusive-connection-leases, and every changed .ts file parses.

Thanks @backryun.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…cheaper-inference#12394)

getDbInstance() ran PRAGMA journal_mode = WAL as the connection's first statement, before PRAGMA busy_timeout, and openSqliteDatabase() passes no driver-level timeout. A process opening the database while another closed its WAL connection — checkpoint plus WAL delete hold an EXCLUSIVE lock for a few hundred microseconds — therefore died with `database is locked` instead of waiting. That is the flake behind exclusive-connection-leases.test.ts on release/v3.8.51 runs 33525300898 and 33493797519 and on unrelated PR runs.

The second half is worse than the flake: isTransientProbeError matched /SQLITE_BUSY/ against error.message, but both drivers report the plain text `database is locked` and put the code in .code / .errcode. A transient lock during the corruption probe therefore took the corrupt-database path and renamed the file to storage.sqlite.probe-failed-… with "Manual recovery required". The probe now recognises the drivers' real BUSY/PROTOCOL/IOERR signals.

Validated in a combined worktree with all 25 PRs of this batch boarded together: typecheck:core clean, 443/443 node-runner tests plus 14/14 vitest across every test file the batch touches, and check-changelog-integrity, check:cycles (418 files), check:provider-consistency (272 REGISTRY entries, 355 canonical providers), check:docs-counts, check:docs-sync (42 locales) and check-file-size all green.

Thanks @pacocartones.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ntax, uc-image ids (cheaper-inference#12423)

Three regressions inherited by every PR rebased onto release/v3.8.51, caught and documented with the exact failing output.

The one that mattered most: src/shared/providers/webSessionCredentials.ts did not parse. The UC merge (cheaper-inference#11513) inserted the uc: entry inside maxai.storageKeys and lost the array's closing ], plus the entry's }, leaving `ERROR: Expected "]" but found ":"` at line 351. That module is imported by the provider API routes, bulk-web-session, autoCombo's virtualFactory, keepaliveThreshold and dashboard components, so the break was live on the tip and flooded unrelated catalog tests with transform failures. That was my conflict resolution, not the contributor's code — thank you for catching it and for tracing it to the root commit rather than patching around the symptom.

Also fixed: the duplicate bin/cli/utils/volatileEnvPath.mjs entry in PACK_ARTIFACT_REQUIRED_PATHS (findMissingArtifactPaths reported it twice), and UC image models made prefix-addressable without letting them claim historical bare model ids belonging to other providers.

Reconciled on merge: cheaper-inference#12394 landed the busy_timeout/probe work first, so src/lib/db/core.ts takes the tip's side. probeUtils.ts is the union of both rather than either side — this PR's message regex is wider (SQLite also reports "database table is locked", "database schema is locked" and "database is busy"), while cheaper-inference#12394 added the driver code/errcode path that keeps a transient lock from being classified as corruption and renaming the database away. Taking either alone would have dropped the other half; this PR's own ENOENT test is what surfaced it.

Verified: 76/76 across uc-image, probe-9541-repro, web-session-contract, pack-artifact-policy, bulk-web-session-import and exclusive-connection-leases, and every changed .ts file parses.

Thanks @backryun.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants