fix(router): repair main build breaks and deflake client timeout test - #2238
Conversation
Two concurrently merged changes raced: the owned-body dispatch change retyped the request body as Bytes while another branch still wrapped it in Bytes::from and matched an unboxed selection failure. The merged result fails cargo test compilation (E0308 on the boxed PdSelectionFailure) and clippy (useless_conversion behind an unused_qualifications shadow), breaking every PR merge build. Pass the already-Bytes body through directly and deref the boxed failure in the empty-fleet test. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
per_request_timeout_overrides_client_default raced a zero-duration total timeout against a loopback echo response; the fast 200 wins often enough to fail roughly half of local runs. Point the client-default leg at a route that sleeps far past any deadline so the timeout always fires, keeping the per-request override leg on the instant route. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe HTTP routers now forward existing ChangesHTTP request handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Clean post-merge fixup. All three changes are correct: redundant Bytes::from() removal matches the already-Bytes parameter types, the box deref aligns with the boxed PdSelectionFailure return type, and the hanging-route deflake soundly eliminates the timer race. No issues found.
Description
Problem
maincurrently failscargo testcompilation andcargo clippy -D warnings, which breaks the unit-tests lane on every PR merge build. Two concurrently merged changes raced: the owned-body dispatch refactor retyped the request body asBytesand boxedPdSelectionFailure, while another branch merged code still wrapping the body inBytes::from(...)and matching the selection failure unboxed. Each PR was green alone; their merge is not.Separately,
worker::http_client::tests::per_request_timeout_overrides_client_defaultis flaky: it races a zero-duration total timeout against a loopback echo response, and the fast local 200 wins often enough to fail roughly half of runs (reproduced locally at that rate; it also failed a PR lane today).Solution
Bytesbody through directly at both dispatch sites and deref the boxed failure in the empty-fleet test — restores compile and clippy cleanliness.Changes
model_gateway/src/routers/http/pd_router.rs: drop redundantBytes::from, deref boxedPdSelectionFailureintest_empty_worker_listsmodel_gateway/src/routers/http/router.rs: drop redundantBytes::frommodel_gateway/src/worker/http_client.rs: add a hanging/hangroute to the test echo server; aim the client-default timeout assertion at itTest Plan
cargo clippy -p smg --all-targets -- -D warningspasses (was 4 errors)cargo test -p smg --libpasses: 1789 passed, 0 failed (compilation was broken before)per_request_timeout_overrides_client_defaultpasses 10/10 consecutive runs (previously ~half failed)cargo test -p smg --test inflight_tracker_test --test routing_testspasses (132 tests, closest integration coverage to the dispatch sites)cargo +nightly fmtcleanChecklist
cargo +nightly fmtpassescargo clippy --all-targets -- -D warningspasses for the touched crate