mirror of
https://github.com/n0-computer/noq.git
synced 2026-09-11 22:10:32 +00:00
Grookie
1a26a8b064
## Description We saw `open_path_validation_fails_server_side` fail intermittently in [daily CI on FreeBSD](https://github.com/n0-computer/noq/actions/runs/33142246096), and matheus23 asked me to look into it. The test advanced with a single `advance_time()`, which jumps to the earliest timer pending on *either* endpoint, and assumed that jump lands on the client's 8s path-idle deadline for the blackholed path. It usually does. When the client happens to do a routine key update shortly before that deadline — PN phase exhaustion, which depends on the randomly chosen initial packet number — the server arms `KeyDiscard` at 3x PTO, and that fires about 100ms earlier. The jump lands there instead, `drive()` then stops stepping because `Connection::is_idle` discounts the idle timers that remain, and virtual time never reaches the deadline. The path-idle timer stays armed the whole time and the connection behaves correctly; the single jump in the test is the bug. The fix, as suggested by matheus23: a `drive_until_timer` helper that steps to each next wakeup and drives both endpoints until the target timer is no longer armed. `drive()` can't reach an idle timeout on its own — `is_idle` discounts exactly the timers a test would be waiting for — so this needs its own loop. The test now calls it instead of `advance_time()`. ## API Changes None, test changes only. ## Notes & open questions - I'm an agent posting from my own account (`n0-grookie`), so I left the "created by a human" box below unticked. matheus23 asked for the PR after reviewing the commits. - Measured on a seeded sweep over `StdRng::seed_from_u64()` driving the same `ConnPair` setup as the test: 82 of 20000 seeds failed before, 0 of 50000 after. Seed-dependent rather than platform-dependent — the same seeds fail on Linux, which is how I measured it. The sweep is scratch code and not in the commit; it can go in behind an `#[ignore]` if a repeatable sweep is worth having. - `drive_until_timer` panics rather than hangs if the timer is still armed after 1024 steps, and a *cancelled* timer also ends the loop — callers should keep asserting on the event they expect. Both are in the doc comment. - I went through the other `advance_time()` call sites in `src/tests`. They step to the next scheduled timer and assert on whatever arrives (`multipath.rs:2124` expects `Established`, the `mod.rs:4746` pair checks draining delays), so they don't carry this test's assumption that the jump lands on a specific deadline. Left them as they are. - No issue linked: it came out of daily CI and the fix was reviewed out of band. ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [x] Tests if relevant. - [ ] 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. - [x] This PR isn't slop, and is carefully crafted to do have the intented effect. - [x] `cargo make` passes locally. --------- Co-authored-by: n0-grookie <322172879+n0-grookie@users.noreply.github.com>