Skip to content

[Router] Abort the engine when a client disconnects mid-request - #39461

Merged
ShangmingCai merged 11 commits into
mainfrom
router-4a-abort-on-disconnect
Sep 22, 2026
Merged

ShangmingCai merged 11 commits into
mainfrom
router-4a-abort-on-disconnect

Conversation

@Kangyan-Zhou

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

Copy link
Copy Markdown
Collaborator

Client disconnects and router timeouts now trigger best-effort engine cancellation for plain, single-sample chat requests.

  • One small drop guard covers dispatch and streaming. Completed responses and rejected streams disarm it.
  • Mint a fresh 32-character UUID, splice it into the body, and log it alongside the caller's correlation ID.
  • Preserve caller-supplied IDs without aborting by them: the engine matches abort IDs by prefix. Skip fan-out requests (n > 1, including configured defaults) and PD requests.
  • Replay authorization on /abort_request, with a five-second timeout. Abort failures warn without affecting the circuit breaker. Engines requiring a separate admin key remain unsupported.

Consolidated tests cover completion, cancellation before headers, silent-stream disconnects, deadlines, repeated correlation IDs, opt-outs, and authentication. Incorporates the compact guard and silent-stream regression from #40605.

Validation: 1,117 workspace library/integration tests passed; all-target cargo check, strict Clippy, and nightly formatting passed.


CI States

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

kzhou-radixark and others added 3 commits September 14, 2026 14:00
…r's status

The router's HTTP status for a router-originated error is now a pure function
of a new `ErrorClass`, and the precise condition travels in
`x-router-error-code`. Two defects fall out of that:

- `UpstreamTimeout` mapped to 502 while `StaleRequestExpired` mapped to 504,
  even though both are "the router gave up waiting". They now share
  `ErrorClass::Timeout` and cannot drift apart; `UpstreamTimeout` becomes 504.
- A mid-body drop discarded the worker's real status. Its code is now
  `upstream_body_incomplete` and the worker's status is echoed in a new
  `x-router-upstream-status` header, routed through an exhaustive
  `ApiError::upstream_status()` so a future variant that synthesizes a status
  over a responding worker has to decide whether it echoes one. The variant's
  `Display` and its client-facing message name an incomplete body rather than
  an error status, since the status it carries is usually a worker 200 that
  then dropped its body.

A worker's own response is still forwarded verbatim and carries neither header
— that absence is how a gateway tells engine-origin from router-origin.

The dashboard's status panel no longer reads 504 as "stale-cancel": after this
change three different conditions produce one, and `worker_requests_total`
is what tells them apart.

Tests: a table pinning the (status, error-code, upstream-status) triple for
every `ApiError` variant; the 502->504 change on both timeout paths; the
non-streaming mid-body drop echoing the worker status; and the asymmetric
streaming half, where headers already went out as 200 so the client keeps the
200 with no router headers at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5
A saturated engine returning its own queue-full 503s trips the router's
circuit breaker on that backpressure. With a single worker the router then
sheds *every* request for the whole cool-down — including after the engine has
drained and gone idle.

Classify the upstream status through a new `breaker_outcome`: 503 and 429 map
to a `Neutral` outcome wired to `CircuitBreaker::record_backpressure`, which
never opens the breaker and, while Closed, leaves an in-progress failure streak
intact — but still resolves a half-open probe, so a recovered-but-busy worker
answering a probe with 503 isn't wedged shut forever. A backpressure answer
that arrives while the breaker is already Open is a no-op rather than a reset:
`allow` gates admission, not completion, so a request admitted while Closed can
land there after concurrent failures have tripped it. Genuine 5xx faults
(500/502/504/…) and transport errors, timeouts and mid-body drops still count
as failures. Applied to both the JSON and the streaming dispatch arm.

The tradeoff is stated at `breaker_outcome`: a worker stuck returning 503
forever is no longer detected here, because HTTP status alone can't tell "busy"
from "broken-and-saying-503" — and counting it caused the worse fleet-wide
false-shed.

Tests, all three failing without the fix: breaker-level (probe resolution,
streak preservation, backpressure-alone-never-opens) and end-to-end through
`forward_json_to` / `forward_streaming_to` (engine 503 stays closed, 500 still
trips, half-open 503 recovers). The threshold tests loop on `would_allow()`
rather than a hard-coded count, so a change to the default
`CircuitBreakerConfig` cannot make them vacuous.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5
…inal status

Three observability gaps, one root cause: per-request accounting lived in the
chat handler, so anything the handler never saw was invisible.

