fix(proto): On abandoned paths declare packets as lost after some time - #770
fix(proto): On abandoned paths declare packets as lost after some time#770Frando wants to merge 4 commits into
Conversation
…doned When a path is abandoned, all in-flight packets on that path are now immediately declared as lost, causing their retransmittable frames to be queued for retransmission on another active path. Previously, in-flight packets remained in the abandoned path's sent_packets until the PathDrained timer fired. That timer only arms once we receive the peer's own PATH_ABANDON frame for this path (not an ACK of ours - the incoming PATH_ABANDON handler), 3*PTO after that; and that in turn requires some live path to carry the frame at all. In the meantime, loss detection kept cycling on the dead path via PTO probes that could never be ACKed, and the actual stream data was never retransmitted on the new active path. This caused multi-second stalls (up to ~20s observed) after path migration: the application's stream data was stuck on the abandoned path while a perfectly good new path was available. The LossDetection timer on the abandoned path is now also stopped, since all packets are declared lost immediately and there is nothing left to detect.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/770/docs/noq/ Last updated: 2026-07-27T08:05:32Z |
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5238.3 Mbps | 7883.7 Mbps | -33.6% | 97.9% / 150.0% |
| medium-concurrent | 5324.4 Mbps | 7467.7 Mbps | -28.7% | 92.9% / 99.0% |
| medium-single | 3352.3 Mbps | 4688.9 Mbps | -28.5% | 92.3% / 100.0% |
| small-concurrent | 3726.5 Mbps | 5229.2 Mbps | -28.7% | 98.0% / 153.0% |
| small-single | 3327.2 Mbps | 4749.5 Mbps | -29.9% | 97.5% / 153.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3072.9 Mbps | 4134.7 Mbps | -25.7% |
| lan | 782.4 Mbps | 810.3 Mbps | -3.5% |
| lossy | 69.9 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 28.8% slower on average
45363044e5a5a8088e83d8e083ab530e4883552f - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5418.9 Mbps | 7894.5 Mbps | -31.4% | 93.7% / 98.0% |
| medium-concurrent | 5283.5 Mbps | 7646.4 Mbps | -30.9% | 94.2% / 99.0% |
| medium-single | 3578.8 Mbps | 4684.7 Mbps | -23.6% | 93.6% / 100.0% |
| small-concurrent | 3804.8 Mbps | 5106.8 Mbps | -25.5% | 99.8% / 152.0% |
| small-single | 3470.7 Mbps | 4768.7 Mbps | -27.2% | 89.5% / 97.3% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2988.7 Mbps | 4008.9 Mbps | -25.4% |
| lan | 782.4 Mbps | 810.3 Mbps | -3.4% |
| lossy | 69.9 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.3% slower on average
c3d26e4fc5ca67289769783ff8c18b5e307fd5c3 - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5398.6 Mbps | 8198.5 Mbps | -34.2% | 97.4% / 151.0% |
| medium-concurrent | 5296.8 Mbps | 7255.2 Mbps | -27.0% | 92.7% / 99.0% |
| medium-single | 4236.3 Mbps | 4628.5 Mbps | -8.5% | 91.4% / 102.0% |
| small-concurrent | 3769.3 Mbps | 5244.9 Mbps | -28.1% | 95.5% / 104.0% |
| small-single | 3435.0 Mbps | 4683.3 Mbps | -26.7% | 94.8% / 103.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | N/A | 4050.5 Mbps | N/A |
| lan | N/A | 812.7 Mbps | N/A |
| lossy | N/A | 69.9 Mbps | N/A |
| wan | N/A | 83.8 Mbps | N/A |
Summary
noq is 26.2% slower on average
|
Declaring all in-flight packets as lost when the path is abandoned is bad I think. These in-flight packets may still arrive. So you're potentially wasting a lot of resources. This should follow normal retransmission and loss detection as closely as possible IMHO. The problem is of course that loss is only declared once a new packet is received, which won't happen on an abandoned path. I've long been thinking that when the PTO fires for an abandoned path is the right time to start re-transmitting in-flight data on a new path. I also kind of wanted this to be more progressive, so that not all data is re-sent on a different path immediately, but rather more based on sent-time. That is of course not ideal either. Plus that's hard, the PTO timer only fires for the last packet sent. I almost want to set a per-packet PTO timer once path is abandoned, and each time it first send that packet's data on a different path. But that might just be too expensive. I looked for an issue and couldn't find any right away, which surprises me because this has been an issue we've been aware off for a while too. |
…ot immediately Per review: the in-flight packets may well have been delivered, and their ACKs can still arrive after the abandon, coalesced onto other paths (PathAck names the acked path explicitly, independent of the carrying path). Declaring everything lost at abandon time would retransmit data the peer already received. abandon_path now keeps the path's LossDetection timer and arms it at now + 2*PTO instead of declaring in-flight lost inline. When it fires on an abandoned path, whatever was acknowledged in the meantime is already gone from sent_packets and only the remainder is declared lost and requeued onto the remaining paths. set_loss_detection_timer leaves the timer alone for abandoned paths, since an ACK arriving during the window would otherwise stop it (no PTO gets armed for abandoned paths) and strand the rest again. 2*PTO is comfortably enough for any straggler ACK to make it back while still bounding the retransmission delay by the abandoned path's own RTT scale rather than the PathDrained round trip.
…detection_timer Keep all loss-detection timer arming in one place: abandon_path now stops the stale timer and calls set_loss_detection_timer like it did before, and the abandoned-path branch there arms the 2*PTO deadline itself (only when the timer is unarmed, so later calls still leave the deadline untouched). The rationale comment moves along with the code. One corner changes slightly: if something lands in an abandoned path's sent_packets after the deadline already fired (a PATH_ABANDON retransmitted on the last path), a fresh 2*PTO cycle is armed for it instead of leaving it for the PathDrained cleanup. Also from review: restructure on_loss_detection_timeout's doc into one list (the former/latter phrasing no longer worked with three reasons), correct the PathDrained keep-timer comment in abandon_path (the timer is armed when the peer's PATH_ABANDON arrives, not when ours is sent - it contradicted the arming comment a few lines below), and import StreamId in the tests instead of an inline crate:: path.
Three tests for the previous two commits, next to the existing abandon tests in multipath.rs: - abandoned_path_in_flight_retransmits_after_pto_delay: stream data lost on path 0, path 0 abandoned, all server-to-client traffic dropped from then on so the PathDrained cleanup never arms. Only the declare-in-flight-lost timer can free the data; asserts it has not fired below the 2*PTO lower bound (probed via handle_timeout without advancing the clock) and that the data arrives over path 1 after it. Fails on main (data stranded forever) and on the immediate variant (retransmit before the deadline). - abandoned_path_acked_in_flight_not_retransmitted: the flight's delayed acknowledgment is in the client's inbound queue when the path is abandoned; processing it during the window leaves nothing to declare lost, so no stream frame is sent twice. Fails on the immediate variant - this is the reason the declare-lost is deferred. - abandoned_path_partially_acked_retransmits_only_rest: two flights in the air at abandon time, the first acknowledged mid-window and the second lost; exactly one stream frame is retransmitted. Also covers the deadline surviving mid-window ACK processing (set_loss_detection_timer leaving abandoned paths alone). Verified against both predecessors: on main (5cf07f4) the first test fails, on the immediate declare-lost variant (630db78) all three fail; on this branch 390/390 pass.
4536304 to
c3d26e4
Compare
|
The last commits move this branch in the direction that @flub outlines above. It does not add per-packet PTO timers though. Instead of declaring all packets lost immediately, it now arms the loss detection timer to 2*PTO after the abandon, and when that timer fires for an abandoned path, it declares the path's remaining in-flight packets lost. ACKs that arrive during the window are honored regularily (so only what is still unacknowledged is declared lost and thus gets resent over the remaining paths). To me this looks like a sensible compromise: we give in-flight data 2*PTO to be acknowledged, but then resend the rest instead of having it sit in the dead path's The case in question is exercised by a patchbay test in n0-computer/iroh#4435: a connection has a single established path, a relay path. We send data, and while it is in flight the relay is shut down. With 4434 iroh noq reacts by establishing a new relay path (takes a few seconds), but the in-flight data never reached the server, and without this change noq doesn't retransmit it until the dead relay path is drained. So nominally we go from 3*PTO to 2*PTO, but there's two further differences at play: First, the draining timer of 3*PTO only starts counting once the server's PATH_ABANDON arrives. Second, once the new relay path is up, it delivers the server's ACKs for the lastly delivered packtes on the now-dead previous relay path. The last packet the server received on the dying path was never acknowledged before, so this late ACK advances largest-acked and triggers an RTT sample. This RTT sample now spans The RTT sampling behavior for dying paths might be worth looking into on its own. |
Frando
changed the title
|
So I've also been thinking about this more over the weekend and changed my opinion a bit since the rushed comment I wrote on friday. I no longer think the gradual thing makes much sense. When a PATH_ABANDON is received you are supposed to send ACKs for any packets on that path immediately, also any packets arriving later need to be ACKed immediately. This means you can use a tighter deadline before starting to retransmit these lost packets on another path. In theory 1-RTT (i.e. without ack_delay) after the PATH_ABANDON was sent. But of course it is more complicated than that since the ACKs may have been previously flowing over the now abandoned path, and so they may take longer to arrive now. So I'm not exactly sure what the right tradeoff is. But also: we absolutely need to check that we send PATH_ACKs immediately once a PATH_ABANDON was received.
Ouch, that's crazy. Perhaps this needs to be capped as well. We're not sending things in space here. Maybe we can write this down in an issue as it seems this can be something independent to work on. Also, thanks for adding the reason why you're looking at this! (btw, this is still in draft. I've never looked at any code yet, only read the description) |
2 participants
Description
Experiment / draft
Change: When a path is abandoned, all in-flight packets on that path are now immediately declared as lost, causing their retransmittable frames to be queued for retransmission on another active path.
Previously, in-flight packets remained in the abandoned path's sent_packets until the PathDrained timer fired. That timer is only armed once we receive the peer's own PATH_ABANDON frame for this path, and then runs for 3*PTO. This means stream data is not retransmitted for at least 3*PTO when the previously-active path dies.
Breaking Changes
Notes & open questions
I had claude compare to picoquic and spec (not checked myself yet):
Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.
cargo makepasses locally.