test(relay): run the outage lease test on the real clock - #4244
Conversation
A paused tokio clock auto-advances to the next timer whenever the runtime waits on a socket. macOS delivers loopback asynchronously, so the test's 10s connect timeout fired mid-handshake: ~9 in 10 runs failed once #4215 dropped the listener probe that happened to mask it. 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 56 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 |
![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: 1973488506
ℹ️ 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".
| const SLACK: Duration = Duration::from_secs(30); | ||
| // Whole seconds, as the grant crosses the wire, so the relay sees this exact instant. | ||
| 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 deadline is only 2–3 seconds away, but it starts before binding the auth server and relay and before completing two sequential client handshakes plus the media round-trip, even though those setup operations individually allow up to 10 seconds. On a sufficiently loaded runner the grant can therefore expire during setup, causing connection failure or exercising the clock-skew path instead of the intended live-session outage behavior; load does not merely delay the observed close as the comment claims.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this. The grant's expires is fixed by the connect answer, and re-checks answer 503, so it can't start after setup. Setup is a few ms of loopback round-trips against a 2-3s budget, and if it ever overran expires the connect would be refused and the test fails loudly at "publisher/subscriber connect failed", never a false pass. Widening the margin would add real seconds to every run for a load level that would already time out other tests.
(Written by Claude Opus 5.5)
|
Merged origin/main (#4240) and re-ran (Written by Claude Opus 5.5) |
1 participant
Problem
an_outage_keeps_the_session_until_expiresfails ~9/10 runs on macOS withpublisher connect timeoutafter ~0.1s. It ran understart_paused = true, which auto-advances the clock to the next timer whenever the runtime has no ready work. The relay accepts the TCP connection, then waits on the kernel for the client's bytes; macOS delivers loopback asynchronously, so the idle poll finds nothing and jumps straight past the 10s timeout. Linux loopback completes inside the sender's syscall, so Linux CI passes (checked runs 36213200307 and 36212266674, both with #4215).#4215 exposed it by removing
wait_for_listener: restoring that probe makes it pass 10/10 again, but only by luck, not by removing the race.Approach
The test spans real sockets (auth HTTP, TCP sessions), so a virtual clock can't bound it. Run it on the real clock with a whole-second
expires~2-3s out (the wire carries whole seconds, so the relay sees that exact instant), and assert the close is observed no earlier thanexpires. Load only delays the close, so the assertion holds on a busy machine; the old 100ms "still open" sample is gone.Verified on macOS (M4): 10/10 passes (~2.1s each); the whole
auth_lifetimefile 20/20 under--stress-countwith every core pinned byyes; forcing the relay deadline to 1s fails with "an outage must not close the publisher before expires".Impact
Alternatives
wait_for_listener: masks the race; any later socket wait under the paused clock can still fire a virtual timeout.endreports still travel over sockets whileassert_closed's timeout is pending.Follow-ups
start_pausedtests near sockets (moq-relaycluster.rs,internal.rs,websocket.rs;moq-tokiomdns.rs,resolve.rs) weren't audited for the same race.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)