fix(proto): ignore coalesced datagram tail for unknown path - #782
Open
axelbaumlisto wants to merge 7 commits into
Open
fix(proto): ignore coalesced datagram tail for unknown path#782axelbaumlisto wants to merge 7 commits into
axelbaumlisto wants to merge 7 commits into
Conversation
`handle_coalesced` accounted for the datagram via `path_data_mut(path_id)`, which
panics with `expect("known path")` when the path is not in `paths`. Handling the
first packet of a datagram can remove the path, so the coalesced remainder must
not assume it still exists.
Discard the remainder instead, matching the existing unknown-path handling in
`process_decrypted_packet`.
axelbaumlisto
force-pushed
the
clipshot/pr-known-path-guard
branch
from
August 2, 2026 08:13
97a4925 to
659ef7a
Compare
added 2 commits
August 2, 2026 15:20
`stale_coalesced_datagram_after_path_discard_is_ignored` inlined the whole connect-and-capture sequence. Move it behind `connect_capturing_coalesced_datagram` and `CoalescedDatagram::replay` so the setup can be shared.
Exercises a coalesced datagram whose path id is in neither `paths` nor `abandoned_paths`, which `early_discard_packet` does not cover. The existing `stale_coalesced_datagram_after_path_discard_is_ignored` only covers the abandoned case.
axelbaumlisto
force-pushed
the
clipshot/pr-known-path-guard
branch
from
August 2, 2026 08:21
659ef7a to
a82ab6e
Compare
divagant-martian
requested changes
Aug 4, 2026
divagant-martian
left a comment
divagant-martian
left a comment
Collaborator
There was a problem hiding this comment.
The direction is good
added 4 commits
August 5, 2026 03:30
…l explicit Three review comments, all correct. Verified each against the code before changing anything. 1. The comment's mechanism does not exist. It claimed handling the first packet may have removed the path. `paths.remove()` appears once, in `discard_path`, whose only caller is `handle_timeout` — so a packet cannot remove a path mid-datagram, and as the reviewer notes, a synchronous state machine cannot get there by a data race either. The real reason an unknown id arrives here: `early_discard_packet`'s handshake guard needs `is_handshaking()`, and its discarded-path guard needs the id to be in `abandoned_paths`. An id that was never opened is in neither map, so the datagram proceeds; `process_decrypted_packet` drops the first packet through its own unknown-path check while the coalesced remainder assumed that lookup had succeeded. The comment now says that instead. 2. On the handshake/PathId::ZERO point — correct, and worth recording why replaying under another id is still a faithful model rather than a fabricated input: the receiver never reads the path id off the wire. The endpoint resolves it as `connection_ids[dst_cid]`, so attribution is local receiver state, not something a sender chose. Noted in the test. 3. `replay` is gone. `to_connection_event(now, path_id)` returns the event and each test calls `handle_event` itself, so the public API call that panics is visible where it matters. Evidence the new test earns its place: with the guard reverted, `coalesced_datagram_for_never_opened_path_is_ignored` panics at the `expect` while the pre-existing `stale_coalesced_datagram_after_path_discard_is_ignored` still PASSES — the abandoned-path case does not exercise this panic at all. 388 noq-proto tests pass, clippy clean, both touched files rustfmt-clean.
Removes every comment this PR added: the rationale block in `handle_coalesced`, the doc comments on `CoalescedDatagram` / `to_connection_event` / `connect_capturing_coalesced_datagram`, and the explanation block in the new test. One of them had to go regardless of style preference. The test carried an argument that replaying under a different path id is "a faithful model of the receive path" because the endpoint resolves the id from `connection_ids[dst_cid]` rather than the wire. I checked that after the review and it does not hold: a handshake `dst_cid` is registered as `PathId::ZERO` explicitly, so local state says ZERO too. Leaving a claim in the code that I had already conceded in the review thread would have been worse than leaving no comment at all. Code and tests are unchanged. 388 noq-proto tests pass; with the guard reverted, `coalesced_datagram_for_never_opened_path_is_ignored` still panics at the `expect` while `stale_coalesced_datagram_after_path_discard_is_ignored` still passes, so the new test is carrying the coverage on its own.
Clippy's wrong_self_convention denies a to_* method that consumes self when the type is not Copy; CoalescedDatagram is not. The reviewer asked for "to_connection_event (or something similar)", and into_ is the convention-correct spelling of the same idea, so the rename keeps the requested shape. Verified with the exact lint that failed CI (clippy --all-features --all-targets -p noq-proto: clean) and both affected tests still pass.
This PR touched noq-proto/src/connection/state.rs for no reason other than that I
had run plain `cargo fmt` on it. The project's format-check does not use plain
rustfmt defaults — Makefile.toml passes imports_granularity=Crate among others —
so my reflow of the `use crate::{...}` block was a formatting REGRESSION that would
have failed the check_fmt job.
Reverted the file to origin/main. Clippy (all three feature combinations) and the
full test suite are unaffected, confirming the edit was never needed for the fix.
Verified locally with the exact CI commands:
cargo fmt --all --check -- --config unstable_features=true --config 'imports_granularity=Crate,...'
cargo clippy --workspace --exclude fuzz {--all-features,--no-default-features,} --all-targets ... -D warnings
cargo test --workspace --exclude fuzz --all-features
codespell --ignore-words-list=... --skip=CHANGELOG.md,*.lock
All clean; the two tests this PR adds pass.
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
2 participants
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.
Description
Fixes #781.
handle_coalescedaccounted for the datagram withpath_data_mut(path_id), which panics viaexpect("known path")when the path id is not inpaths. Handling the first packet of a datagram can remove the path, so the coalesced remainder must not assume it still exists.The remainder is now discarded, matching the existing unknown-path handling in
process_decrypted_packet.Also adds a test for a path id present in neither
pathsnorabandoned_paths, whichearly_discard_packetdoes not cover — the existingstale_coalesced_datagram_after_path_discard_is_ignoredonly covers the abandoned case. It panics atmod.rs:4163without the fix and passes with it, while the existing test stays green, confirming the two cover different cases.The connect-and-capture setup the new test needs was inlined in the existing one, so it is extracted first (
connect_capturing_coalesced_datagram,CoalescedDatagram::replay) and both tests share it.Breaking Changes
None.
Notes & open questions
Three further
expect("known path")call sites remain (migrate,populate_packet); those look like genuine internal invariants and are untouched here.Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.