fix(runtime): retry the sqlite WAL switch under the busy timeout; await MCP session admission in tests - #567
Merged
Conversation
…it MCP session admission in tests Two CI flakes in `Verify (Node 24)`, each fixed at its root cause. state-sqlite-cross-process: `PRAGMA journal_mode = WAL` on a rollback-mode database upgrades a read transaction to a write (SHARED -> RESERVED), the one lock transition SQLite never routes through the busy handler, so two processes racing the first open of a state file failed SQLITE_BUSY at once regardless of `busy_timeout`. The driver now retries that single statement for up to `busyTimeoutMs`; once the header says WAL the statement is a plain read and never contends again. Extended result codes map by primary code. mcp-session-service: the 10 ms / 25 ms sleeps standing in for "the call has been admitted" are replaced by an admission promise resolved from inside the stub client, and by a trace subscription that resolves when the `tools/call` frame is put on the wire.
🦋 Changeset detectedLatest commit: d36cb68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
commit: |
Schedule.upTo compared elapsed time before sleeping the next interval, so an attempt could start up to one pause past the budget (and a budget below 5 ms still waited 5 ms). Clamp the pause to the budget and recur only while the next attempt starts inside it. Self-review finding on #567.
This was referenced Sep 5, 2026
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.
Eliminates two intermittent
Verify (Node 24)failures at their root cause. Each was observed in the last 40 CI runs and previously "fixed" by a blind re-run (~10–13 min per occurrence).Flake A —
packages/rsc-runtime/tests/state-sqlite-cross-process.test.tsSymptom (run 33926598550, attempt 1): a writer child exits 1 with
AgentStateError: State 'state-cross-process/tasks' storage stayed locked beyond the busy timeout (configure storage), causedatabase is locked(SQLITE_BUSY), thrown fromdist/state/sqlite.js:627:20— which is exactlydb.exec('PRAGMA journal_mode = WAL').Root cause —
packages/rsc-runtime/src/state/sqlite.ts(SqliteStore#initialize, formerly lines 929–936). The driver already setPRAGMA busy_timeoutbefore switching to WAL, so hypotheses (1)/(2) as literally stated were not it. The actual rule is SQLite's own (pager.c,sqlite3PagerSetBusyhandler): the busy handler is invoked only for theNO_LOCK → SHAREDandRESERVED → EXCLUSIVElock transitions, never forSHARED → RESERVED(deadlock avoidance).PRAGMA journal_mode = WALon a rollback-mode file runssqlite3BtreeSetVersion, which opens a read transaction (SHARED) and then upgrades it to a write (RESERVED) to rewrite header bytes 18–19. When a second process is mid-switch and holds RESERVED, that upgrade returns SQLITE_BUSY immediately, regardless ofbusy_timeout. The window is the duration of one fsync-bounded write transaction, which is why it shows on loaded CI runners with slow disks and almost never locally.Proof, in-process (a second connection holding
BEGIN IMMEDIATEon the fresh file,busy_timeout = 300):Fix (production code,
sqlite.ts): split the three PRAGMAs and wrap only the WAL switch inEffect.retry—walSwitchRetries(busyTimeoutMs):Schedule.spaced(min(5 ms, busyTimeoutMs))recurring only while the next attempt still starts insidebusyTimeoutMs(Schedule.while(({ duration, elapsed }) => elapsed + duration <= busyTimeoutMs)), retrying only while the mapped error's cause isSQLITE_BUSY. Once the header says WAL, the statement is a plain read and never contends again, so the retry is confined to the first-ever open of a file (or a legacy rollback-mode file). Same knob, same budget, sameunavailableerror when it runs out. Extended result codes (SQLITE_BUSY_*,SQLITE_CORRUPT_*) are now compared by primary code (errcode & 0xff), so the retry predicate andmapSqliteErroragree on what "busy" means.Regression tests (
packages/rsc-runtime/tests/state-sqlite.test.ts):waits for another process mid-WAL-switch instead of failing the first open(fails on the unfixed driver withexpected 'failed' to be 'pending'— the CI failure in miniature) andfails the first open as unavailable once the WAL switch outlives busyTimeoutMs(pins the budget bound for budgets of 100 ms and 1 ms — the latter below the retry pause).Reproduction counts (two/three
nodechildren opening + configuring the same fresh file behind a wall-clock start barrier, 50 iterations each):node:sqlite, driver's statement order, 2 writersjournal_modenode:sqlite, 3 writersnode:sqlite, file pre-switched to WAL (control)dist), 2 writersdist), 3 writersThe test file never reproduced locally without the barrier; the barrier harnesses are the reliable trigger and the built-driver row shows the identical
AgentStateErrorCI printed.Flake B —
packages/agent-bundle/tests/mcp-session-service.test.tsSymptom (run 33915324117):
expected [] to have a length of 1 but got +0at line 1138.Root cause — test-only. Lines 1137 and 1150 slept 10 ms as a stand-in for "the first
callToolhas been admitted and its abort signal registered"; line 272 slept 25 ms as a stand-in for "thehangrequest is admitted socancel()returns true".McpSession#callToolruns#assertEpochCurrentEffect()(an async epoch-store probe) before#admitRequest, all inside an Effect runtime, so under CI load 10 ms is not enough.Fix: the stub client's
callToolnow resolves an admission promise (nextAdmission()) on entry, after pushing the signal and the release; the test awaits that instead of sleeping. For the real fixture server,toolCallSent(session, 'hang')subscribes to the session trace and resolves when the client-directiontools/callframe for that tool is put on the wire (the request slot is admitted before the SDK sends, socancel()finds it).mcp-session-routes.test.ts(the only sibling; there is nomcp-session.test.ts) contains no such sleeps. Test-only, soagent-bundleis not named in the changeset.Counts: 30 runs before (0/30 locally, concurrent with the sqlite loop) → 30 runs after (0/30, concurrent with
pnpm typecheckand the sqlite loop). Not reproducible on this machine; the change removes the timing assumption rather than widening it.Gate
pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit— 3578 passed, 0 failed, 6 skipped. Both target files also run individually 30× post-fix (above). Nowebsite/docschange: no hand-written page documentsbusyTimeoutMsor the open phase, the error text is unchanged, and@agent-bundle/runtimeis outside the TypeDoc surface.Self-review
Reviewer:
change-risk-reviewersubagent, modelgpt-5.6-sol-medium, asked for concrete merge risks only against the diff vsorigin/main.sqlite.tsretry schedule:Schedule.upTo({ duration })compareselapsedbefore sleeping the next interval, so an attempt could start up to one 5 ms pause pastbusyTimeoutMs, and a budget below 5 ms still waited 5 ms.walSwitchRetries(busyTimeoutMs)clamps the pause tomin(5 ms, busyTimeoutMs)and recurs only whileelapsed + nextDelay <= busyTimeoutMs; the budget regression test now also coversbusyTimeoutMs: 1.Categories the reviewer checked and found clean: other SHARED→RESERVED upgrades or
busy_timeoutbypasses in the open path (none —BEGIN IMMEDIATEandBEGIN DEFERREDacquire fromNO_LOCK, which the busy handler covers), extended-result-code mapping, public./state/sqlitesurface and error semantics, the mcp-session trace-subscription synchronization (toolCallSentcannot resolve early, match a stale frame, or leak —afterSequenceis the current cursor andfinallyunsubscribes), the 50 msPromise.racein the new pending-open test (neither outcome is reachable while the lock is held: the switch cannot succeed and the 5 s default budget cannot expire), changeset, and docs parity.Second pass after the fix (same reviewer, same model): no concrete merge risks remain.