Fix stdio frame interleaving under backpressure - #266
Draft
rohitsalla wants to merge 1 commit into
Draft
rohitsalla wants to merge 1 commit into
rohitsalla wants to merge 1 commit into
Conversation
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
modelcontextprotocol#266, jameswilson) Manifest entry 9. Upstream refs/pull/266/head at 044e2b3, unmodified. StdioTransport.send wrote directly to the descriptor, so two concurrent sends could interleave while one was retrying EAGAIN on a full pipe: the second message was spliced into the middle of the first, producing invalid JSON on the wire. Sends are now serialized FIFO through a task chain. Reproduced before the merge with the PR's own regression test on integration: the small frame landed inside the large one, ahead of its terminator — inserted [{"id":2}\n], removed [\n{"id":2}]. One of seven stdio tests failed there; all seven pass after. Private implementation only: a lastSend task handle and a private write(_:). No public API change, no wire format change. Upstream PR is still a draft — the author could not run the suite locally, which is the gap this fork can close. The recorded SHA may move if they revise it. Fixes modelcontextprotocol#263. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
StdioTransport.write checks isConnected once on entry and never again inside the EAGAIN retry loop. A send that fills the pipe and parks there keeps retrying every 10ms after disconnect(), for as long as the reader stays stalled. Since modelcontextprotocol#266 serializes sends, everything queued behind it waits with it, so one stalled write now blocks the transport rather than just itself. Re-check isConnected after each backpressure sleep and throw ENOTCONN. Abandoning a partially written frame is safe here and only here: the transport is being torn down, so there is no peer left to mis-frame. That is why this is not the same as propagating caller cancellation into the write, which would abandon frames on a live connection and recreate the interleaving modelcontextprotocol#266 fixes. The loop predates modelcontextprotocol#266; only its blast radius changed. The accompanying test drives a real 512KB write into an undrained pipe, disconnects, and races the send against a 3s deadline. It passes with this change and does not terminate without it. Its cleanup is best-effort: a send already parked in the retry loop cannot be reclaimed from the test, so on regression the expectation fails but the run may stall. Failing cleanly would need a seam in the retry loop the transport does not expose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
…nnect Manifest entry 9a. Fork addition at a0bac05, one clean commit stacked on modelcontextprotocol#266's 044e2b3, offerable upstream as submitted. StdioTransport.write checked isConnected once on entry and never again inside the EAGAIN retry loop, so a send parked on backpressure kept retrying every 10ms after disconnect(). Entry 9's serialization means everything queued behind it waited with it. Abandoning a partial frame is safe on teardown and only on teardown, which is why this is not the same as propagating caller cancellation into the write — that would abandon frames on a live connection and recreate the interleaving entry 9 fixes. Cancellation into the send remains unaddressed by design. The retry loop predates modelcontextprotocol#266; only its blast radius changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
…cle as entry 9a StdioTransport.send wrote straight to the descriptor, so concurrent sends could interleave while one retried EAGAIN on a full pipe, splicing one message into the middle of another and putting invalid JSON on the wire. Reproduced with the PR's own regression test before the merge. Entry 9 records what the merge accepts rather than fixes: caller cancellation never reaches the send, which is accidentally protective because naive propagation would throw mid-frame and recreate the corruption; and the trade made is frame corruption exchanged for head-of-line blocking, which is the right direction but is a trade. It also records that modelcontextprotocol#275 does not address the transport's cancellation, and that the upstream PR is a draft whose SHA may move. Entry 9a is the follow-up: re-check isConnected inside the retry loop so a backpressured send terminates on disconnect. Its test caveat is recorded — it passes with the change and does not terminate without it, so it is a positive check whose cleanup is best-effort. Tracking: fork issue #16. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
ianegordon
added a commit
to ianegordon/swift-sdk
that referenced
this pull request
Sep 12, 2026
Issue titles are now bare 'Upstream PR#<n>'; the merge/investigate/decline label carries the decision, so the title does not duplicate it. Two places named the old convention and are updated. The investigate count was also stale: it said twelve, which was true at triage on 2026-09-10. Four were declined since (modelcontextprotocol#280, modelcontextprotocol#226, modelcontextprotocol#118, modelcontextprotocol#204) and two were promoted and merged (modelcontextprotocol#266 as entries 9/9a, modelcontextprotocol#275 as 10/10a/10b), leaving six — modelcontextprotocol#178, modelcontextprotocol#213, modelcontextprotocol#216, modelcontextprotocol#257, modelcontextprotocol#258, modelcontextprotocol#259 — now listed by number rather than by count, so the sentence cannot drift again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Eb9yGSXH1TVkg5Afsu9phk
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
1 participant
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 #263
Summary
StdioTransport.send()calls in FIFO order.EAGAINretry behavior.Root cause
StdioTransportis an actor, but its write loop suspends while retryingEAGAIN. Actor reentrancy allowed a secondsend()call to write while the first frame was only partially written, splicing one JSON-RPC frame into another.Each send now waits for the previously enqueued send to complete before entering the write loop. A failed send does not prevent later queued sends from running.
Verification
ordered=false, with the second frame's newline inside the first payload.ordered=true, first newline at the expected 524288-byte boundary.git diff --checkpasses.The full
swift testtarget could not run locally because the installed macOS Command Line Tools lack the repository's existingTestingmodule and have a Swift compiler/SDK patch mismatch. The pull request CI matrix provides the required Swift 6.1 macOS and Linux coverage.