fix(dm): let a lost chunk be asked for again - #36
Merged
Conversation
A transfer that lost a single chunk hung forever. The receiver did ask again — `next_chunk_request` re-requests gaps below its cursor on every pump — but the repeat request was byte-identical to the previous one, and `has_seen_message` dedups on `channel + sha256(payload)`. So the sender dropped every repeat before `handle_blob` ever saw it and never re-served. Re-served chunk frames are byte-identical too, so they were dropped on the way back. Users saw transfers stick at 63%, 32% and 0%. This is the same trap that kept the MLS handshake silent: the recovery mechanism is re-sending the identical bytes, and a payload-hash dedup makes recovery unreachable. Exempt the blob channel like the control channel. Both directions are idempotent — `serve_chunks` re-encrypts, `ingest_chunk` answers Duplicate for a chunk already held. What keeps the repeats from becoming a flood is now an in-flight window in `next_chunk_request`: a chunk asked for less than CHUNK_REQUEST_TIMEOUT ago is not asked for again, so the once-a-second pump stops re-requesting a batch that is still on the wire. The cursor still advances a fresh window every pump, so throughput is unchanged. `served_chunks` becomes a count rather than a set, which is what makes "the sender re-served it" observable. The new DM test drives `handle_moss_message`, not `handle_blob`. The existing attachment tests call the handlers directly and so never crossed the layer the bug lived in — which is how it stayed hidden.
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.
Problem
One lost chunk hung a transfer forever. Users saw 63%, 32% and 0%.
The receiver did ask again —
next_chunk_requestre-requests gaps below its cursor on every pump. But the repeat was byte-identical to the previous request, andhas_seen_messagededups onchannel + sha256(payload), so the sender dropped every repeat beforehandle_blobsaw it and never re-served. Re-served chunk frames are byte-identical too, so they died on the way back.Same trap that kept the MLS handshake silent: the recovery mechanism is re-sending identical bytes, and a payload-hash dedup makes recovery unreachable.
Fix
serve_chunksre-encrypts,ingest_chunkanswersDuplicatefor a chunk already held.next_chunk_request: a chunk asked for less thanCHUNK_REQUEST_TIMEOUT(10s) ago is not asked for again. This is what stops the once-a-second pump from re-requesting a batch that is still on the wire. The cursor still advances a fresh window every pump, so throughput is unchanged.served_chunksbecomes a count, which makes "the sender re-served it" observable.Verification
a_repeated_chunk_request_still_reaches_the_senderdriveshandle_moss_message, nothandle_blob— the existing attachment tests call handlers directly and so never crossed the layer the bug lived in. Confirmed red without the fix (served_count1, want 2).a_lost_chunk_is_asked_for_again_once_the_window_passescovers the pacing and the retry.-D warningsclean.private_dm_runtime_transfers_attachment_over_mossover real moss still completes in ~5s, so the window costs no throughput.