- The access log missed every request that did not reach a handler: a 413 from
  the body-limit layer, a 400 from the body extractor when a client drops the
  connection mid-upload, an unrouted 404, and the handler's own `?`
  short-circuits (body-validation 400, model-not-found 404). This is what made
  a "the router throws a lot of 400s" report untraceable in the logs. One
  `access_log_and_record` middleware now emits an `http_request` line for every
  request at the same site that already owns the edge counters. A SUCCESSFUL
  infra poll (/healthz, /readyz, /metrics) logs at DEBUG so kubelet and
  Prometheus traffic doesn't bury real requests; a FAILING probe keeps its INFO
  line, because a pod dropping out of readiness is the transition an operator
  needs to see. Dispatched requests attach a `RequestLogContext` so the line
  still names the worker, model, stream flag and outcome — the worker being the
  one the client-visible response came from, which in PD mode is the decode
  peer rather than the policy-selected prefill worker.

- `worker_requests_total`'s outcome came from `Result::Ok`/`Err`. A worker
  4xx/5xx the router forwards is an `Ok(Response)` at the handler — only
  transport failures bubble up as `Err` — so every forwarded engine error was
  counted `outcome="success"`. Both the metric and the log line now derive the
  outcome from the client-visible status through one `outcome_from_status`, so
  the two cannot disagree.

