fix: serialize frame writes per socket, not per writer - #158
Merged
Conversation
Opening a new workspace could greet you with "unrecognized command — graphcoded may be older than the client that sent it", and a quit and relaunch fixed it. Nothing was out of date: the bytes were spliced. A frame goes out as two writes — a 4-byte length header, then the body — and nothing gave a connection a single writer. The app sends every command from `DispatchQueue.global()`, and `AppFeature`'s launch effect merges three of them (`listRecentProjects`, `restoreOpenProjects`, `openGlobalGraph`). Overlap two and the reader takes header A followed by body B, then reads a length that belongs to someone else's message and is wrong from that byte onward — it decodes garbage, which is what raises that error, or waits for bytes that never come. A new workspace is where it surfaces because the three sends wait together on `connectWithBackoff` while the just-bootstrapped daemon comes up, and are released at the same instant. On the next launch the daemon is already listening, the sends stagger, and the collision does not land. The same hazard runs the other way: the daemon answers one client from two separate actors (`ProjectRegistry`, `GraphStore`) plus the skew reply in `graphcoded/main.swift`, so a broadcast could corrupt a reply in flight. `writeFrame` now takes a lock for the descriptor it is writing to, which fixes every call site at once. Per descriptor rather than one lock for the process: a write blocks while its socket buffer is full, and the daemon broadcasts to every client — one lock would let a wedged client stall writes to all of them. The test fails on the code before this commit: 1 of 8 frames arrived and the reader then hung on a spliced header until its 30-second timeout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: scgopi <scgopireddy@gmail.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.
Fixes the "unrecognised command — graphcoded may be older than the client" error seen on first open of a new workspace in 0.1.46-beta1, which a quit and relaunch clears.
It isn't version skew
Nothing is out of date — the bytes are spliced. A frame goes out as two writes (a 4-byte length header, then the body), and nothing gave a connection a single writer:
DaemonConnection.sendhops ontoDispatchQueue.global();AppFeature.start()merges three at launch —listRecentProjects,restoreOpenProjects,openGlobalGraphProjectRegistryandGraphStoreare separate actors, plus the skew reply ingraphcoded/main.swift:136Overlap two and the reader consumes header A followed by body B, then reads a length belonging to someone else's message and is wrong from that byte onward — it either decodes garbage (which raises exactly this error, the only place that string exists) or blocks waiting for bytes that never arrive.
Why a new workspace specifically: the three launch commands wait together on
connectWithBackoffwhile the freshly bootstrapped daemon comes up, then resume at the same instant — the widest possible collision window. On the next launch the daemon is already listening, the sends stagger, and it doesn't land. That is precisely "first time only, restart is fine".The fix
FramedMessageIO.writeFramenow takes a lock for the descriptor it is writing to, so a frame can never interleave with another on the same connection. One call site changed, all five writers fixed.Per descriptor rather than one lock for the process: a write blocks while its socket buffer is full and the daemon broadcasts to every connected client, so a single lock would let one wedged client stall the writes to all the others. Locks are kept rather than reclaimed on close — descriptor numbers are small and reused, so the table stays about as big as the peak connection count, and a reused number gets the same lock back.
Verification
concurrentWritersDoNotInterleaveTheirFrames— 8 writers, 64 KB bodies (larger than a socket buffer, so a singlewritecannot complete in one syscall and the interleaving is certain rather than occasional):Full suite: 989 tests passing.
swift format lint --strictclean.🤖 Generated with Claude Code