Repository navigation
test(router): hang guards instead of 5 s budgets around the PD router's round trips - #2660
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughPD router tests define a shared 60-second timeout guard. Count-tokens, streaming, and prefill-drain tests use the guard instead of 5-second timeouts. Router runtime logic is unchanged. ChangesPD router test timeouts
Estimated code review effort: 1 (Trivial) | ~4 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// Ceiling on a real HTTP round trip in these tests: a guard against a | ||
| /// hang, not a budget, since a loaded machine can take seconds. | ||
| const ROUND_TRIP_GUARD: std::time::Duration = std::time::Duration::from_secs(60); |
There was a problem hiding this comment.
🟡 Nit: The const is declared above the module's use statements, which is unconventional item ordering for Rust (imports first, then items). Moving it below the use block at line 2287 would match the usual layout — purely cosmetic, no functional difference.
There was a problem hiding this comment.
Fixed in 4b9b9ac (also rebased onto main): the const sits below the module's use block now.
There was a problem hiding this comment.
Reviewed the full diff: a test-only change replacing five 5-second timeouts with a shared 60-second ROUND_TRIP_GUARD in the PD router tests. All five sites are converted, the guard semantics preserve what the tests actually verify (the prefill stub blocks until explicitly released, so a real regression still trips the guard rather than passing slowly), and no timeout sites in the module were missed. One 🟡 nit posted (item ordering); no blocking issues.
…'s round trips check_prefill_drain and messages_count_tokens_tracks_load_auth_and_body_failure wrap real HTTP round trips in 5 s timeouts and time out under parallel load (4/4 on a busy machine, passing in isolation). The timeouts are a 60 s guard against a hang now, one constant for the module, rather than a budget the tests were never about. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
478cf1c to
4b9b9ac
Compare
2 participants
Description
Problem
From the reviews of #2638 and #2639:
check_prefill_drain(four timeouts) andmessages_count_tokens_tracks_load_auth_and_body_failure(one) wrap real HTTP round trips through stub servers in 5 stokio::time::timeouts, and fail under parallel load (reproduced 4/4 on a busy machine, passing in isolation). The tests assert on outcomes, not on latency; the timeouts only exist so a hang cannot wedge the suite.Solution
One
ROUND_TRIP_GUARDof 60 s for the tests module, used by all five sites. Test-only change.Test Plan
cargo test -p smg --lib -- pd_router: 27 passed.cargo +nightly fmt --all -- --checkclean;cargo clippy -p smg --all-targets -- -D warningsclean (without--all-features, no OpenCV here).Checklist
cargo +nightly fmtpassescargo clippy --all-targets -- -D warningspasses (without--all-features, see above)