- A handler panic was caught by hyper, which drops the connection without
  producing a Response, so it reached neither the counters nor the log. A
  `CatchPanicLayer` (tower-http's `catch-panic` feature) synthesizes a 500,
  positioned inner to the middleware so the 500 is counted and logged. It
  renders through `ApiError::Internal`, so the one router-originated error that
  would otherwise answer with tower-http's plain-text body keeps the JSON
  envelope and `x-router-error-code` the previous commit defines.

`outcome` is a four-way split, not a boolean, so that `error` means *this
worker failed* and agrees with what `breaker_outcome` counts as a fault:
`backpressure` (429/503) and `client_error` (other 4xx) are responses a healthy
worker gives, and folding them into `error` pegs the per-worker error ratio at
~100% during exactly the 503 storm the previous commit exists to survive.
`cancelled` is never derived from a status — the stale-request deadline, the
router's own upstream timeout and a worker's forwarded 504 are all 504, and
only the `ApiError` variant can tell them apart — so the handler names it and
carries it on `RequestLogContext`. A hung worker therefore stays in
`outcome="error"`, which is the series a per-worker error-ratio alert watches.

Also bounds the `method` metric label: `http::Method` accepts any RFC-7230
extension token and this middleware runs before axum's method-router returns
405, so the raw method on requests_total / responses_total was
caller-controlled unbounded-cardinality input. `normalize_method` collapses
unknown verbs to `other`; the access log keeps the real method.

Note for log consumers: the per-handler `chat_completions` event is replaced by
the `http_request` event, and its `http_status` field is now `status`. The
outcome taxonomy and the access-log field list are documented in
monitoring/README.md.

Tests: a forwarded 5xx counted as `outcome="error"` and not as a success; an
upstream timeout pinned to `outcome="error"`; the full `outcome_from_status`
table including the 429-before-4xx ordering; an unrouted request still logged;
a dispatched failure naming its worker in the log; the raw URI never reaching a
metric label; an unknown verb collapsing to `other`; a failing readiness probe
reaching INFO while a healthy one does not; and a handler panic surfacing as a
counted 500 with the router error envelope. The panic and `method` tests drive
the real `build_router` rather than a hand-composed one, so deleting a layer
fails a test; the infra-probe test carries a positive control so its negative
assertion cannot pass vacuously; and a global no-op subscriber keeps `tracing`
callsite interest live so log capture is not order-dependent under the parallel
harness.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012xa7Ey1ujKRSrfw4CMrAF5
@Kangyan-Zhou
Kangyan-Zhou changed the base branch from router-4d-outcome-attribution to router-4d-3-access-log September 14, 2026 21:17
The router already noticed client disconnects — #38737 even reports one as
`StreamEnd::client_disconnect` — but never told the engine, so the engine kept
generating on a dead connection and burned GPU until it hit its own token
limit. Wire up an abort.

Mint a `router-<x-request-id>-<uuid>` rid per plain-mode chat request and
inject it into the forwarded body so the engine adopts it, then POST
`/abort_request {rid, abort_all:false}` when the request is torn down early:

- Streaming: the SSE pump's completion report decides. `engine_may_still_be_generating`
  spares only a stream drained to its clean end with the client still attached
  — the one shape that proves the engine stopped on its own. A
  `client_disconnect` means the reader went away mid-generation; a transport
  failure (which folds in a pump panic) means the router lost the connection,
  which is no evidence about the engine at all. `saw_error_event` is not
  consulted: an engine that reports its own failure as an SSE `data:
  {"error"…}` event still closes cleanly, and that clean close is what says it
  is done.
- Non-streaming: an `AbortOnDrop` guard, disarmed once a complete response is
  in hand. A handler-future drop (client disconnect) or a stale-request-janitor
  timeout leaves it armed. The streaming arm takes the same guard for the
  pre-headers window, where no pump — and so no completion report — exists yet.
- `abort_would_be_pointless` stands the guard down for a pre-dispatch failure
  (`BreakerOpen`, `WorkerMisconfigured`): the request never went out, so there
  is no rid to abort, and POSTing anyway would add load to the one worker the
  router just decided to stop using. A transport error or a timeout keeps it
  armed — either can leave the engine still generating.
- The abort replays the request's own `Authorization`. SGLang marks
  `/abort_request` `ADMIN_OPTIONAL`, so an engine started with `--api-key` 401s
  an unauthenticated abort; without this the feature silently does nothing on
  exactly the deployments that secure their engines. A refused abort warns
  rather than logging at debug.
- `send_abort` stays best-effort otherwise: 5s timeout, failures logged not
  propagated, and never circuit-breaker gated — an abort is a courtesy to the
  engine, not a verdict on the worker.

Two cases opt out and keep today's behavior exactly:

- PD-disaggregated mode. Prefill is deliberately detached so it outlives the
  client for KV-transfer correctness; aborting only the decode half
  mid-transfer is a riskier change, out of scope here.
- A request that arrived with its own `rid`. The scheduler aborts every
  in-flight request whose rid *starts with* the one it is handed
  (`req.rid.startswith(recv_req.rid)`), so honouring a caller-chosen abort key
  would let `{"rid": "router-"}` cancel a worker's entire router-minted
  population on disconnect. Overwriting the caller's `rid` is not an option
  either — it is the handle they asked the engine to file the request under.

That same prefix rule is why a minted rid ends in a fresh UUID rather than
being the correlation id alone: two callers can pick colliding `x-request-id`
values, but neither can predict the other's UUID, so one cannot steer an abort
onto the other's request. The `x-request-id` half is what lets an operator take
a rid out of an engine log line and find the caller's own request; it is
dropped when absent, empty, over 64 chars, or outside an id alphabet.

Note the minted rid is client-visible: SGLang reports a request's rid as
`meta_info["id"]`, which is what the OpenAI response `id` carries, so a
plain-mode response's `id` becomes this value instead of the engine-minted
`uuid4().hex`.

`RequestProbe` probes `rid` as `Option<IgnoredAny>` — presence only. SGLang
accepts `rid` as a string *or* a list of strings, so typing it as
`Option<String>` would turn a body the engine accepts today into a router-side
400.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ae21XCf9NZQ2tcZ68HHqnr
@Kangyan-Zhou
Kangyan-Zhou force-pushed the router-4a-abort-on-disconnect branch from ab7feaf to 505ef50 Compare September 14, 2026 21:19
@Kangyan-Zhou
Kangyan-Zhou added this pull request to stack #39890 September 17, 2026 03:30
Base automatically changed from router-4d-3-access-log to main September 18, 2026 19:11
@ShangmingCai
ShangmingCai marked this pull request as ready for review September 19, 2026 12:59
#39460's three commits landed on main as #39463 / #39464 / #39465, reworked in
review, so this branch's own copies of them are superseded. The merge is
resolved to main's version of everything they touched, plus this branch's abort
commit rebuilt on top — the resulting tree is identical to rebasing 505ef50
straight onto main, so none of the pre-review code can come back.

Three collisions needed real decisions:

* `Proxy` no longer has one `client`. #39006 split it into `default_client` /
  `h2c_client`, selected per worker by `WireProtocol`. `abort_guard_for` now
  takes the worker's `protocol` and the streaming arm's abort uses
  `client_for(protocol)`, so the abort rides the same client as the forward it
  covers. `admin_client()` would have been wrong: it is the negotiating client,
  which cannot reach an h2c-only worker on a cleartext port — the abort would
  have silently failed on exactly the fleets that set `--enable-http2`.

* `RequestProbe` is no longer `#[derive(Deserialize)]`. Main hand-writes the
  visitor for the sampling array, so the `#[serde(default)] rid` field was
  inert. `rid` is now a `ProbeKey::Rid` variant, deliberately NOT a
  `RoutingKey`: the router does not route on it, so a repeated `rid` stays
  last-wins like the engine's own `json.loads` instead of becoming a 400.
  An explicit `null` still reads as absent.

* `build_outgoing_body` gained main's `sampling` parameter alongside `rid`.
  Note the interaction: #39002 added `splice_top_level`, a raw-bytes injection
  that skips the parse entirely, taken when only sampling is injected. A minted
  `rid` never overwrites a client key either — `resolve_engine_rid` returns
  `None` the moment the caller sets its own — so it has the same splice-safe
  shape, but `splice_top_level` writes `(SamplingField, Number)` members and so
  cannot carry it. Until that is generalized, minting a rid pulls every
  plain-mode request back onto the full `serde_json::Value` round-trip that
  #39002 had just removed. Flagged in the code; worth a decision before merge.

Seven main-side tests pinned "the forwarded body equals the request exactly",
which a minted `rid` breaks. They now strip the router-minted `rid` and assert
its shape separately, keeping each test's actual subject (no `input_ids` added,
`messages` preserved). `h2c_forward.rs` gained an explicit `None` abort rid.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test
--lib --tests (758 + 7 + 65 + 132 = 962 passed, 0 failed) are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the npu label Sep 19, 2026
@ShangmingCai

