Skip to content

fix(pd): name an unavailable PD leg with the same 503 code as the other routers #2479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
smg-project-bot merged 1 commit into from
Sep 9, 2026
Merged

fix(pd): name an unavailable PD leg with the same 503 code as the other routers #2479

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion model_gateway/src/routers/http/pd_router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,12 @@ impl PDRouter {
PdSelectionFailure::Shed(shed) => shed,
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

@claude claude Bot Sep 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

// retry on one code whatever the transport; the message
// still names the leg.
error::service_unavailable(
"server_selection_failed",
"no_available_workers",

@claude claude Bot Sep 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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 / unhealthy
  • PlacementFailure::PolicyDeclined
  • PlacementFailure::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.

format!("No available servers: {error}"),
)
}
Expand Down
Loading