Skip to content

[Router] Pin the backpressure disposition at the dispatch sites - #40218

Draft
Kangyan-Zhou wants to merge 1 commit into
sgl-project:mainfrom
Kangyan-Zhou:router-4d-4-backpressure-tests
Draft

Kangyan-Zhou wants to merge 1 commit into
sgl-project:mainfrom
Kangyan-Zhou:router-4d-4-backpressure-tests

Conversation

@Kangyan-Zhou

@Kangyan-Zhou Kangyan-Zhou commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

Follow-up to #39464 (merged as 4e0b56c8), which taught the circuit breaker
to treat an upstream 503/429 as backpressure rather than a fault.

That PR's end-to-end tests asserted only that a 503 leaves the breaker Closed.
record_success satisfies that identically — so the dispatch-site wiring was
not actually pinned. Verified by mutation on the merged commit: replacing
BreakerOutcome::Neutral => breaker.record_backpressure() with
record_success() in both dispatch arms passed the entire suite, all 918
tests, undetected.

The property that makes Neutral a third verb rather than an alias for success
is that it does not reset an in-progress failure streak — "a worker
interleaving genuine 5xx faults with 503s still trips". That was covered on the
breaker alone (backpressure_in_closed_state_preserves_failure_streak), never
through forward_json_to / forward_streaming_to.

Modifications

Tests only — no behaviour change.

A scripted mock worker. spawn_scripted_worker serves a sequence of
statuses, repeating the last. A fixed-status worker structurally cannot express
the interleaving that separates the two verbs.

Three tests that fail if a dispatch site records backpressure as success.
Each drives fault, fault, backpressure, fault against an explicit
threshold of 3 and asserts the final fault still opens the breaker:

test arm
engine_503_does_not_reset_an_in_progress_failure_streak JSON
engine_429_does_not_reset_an_in_progress_failure_streak JSON — 429 shares the Neutral disposition but only 503 had ever been exercised end to end
engine_503_does_not_reset_an_in_progress_failure_streak_streaming streaming, which records on a separate code path (up front, skipping the pump hook)

The threshold is pinned via STREAK_THRESHOLD rather than inherited from
CircuitBreakerConfig::default(), because these assert on an exact streak
length — a change to the default must not silently move the trip point.

Two documented-but-untested states.

  • backpressure_while_open_does_not_reset_the_breaker — record_backpressure's
    Open arm is justified at length in its docstring and had no coverage. Asserts
    a late 503 neither untrips the breaker nor restarts its cool-down.
  • engine_503_recovers_a_half_open_breaker_streaming — the JSON arm had a
    half-open wedge guard; the streaming arm did not, and the probe can just as
    easily land there.

Also fixes the unresolved [`record_success`] intra-doc link in the same
docstring (Self::-qualified, matching its siblings).

Tests

cargo test green — 730 lib, 7 bin, 65 component, 121 proxy.
cargo clippy --all-targets -- -D warnings clean; pre-commit clean.

Mutation-checked in both directions: green as committed, and with
record_backpressure → record_success re-applied at both dispatch sites
exactly the three streak tests fail.

Checklist

🤖 Generated with Claude Code

https://claude.ai/code/session_01Wt25p6zfdw6QFLguPRsj82


CI States

Latest PR Test (Base): ✅ Run #35379854356
Latest PR Test (Extra): ❌ Run #35379854013
Latest PR Test (AMD ROCm 10): ➖ No AMD PR run found for this commit.

The end-to-end tests asserted only that a 503 leaves the breaker Closed,
which `record_success` satisfies identically — so swapping
`record_backpressure` for `record_success` in both dispatch arms passed the
entire suite. The property that makes Neutral a third verb rather than an
alias for success is that it does NOT reset an in-progress failure streak,
and that was covered on the breaker alone, never through dispatch.

Drive the streak across a backpressure answer instead: a scripted worker
serves fault, fault, backpressure, fault against an explicit threshold of 3,
and the final fault must still open the breaker. Added for the JSON arm, for
429 as well as 503 (the streaming/JSON arms act on `breaker_outcome`, but
only 503 had ever been exercised end to end), and for the streaming arm,
which records on a separate code path. Each of the three fails if the
dispatch site records backpressure as success.

Also cover two states that were documented but untested: backpressure
arriving while the breaker is already Open must neither untrip it nor
restart its cool-down, and the streaming arm must resolve a half-open probe
answered with 503 — the JSON arm had that guard, the streaming arm did not.

Fix the unresolved `record_success` intra-doc link in the same docstring.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wt25p6zfdw6QFLguPRsj82
@Kangyan-Zhou
Kangyan-Zhou marked this pull request as draft September 18, 2026 18:24

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants