test(proto): fix flaky open_path_validation_fails_server_side

[n0-grookie]

Description

We saw open_path_validation_fails_server_side fail intermittently in daily CI on FreeBSD, 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

• Self-review.
• Documentation updates following the style guide, if relevant.
• 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.
• This PR isn't slop, and is carefully crafted to do have the
  intented effect.
• cargo make passes locally.

[flub]

I kind of like the drive_until_timer helper.

I wonder if it is better to make the tests more deterministic by default? I think this would also be fixed by disabling the keyupdate when building the endpoints for the tests. I'm very tempted to make the ConnPair endpoints deterministic by default because I think that is likely to more systematically fix this bug also in new tests. And then for cases where we need this we could have ConnPair::builder().with_keyupdate() or something.

I say this because I basically think of these tests as deterministic when writing them. I realise that means we lose some test coverage. And we need to make sure that weird interactions still get picked up.

[flub]

I don't understand why there are now two drives right after each other?

[n0-grookie]

They aren't the same work — measured rather than guessed:

The PathIdle timer fires while the loop is driving the client, and the abandonment still needs steps to reach the server: drive_until_timer returns with 3 steps of traffic still queued (while pair.step() {} immediately after it, on several seeds). So the trailing drive() is what puts the abandonment in front of the server. Without it poll(Server) is None because the server never heard about the abandonment, not because it stayed quiet — the assertion would pass vacuously.

The drive() above drive_until_timer is load-bearing in the other direction. Each step advances the clock before driving, so without it the keep-alive ping goes out at the deadline instead of before it. Seed 0, path 0 frame_tx.ping: with the drive the loop takes one step, to 8006ms, ping already sent; without it the first step lands at 8005ms with the ping still unsent, and it goes out on the next step at 8006ms.

Both variants pass 4000 seeds, which is why I left a comment at each site instead of deleting one. Comments are in 6c0d7ea.

[matheus23]

I agree about disabling key updates for these by default. We test key updates in proptest IIRC.
These tests aren't about testing key updates in the middle, so they shouldn't do that and rather be as deterministic as possible.

I don't think only disabling key updating is right. I think the helper better conveys the test's intent.
Just advance_time doesn't really tell us why we're doing so. Adding the timer we're looking for makes it more readable IMO.

[flub]

I don't think only disabling key updating is right. I think the helper better conveys the test's intent.
Just advance_time doesn't really tell us why we're doing so. Adding the timer we're looking for makes it more readable IMO.

Sure, I did say I kind of like it. Can we do both? Also make ConnPair have the keyupdates disabled by default?

[matheus23]

I'm not convinced we should do both in this PR though. Disabling key update would be a neat, scoped PR on its own, likely with some changes to existing tests that are unrelated to this particular flaky test fix.

[flub]

Sure, if you prefer splitting this up. I would prefer to include it but that's no big deal. But please do the followup.