Repository navigation
fix(router): answer 503 on the gRPC path when a model's workers are all unavailable - #2465
Conversation
…ll unavailable The gRPC selection stage answered 404 model_not_found whenever no worker could take a request, whether the model was unknown or its workers were merely unhealthy or circuit-broken. The HTTP router already separates the two: an unknown model is a 404, workers that exist but cannot serve are a 503 no_available_workers. A client of the gRPC path was told the model did not exist while its workers restarted, and for a disaggregated model the same happened whenever one leg lost its last worker. Both the per-leg fallback and the pair verdict now map Unavailable and PolicyDeclined to the 503 with the HTTP router's code and keep 404 for a model nobody serves. Unit tests pin the regular and the decode-leg cases. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
github-actions
Bot
added
grpc
The first CI run of the worker-restart test caught the gateway answering 404 model_not_found while the only worker was restarting, on SGLang and on vLLM. That answer is fixed in #2465; the test now asserts the 503 with no_available_workers instead of any 5xx. The queue-depth helper raises instead of calling pytest.fail so mypy sees the missing return without pytest stubs, which is how the lint job runs it. 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: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughWorker selection now distinguishes unavailable or policy-declined workers from absent models. Regular, disaggregated, and paired selections return ChangesWorker selection failure classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Worker-selection failures now distinguish unavailable workers from unknown models, returning 503 for temporary worker unavailability and retaining 404 for absent models. Covered regular and disaggregated failure paths have no identified merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant WorkerSelection
participant PlacementFailure
participant gRPCResponse
WorkerSelection->>PlacementFailure: evaluate candidate placement
PlacementFailure-->>WorkerSelection: return failure verdict
WorkerSelection->>gRPCResponse: return 503 for unavailable workers
WorkerSelection->>gRPCResponse: return 404 for absent models
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| match verdict { | ||
| PlacementFailure::AllOverloaded(shed) => return shed, | ||
| PlacementFailure::Unavailable | PlacementFailure::PolicyDeclined(_) => { | ||
| unavailable = true; | ||
| } | ||
| PlacementFailure::NoCandidates => {} | ||
| } | ||
| } | ||
| if unavailable { | ||
| return self.workers_unavailable(model_id); | ||
| } |
There was a problem hiding this comment.
🔴 Important: A healthy leg reports Unavailable, so the 404 this PR intends to keep for "nobody serves this leg" is unreachable whenever any other leg has workers.
placement::failure_from is a three-way classifier that only knows empty / all-overloaded / everything else — it returns Unavailable for any non-empty pool that isn't fully vetoed, including a pool whose workers are all Ready and available. That is sound in the single-leg callers (single_failure runs only after selection over that exact pool failed), but here the loop asks the question of every demanded leg, including legs that were perfectly fine.
Concrete failure: EPD mode, a model with healthy prefill+decode workers and zero encode workers registered, request carries encode items. select_encode_prefill_decode_workers returns None and legs = [Prefill, Decode, Encode]:
Prefill→ pool non-empty, workers available →Unavailable→unavailable = trueDecode→ sameEncode→NoCandidates
Result: 503 no_available_workers / "All workers for model 'X' are unavailable (unhealthy or circuit breaker open)" — for a permanent deployment gap, in which no worker is unhealthy and no breaker is open. Before this PR that was a 404. The 503 is retryable, so clients and proxies now retry a misconfiguration forever instead of failing fast. The same happens for the mixed-runtime EPD case (no shared runtime across legs) with legs = [Prefill, Decode].
The verdict for a leg is only meaningful if that leg is the one that had nothing to give. Consider deriving unavailable from the availability of each leg's own pool, e.g. treat a leg as blocking only when it has candidates but none passes is_available():
PlacementFailure::Unavailable | PlacementFailure::PolicyDeclined(_) => {
// `failure_from` cannot distinguish "this leg is drained" from
// "this leg was fine and another leg is why we are here", so ask
// the pool directly.
unavailable |= !leg_has_available_worker(...);
}or, since the shared-runtime/encode-leg cause is already known at the call site, pass the failing leg in rather than re-deriving it from all of them.
| WorkerSelectionMode::PrefillDecode, | ||
| ); | ||
|
|
||
| let fallback = | ||
| stage.selection_failure(model_id, &[WorkerType::Prefill, WorkerType::Decode], None); | ||
| let pair = stage.pair_failure( |
There was a problem hiding this comment.
🟡 Nit: an_unavailable_decode_leg_answers_503_not_404 passes without exercising the decode leg at all — the fallback half of it would be green even if decode.set_status(WorkerStatus::NotReady) were deleted.
register_pd_workers builds Ready workers with health checks disabled, so when selection_failure walks [Prefill, Decode] the prefill leg is the one that sets unavailable = true: its pool is non-empty and not all-overloaded, so failure_from says Unavailable even though that worker is fine (see the comment on selection_failure). The NotReady decode worker never influences the outcome.
The pair and absent halves do test real mappings, since they hand pair_failure a verdict directly. To make the fallback half meaningful, assert the negative too — with every leg healthy, selection_failure should not answer 503:
// Sanity: the 503 must come from the downed leg, not from a healthy
// leg that `failure_from` cannot distinguish from a drained one.
let healthy = WorkerRegistry::new();
register_pd_workers(&healthy, model_id, 1);
let healthy_stage = WorkerSelectionStage::new(
Arc::new(healthy),
Arc::new(PolicyRegistry::new(PolicyConfig::RoundRobin)),
WorkerSelectionMode::PrefillDecode,
);
assert_ne!(
healthy_stage
.selection_failure(model_id, &[WorkerType::Prefill, WorkerType::Decode], None)
.status(),
StatusCode::SERVICE_UNAVAILABLE
);That assertion fails today, which is exactly the bug in the loop.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| fn workers_unavailable(&self, model_id: &str) -> Response { | ||
| error!( | ||
| function = "WorkerSelectionStage::execute", | ||
| mode = ?self.mode, | ||
| model_id = %model_id, | ||
| "No available workers for model" | ||
| ); | ||
| error::service_unavailable( | ||
| "no_available_workers", | ||
| format!("All workers for model '{model_id}' are unavailable (unhealthy or circuit breaker open)"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Nit: On the reselect path this 503 is retryable, so it converts a fail-fast into max_retries reselect attempts with backoff — worth an explicit decision rather than an accident of the status code.
reselect reaches selection_failure from inside the attempt loop (pipeline.rs:457), and the loop gates on is_retryable_response(&failure) (pipeline.rs:610). A 404 was terminal there; this 503 is not marked non-retryable, so a request whose workers are down now burns every remaining attempt: each one calls record_error and the retry counters, so one client request inflates the error/retry metrics N-fold and pays the full backoff before returning the same 503.
overload.rs faced exactly this and chose mark_non_retryable, with the reasoning that "the veto clears at the poll interval, which no backoff window outlives". Health and circuit-breaker state can flip faster than the overload flag, so retrying here is arguably right — but if that's the intent, please say so in the doc comment (it currently says "the client should retry", which reads as advice to the client, not a statement about the internal retry layer) and consider a test pinning the choice, mirroring "a shed must be terminal for the retry layer" at line 924.
Separately, the doc on selection_failure (lines 321-322) is now stale — "a 503 shed when a leg's whole candidate pool is vetoed, the existing 404 otherwise" no longer describes the three outcomes this function has.
| PlacementFailure::Unavailable | PlacementFailure::PolicyDeclined(_) => { | ||
| self.workers_unavailable(model_id) | ||
| } |
There was a problem hiding this comment.
🟡 Nit: Two of the three verdicts routed here produce a factually wrong message.
workers_unavailable says "All workers for model 'X' are unavailable (unhealthy or circuit breaker open)", but:
PolicyDeclined(policy)is raised byselect_pairafter both legs were filtered for availability (placement.rs: "Both legs were filtered for availability above, so a miss here is the policy's own decision, never an overloaded pool"). Every worker is healthy and no breaker is open — the policy just picked none. The HTTP router distinguishes these (router.rs:829-833picks "Policy returned no eligible worker" whennon_dp_workers.iter().any(|w| w.is_available())); this path does not, so the claimed parity is only partial.- The homogeneous-runtime narrowing in
select_pairhardcodesPlacementFailure::Unavailablefor "No available PD pair for runtime {runtime}". A PD model deployed with an SGLang prefill and a vLLM decode is a permanent configuration error with every worker healthy, and it now reports a transient health failure instead of the previous 404.
Since pair_failure has failure.leg in hand, it could pick the message the way the HTTP router does, or workers_unavailable could take the message so PolicyDeclined names the policy.
The first CI run of the worker-restart test caught the gateway answering 404 model_not_found while the only worker was restarting, on SGLang and on vLLM. That answer is fixed in #2465; the test now asserts the 503 with no_available_workers instead of any 5xx. The queue-depth helper raises instead of calling pytest.fail so mypy sees the missing return without pytest stubs, which is how the lint job runs it. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
The first CI run of the worker-restart test caught the gateway answering 404 model_not_found while the only worker was restarting, on SGLang and on vLLM. That answer is fixed in #2465; the test now asserts the 503 with no_available_workers instead of any 5xx. The queue-depth helper raises instead of calling pytest.fail so mypy sees the missing return without pytest stubs, which is how the lint job runs it. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…ow bursts, prefill aborts, batched choices The nightly sweep covered one transport per engine and left the two failure modes we just fixed without a regression test. It now also runs: - SGLang's HTTP PD router at 1p1d and 2p2d (its own pairing, KV handoff and error answers); the other engines skip those rows - vLLM over Mooncake next to its NIXL default, as the PR lane does - a decode pinned to a four-request window against a burst six times wider: every request is served or shed with the overload code and a Retry-After, the burst finishes in bounded time, and no engine leg logs a bootstrap timeout (the gateway's admission gate at work) - clients that drop the connection before the first token, which must leave both legs idle like an abandoned stream does - a completion with four choices, which fans out one room per choice The sole-leg outage now asserts the 503 the gateway answers since #2465. The PR lane keeps one gRPC topology plus the runtime-assembled fleet and adds the over-window burst; HTTP PD runs nightly only. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
2 participants
Description
Problem
The gRPC selection stage answered 404
model_not_foundwhenever no worker could take a request, no matter why. An unknown model and a model whose only worker is restarting produced the same answer. The HTTP router already distinguishes the two (routers/http/router.rs):NoCandidatesis a 404,Unavailable/PolicyDeclinedis a 503no_available_workers.For a client of the gRPC path this reads as "the model does not exist" during any worker outage, and for a disaggregated model it happens whenever one leg loses its last worker. It surfaced while writing the PD topology suite (#2464), whose sole-leg outage test had to accept both answers, and it is what the worker-restart test on #2462 hit.
Solution
selection_failurenow collects each demanded leg's verdict: a shed is returned as built (unchanged),Unavailable/PolicyDeclinedon any leg yields the 503, and only when every leg has no candidates at all does the 404 remain.pair_failuremaps the same way for the pair verdict.disaggregated_leg_shedbecomesdisaggregated_leg_verdictand returns the fullPlacementFailureviaplacement::failure_from, so the per-leg fallback can tell "nobody serves this leg" from "the leg's workers are down".All workers for model '<id>' are unavailable (unhealthy or circuit breaker open), the HTTP router's codeno_available_workers.Changes
model_gateway/src/routers/grpc/common/stages/worker_selection.rs: mapping above; two unit tests (an_unavailable_regular_worker_answers_503_not_404,an_unavailable_decode_leg_answers_503_not_404); the existing 404 tests for absent models still pass.Test Plan
cargo +nightly fmt --all,cargo clippy -p smg --all-targets -- -D warnings: clean.cargo test -p smg --lib worker_selection: 19 passed (17 before plus the two new ones);cargo test -p smg --lib placement: passes.api_testsassertions on 404 concern unknown models, deleted responses and unknown endpoints (NoCandidatespaths) and are unaffected.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses (-p smg --all-targets; the all-features build needs OpenCV locally)