Skip to content

fix(proto): send a path's acknowledgements on that path - #801

Open
richinsley wants to merge 1 commit into
n0-computer:mainfrom
richinsley:pr/own-path-acks
Open

richinsley wants to merge 1 commit into
n0-computer:mainfrom
richinsley:pr/own-path-acks

Conversation

@richinsley

@richinsley richinsley commented Sep 11, 2026

Copy link
Copy Markdown

Description

With multipath, PATH_ACK frames for every path were coalesced into whichever path's packet was built first — in practice path 0, since the transmit loop visits paths in order and path 0 usually has something to send. Path 1's acknowledgements therefore rode path 0, which ties path 1's loss recovery and congestion feedback to path 0's condition: when path 0 degrades, path 1's acks queue behind it and path 1's RTT and delivery-rate samples inflate with a queue path 1 is not on.

This change sends a path's acknowledgements on that path whenever it can carry them (known CIDs, not abandoned, validated), and only piggybacks them onto another path's packet when it cannot. PacketSpace::can_send takes the same predicate so a path with only foreign acks pending is not asked to build a packet it will not fill.

  • Connection::path_carries_own_acks — the predicate. It takes the three fields rather than &self so callers can hold &mut self.spaces at the same time.
  • populate_packet — other paths' acks are filtered out of this packet when those paths can carry their own. The filter list is built only when more than one path has a packet-number space and is at most the number of paths.
  • PacketSpace::can_send — the same filter when deciding whether a path has acks to send.
  • Test path_acks_sent_on_own_path: two validated paths with stream data on both (path 0 toggled to Backup and back so both carry data); the server must transmit PATH_ACKs on path 1 as well as on path 0. On main the counts are 6 and 0; with this change 2 and 4.

Fixes #799.

API Changes

None. PacketSpace::can_send gains a predicate argument but is pub(super).

Notes & open questions

  • A path that cannot carry its own acks (not yet validated, abandoned, no CIDs) still gets them piggybacked as before, so nothing waits on a path that cannot send.
  • Where I hit it: a link-bonding experiment over two netem-shaped links with a paced media source and one link throttled mid-stream; the surviving link's delivery went from ~65 % to 97–98 % with this change. I did not add that scenario as a test because I could not see a way to point BwLimitedRouting at a single path in the harness — happy to add one if there is.
  • This came out of a Rust port of a bonding transport I am prototyping; I prepared the change with tooling assistance and have gone through the mechanism and the test myself.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All API changes documented.
  • This PR was created by a human that thought critically about the
    proposed change and wrote an as clear and concise description as
    they could.
  • This PR isn't slop, and is carefully crafted to do have the
    intented effect.
  • cargo make passes locally.

With multipath, PATH_ACK frames for every path were written into whichever
path's packet was built first. In practice that is path 0: the transmit
loop visits paths in order, and path 0 usually has something to send. So
path 1's acknowledgements rode path 0.

That ties path 1's loss recovery and congestion feedback to path 0's
fate. When path 0 degrades (a cellular link losing signal, a queue
building behind it), path 1's acks sit in path 0's queue and path 1's RTT
and delivery-rate samples inflate with a queue it is not on, even though
path 1 itself is healthy. Observed with a paced media source over two
links where one is throttled mid-stream: the surviving link never ramped.

Acks for a path are now sent on that path whenever it can carry them
(known CIDs, not abandoned, validated), and only piggybacked onto another
path's packet when it cannot. `PacketSpace::can_send` takes the predicate
so a path with only foreign acks pending is not asked to build a packet.

Test: two validated paths with data on both; the server must transmit
PATH_ACKs on path 1 as well as on path 0. On main the counts are
6 and 0; with this change 2 and 4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

PATH_ACKs for every path are sent on whichever path transmits first

1 participant