Skip to content

fix: make transport flight-size accounting consistent (plaintext vs encrypted)#4528

Open
mvanhorn wants to merge 1 commit into
freenet:mainfrom
mvanhorn:fix/4402-flightsize-plaintext-encrypted-accountin
Open

fix: make transport flight-size accounting consistent (plaintext vs encrypted)#4528
mvanhorn wants to merge 1 commit into
freenet:mainfrom
mvanhorn:fix/4402-flightsize-plaintext-encrypted-accountin

Conversation

@mvanhorn

Copy link
Copy Markdown

Summary

Flight-size accounting now adds and releases the same byte count for every packet, so the connection-wide flight-size counter no longer drifts downward over the life of a connection.

Why this matters

on_send/on_send_with_token add the plaintext fragment/packet size to flight size, but all three release paths (ACK via report_received_receipts -> on_ack*, Abandon via ResendAction::Abandon -> release_flightsize, and drop_stream) released the encrypted stored payload length (payload.len(), which includes the AEAD tag and framing overhead). Every release subtracted slightly more than was added, biasing flight size low. It is low-impact under the default FixedRate controller but matters for BBR/LEDBAT, where flight size is load-bearing. Fixes #4402.

Changes

  • Store the plaintext packet_size alongside the encrypted payload in each pending_receipts entry, and release that stored plaintext size on all three paths (ACK, Abandon, drop_stream). This makes sum(on_send) == sum(release).
  • Thread the exact packet_size the caller passed to on_send_with_token down through packet_sending into the tracker, rather than recomputing it from the serialized payload. This handles fragment Overall architecture RFC #1 with embedded metadata, where the actual fragment payload is smaller than the byte count added to flight size, so a recomputed value would under-release.
  • Resend re-registration (refresh_sent_packet and the insert-or-refresh path) preserves the original plaintext byte count, since flight size is added only on the first send.
  • Update the accounting-convention doc comment in sent_packet_tracker.rs to describe the new plaintext-on-both-sides convention.

Testing

  • cargo test -p freenet --lib transport::sent_packet_tracker — 61 passed, including a new test_issue_4402_plaintext_flightsize_releases_balance_all_paths that asserts flight size returns to zero after a stream is fully ACKed, fully abandoned, dropped via drop_stream, and a mixed ACK/drop sequence.
  • cargo test -p freenet --lib transport::peer_connection::tests::packet_sending — 2 passed, including a new packet_sending_tracks_caller_flight_size_for_metadata_stream_fragment regression test proving a metadata-bearing fragment releases the caller's flight-size bytes, not payload.len().
  • cargo check -p freenet --lib — clean.

AI was used for assistance.

@github-actions

Copy link
Copy Markdown
Contributor

Now I have everything I need. Let me verify the consistency of the 50 value in noop() to confirm add and release paths match.

The grep confirmed it: peer_connection.rs:1852 has .on_send_with_token(50) (the ADD side) and the new diff adds 50 as packet_size to packet_sending(...) (the RELEASE side). They match — the noop() function consistently uses 50 bytes for both accounting directions, with a comment explaining it's an estimate.


Rule Review: Minor documentation gap; no blocking issues

Rules checked: git-workflow.md, code-style.md, testing.md, transport.md
Files reviewed: 3

Warnings

None.

Info

  • crates/core/src/transport/peer_connection.rs (commit subject) — commit subject is 78 characters, over the 72-character limit required by git-workflow.md. Suggested: fix: make transport flight-size accounting consistent (51 chars) with the (plaintext vs encrypted) detail moved to the body.

  • .claude/rules/transport.md (not changed in this PR) — the flight-size release invariant docs describe the three release paths as each returning "the packet size" and "len" without specifying plaintext or encrypted convention. This PR's entire purpose is making all three paths use the plaintext convention, yet the rule file is silent on the distinction. Per AGENTS.md's "WHEN you discover outdated or missing documentation" policy, the same PR should update the rule to say "plaintext byte count" for all three paths, so future contributors cannot reintroduce the same encrypted/plaintext asymmetry while believing the rule permits it.

All other checks passed:

  • Regression tests present: test_issue_4402_plaintext_flightsize_releases_balance_all_paths covers all three release paths (ACK, Abandon, drop_stream) with distinct plaintext/encrypted sizes; packet_sending_tracks_caller_flight_size_for_metadata_stream_fragment covers the metadata-stream-fragment caller path. Both would catch the exact bug if reintroduced. ✓
  • No .unwrap() in new production code. ✓
  • ADD side (on_send_with_token(packet_size)) and RELEASE side (PendingReceiptEntry.3) use the same caller-supplied plaintext size consistently across all three call paths. ✓
  • report_sent_stream_packet correctly marked #[cfg(test)]; production callers now always go through report_sent_stream_packet_with_size. ✓
  • Resend re-registration preserves original plaintext size (let packet_size = slot.3), correctly not re-adding to flight size. ✓

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Transport flight-size accounting mixes plaintext (on_send) and encrypted (release) byte counts

1 participant