fix(sessions): assign session output seq atomically - #88
Merged
ralyodio merged 1 commit intoJul 30, 2026
Merged
Conversation
A CLI streams output as it arrives, so two POST /api/sessions/:id/output calls are in flight at once. The handler read MAX(seq) and then inserted in a separate statement, so against a network database (Turso) both reads land before either insert does and both chunks are written with the same seq. The mirror's resume contract is that the page remembers the last seq it rendered and reconnects with ?since=<seq>, which replays `seq > since`. A chunk sharing the seq the browser already reported is therefore skipped permanently: the operator is looking at a terminal that silently dropped a line, with no indication anything is missing. Pick the seq inside the INSERT via SELECT ... RETURNING, the same single-statement discipline credit reservations and the command claim already use, so concurrent appends serialize on the write lock. Co-Authored-By: Claude <noreply@anthropic.com>
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.
The bug
POST /api/sessions/:id/outputpicks the nextseqin two statements:A CLI streams output as it arrives, so two output posts for one session are genuinely in flight at once. Against a network database (Turso) every statement is a round trip, so both reads land before either insert does and both chunks are written with the same seq. There is no unique constraint on
(session_id, seq)to catch it, so it fails silently.Why it matters
The mirror's resume contract is that the page tracks the last seq it rendered and reconnects with
?since=<seq>, and/sessions/:id/streamreplaysseq > since. So a chunk that shares the seq the browser already reported is skipped permanently. The operator is looking at a terminal that quietly dropped a line, with nothing to indicate output is missing.sessions.test.mjsalready pins this invariant for sequential posts —"seq must be monotonic per session". This makes it hold under the concurrency the endpoint actually sees.Repro on unmodified main (cb55ab3)
Real
sessionsRouteragainst a throwaway libsql file DB, no stubs, no network. Statements deferred to a macrotask so two handlers interleave the way they do against a remote DB — the same trickapprovals-credits.test.mjsalready uses, and for the same reason. Two concurrent posts, fresh socket each:Patched:
seqs: [1,2], and the resuming browser gets the chunk it had not seen.The fix
Assign the seq inside the INSERT, so the read and the write are one statement and concurrent appends serialize on the write lock:
This is the discipline already used elsewhere in the app rather than a new pattern:
reserve()inlib/credits.mjsputs itsWHEREinside the insert, and the command claim in this same file uses theUPDATEas the lock.RETURNINGgives the assigned seq back forpublish()and the scrollback prune. An aggregateSELECTwith noGROUP BYstill yields one row on an empty table, soCOALESCEmakes the first chunk seq 1.15 insertions / 4 deletions in
sessions.mjs, 8 of them comment.Deliberately not included
A
UNIQUE (session_id, seq)index would make this a hard error rather than silent corruption, and I would normally add one. I left it out because v0.12.0 is already shipped: any deployment that has hit this race has duplicate rows, and the migration would fail on them. Doing it properly needs a dedupe-then-constrain migration that decides what to do with the colliding chunks, which is your call, not a drive-by. Happy to send that separately if you want it.I also left
SCROLLBACKpruning, the SSE fan-out and the command paths alone — they were correct and are covered as controls below.Tests
New
apps/pwa/test/sessions-output-seq.test.mjs, 8 tests, built on theapprovals-credits.test.mjsharness.Three are the bug:
[1,2]seq > sincepredicate/streamreplays with)1..5with no gaps, repeats or dropped chunksFive are controls that pass both with and without the fix, so the change is provably scoped: sequential posts still number in order; seq is still per-session not global; an empty chunk still writes no row and still returns 200 as a keep-alive; a 20050-char chunk is still capped at 20000; a foreign API key still gets 404 and writes nothing.
Fail-before verified by restoring pristine
sessions.mjsviagit checkout --: 3 fail / 5 pass unpatched, 8/8 patched.Full suites green:
apps/pwa89 -> 97, root 283 -> 291, 0 failures.