test(auth): wait on the recorded request instead of a fixed sleep - #4194
Conversation
The client tests slept 50ms for the background end POST to reach the mock server, which lost the race on a loaded CI runner. The request log is now a kio::Shared, so tests await the request itself. Drops a_nudge_during_backoff_posts_at_once: backoff leaves the driver in the same state as idle, and its 2s timeout outlasted the ~1s backoff, so it could not fail. 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 5 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: 4ff2cec844
ℹ️ 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".
| for _ in 0..2 { | ||
| tokio::time::timeout(Duration::from_secs(3), consumer.changed()) | ||
| .await | ||
| .expect("the in-flight nudge POSTs once more when the reply lands") | ||
| .unwrap(); |
There was a problem hiding this comment.
On a loaded runner that does not reschedule this test within the mock's 400 ms delay, both revalidation replies can update the lease before the first consumer.changed() resumes. Consumer::poll_changed then advances seen directly to the latest epoch, so the two updates coalesce into one observation and the second loop iteration times out even though exactly two POSTs occurred. Use a non-coalescing reply counter or barrier to wait for the two distinct completions rather than introducing another scheduling-dependent test failure.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The test now waits until the request log has two revalidate POSTs, then still asserts the count stays at two after the session ends.
(Written by Grok 4.7)
changed() jumps to the latest grant epoch, so two replies can land as one observation and the inflight nudge test times out even when both POSTs happened. Wait on the request log, which records each one. Co-authored-by: Grok 4.7 <noreply@x.ai>
|
Landing this on main. The auth client tests wait on the recorded request instead of a fixed sleep. The inflight nudge test counts revalidate POSTs on that log, because (Written by Grok 4.7) |
1 participant
Problem
The
moq-authclient tests usedsettle()(a fixed 50ms sleep) to wait for the backgroundendPOST to reach the mock server. On a loaded runner the POST lands later, soclient::tests::a_bare_drop_ends_as_droppedfailed withexpected a dropped end, got Connect(seen in #4190's Test job).Approach
Logis now akio::Shared<Vec<Request>>.Log::untilawaits a predicate over the recorded requests, andLog::endwaits for theendPOST and returns it.settle()is gone.Log::untiltoo.a_nudge_during_inflight_...used to check for "no third POST" by sleeping. It now waits for both re-check replies to land, closes, waits forend, and asserts exactly two re-checks.a_nudge_during_backoff_posts_at_once. Its sleep was waiting for a 503 to land, which the test can't observe. Backoff also leaves the driver in the same state as idle (nothing in flight, timer armed), whicha_nudge_while_idle_posts_at_oncealready covers. And its 2s timeout was longer than the ~1s first backoff, so it would have passed even if nudges were ignored.Stress run: 20 iterations of
client::testswith every core pinned, all passing. The old code also passed 20/20 locally, so this box couldn't reproduce the CI flake. The fix removes the timing assumption rather than depending on one.Impact
Alternatives
Follow-ups
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code