Repository navigation
fix(pd): name an unavailable PD leg with the same 503 code as the other routers - #2479
Conversation
…er routers The HTTP PD router answered a request with no healthy prefill or decode worker with 503 server_selection_failed, while the regular HTTP router and the gRPC routers answer the same condition with 503 no_available_workers. A client keying its retry on the code had to know the transport. Use no_available_workers here too; the message still names the missing leg. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
|
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: 5 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
WalkthroughThe PD router now uses ChangesPD error classification
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to Unavailable PD legs now return the standardized 503 no_available_workers error while retaining the existing decode/prefill detail. No actionable current-head merge risk remains. 🚥 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}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // still names the leg. | ||
| error::service_unavailable( | ||
| "server_selection_failed", | ||
| "no_available_workers", |
There was a problem hiding this comment.
🔴 Important: This path carries more than "a leg that is merely down", so unifying on no_available_workers here creates a new false-retry signal for the model-not-found case.
pair_failure (line 1368) folds three verdicts into PdSelectionFailure::Unavailable:
PlacementFailure::Unavailable— all circuits open / unhealthyPlacementFailure::PolicyDeclinedPlacementFailure::NoCandidates— nobody serves this leg at all
The two paths this PR is aligning with both split NoCandidates out and answer it terminally:
router.rs:518→PlacementFailure::NoCandidates => error::model_not_found(model_id)grpc/common/stages/worker_selection.rs:406→PlacementFailure::NoCandidates => error::model_not_found(model_id)("a leg nobody serves is a 404")
select_pd_pair reaches NoCandidates for an unknown model: model_routing_snapshot(model_id) returns None, both leg pools come back empty, and placement reports NoCandidates. So after this change, POST /v1/chat/completions for a model no PD worker serves answers 503 no_available_workers on HTTP PD, while the same request answers 404 model_not_found on regular HTTP and on gRPC PD. A client that keys its retry on no_available_workers — exactly the client this PR is written for — now retries forever against HTTP PD on a permanently nonexistent model, where before it saw the distinct server_selection_failed and stopped.
Splitting the verdict keeps the unification and closes the collision:
fn pair_failure(failure: PairFailure) -> PdSelectionFailure {
// ...
match failure.verdict {
PlacementFailure::AllOverloaded(shed) => PdSelectionFailure::Shed(shed),
PlacementFailure::NoCandidates => PdSelectionFailure::NotFound(/* leg */),
// ...
}
}with handle_server_selection_error answering NotFound as error::model_not_found. If turning the empty/unconfigured-fleet case into a 404 is more behavior change than you want here (the current message reads "Please check if prefill servers are configured and healthy", which is operator-facing rather than client-facing), the minimum is to leave NoCandidates on a code distinct from no_available_workers so the retry key stays meaningful. Either way, the added comment overstates what reaches this arm.
| PdSelectionFailure::Unavailable(error) => { | ||
| error!("Failed to select PD pair error={}", error); | ||
| // Same code the regular HTTP router and the gRPC routers use | ||
| // for a leg that is merely down, so a client can key its |
There was a problem hiding this comment.
🟡 Nit: No test pins the new code, so nothing in-repo stops it drifting back.
no_available_workers now appears in pd_router.rs only at line 217 — production code, no assertion. The two paths this PR aligns with each lock their code in with a header assertion (router.rs:2358, worker_selection.rs:1266 and :1313), and test_empty_worker_lists at line 2357 only matches on the PdSelectionFailure variant and its message string, never on the rendered response.
Since the point of the change is a client-visible contract rather than an internal refactor, a test that drives handle_server_selection_error and asserts HEADER_X_SMG_ERROR_CODE == "no_available_workers" (plus the 503) would be worth adding — it's also what would have caught the NoCandidates overlap flagged below.
2 participants
Description
Problem
With a PD pair's only decode worker down, the HTTP PD router answers
503 {"code": "server_selection_failed", "message": "No available servers: No available decode workers"}, while the regular HTTP router and the gRPC routers answer the same condition with503 no_available_workers(#2465 unified the gRPC side). A client that keys its retry on the error code has to know which transport it is talking to. Found by the PD topology sweep in #2464 (test_sole_leg_outage_is_reported_promptly[1p1d-http]).Solution
Use
no_available_workersin the HTTP PD router's selection-failure path as well. The status stays 503 and the message still names the missing leg. Overload sheds keep their own code.Changes
model_gateway/src/routers/http/pd_router.rs:handle_server_selection_erroremitsno_available_workersinstead ofserver_selection_failedforPdSelectionFailure::Unavailable.Test Plan
cargo +nightly fmt --all,cargo clippy -p smg --all-targets -- -D warnings,cargo test -p smg --lib pd_routerpass locally.server_selection_failed.Before (HTTP PD, sole decode down):
503 server_selection_failed. After:503 no_available_workers, same message.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses