fix(registry): give the SQLite contention retry a time budget - #110
Conversation
|
Gated on top of The change is sound and the reasoning in the body is right. I checked the thing I most wanted to check — whether the budget can truncate the existing ladder and reintroduce the race #104 fixed — and it cannot:
The deadline is set once before the first attempt and every sleep is One blocker, one line: the budget uses a wall clock. const deadline = performance.now() + SQLITE_RETRY_BUDGET_MS;
// ...
const remainingMs = deadline - performance.now();This matters more here than it usually would, because this runs at process start and Two non-blocking notes, your call:
I also want to credit the framing in the PR body: you were straight that this raises a ceiling from 1.76 s to 15 s against observed 3.5 s contention, that it does not remove the contention, and that the real fix is serialising init behind a lock file. That is the right call for a flake fix and the right way to describe it. Push the |
The Windows leg failed on hristo2612#105 with "attempt to write a readonly database" from one of 16 processes initializing the registry -- the same class hristo2612#104 addressed at journal_mode, now at the schema-init transaction one frame along. The retry was engaging correctly: the error code is SQLITE_READONLY, which the predicate matches, and runSqliteBusyRetry is in the stack. It simply ran out. The ladder [10, 50, 200, 500, 1000] spends 1.76s, and the worker that died had been contending for 3.5s. An attempt count is the wrong unit for this. What is being waited out is a window of contention whose length has nothing to do with how many times we have asked, so this is now a time budget: 15s on Windows, 5s elsewhere, matching the busy_timeout already set on the connection. Backoff is exponential and jittered -- without jitter, peers that collide once back off by the same amount and collide again, which is how a ladder that looks generous still exhausts itself. Measured rather than assumed. Instrumenting the give-up path at six times CI's concurrency produced `RETRY-GAVEUP elapsed=15015ms code=SQLITE_READONLY`: the loop engages, backs off, and exhausts the whole budget. So this raises the ceiling from 1.76s to 15s against observed contention of 3.5s, and it is not a guarantee -- no bounded wait can be one. The change that would remove the ceiling is serializing initialization across processes, which is larger and deserves its own review. At CI-equivalent load (16 processes) this is 0 failures in 10 local runs. A comparison against main at 96 processes is within noise, because at that concurrency both exhaust whatever budget they are given.
Date.now() is a wall clock. This runs at process start and Atomics.wait blocks the thread, so a backward clock step during the wait would extend a synchronous block by the size of the step, unbounded and unlogged; a forward step would silently truncate the budget. performance.now() is monotonic from process start.
d48cdc2 to
b5a49d6
Compare
|
Applied the Re-gated after the rebase, since Merging. I have also opened #121 for the real fix, and it is yours if you want it. Your framing is what the issue is built on: this raises a ceiling rather than removing one, and no bounded wait suffices for unbounded contention. What tipped it from a nice-to-have to a priority is that I watched the same bug fail different tests on two separate merges today — The issue proposes the lock file you flagged, with crash-safety by pid liveness plus an mtime bound rather than a timeout, and acceptance at ten consecutive green Windows runs plus a killed-mid-migration test. No obligation at all — say either way and I will pick it up if you would rather not. |
… registry (#113) Database ownership moves out of the Sessions registry. sessions/registry.ts sheds 1239 lines into three new files: sessions/migrate.ts (schema and migrations), shared/db.ts (connection ownership), and shared/sanitize.ts. The other 104 files are one-line import-path updates. This is relocation, verified mechanically rather than asserted: a line-multiset of the old registry.ts minus the new one was diffed against the three new files, and every line that left reappears verbatim. The only unaccounted items are new file headers, a deleted doctrine prose block, two dropped ticket refs, and one inline CREATE TABLE hoisted into a named const with an identical SQL body. The single real code change is mcp/knowledge-tools.ts dropping its local hasControlBytes for the shared one, whose body is byte-identical. initDb() semantics were confirmed empirically, not by reading: initDb was booted against a throwaway home on both main and this branch, and sqlite_master plus pragmas dumped. 97 objects, identical SQL text, identical journal_mode, busy_timeout, user_version, foreign_keys, synchronous and wal_autocheckpoint. shared/paths.ts is untouched, so assertTestRunIsIsolated still fires before any database open: shared/db.ts imports SESSIONS_DB from it, keeping the guard on the path of every open. A value-import cycle check over packages/jinn/src reports zero cycles through the new modules. Two PRs landed on registry.ts during the rebase. #112's dropActivityLedgerSchema and #110's monotonic retry budget were both hand-ported into their new homes and verified string-identical to main, which matters because a relocation reverts such changes silently rather than raising a conflict.
The Windows leg failed on #105 with
attempt to write a readonly databasefrom one of the 16 processes incallback-concurrent-init. It is not #105's — that PR touches engine spawning — and it is not a new flake: it is the same class #104 addressed atjournal_mode = WAL, surfacing one frame along at the schema-init transaction.Raising this separately so #105 is not held behind it, and so the reasoning is reviewable on its own.
The retry was working; it ran out
Worth establishing before changing anything, since "add more retries" is the kind of fix that is easy to apply and hard to justify:
SQLITE_READONLY(verified directly against better-sqlite3, not assumed), whichisTransientSqliteErroralready matches;runSqliteBusyRetryis in the failing stack, so the wrapper engages;[10, 50, 200, 500, 1000]spends 1.76s, and the worker that died had been contending for 3.5s.So it was giving up mid-race.
An attempt count is the wrong unit
What is being waited out is a window of contention whose length has nothing to do with how many times we have asked. This is now a time budget — 15s on Windows, 5s elsewhere — matched to the
busy_timeout = 10000already set on the connection, so the two agree about how long this class of contention is worth waiting for.Backoff is exponential and jittered. Without jitter, peers that collide once back off by the same amount and collide again on every subsequent attempt, which is how a ladder that looks generous still exhausts itself.
What I can and cannot claim
I instrumented the give-up path and ran it at six times CI's concurrency (96 processes against one database):
The loop engages, backs off, and exhausts the entire budget. That is the honest result, and it cuts both ways:
A straight comparison against
mainat that concurrency is within noise (1/12 vs 2/12), for the same reason: at 96 processes both exhaust whatever they are given. At CI-equivalent load (16 processes) I get 0 failures in 10 local runs, but I would not lean on that either —mainalso passes locally at that load, which is precisely why this only ever showed up on the runner.The change that would remove the ceiling is serializing initialization across processes — a lock file around the migration, so peers wait once rather than colliding repeatedly. That is a larger change to the boot path and deserves its own review rather than being smuggled in behind a flake fix. Happy to take it if you want it; the comment in the code says the same thing so the next reader does not mistake this for a guarantee.
Why it matters outside CI
The gateway, the CLI and session workers all open this database. Sixteen simultaneous openers is a test construct, but two or three is an ordinary Windows session, and those were exposed to the same race with a 1.76s tolerance.