Repository navigation
fix(opencode): stop dropping accepted prompts under concurrent writers - #194
Merged
Merged
Conversation
Plan for fixing forward accepted-prompt loss under concurrent writers on top of the reverted base (60f779b). Evidence: data/opencode-fixed-binary-155/ report.md sections 5 and 6, and data/opencode-revert-155/report.md section 7. Mechanism, from the code (not yet measured): - The admission transaction is SessionQueue writable() in packages/opencode/src/session/queue.ts. It reads first (EventV2.assertWritableIn) and then writes (sequence upsert and queue insert). It begins as a deferred transaction, so its read snapshot exists before its write lock is taken. - A plain-busy wait inside a deferred transaction can only end in SQLITE_BUSY_SNAPSHOT when another connection commits during the wait: the snapshot goes stale, and the driver does not restart the transaction. This is the mechanism that predicts the report's section 6.3 observation that the snapshot class grew when the wait was added. It is not measured yet. - The admission and promotion writes are the deferred read-then-write transactions found on the accepted-prompt path by grepping its call sites. The EventV2 append that persists the user message already begins with behavior immediate (event.ts:467). The proof decides whether that is complete. Correction to #162. Its option 2 names Client.make beginTransaction, which the production path never reaches. opencode runs transactions through effect-drizzle-sqlite, whose withTransaction issues "begin <behavior>" itself (packages/effect-drizzle-sqlite/src/effect-sqlite/session.ts), default deferred. Effect's SqlClient.withTransaction, which reads beginTransaction, has no caller in opencode. The mechanism that works is the existing per-transaction option, behavior immediate, which the codebase already uses in event.ts and project.ts. Choice: (a) BEGIN IMMEDIATE on write-capable transactions, not (b) restart. - (a) takes the write lock before any read snapshot exists, so the failing class cannot occur for that transaction. The lock wait happens at BEGIN, where a retry is safe because nothing has been read yet. - (b) restarts a body that has already run. The admission body is pure SQL, but restart needs a per-site audit of every transaction body for duplicated side effects, and it leaves the stale-snapshot race in place. Steps: 1. Restore the #155 plain-busy wait and the #154 fail-fast on SQLITE_BUSY_SNAPSHOT in packages/core/src/database/sqlite.bun.ts, sqlite.node.ts and packages/effect-sqlite-node/src/index.ts. The three copies stay identical, as #162 requires. Dedupe stays with #165. sqlite.node.ts is outside the brief's named scope; it is named here because it is the third copy of the same rule. 2. Begin writable() with behavior immediate in queue.ts. This covers every queue write: the callers are admit, reserve, withdraw, restore, and update. 3. Restore the #155 test versions that cover the wait and the fail-fast: packages/core/test/sqlite-busy.test.ts, and the opencode lock fixture and consumer bounds, which #155 raised for the 5 s window. 4. Add a regression at the admission path. A second connection holds the write lock across the admission read, then commits. The admission must succeed and persist its row. Red proof: revert writable() to deferred. Also run the restored core tests against the reverted driver for red. 5. Proofs, in the brief's order: focused suites with CI commands; the candidate against the reverted parent, concurrently on separate DBs, same load, several pairs, raw counts; fresh-store proof. The parent is the reverted build (tree equal to bf5180a). Not in this change: session queue consumption (#167); the CHANGESET entry (needs the PR number at publication); the snapshot-tracking PATH issue (#164). Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
Restores the plain-busy wait that #155 added. A statement that meets a lock retries asynchronously for 5 seconds of elapsed contention. A stale-snapshot failure (SQLITE_BUSY_SNAPSHOT) is still not retried (#154). The three retry copies stay identical in sqlite.bun.ts, sqlite.node.ts and effect-sqlite-node. Red on the reverted driver: the restored sqlite-busy suite fails 14 of 23. The 250 ms window cannot absorb a 1.5 s hold, and a stale-snapshot statement is executed more than once. Green with the window: 23 of 23. The opencode lock-consumer suites pass 36 of 36 with their #155 bounds. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
SessionQueue writable() runs its read (EventV2.assertWritableIn) and then its write in one transaction. Begun deferred, that transaction pins a read snapshot before it takes the write lock. If another writer commits while the write waits on the lock, the snapshot goes stale, and the write fails with SQLITE_BUSY_SNAPSHOT, which is not retried. The prompt was already accepted (204), so it was lost with no user message. With behavior immediate the write lock is taken at BEGIN, before any read. The lock wait then happens where a retry is safe, and no snapshot can go stale inside the transaction. The restored lock-wait window absorbs the wait. The regression holds the write lock from a second connection across the admission read, then commits. Red with the deferred writable: the sequence upsert fails with SQLITE_BUSY_SNAPSHOT after the commit. Green with immediate. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
Brings in 8d536e3 (#159) so the branch is current with the default branch. It does not touch the SQLite driver, the queue, or the lock tests. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
The busy-retry schedule and the stale-snapshot gate were copied into three statement paths: sqlite.bun.ts, sqlite.node.ts and effect-sqlite-node. #155 changed all three, so a rule change had to land three times, and nothing said which copy was authoritative. effect-sqlite-node/src/retry.ts now owns both. It exports retryLocked, which every driver routes its statements through, and it recognizes the stale-snapshot class in both native error shapes: bun:sqlite reports it by code and node:sqlite by errcode. The module imports only effect, so the Bun binary can consume it without loading node:sqlite. The drivers no longer carry a schedule or a gate. Behavior is unchanged. The core busy suite is 23 of 23 across the three clients. Removing the snapshot exclusion from the gate turns the three stale-snapshot tests red. Dropping the node error shape turns two of them red (core-node, standalone-node), and dropping the bun shape turns one red (bun). Removing the retry turns the six lock-wait tests red. statementError, the native error classification, is still copied in the same three files. It is not part of this change, and #87 tracks it. Refs #162 Refs #165 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
Review round 1. The admission test held the write lock for a fixed 1500 ms, then committed. An admission that started late ran after the commit and passed even with a deferred write, so the test did not prove the fix. The test now counts the admission's statement attempts at bun:sqlite's query path, as the core sqlite-busy test does, and records the native codes of the attempts that throw SQLITE_BUSY. It commits only after the admission has met the held lock, asserts the admission is still pending, then checks the persisted row. The timeout is a backstop, not the signal. On failure, the message lists the admission's attempts and busy codes. Checked locally: - writable() immediate: pass, with and without a 1600 ms delay before the fork - writable() reverted to deferred: fail with SQLITE_BUSY_SNAPSHOT after the commit, with and without the 1600 ms delay No product code changes in this commit. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
…escriptor Referencing prototype.query unbound tripped the unbound-method lint rule. Reading the descriptor's value keeps the method unbound and adds no behavior change. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
The #162 fix makes writable() open every queue write with BEGIN IMMEDIATE. The rule lived only in a code comment, so a future queue write could bypass it. The V1 Prompt Queue section now states it where contributors and agents read queue rules. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
The core sqlite-busy test and the opencode admission test each patched bun:sqlite's query method to count statement attempts. That job now has one owner, the fixture-only module packages/core/src/database/sqlite-attempts.ts. Both tests import it, and the core test's node branches count through its countStatement. No runtime module imports the new file. It sits in core's src because opencode tests reach core through its wildcard export, and no shared test-support package exists. Checked: core sqlite-busy (23 pass); admission green with and without a 1600 ms delay before the fork; admission red with writable() reverted to deferred, with and without the delay (busy codes end in SQLITE_BUSY_SNAPSHOT). Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
Author
|
Claim stamp (per the working rules for this stream):
|
Adds the fork changeset entry for PR #194 (#162 and #165) as the first item under Runtime fixes. Nothing else changes. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
Brings in b18e7ff (PR #161) so CI runs on the current base before the merge. No other changes. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo
This was referenced Oct 10, 2026
brentyates-swx
added a commit
that referenced
this pull request
Oct 10, 2026
* docs(core): plan the SQLite snapshot-loss fix for #162 Plan for fixing forward accepted-prompt loss under concurrent writers on top of the reverted base (60f779b). Evidence: data/opencode-fixed-binary-155/ report.md sections 5 and 6, and data/opencode-revert-155/report.md section 7. Mechanism, from the code (not yet measured): - The admission transaction is SessionQueue writable() in packages/opencode/src/session/queue.ts. It reads first (EventV2.assertWritableIn) and then writes (sequence upsert and queue insert). It begins as a deferred transaction, so its read snapshot exists before its write lock is taken. - A plain-busy wait inside a deferred transaction can only end in SQLITE_BUSY_SNAPSHOT when another connection commits during the wait: the snapshot goes stale, and the driver does not restart the transaction. This is the mechanism that predicts the report's section 6.3 observation that the snapshot class grew when the wait was added. It is not measured yet. - The admission and promotion writes are the deferred read-then-write transactions found on the accepted-prompt path by grepping its call sites. The EventV2 append that persists the user message already begins with behavior immediate (event.ts:467). The proof decides whether that is complete. Correction to #162. Its option 2 names Client.make beginTransaction, which the production path never reaches. opencode runs transactions through effect-drizzle-sqlite, whose withTransaction issues "begin <behavior>" itself (packages/effect-drizzle-sqlite/src/effect-sqlite/session.ts), default deferred. Effect's SqlClient.withTransaction, which reads beginTransaction, has no caller in opencode. The mechanism that works is the existing per-transaction option, behavior immediate, which the codebase already uses in event.ts and project.ts. Choice: (a) BEGIN IMMEDIATE on write-capable transactions, not (b) restart. - (a) takes the write lock before any read snapshot exists, so the failing class cannot occur for that transaction. The lock wait happens at BEGIN, where a retry is safe because nothing has been read yet. - (b) restarts a body that has already run. The admission body is pure SQL, but restart needs a per-site audit of every transaction body for duplicated side effects, and it leaves the stale-snapshot race in place. Steps: 1. Restore the #155 plain-busy wait and the #154 fail-fast on SQLITE_BUSY_SNAPSHOT in packages/core/src/database/sqlite.bun.ts, sqlite.node.ts and packages/effect-sqlite-node/src/index.ts. The three copies stay identical, as #162 requires. Dedupe stays with #165. sqlite.node.ts is outside the brief's named scope; it is named here because it is the third copy of the same rule. 2. Begin writable() with behavior immediate in queue.ts. This covers every queue write: the callers are admit, reserve, withdraw, restore, and update. 3. Restore the #155 test versions that cover the wait and the fail-fast: packages/core/test/sqlite-busy.test.ts, and the opencode lock fixture and consumer bounds, which #155 raised for the 5 s window. 4. Add a regression at the admission path. A second connection holds the write lock across the admission read, then commits. The admission must succeed and persist its row. Red proof: revert writable() to deferred. Also run the restored core tests against the reverted driver for red. 5. Proofs, in the brief's order: focused suites with CI commands; the candidate against the reverted parent, concurrently on separate DBs, same load, several pairs, raw counts; fresh-store proof. The parent is the reverted build (tree equal to bf5180a). Not in this change: session queue consumption (#167); the CHANGESET entry (needs the PR number at publication); the snapshot-tracking PATH issue (#164). Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * fix(core): restore the SQLite lock-wait window for busy statements Restores the plain-busy wait that #155 added. A statement that meets a lock retries asynchronously for 5 seconds of elapsed contention. A stale-snapshot failure (SQLITE_BUSY_SNAPSHOT) is still not retried (#154). The three retry copies stay identical in sqlite.bun.ts, sqlite.node.ts and effect-sqlite-node. Red on the reverted driver: the restored sqlite-busy suite fails 14 of 23. The 250 ms window cannot absorb a 1.5 s hold, and a stale-snapshot statement is executed more than once. Green with the window: 23 of 23. The opencode lock-consumer suites pass 36 of 36 with their #155 bounds. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * fix(opencode): admit queue writes under BEGIN IMMEDIATE SessionQueue writable() runs its read (EventV2.assertWritableIn) and then its write in one transaction. Begun deferred, that transaction pins a read snapshot before it takes the write lock. If another writer commits while the write waits on the lock, the snapshot goes stale, and the write fails with SQLITE_BUSY_SNAPSHOT, which is not retried. The prompt was already accepted (204), so it was lost with no user message. With behavior immediate the write lock is taken at BEGIN, before any read. The lock wait then happens where a retry is safe, and no snapshot can go stale inside the transaction. The restored lock-wait window absorbs the wait. The regression holds the write lock from a second connection across the admission read, then commits. Red with the deferred writable: the sequence upsert fails with SQLITE_BUSY_SNAPSHOT after the commit. Green with immediate. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * refactor(core): give the SQLite busy-retry gate one owner The busy-retry schedule and the stale-snapshot gate were copied into three statement paths: sqlite.bun.ts, sqlite.node.ts and effect-sqlite-node. #155 changed all three, so a rule change had to land three times, and nothing said which copy was authoritative. effect-sqlite-node/src/retry.ts now owns both. It exports retryLocked, which every driver routes its statements through, and it recognizes the stale-snapshot class in both native error shapes: bun:sqlite reports it by code and node:sqlite by errcode. The module imports only effect, so the Bun binary can consume it without loading node:sqlite. The drivers no longer carry a schedule or a gate. Behavior is unchanged. The core busy suite is 23 of 23 across the three clients. Removing the snapshot exclusion from the gate turns the three stale-snapshot tests red. Dropping the node error shape turns two of them red (core-node, standalone-node), and dropping the bun shape turns one red (bun). Removing the retry turns the six lock-wait tests red. statementError, the native error classification, is still copied in the same three files. It is not part of this change, and #87 tracks it. Refs #162 Refs #165 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * test(opencode): wait on the admission's busy attempt, not a sleep Review round 1. The admission test held the write lock for a fixed 1500 ms, then committed. An admission that started late ran after the commit and passed even with a deferred write, so the test did not prove the fix. The test now counts the admission's statement attempts at bun:sqlite's query path, as the core sqlite-busy test does, and records the native codes of the attempts that throw SQLITE_BUSY. It commits only after the admission has met the held lock, asserts the admission is still pending, then checks the persisted row. The timeout is a backstop, not the signal. On failure, the message lists the admission's attempts and busy codes. Checked locally: - writable() immediate: pass, with and without a 1600 ms delay before the fork - writable() reverted to deferred: fail with SQLITE_BUSY_SNAPSHOT after the commit, with and without the 1600 ms delay No product code changes in this commit. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * test(opencode): read the admission probe's query method through its descriptor Referencing prototype.query unbound tripped the unbound-method lint rule. Reading the descriptor's value keeps the method unbound and adds no behavior change. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * docs(opencode): require queue writes to go through writable() The #162 fix makes writable() open every queue write with BEGIN IMMEDIATE. The rule lived only in a code comment, so a future queue write could bypass it. The V1 Prompt Queue section now states it where contributors and agents read queue rules. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * test(core): one statement-attempt counter for the busy tests The core sqlite-busy test and the opencode admission test each patched bun:sqlite's query method to count statement attempts. That job now has one owner, the fixture-only module packages/core/src/database/sqlite-attempts.ts. Both tests import it, and the core test's node branches count through its countStatement. No runtime module imports the new file. It sits in core's src because opencode tests reach core through its wildcard export, and no shared test-support package exists. Checked: core sqlite-busy (23 pass); admission green with and without a 1600 ms delay before the fork; admission red with writable() reverted to deferred, with and without the delay (busy codes end in SQLITE_BUSY_SNAPSHOT). Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * docs: record the #162 lock fix in CHANGESET.md Adds the fork changeset entry for PR #194 (#162 and #165) as the first item under Runtime fixes. Nothing else changes. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * docs(core): plan SQLite lock-hold and stop-site diagnostics (#180) Diagnostic-only logging on candidate 7b0ea4a (fm/opencode-lock-fix-162) so the next SQLITE_BUSY session stop names its lock holder and its call site. Every new line goes through the existing Effect logger; no value, cause, SQL, schedule, timeout or transaction behaviour changes. 1. Write-lock hold timing (effect-drizzle-sqlite session.withTransaction). For a top-level BEGIN IMMEDIATE/EXCLUSIVE that succeeds, time from BEGIN returning to a successful COMMIT or ROLLBACK (including the rollback after a failed commit). Over 250 ms, log "sqlite write lock held" with pid, transaction purpose, current span and duration. Savepoints, deferred transactions and failed BEGINs are not holds. Purpose comes from a new TransactionPurpose context reference, falling back to the current span. event.ts commitDurableEvent provides the event definition type. 2. Retry exhaustion (effect-sqlite-node retryLocked, given the statement SQL by all three drivers). Per execution, count attempts and elapsed time; when a lock failure leaves the gate, log once: statement head, attempts, elapsed ms, and the native cause code and full errcode (item 4), with separate messages for an exhausted window and a stale snapshot that fails without retry. Recovering statements log nothing. 3. Stop call site. A core side table keyed by the failure object records where a failure came from without changing it: commitDurableEvent records the event type and the span chain at the failed commit; the processor records the stream case (handler) or the provider stream (stream). halt's existing "process" error line adds those fields. Tests (focused, producer-driven, removal-to-red proven): - packages/core/test/sqlite-lock-diagnostics.test.ts: long vs short holds, commit and rollback, wait-then-short-hold, nested savepoints; one exhaustion summary with observed attempt count; none on recovery; BUSY vs BUSY_SNAPSHOT native codes on bun, core-node, standalone-node. - packages/opencode/test/session/processor-effect.test.ts: halt line names event, case and call site for reasoning-end (finishReasoning helper) and step-finish (message.updated); provider-stream failure has no event. Preservation: core sqlite-busy and opencode queue-admission suites unchanged. Live proof: env -i scratch store run whose file log shows the hold line. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * feat(core): log long SQLite write-lock holds and lock retry give-ups Diagnostic logging only (#180); every value, cause, SQL statement, retry schedule and transaction step is unchanged. - effect-drizzle-sqlite withTransaction times a top-level immediate or exclusive BEGIN from its success to the successful COMMIT or ROLLBACK (including the rollback after a failed commit) and logs "sqlite write lock held" with pid, purpose, span, outcome and duration over 250 ms. A new TransactionPurpose reference names the purpose, else the caller's span. - retryLocked takes the statement SQL from all three drivers and, per execution, logs one summary when a lock failure leaves it: statement head, attempts, elapsed ms, and the native code and full errcode, so SQLITE_BUSY and SQLITE_BUSY_SNAPSHOT stay distinguishable. - A core FailureOrigin side table records a failure's event type and span chain at the durable-event commit without changing the failure. - commitDurableEvent supplies its definition type as the purpose. - SessionProcessor records the stream case (or the provider stream) on a failure, and halt's "process" line adds the recorded event, case and call site. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * test(opencode): prove halt names the failed write's event, case and call site Adds focused processor tests driving real lock failures into halt (#180): - reasoning-end: the finishReasoning helper's message.part.updated commit meets a held lock; halt names the event, the case and a call-site chain running Session.updatePart < SessionProcessor.finishReasoning. - step-finish: the message.updated write meets the lock instead, so the same case path reports a different event identity. - a provider stream that dies after its handled cases succeed is attributed to the stream, with no invented event or case. The held-lock fixture gains a trigger choosing which write takes the lock; its default keeps the existing tests' reasoning-end behaviour. The core nested-savepoint hold test now nests an immediate transaction long enough to log on its own, and withTransaction drops a guard that was dead: only the top-level COMMIT and ROLLBACK report a release. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * fix(core): omit an absent span from the lock-hold line A hold with no caller span printed span=undefined in the file log. The bun stale-snapshot and BEGIN exhaustion tests now also pin the extended errno bun reports (517 and 5), alongside its code. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * docs: record the stop-instrument diagnostics in CHANGESET.md Adds the fork entry under Runtime fixes now, so review covers its text. The PR number does not exist until publication; script/changeset-check.ts keys the entry on it and fails the PR until `#PR` is replaced with that number. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * docs: say the stop-instrument entry follows #194, which has landed Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * fix(core): name an exhausted statement by its operation only Review round 1, required 1: the retry gate's summary logged the first three SQL tokens, so a leading comment such as /* secret */ or a quoted identifier reached the log and the operation itself could be lost. It now logs only the leading keyword (and a BEGIN's deferred/immediate/exclusive), after skipping leading comments and whitespace; nothing after the operation is read. Returned errors and parameters are unchanged. The new focused test runs real statements through the retry gate on held production connections (leading block and line comments, a string literal, a quoted identifier, mixed case and whitespace, a multi-statement string and a bound parameter) and asserts each summary names its operation and none contains the payload. The first hold test gets an explicit 20 s timeout like its siblings. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * fix(core): take the stop call site from where the failure was raised Review round 1, required 2: FailureOrigin read the observing fiber's span after the failing scopes had unwound, so a provider-stream failure recorded only SessionProcessor.process. It now reads the stack-frame chain the Effect runtime annotates onto the cause where the failure is raised (Cause.StackTrace), leaving out Effect.fn's definition-site frames, so the call site names the failing callable across the stream and helper boundary. The provider-stream halt test now fails inside a named callable and asserts the call site starts with it; the held-lock tests pin their innermost frame to Session.updatePart and Session.updateMessage. Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB * docs: record the stop-instrument entry as #201 in CHANGESET.md Claude-Session: https://claude.ai/code/session_01QuyVZfg7zCF6ZJmDUf8BnB
brentyates-swx
added a commit
that referenced
this pull request
Oct 11, 2026
Brings in #161, #194 and #201 on top of the reviewed commit 95672df. Conflicts, all resolved with the swxtch side: - packages/core/src/database/sqlite.bun.ts - packages/core/src/database/sqlite.node.ts - packages/effect-sqlite-node/src/index.ts - packages/effect-sqlite-node/src/retry.ts (add/add) - CHANGESET.md This branch carries the unsquashed #162 commits; swxtch carries the same change squashed as #194. For each conflicted code file the reviewed side is byte-identical to #194's version, so the conflict is only #201's edits on top of #194, and the swxtch side keeps both. CHANGESET.md's only conflict hunk is the #201 entry, which this branch never touched. The branch's own changes (session/queue.ts, test/session/prompt.test.ts) merged cleanly and are byte-identical to 95672df. Claude-Session: https://claude.ai/code/session_019DxEchLHHdPjd4GUE1EJAj
brentyates-swx
added a commit
that referenced
this pull request
Oct 11, 2026
* docs(core): plan the SQLite snapshot-loss fix for #162 Plan for fixing forward accepted-prompt loss under concurrent writers on top of the reverted base (60f779b). Evidence: data/opencode-fixed-binary-155/ report.md sections 5 and 6, and data/opencode-revert-155/report.md section 7. Mechanism, from the code (not yet measured): - The admission transaction is SessionQueue writable() in packages/opencode/src/session/queue.ts. It reads first (EventV2.assertWritableIn) and then writes (sequence upsert and queue insert). It begins as a deferred transaction, so its read snapshot exists before its write lock is taken. - A plain-busy wait inside a deferred transaction can only end in SQLITE_BUSY_SNAPSHOT when another connection commits during the wait: the snapshot goes stale, and the driver does not restart the transaction. This is the mechanism that predicts the report's section 6.3 observation that the snapshot class grew when the wait was added. It is not measured yet. - The admission and promotion writes are the deferred read-then-write transactions found on the accepted-prompt path by grepping its call sites. The EventV2 append that persists the user message already begins with behavior immediate (event.ts:467). The proof decides whether that is complete. Correction to #162. Its option 2 names Client.make beginTransaction, which the production path never reaches. opencode runs transactions through effect-drizzle-sqlite, whose withTransaction issues "begin <behavior>" itself (packages/effect-drizzle-sqlite/src/effect-sqlite/session.ts), default deferred. Effect's SqlClient.withTransaction, which reads beginTransaction, has no caller in opencode. The mechanism that works is the existing per-transaction option, behavior immediate, which the codebase already uses in event.ts and project.ts. Choice: (a) BEGIN IMMEDIATE on write-capable transactions, not (b) restart. - (a) takes the write lock before any read snapshot exists, so the failing class cannot occur for that transaction. The lock wait happens at BEGIN, where a retry is safe because nothing has been read yet. - (b) restarts a body that has already run. The admission body is pure SQL, but restart needs a per-site audit of every transaction body for duplicated side effects, and it leaves the stale-snapshot race in place. Steps: 1. Restore the #155 plain-busy wait and the #154 fail-fast on SQLITE_BUSY_SNAPSHOT in packages/core/src/database/sqlite.bun.ts, sqlite.node.ts and packages/effect-sqlite-node/src/index.ts. The three copies stay identical, as #162 requires. Dedupe stays with #165. sqlite.node.ts is outside the brief's named scope; it is named here because it is the third copy of the same rule. 2. Begin writable() with behavior immediate in queue.ts. This covers every queue write: the callers are admit, reserve, withdraw, restore, and update. 3. Restore the #155 test versions that cover the wait and the fail-fast: packages/core/test/sqlite-busy.test.ts, and the opencode lock fixture and consumer bounds, which #155 raised for the 5 s window. 4. Add a regression at the admission path. A second connection holds the write lock across the admission read, then commits. The admission must succeed and persist its row. Red proof: revert writable() to deferred. Also run the restored core tests against the reverted driver for red. 5. Proofs, in the brief's order: focused suites with CI commands; the candidate against the reverted parent, concurrently on separate DBs, same load, several pairs, raw counts; fresh-store proof. The parent is the reverted build (tree equal to bf5180a). Not in this change: session queue consumption (#167); the CHANGESET entry (needs the PR number at publication); the snapshot-tracking PATH issue (#164). Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * fix(core): restore the SQLite lock-wait window for busy statements Restores the plain-busy wait that #155 added. A statement that meets a lock retries asynchronously for 5 seconds of elapsed contention. A stale-snapshot failure (SQLITE_BUSY_SNAPSHOT) is still not retried (#154). The three retry copies stay identical in sqlite.bun.ts, sqlite.node.ts and effect-sqlite-node. Red on the reverted driver: the restored sqlite-busy suite fails 14 of 23. The 250 ms window cannot absorb a 1.5 s hold, and a stale-snapshot statement is executed more than once. Green with the window: 23 of 23. The opencode lock-consumer suites pass 36 of 36 with their #155 bounds. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * fix(opencode): admit queue writes under BEGIN IMMEDIATE SessionQueue writable() runs its read (EventV2.assertWritableIn) and then its write in one transaction. Begun deferred, that transaction pins a read snapshot before it takes the write lock. If another writer commits while the write waits on the lock, the snapshot goes stale, and the write fails with SQLITE_BUSY_SNAPSHOT, which is not retried. The prompt was already accepted (204), so it was lost with no user message. With behavior immediate the write lock is taken at BEGIN, before any read. The lock wait then happens where a retry is safe, and no snapshot can go stale inside the transaction. The restored lock-wait window absorbs the wait. The regression holds the write lock from a second connection across the admission read, then commits. Red with the deferred writable: the sequence upsert fails with SQLITE_BUSY_SNAPSHOT after the commit. Green with immediate. Refs #162 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * refactor(core): give the SQLite busy-retry gate one owner The busy-retry schedule and the stale-snapshot gate were copied into three statement paths: sqlite.bun.ts, sqlite.node.ts and effect-sqlite-node. #155 changed all three, so a rule change had to land three times, and nothing said which copy was authoritative. effect-sqlite-node/src/retry.ts now owns both. It exports retryLocked, which every driver routes its statements through, and it recognizes the stale-snapshot class in both native error shapes: bun:sqlite reports it by code and node:sqlite by errcode. The module imports only effect, so the Bun binary can consume it without loading node:sqlite. The drivers no longer carry a schedule or a gate. Behavior is unchanged. The core busy suite is 23 of 23 across the three clients. Removing the snapshot exclusion from the gate turns the three stale-snapshot tests red. Dropping the node error shape turns two of them red (core-node, standalone-node), and dropping the bun shape turns one red (bun). Removing the retry turns the six lock-wait tests red. statementError, the native error classification, is still copied in the same three files. It is not part of this change, and #87 tracks it. Refs #162 Refs #165 Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * test(opencode): wait on the admission's busy attempt, not a sleep Review round 1. The admission test held the write lock for a fixed 1500 ms, then committed. An admission that started late ran after the commit and passed even with a deferred write, so the test did not prove the fix. The test now counts the admission's statement attempts at bun:sqlite's query path, as the core sqlite-busy test does, and records the native codes of the attempts that throw SQLITE_BUSY. It commits only after the admission has met the held lock, asserts the admission is still pending, then checks the persisted row. The timeout is a backstop, not the signal. On failure, the message lists the admission's attempts and busy codes. Checked locally: - writable() immediate: pass, with and without a 1600 ms delay before the fork - writable() reverted to deferred: fail with SQLITE_BUSY_SNAPSHOT after the commit, with and without the 1600 ms delay No product code changes in this commit. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * test(opencode): read the admission probe's query method through its descriptor Referencing prototype.query unbound tripped the unbound-method lint rule. Reading the descriptor's value keeps the method unbound and adds no behavior change. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * docs(opencode): require queue writes to go through writable() The #162 fix makes writable() open every queue write with BEGIN IMMEDIATE. The rule lived only in a code comment, so a future queue write could bypass it. The V1 Prompt Queue section now states it where contributors and agents read queue rules. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * test(core): one statement-attempt counter for the busy tests The core sqlite-busy test and the opencode admission test each patched bun:sqlite's query method to count statement attempts. That job now has one owner, the fixture-only module packages/core/src/database/sqlite-attempts.ts. Both tests import it, and the core test's node branches count through its countStatement. No runtime module imports the new file. It sits in core's src because opencode tests reach core through its wildcard export, and no shared test-support package exists. Checked: core sqlite-busy (23 pass); admission green with and without a 1600 ms delay before the fork; admission red with writable() reverted to deferred, with and without the delay (busy codes end in SQLITE_BUSY_SNAPSHOT). Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * docs: record the #162 lock fix in CHANGESET.md Adds the fork changeset entry for PR #194 (#162 and #165) as the first item under Runtime fixes. Nothing else changes. Claude-Session: https://claude.ai/code/session_01LrS7BTRTnc6eYrSxBFoWAo * docs(opencode): plan bounded consume recovery for #167 and #180 Plan for option A (captain decision, 2026-10-10): recover retained queued intent after a consume that met a held write lock, through the existing queue owner, with a bounded terminal refusal. Starting head 7b0ea4a (the #162 fix branch); its runtime source equals candidate 9e088df: queue writable() opens BEGIN IMMEDIATE, and every statement goes through retryLocked/retrySchedule in packages/effect-sqlite-node/src/retry.ts (5 s window). Neither changes here. Failure boundary, from the code: - SessionPrompt.run (runLoop in prompt.ts) calls queue.consume at every loop top, including the one after a tool step. consume is exclusive(writable(DELETE promoted rows)).pipe(Effect.orDie). - With a second connection holding the write lock, BEGIN IMMEDIATE fails and is retried by retryLocked until its window ends; the LockTimeoutError becomes a defect. The transaction body never ran. - The defect leaves runLoop before `park`, the runner's finishRun completes every joiner's Deferred with it, and drain dies at state.ensureRunning, so its awaitingDrain re-check never runs. The pending queue row, the completed tool part and the unanswered tool-calls turn stay in the database, and nothing drives them until another prompt wakes the session. Change (queue.ts only; prompt.ts keeps its one consume call): - consume re-attempts its own transaction when the failure is a lock failure and the transaction body never began (tracked by the body, so an executed body is never replayed). Each attempt takes the session permit afresh and waits on the lock through the unchanged statement retry policy. - Recovery condition: a consume attempt commits, which is a held write lock taken by this connection and the promoted rows deleted. The loop then reads history and goes on to the provider turn, so the turn and the queued prompt continue in the same run, answered once. Time never decides success; each attempt is bounded by the existing window. - Bound: 3 attempts in total. Every failed attempt logs a warning naming the stage and the attempt number. - Refusal: when the last attempt fails the same way, consume dies with SessionQueue.ConsumeLockedError, naming the consume stage, the session and the attempts. The run ends as a run defect does today: joiners receive it, prompt_async publishes it as session.error, the runner goes idle, and the drain re-check is not reached, so nothing re-drains it automatically. The queue row, the tool part and the turn stay as they were, for a later wake. - No new re-drain path. The one re-check in drain stays the only one. Proof: - prompt.test.ts, on a scratch file database: a tool turn with a queued continuation, the write lock taken from a second connection at the loop-top busy status after the tool step. Recovery: release once the first failed attempt is logged; the turn and the continuation are each answered once, the bash tool ran once, one continuation user message, queue empty. Refusal: keep the lock; the run dies with ConsumeLockedError after 3 attempts, the row stays pending, the tool ran once, the runner is idle; a later prompt delivers the continuation once. - Red/green by reverting the recovery in queue.ts. - The archived continuation harness, copied, against a built binary: below 2 s unchanged, above 8 s continues exactly once, and a hold past the bound ends in the refusal. Out of scope: #175, #176, #166, #179; no running server is touched. Refs #167, #180, #181 Claude-Session: https://claude.ai/code/session_01KdxgEDScVEkwpbHyhm1EBq * test(opencode): reproduce the stranded continuation after a locked consume A bash tool turn with a queued continuation, on a scratch file database. A second connection takes the write lock at the loop-top busy status after the tool step, so the run's own SessionQueue.consume meets it. On this head both cases fail: consume dies with LockTimeoutError at SessionPrompt.run after the statement retry window, and the continuation is left pending with no recovery and no named refusal. Refs #167, #180 Claude-Session: https://claude.ai/code/session_01KdxgEDScVEkwpbHyhm1EBq * fix(opencode): recover a consume that meets a held write lock SessionQueue.consume runs at every loop top, including the one after a tool step. When another connection held the write lock past the statement retry window, its BEGIN IMMEDIATE failed, the defect ended the run before `park` and before drain's awaitingDrain re-check, and the completed tool turn and its queued continuation waited for an unrelated prompt. consume now makes the attempt again while the failure is a lock failure and its transaction never began, so no executed body is replayed. Each attempt takes the session permit afresh and waits on the lock under the unchanged statement retry policy; success is the attempt's own commit, after which the loop reads history and goes on to the provider turn in the same run. The bound is three attempts, each logged as "queue consume met a held write lock". A lock that outlasts all three ends the run with SessionQueue.ConsumeLockedError, which names the consume stage, the session and the attempts, and leaves every queued row, the tool result and the unanswered turn as they were. Nothing re-drains it automatically: the run fails as a run defect does, so drain's re-check is not reached, and the next prompt delivers the kept work. Refs #167, #180 Claude-Session: https://claude.ai/code/session_01KdxgEDScVEkwpbHyhm1EBq * fix(opencode): bound each consume attempt's acquisition with a backstop Review round 1. A consume attempt could wait without bound before its transaction began: for the session permit, or for the shared native connection, which the driver acquires and retries BEGIN on uninterruptibly. Attempt 3 and ConsumeLockedError were then never reached. Each attempt now runs in its own fiber, and the caller waits at most consumeAcquireBackstop (10 s: the attempt's own statement window plus one other statement's ahead of it on the connection) for the attempt's body to begin. An attempt that has not begun by then is abandoned and counted as a failed attempt, logged as "queue consume waited past its acquisition backstop". Its fiber is interrupted without waiting, and if it still reaches its body it rolls back without deleting anything. A body that began is waited for untimed. The attempt count (3) and the statement retry windows are unchanged, and recovery is still the same re-attempt inside the existing owner. Tests (prompt.test.ts): the fixture takes a pluggable hold at the loop top and reads records through its own read-only connection. Two cases are new: - the session permit held by a writer that never finishes: three backstop attempts, then ConsumeLockedError, the continuation kept, and a later prompt delivers it once; - a SQLITE_BUSY failure of the consume's delete, after its body began: the run dies with that lock error, and no further attempt is made (replay guard). The refusal test binds SessionStatus and SessionRunState to named variables. Refs #167, #180 Claude-Session: https://claude.ai/code/session_01KdxgEDScVEkwpbHyhm1EBq * fix(opencode): give a consume five attempts before it refuses Captain decision in review round 2: keep the count design and the per-attempt acquisition backstop, and raise the attempt count from 3 to 5. Each attempt still waits on the lock within its existing statement window. The refusal's attempt count and message now come from consumeAttempts rather than the loop counter. The refusal and permit tests expect five attempts and five logs, and their own bounds grow with the budget. The replay-guard test's bound grows too, so its red, five replayed attempts, still ends within it. Refs #167, #180 Claude-Session: https://claude.ai/code/session_01KdxgEDScVEkwpbHyhm1EBq * test(opencode): cover a consume whose attempts wait on a held connection Review round 2. A new prompt.test.ts regression drives the real prompt and runner. The run's first consume attempt meets a native SQLITE_BUSY on its BEGIN IMMEDIATE. Inside that attempt's log, before the next attempt, an unrelated deferred transaction takes the shared connection and keeps it, and the external lock is then released. The remaining attempts wait on that connection and are abandoned at the backstop. The run must refuse with ConsumeLockedError, and the runner must stop, while the transaction still holds the connection. Once the transaction ends, the abandoned attempt that kept the session permit begins its transaction, deletes nothing, and frees the permit. The records are unchanged, and a later wake delivers the continuation once. Replacing the detached interrupt of an abandoned attempt with an awaited one makes it red, because the run cannot stop until the connection is free. The state.abandoned guard is not reached while the interrupt is in place: the body is interrupted at its interruptible boundary first. Removing the guard alone stays green, and no coverage is claimed for it. Also, abandon now reports whether it won. An abandon that loses to a body that already began waits for that body, so a committed consume is never reported as a backstop. Refs #167, #180 Claude-Session: https://claude.ai/code/session_01KdxgEDScVEkwpbHyhm1EBq * docs: record the #167 consume recovery in CHANGESET.md Claude-Session: https://claude.ai/code/session_019DxEchLHHdPjd4GUE1EJAj
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.
Closes #162 and #165. Closes #176 when the CHANGESET entry lands (see Changeset below).
Summary
Accepted V1 prompts were dropped under concurrent SQLite writers. The queue's writes ran as deferred transactions. A deferred write that follows a read pins a snapshot, so a commit by another writer while the write waits makes it fail with
SQLITE_BUSY_SNAPSHOT, and the driver does not retry that class.writable()now opens every queue write withBEGIN IMMEDIATE, so the write waits for the lock at BEGIN, before any read. The lock-wait window from #155 is restored, and the busy-retry gate has one owner,packages/effect-sqlite-node/src/retry.ts.Verdict and evidence: report section 1.
Scope and limits
writable()now usesBEGIN IMMEDIATEfor every queue write on the consumption path (reserve, withdraw, restore, update), which is beyond Queue-consume SQLite failure strands an ordinary session after completed tool calls #167 scope. Every queue write inqueue.tsruns insidewritable(): admit, reserve, withdraw, restore, update, markPromoted, drop, unreserve and consume. Before this change, all of them began deferred. The only other transactions there are read-only and stay deferred.BEGIN IMMEDIATEmechanism. The candidate and the parent differ in both. Report section 4.2 reverts each change on its own. The pairs' isolation is tracked as Concurrent pairs do not isolate the restored wait window from BEGIN IMMEDIATE #189.opencode-consume-recovery-167, and held until this PR merges. This PR Refs Queue-consume SQLite failure strands an ordinary session after completed tool calls #167 and Queued continuation stays pending after its consume exhausts the lock window: re-drain on lock release (decided A) #180 and does not close them.Refs
#164, #167, #175, #145, #87, #180, #181, #183 (the final pre-merge gate), #184, #185, #189, #191. #188 and #190 are closed in this branch.
Changeset
This PR needs its
CHANGESET.mdentry under "Fork changes" (script/changeset-check.tsmatches it to this PR's number). Firstmate adds the entry once the PR number exists, so the changeset check stays red until then.Report
Full report, with raw evidence paths:
/home/byates/fm-llmrouter/data/opencode-lock-fix-162/report.md(firstmate home; not on GitHub).🤖 Generated with Claude Code