test(auth): run the outage grant test on the real clock - #4291
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 30 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
MERGE Positive improvement: yes. The old test mixed a paused tokio clock with a real loopback server, so idle waits on the socket let the clock jump past the mid-lease sleep and miss the re-check assertion—especially on macOS, and reproducible on Linux with a short server-side delay. Moving this onto the wall clock matches the race that actually happens in production-shaped I/O. Worth the complexity: yes. The change is small (one test, ~3s wall time), drops Different approach: no. Waiting on the log under a paused clock was already tried and is unsound here—the virtual expiry timer can outrun real loopback and drop the in-flight re-check. Keeping the paused clock would just reintroduce the race. Aligning with the merged pattern from #4244 ( This is an automated review, not the maintainer's decision |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85f5cc595b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let now = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap(); | ||
| let expires = SystemTime::UNIX_EPOCH + Duration::from_secs(now.as_secs() + 3); |
There was a problem hiding this comment.
This makes an explicitly time-dependent test wait 2 to 3 real seconds and depend on the adjustable system clock. Every run now slows the suite, a backward clock adjustment can fail the post-close comparison, and a setup stall beyond the expiry plus five-second skew makes connect(...).unwrap() fail before the behavior is tested. Please remove the socket/virtual-clock coupling at its source, such as by isolating the driver's HTTP transport, rather than moving this test to wall-clock time; the scoped testing convention requires time-dependent async tests to pause Tokio time.
AGENTS.md reference: rs/AGENTS.md:L51-L54
Useful? React with 👍 / 👎.
1 participant
Problem
an_outage_keeps_the_grant_until_expires(rs/moq-auth) ran on a paused tokio clock against a real loopback server. When the runtime idles while the 1s re-check is still on the socket, tokio auto-advances the clock past thesleep(1500ms)andlog.revalidates() >= 1fails. Linux delivers loopback inside the send syscall so CI rarely hits it; macOS does.Reproduced on Linux by delaying the server's revalidate handling 20ms on an OS thread: fails every run.
Approach
Run the test on the real clock, the same way #4244 fixed
an_outage_keeps_the_session_until_expires: expire at a whole second about 3s out, answer re-checks with 503, wait for the close, and assert lower bounds (closed asExpired, not beforeexpires, at least one re-check seen). It now uses the wiremockserverhelper since nothing needs the paused clock. Adds ~3s of wall time.Waiting on the log instead of the fixed sleep was tried first and is not sound: the clock then jumps to the 3s expiry timer, the driver drops the in-flight re-check, and the server never logs it (the repro hangs). Any virtual timer can outrun real loopback I/O here.
With a 200ms server-side delay the new test still passes.
Impact
Alternatives
log.until(...): hangs under the repro, see above.Follow-ups
An audit of every other paused-clock test that uses real sockets found no other instance of this race. Two are safe today but would break if a connect or handshake timeout were added on their paths:
fixed_addresses_keep_tls_name_and_request_host(rs/moq-tokio/src/websocket.rs:709)a_grant_within_clock_skew_stays_live(rs/moq-auth/src/client.rs:496)(Written by Claude Opus 5.5)
🤖 Generated with Claude Code