Copy link
Copy Markdown
Collaborator

/tag-and-rerun-ci

@github-actions github-actions Bot added the run-ci CI: run the baseline test suite on this PR label Sep 19, 2026
ShangmingCai and others added 4 commits September 20, 2026 12:46
#39002 moved top-level injection off `serde_json` and onto `splice_top_level`,
a raw-bytes write before the closing brace, so a plain-mode request stops
paying a full `Value` parse + re-serialize on a body that runs to
MAX_CHAT_BODY_BYTES (32 MiB). Minting an abort rid put every plain-mode request
straight back on the parse path, because `splice_top_level` could only write
`(SamplingField, Number)` members. That silently undid #39002 for the whole
plain-mode population, unflagged.

A minted rid has exactly the property the splice path requires: it never
overwrites a key the client sent, because `resolve_engine_rid` returns `None`
the moment the caller files the request under its own rid. So it belongs on
the same path as the sampling inject-set, not on the parse path.

* `splice_top_level` now takes `rid: Option<&str>` alongside the sampling
  members and writes it last. Placement is load-bearing in the same way it is
  for sampling: before the CLOSING brace, so an injected rid wins the last-wins
  reading every JSON parser performs — which is what a request sending an
  explicit `"rid": null` needs, since the probe reads null as absent and the
  router therefore mints one.

* Escaping: a `serde_json::Number` renders as valid JSON unescaped, but a rid
  is a STRING, so splicing it raw would be an injection hole if it could ever
  carry a quote. It cannot — `correlation_id` admits only an id alphabet and
  the rest is `router-` plus a hex uuid — but `rid_is_splice_safe` checks
  anyway rather than trusting a caller two functions away. Anything unexpected
  declines the splice and falls back to the parse path, where `serde_json`
  escapes it correctly. `build_outgoing_body_declines_to_splice_an_unsafe_rid`
  pins that a rid containing `","messages":["pwned` lands as a rid value and
  not as a sibling key.

