fix(transport): re-acknowledge retransmitted packets so streams stop stalling

[sanity]

Problem

A retransmitted packet was never acknowledged, so a receipt that is destroyed outright could never be repaired by the retransmit it caused.

ReportResult::AlreadyReceived documents that the packet "will be re-acknowledged but should otherwise be ignored" (received_packet_tracker.rs:141-143). The implementation did not do this: the Occupied branch returned the variant without queueing anything (:67), and the only caller logs at trace! and continues (peer_connection.rs:1119-1126). pending_receipts.push existed at exactly one site — the Vacant branch — so nothing could re-acknowledge.

A lost receipt is normally recoverable, and an earlier version of this description wrongly said otherwise. Receipts ride on noop packets, but those go through packet_sending and are tracked and retransmitted like any other, so a single lost noop is repaired by its own resend.

The hole is narrower than that, and real. Both flush sites in the recv loop call get_receipts() — a mem::take — and move the list into noop():

let receipts = self.received_tracker.get_receipts();
if !receipts.is_empty() {
    if let Err(e) = self.noop(receipts).await {
        if e.is_transient_send_failure() {
            tracing::warn!(..., "ACK noop send failed, will retry");

peer_connection.rs:1104-1115 and :1477-1493. On a transient send failure those receipts are already gone — there is nothing left to retry with, and the log message says otherwise. Nothing regenerates them. The same applies when all 12 retransmits of the receipt-carrying noop are lost.

When that happens the sender retransmits into silence and its packet stays pinned in flight. If the congestion window fills before the retransmit budget runs out, the stream aborts at CWND_WAIT_TIMEOUT (3s) via drop_stream, which purges the stream's packets from the resend queue — and the receiver sees a transfer that simply stopped. Answering the retransmit is the only signal that can repair it.

(The stranding is bounded by the Abandon path from #4345, so "permanent" — also in an earlier version of this description — was wrong too. The harm is an aborted stream, not a permanently pinned window.)

Evidence, and how far it actually goes

The receiver-side signature is real and common. From vega, 03:00–05:00 UTC across August: 1441 stream-assembly inactivity timeouts and 145 cwnd-wait aborts, e.g.

Stream assembly timed out — no fragments received within inactivity window
    stream_id=2196300212 received_fragments=46 total_fragments=187 timeout_secs=5

Stall points across those 1441 are spread throughout the transfer (2.4% at ≤2 fragments, 5.8% at 3–10, 21.7% at 11–60, 69.0% mid-to-late, 1.1% near-complete), which fits an any-time receipt-loss event and does not fit a fixed window cap.

What this evidence does not establish: an earlier version cited flightsize == cwnd on every cwnd-wait abort as the signature of a window that cannot drain. That is close to tautological — the log at outbound_stream.rs:199-209 only fires from inside a loop entered when flightsize + packet_size > cwnd, so it prints those two values whatever the cause. It does not discriminate this mechanism from a slow peer or a congested reverse path.

So: this closes a genuine hole that produces exactly this failure signature. It is not established that it is the dominant contributor to those 1441 timeouts. Nothing currently counts duplicate re-acks, so that cannot be measured today — a counter paired with the existing record_stream_send_abort_cwnd() would settle it, and is worth doing before this is credited with any netcheck improvement.

Approach

Queue the receipt in the Occupied branch — what the enum's documented contract already promised.

• Bounded by MAX_PENDING_RECEIPTS. That cap is load-bearing, not advisory: send_packet's receipt chunker splits an oversized list only once (split_off returns the tail, peer_connection.rs:2390-2412), so a list longer than ~2×289 receipts serializes past MAX_DATA_SIZE, and that error is not a transient send failure, so it kills the connection. The Vacant arm holds this line with QueueFull; this arm must not route around it, or a peer replaying old ids becomes a remote connection-kill. Dropping the re-ack at capacity is safe — the list is about to flush, and an unrepaired receipt draws another retransmit.
• Deduplicated, so a retransmit burst for one id does not queue one receipt per copy.
• Still returns AlreadyReceived, not QueueFull, so the caller can tell a duplicate from a fresh packet. Note this does not by itself guarantee the payload is skipped — the caller's (_, true) arm is matched before (AlreadyReceived, _) and wins whenever the 600ms receipt timer trips. That is pre-existing; an earlier version of this description asserted the opposite. Filed separately as fix(transport): duplicate packet's payload is re-processed when the receipt timer trips, wedging legacy stream reassembly #5277.

Delivery is guaranteed by the unconditional drain on the background ACK tick (peer_connection.rs:1476-1493), so worst-case latency is one tick rather than unbounded.

Verified no double-release of flight size: report_received_receipts only emits an ack-info tuple when the id is still in pending_receipts (sent_packet_tracker.rs:538), so a re-ack for an already-released packet produces nothing and cannot double-decrement. This is the invariant .claude/rules/transport.md:86-110 governs and was the highest-risk interaction.

Testing

Three tests, each verified by mutation to fail without the specific line it pins.

• retransmitted_packet_is_reacknowledged_after_its_receipt_was_sent — the production sequence. The drain between the two reports is what makes it discriminating: without it the first report's receipt is still queued and the assertion passes regardless. That drain is the "receipt went out and was lost" step.
• repeated_retransmits_queue_one_receipt_per_flush — pins the dedup.
• reacknowledging_duplicates_cannot_grow_the_list_past_capacity — pins the bound. Written against 600 distinct replayed ids, deliberately: dedup alone caps the list at the number of distinct ids, so an earlier version of this test that replayed one id repeatedly passed with the bound removed and pinned nothing. With 600 it fails at grew the receipt list to 600, past the 20 cap.

Revert-and-run: removing pending_receipts.push fails exactly the first two (6 others pass); removing the length check fails exactly the third (8 others pass). So each test discriminates its own line rather than blanket-failing.

• transport::received_packet_tracker: 9 passed
• full transport:: suite: 697 passed, 0 failed
• cargo fmt --check clean; cargo clippy adds no new warnings

test_report_receipt_already_received still asserts pending_receipts.len() == 1 after a duplicate — the duplicate must not queue a second copy of an unsent receipt — with a comment saying which of the two things it pins.

Not fixed here

Found while reviewing this, filed rather than folded in, since both are pre-existing and in the recv loop rather than this tracker:

• fix(transport): duplicate packet's payload is re-processed when the receipt timer trips, wedging legacy stream reassembly #5277 — (AlreadyReceived, true) falls through to process_inbound, re-delivering a duplicate payload, which can permanently wedge legacy InboundStream reassembly.
• fix(transport): receipt chunker splits an oversized list only once, so a long receipt list fails the connection #5278 — the receipt chunker splits only once, so >2× the per-packet maximum kills the connection.

Scope

One contributing cause, not the whole netcheck red. That check's largest failure class is separate and not a transport problem at all: the network answers NotFound and the harness discards it, then reports a 120s timeout. Details on #5256 and #5271.

[AI-assisted - Claude]

[github-actions]

This is not directly relevant enough to pursue further — the comment's claim about the chunker is not part of the diff's functional code and doesn't need deep verification for a rule-compliance review. The diff itself is well-scoped, well-tested, and matches the transport module's bounded-collection and testing requirements.

Rule Review: No issues found

Rules checked: git-workflow.md, code-style.md, testing.md, transport.md
Files reviewed: 1 (crates/core/src/transport/received_packet_tracker.rs)

The change re-acknowledges retransmitted packets in the Occupied arm of report_received_packet, bounded by MAX_PENDING_RECEIPTS and deduplicated via contains, matching the code-style rule on bounding per-key/attacker-influenced collections. Three new tests cover the happy-path re-ack, duplicate-burst dedup, and at-capacity/overflow behavior (the reacknowledging_duplicates_cannot_grow_the_list_past_capacity test specifically exercises the boundary condition testing.md requires). The regression test (retransmitted_packet_is_reacknowledged_after_its_receipt_was_sent) reproduces the original bug — it fails without the fix (empty receipts after drain + retransmit) and passes with it, satisfying the fix-PR regression-test requirement. No .unwrap() in production code, no spawns, no biased select, no channel changes, no threshold hardcoding disconnected from config. Comments explain WHY, consistent with code-style.md.

No rule violations detected.

---

Rule review against .claude/rules/. WARNING findings block merge.

[sanity]

Review round 1 — findings and what changed

Ran two independent blind reviewers (skeptical, code-first) per the Full-tier rule for transport changes. Both independently found the same three problems, one of them blocking. Pushed d1c26be5b and rewrote the PR description.

Blocking, fixed. The first commit removed the only bound on pending_receipts — the Occupied arm pushed without the length check the Vacant arm enforces via QueueFull. My description had asserted the list was "bounded by MAX_PENDING_RECEIPTS (20)", which was true before the change and false after it. That matters more than it sounds: the receipt chunker at peer_connection.rs:2390-2412 splits an oversized list only once, so >2×289 receipts serializes past MAX_DATA_SIZE, and that error is not transient, so it kills the connection. A peer replaying old packet ids could have triggered it. Now bounded, with a test.

My stated mechanism was wrong, corrected. I claimed receipts are never retransmitted, so a lost one is unrecoverable. Not true — noop() goes through packet_sending and is tracked and retransmitted like any other packet. The genuinely unrecoverable case is narrower: the recv loop's flush sites have already mem::taken the list into noop() when the send fails, so their "will retry" log has nothing left to retry with. That hole is real and the fix still closes it, but the description now says so accurately rather than overclaiming.

Weak evidence withdrawn. I cited flightsize == cwnd on every cwnd-wait abort as the signature of a window that cannot drain. It is near-tautological: the log only fires from inside a loop entered when flightsize + packet_size > cwnd. It does not discriminate this mechanism from a slow peer. The stall-point distribution is the honest evidence, and I have added an explicit statement that this is not established as the dominant contributor to the 1441 timeouts, plus a note that a duplicate-re-ack counter is what would settle it.

A test that pinned nothing. My first bound test replayed one id repeatedly and passed with the bound removed — dedup alone caps the list at the number of distinct ids. Rewritten against 600 distinct ids; it now fails with grew the receipt list to 600, past the 20 cap. Each of the three tests is individually mutation-verified to fail without the specific line it pins.

Not folded in, filed instead — both pre-existing, both in the recv loop rather than this tracker: #5277 ((AlreadyReceived, true) falls through to process_inbound, re-delivering a duplicate payload and permanently wedging legacy InboundStream reassembly) and #5278 (the chunker bug above).

Confirmed clean by both reviewers and re-verified: no double-release of flight size. report_received_receipts only emits an ack-info tuple when the id is still in pending_receipts (sent_packet_tracker.rs:538), so a re-ack for an already-released packet produces nothing. Also no starvation of the ACK tick (deterministic_select! rotates rather than being biased), and no concurrency hazard (the tracker is a plain field on the single connection task).

One reviewer suggestion I did not take, flagging it as a judgement call: fixing #5277's arm ordering here. It is a separate defect with a different failure mode, and it touches the recv loop, so it deserves its own diff and its own review rather than riding along.

Still open and worth a reviewer's opinion: whether this should land at all without the counter that would let us tell in production whether it helped.

[AI-assisted - Claude]