fix(sessions): retry a WAL conversion that collides on a fresh database - #1
Merged
Merged
Conversation
`busy_timeout` covers an ordinary lock wait on `journal_mode = WAL` — a connection merely holding the database is waited out and the conversion then succeeds. What it does not cover is several processes converting the same brand-new file at the same moment: they collide inside the conversion rather than queueing on a lock, and one loses. Only the first run can hit this, since WAL is persistent in the file, but the first run is exactly when parallel tool calls all open the index at once. Both errors the collision raises are transient. Retrying either inside the existing busy budget takes six concurrent openers of one fresh database from 22 failures in 300 runs to 0 in 480. Tolerating a failed conversion instead does not work: the connection does not survive one, and the next statement on it fails too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to colbymchenry#1702 — targets that branch, not main. It closes the residual first-run race I measured in my comment there.
What's actually broken
colbymchenry#1702's head commit sets
busy_timeoutbefore the schema writes, which is right and fixes most of it. But six processes opening the same freshsessions.dbstill failed 11 times in 120 runs, and attributing each failure by frame put every single one on one statement —PRAGMA journal_mode = WAL— never the schema write, neverrefresh().I first assumed
busy_timeoutjust doesn't apply to that pragma. That was wrong, and I want to be precise about it because it changes the fix. It does apply: a holder takesBEGIN EXCLUSIVEon a fresh database and commits after 400 ms, and a second connection with a 5 s budget waits 429 ms and converts fine. An ordinary lock wait is covered.What isn't covered is several processes converting the same brand-new file at the same moment. They collide inside the conversion rather than queueing on a lock. Same 5 s budget, six concurrent converters on a fresh file: 2/60 still threw. This is a first-run-only defect — WAL is persistent, so nothing converts the file twice — but the first run is exactly when parallel tool calls all open the index at once.
The fix
Retry the conversion on either transient error inside the budget it already has, with jitter.
Both errors are transient.
database is lockedis the conversion losing the race;disk I/O erroris the-shmfile being created underneath a concurrent opener (I saw this on Windows). Retrying only the first is what makes the difference between "mostly fixed" and fixed:database is lockedonlyArms were round-robined across trials so no arm ate the cold start.
I also tested just tolerating a failed conversion and staying in the default journal mode. That does not work — the connection doesn't survive the failure and the next statement on it fails too (
FAIL mode,FAIL schemain my harness). So retry is the option, not fallback.Tests
Two, plus one existing behaviour now pinned:
enterWalModeretries each transient error and succeeds — driven directly, because the collision only reproduces probabilistically and I'm not shipping you a flaky test.enterWalModerethrows a non-transient error rather than spinning until the deadline.busy_timeoutdoes cover, which is what I originally got wrong.Being straight about the limits of these: the unit tests exercise the retry contract, they don't reproduce the race. The evidence that the race is closed is the 480-run harness, not the suite.
enterWalModeis exported for that first test; the alternative was a probabilistic test.Verification
tsc --noEmitclean.__tests__/sessions-index.test.ts— 11/11 pass, and the two new tests fail without the change.Absolute rates are Windows + WAL with six processes hammering one new database, so they're environment-specific. The arm-to-arm comparison is same-host and interleaved, which is the part I'd stand behind.