* `build_outgoing_body`'s guard is now `needs_parse = input_ids.is_some() ||
  bootstrap.is_some()` — only those two can have to OVERWRITE a client key.
  A parse already on hand no longer forces the parse path, so cache-aware and
  bucket-routing traffic (which parses at ingress but forwards no `input_ids`)
  keeps the fast path too.

Six tests, pinned on exact bytes so a reintroduced round-trip fails them: rid
alone, rid with a parse on hand, rid + sampling together, the empty-object
no-leading-comma case, the explicit-null last-wins case, and the escaping
decline.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --lib
--tests (764 + 7 + 65 + 132 = 968 passed, 0 failed) are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Main split `server/routes/chat.rs` into `chat.rs` + `chat/{forward,
preparation,reorg}.rs` since this branch forked, so the abort-on-disconnect
wiring had to be re-applied rather than merged. Conflict resolution took
main's module layout wholesale and re-landed the feature on top:

- `preparation.rs` owns the body half: `RoutingFields` probes `rid` by
  presence, `append_sampling_defaults` becomes `append_top_level_fields` and
  splices the minted rid alongside the sampling defaults (`rid_is_splice_safe`
  guards the escape), and `build_outgoing_body` / `into_outgoing_body` thread
  it through. `resolve_engine_rid` and `correlation_id` moved here with them.
- `forward.rs` owns the guard half. Main funnels both routing paths through a
  single `tokio::select!` over `forward_to_response_worker`, so the branch's
  two separate guards (unary, and streaming's pre-headers window) collapse into
  one `AbortOnDrop` armed across the dispatch and stood down by
  `abort_would_be_pointless`. Semantics are unchanged: a streaming `Ok` hands
  the decision to the SSE pump's completion report, a drop or an expiration
  token leaves it armed.

Because both `ChatRouting::Legacy` and `ChatRouting::Reorg` converge on
`forward_chat_request`, the reorg route now inherits the feature too;
`reorg_route_mints_an_abort_rid_for_plain_but_not_for_pd` pins that, including
the PD opt-out on that path.

The unit tests that lived in the old monolithic `chat.rs` were ported to the
modules that now own the code — the body/rid tests to `preparation.rs`, the
`abort_would_be_pointless` table to `forward.rs`. `kimi_ids_forward_with_
engine_rendering_fallback`, added to main after this branch forked, compares
the forwarded body byte-for-byte and now strips the minted rid like its
siblings.

`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` and
`cargo test` (746 lib + 7 + 112 + 148 integration) are clean. Neutering
`abort_would_be_pointless` to always stand down fails both janitor-expiry
tests, confirming the rewritten guard is load-bearing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… claims

A follow-up on review: one behavior change, three doc corrections where the
code claimed more than it does.

Fan-out requests (`n > 1`) now opt out of the minted rid entirely.
`GenerateReqInput._handle_parallel_sampling` converts such a request to a batch,
and the batch path calls `regenerate_rid()` on every sample — a fresh
`uuid4().hex` that is NOT prefixed by the injected rid. The minted rid is
therefore discarded before the request is abortable: an abort by it matches
nothing, and the response `id` never carries it either. Injecting one bought
nothing and cost a futile `/abort_request` on every disconnect (which, on an
engine with more than one tokenizer worker, also inflates
`sglang:num_aborted_requests`). Fan-out is judged on the `n` the ENGINE will
see — the caller's value, or the default the operator configured for a request
that omits one — and an `n` the probe cannot read counts as fan-out, since
opting out costs one abort while guessing wrong costs a POST per disconnect.
Conservative in one direction: a beam-search request keeps its rid (beam width
means "sequences returned", not fan-out), so opting out there loses an abort
that would have worked.

The doc corrections, none of which change behavior:

- `send_abort` claimed replaying the caller's `Authorization` covers an engine
  started with `--api-key`. It covers that one configuration only. SGLang's
  `ADMIN_OPTIONAL` check accepts the plain api key ONLY when no admin key is
  set; with `--admin-api-key` — alone, or alongside an api key, where the api
  key is explicitly not accepted — the abort needs an admin credential the
  router has no configuration to hold, so every abort 401s and the warning is
  the only signal.
- `send_abort` presented the engine as generating unattended until its own token
  limit. It does not: `TokenizerManager._wait_one_response` polls
  `request.is_disconnected()` after every output chunk and on every 4s idle
  tick, and the router does close its upstream connection. This is a latency
  optimization worth up to one poll interval per disconnect, not a leak fix.
- `engine_may_still_be_generating` said nothing about when its report arrives.
  The pump only learns the client is gone when it next tries to hand a chunk
  downstream, so a client that leaves during a long prefill is not acted on
  until TTFT, and the streaming forward sets no request timeout to cap that.
  Bounding it is the streaming reaper's job.

Also notes the one stable shape of a minted rid — the leading `router-`, since
a request with no usable correlation header mints `router-<uuid>` with no
middle segment — for anything downstream keying on the response `id`.

`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` and
`cargo test` (747 lib + 7 + 112 + 149 integration) are clean. Neutering
`requests_multiple_samples` to always report one sample fails
`streaming_disconnect_does_not_abort_a_fan_out_request`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SGLang reports a request's `rid` as `meta_info["id"]`, which is what the OpenAI
response `id` carries, so injecting one replaces the id the caller used to see.
Minting `router-<x-request-id>-<uuid>` therefore made this a breaking change for
anything pattern-matching or length-checking that id — and a one-way door, since
undoing it after a release is itself a second breaking change.

It bought nothing. Nothing in the router routes, aborts or branches on the rid's
shape: `git grep` finds `router-` only in doc comments and test assertions. The
abort sends the full string, and it is the uuid — not any prefix — that makes it
unguessable, so the prefix-abort safety argument is unchanged. (The prefix did
guarantee a minted rid could never collide with an engine-minted one; at 2^-122
that is not worth a format change.)

So mint a bare `uuid4` hex, identical to what the engine mints for itself in
`GenerateReqInput._normalize_single_inputs`. The response `id` keeps its exact
shape — 32 lowercase hex, before and after — and the compatibility question
disappears rather than being documented or flagged.

The `x-request-id` fold goes with it. It existed only because correlation had
nowhere else to live, which was true: `RequestLogContext` carried no request id.
It does now, logged beside the `x-request-id` the access log already records, so
an operator gets from an engine log line back to the caller's request through
the router's own log instead of through a header echoed into a response body.
That join is partial by construction — the field is empty for PD, fan-out and
caller-supplied-rid requests, which mint no rid — but it is the slice where the
abort is active, and none of those have a router/engine link today either.

Tests assert the SHAPE rather than a marker, since being indistinguishable from
an engine-minted rid is the point; `streaming_disconnect_rid_carries_the_
correlation_header` inverts into `..._does_not_embed_the_correlation_header`.

`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` and `cargo test`
(740 lib + 7 + 112 + 149 integration) are clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Main landed the streaming reaper this branch's description listed as still
unported, which reshapes the one thing the abort feature reads. `StreamEnd`
lost its `client_disconnect` / `transport_ok` booleans for a single
`reason: StreamEndReason` with six variants, and `forward_streaming_to` gained
an `expiration` token. Conflict resolution keeps both sides whole — nothing
here is a pick.

- `proxy/mod.rs`: main's `stream_breaker_outcome` and this branch's abort
  machinery are additive, so both land; `forward_streaming_to` takes both
  `abort_rid` and `expiration`. `engine_may_still_be_generating` is ported onto
  `StreamEndReason` with an exhaustive match: only `Completed` spares the
  abort, exactly as `!client_disconnect && transport_ok` did, and the two
  reasons that did not exist before both abort. `IdleTimeout` and `Expired` are
  the router's own clock running out — they say nothing about whether the
  engine stopped, which is the case this feature exists for. The match is
  exhaustive so a seventh variant cannot silently default to either answer.
- `routes/chat/forward.rs`: the abort guard and main's expiration token are
  independent handles on the same dispatch and both survive. A `select!` that
  now yields `StaleRequestExpired` leaves the guard armed, which
  `abort_stays_armed_when_the_engine_may_still_be_generating` already pinned.

`engine_may_still_be_generating` and `stream_breaker_outcome` answer different
questions and deliberately disagree: an `Expired` stream is not the worker's
fault (Neutral) but the engine is very likely still generating (abort).

The merge creates one behavior neither branch had alone — a stale-request
deadline firing mid-stream, with the client still attached, now aborts the
engine. `janitor_expiry_mid_stream_aborts_the_engine` covers it, and is the
only new test here.

`cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` and
`cargo test` (748 lib + 7 + 112 + 151 integration) are clean, as is
`pre-commit` on the resolved files. Negative checks on the merged tree:
forcing `engine_may_still_be_generating` to `false` fails
`forward_streaming_to_aborts_on_client_disconnect`,
`streaming_abort_replays_the_request_authorization_header`,
`send_abort_failure_does_not_trip_circuit_breaker` and the new mid-stream test
while the pre-headers janitor test stays green; forcing it to `true` fails
`forward_streaming_to_does_not_abort_on_normal_completion`. So the rewritten
classification is load-bearing in both directions, and the pre-headers and
pump paths remain independently covered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KAZkAh6xTkyke3WKEVQ67u
Use one proxy-owned drop guard across dispatch and streaming, retaining fresh engine IDs and caller-ID, fan-out, and PD opt-outs. Consolidate cancellation regressions and cover silent streams, repeated correlation IDs, authorization, and abort failures.

Co-authored-by: Kan Wu <wukanustc@gmail.com>
@ShangmingCai
ShangmingCai merged commit 2032f3a into main Sep 22, 2026
90 of 96 checks passed
@ShangmingCai
ShangmingCai deleted the router-4a-abort-on-disconnect branch September 22, 2026 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

high priority npu run-ci CI: run the baseline test suite on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants