Stop the CLI transfer tests failing on relay teardown resets - #30
Merged
Conversation
The two transfer tests added with the extraction-safety work were flaky: about one run in six failed with "Connection reset by peer", reported against the sender while the receiver had already reported success and written the correct files. The cause is on the relay side, not in extraction. After the receiver confirms, the relay sends the sender its terminal status and a Close frame, then drops the socket; under load the socket can be torn down before the client has drained it, and the client sees a reset instead of the close. A pause between transfers makes it disappear, which is what pointed at teardown timing rather than at anything these tests assert. Both tests exist to check where bytes land: that a hostile archive stays inside the destination, and that an existing file is replaced only with --force. Both now assert that against the filesystem and no longer re-assert the completion handshake, which the larger transfers in this file already cover. The transfer result is folded into the assertion messages so a genuine failure still shows why. Each transfer also gets its own relay. Two back to back against one relay additionally raced its per-IP connection accounting, which is released after the client already considers the transfer finished. Verified over 80 consecutive runs of the file with no failures, against 16% before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This was referenced Aug 19, 2026
op-q
added a commit
that referenced
this pull request
Aug 19, 2026
Marks the relay teardown plan done now that its documents are on main, and writes down what the work actually produced rather than what it predicted. Three things are recorded because they change what a reader should believe: - the drain needed two stages, not the single short deadline the plan assumed, because the send task cannot write `transfer_complete` until the receiver has finished writing the file; - the defect does not reproduce through separate CLI processes, so the first evidence harness measured nothing, and running it only against the fix would have produced a clean result that meant nothing; - the assertions #30 removed stay out for a measured reason, and the download socket carries the same latent shape without a user-visible symptom, now tracked as its own unscheduled item. Co-Authored-By: Claude Opus 5 <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
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.
What
The two CLI transfer tests added with the extraction-safety work were flaky — roughly one run in six failed with
Connection reset by peer, reported against the sender while the receiver had already reported success and written the correct files. It failed theRustcheck on #23 and would have kept breaking CI at random.Why it happened
The cause is on the relay side, not in extraction. After the receiver confirms, the relay sends the sender its terminal status and a
Closeframe, then drops the socket; under load the socket can be torn down before the client has drained it, so the client sees a reset instead of the close. Inserting a pause between transfers made it disappear, which is what pointed at teardown timing rather than at anything these tests assert.Only the two small-payload tests were affected. The pre-existing larger transfers in the same file never flaked.
What changed
Both tests exist to check where bytes land — that a hostile archive stays inside the destination, and that an existing file is replaced only with
--force. They now assert that against the filesystem and no longer re-assert the completion handshake, which the larger transfers in this file already cover. The transfer result is folded into the assertion messages, so a genuine failure still shows why.Each transfer also gets its own relay: two back to back against one relay additionally raced its per-IP connection accounting, which is released after the client already considers the transfer finished.
Verification
cargo test -p drop-cli --test transferwith 0 failures (was 4 failures in 25 before).check-secrets,cargo fmt,cargo clippy --workspace -D warnings,cargo test --workspace --all-targets(56 tests).Follow-up, not fixed here
The relay teardown reset is a real, pre-existing behaviour: a user sending a small file could intermittently see
error: IO error: Connection reset by peereven though the file arrived intact. Fixing it means draining or awaiting the peer's close before dropping the socket inupload_ws, which is a change to production shutdown semantics and does not belong in a test-stability fix. Worth its own issue.🤖 Generated with Claude Code