feat(obs): record client-abandoned requests as 499 - #852
Conversation
Every endpoint writes its access log and metrics from the tail of its own
handler — 29 emit_access_log call sites across 12 modules. When the caller
hangs up before the response head is written, axum drops the handler future
and none of that code runs, so the request is absent from the access log,
the usage events and the metrics at the same time. That hides the case
operators most need: a client giving up during a long time-to-first-token,
and it makes "the client sent N requests but the gateway logged M"
unaccountable.
A cancelled future is only observable from Drop, so arm a guard in one
layer, disarm it when the inner service yields a response, and emit from
Drop when it is still armed. One layer rather than 29 call sites also keeps
the endpoint family from drifting the way the request-id header did before
ensure_request_id.
The recorded outcome is 499 (nginx's client-closed-request convention;
LiteLLM reports the same event the same way) plus a dedicated
aisix_proxy_client_cancelled_requests_total{endpoint} counter. The label set
is endpoint only: a cancelled request has no resolved model, provider key or
team — the body may not even be parsed yet — and endpoint arrives already
collapsed to a bounded route template, so the series stays low-cardinality
by construction.
Mid-stream disconnects are unaffected. By the time SSE bytes flow the
response head is committed, the handler has logged, and the per-stream Drop
guard emits the usage event; response bodies are polled after this
middleware returns, so the guard is already disarmed and nothing is
double-counted.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds cancellation observability and tracks whether streaming responses reach completion. Client-abandoned streams now report status 499, fully delivered streams report 200, and cancellation telemetry is suppressed during panic unwinding. ChangesClient cancellation telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyStream
participant Upstream
participant UsageTelemetry
Client->>ProxyStream: Consume streaming response
ProxyStream->>Upstream: Forward stream data
Upstream-->>ProxyStream: Reach EOF or terminate early
ProxyStream->>UsageTelemetry: Record status 200 or CLIENT_CLOSED_REQUEST (499)
Client-->>ProxyStream: Disconnect before completion
ProxyStream->>UsageTelemetry: Record abandoned stream as 499
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-proxy/src/lib.rs`:
- Around line 350-415: Update ClientCancelGuard::drop to return without emitting
the cancellation AccessLog or calling record_client_cancelled when
std::thread::panicking() is true, while preserving the existing armed-state
check and normal client-cancellation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 065de6db-f801-4ece-a8b2-b1c270d43329
📒 Files selected for processing (2)
crates/aisix-obs/src/metrics.rscrates/aisix-proxy/src/lib.rs
A handler that panics drops the guard mid-unwind with `armed` still set, which is indistinguishable from a cancel at the Drop site. Recording it invents a client disconnect that never happened and buries the panic under a benign-looking 499. A panic already has its own signal — tokio surfaces the task failure and hyper drops the connection — so skip the emit while unwinding and let that stand. It also avoids emitting from a Drop during an unwind, where a panic in the emit path would abort the process.
The head-phase cancel added earlier records 499, but a client that hangs up *after* the response head — mid-stream — still produced a usage event marked 200. The same event was reported two different ways depending on when the caller went away, and 200 claimed a delivery that never finished. LiteLLM records 499 for this case; align with it so an operator running both reads one number. Each streaming path now carries `reached_end` on its completion payload, set when the upstream stream ends and read by the telemetry closure to pick 200 or 499. It is set at upstream EOF rather than after the end-of-stream guardrail scan, and — on the chat path — before the final `[DONE]` yield: `async_stream::stream!` resumes the body only when the consumer pulls again, and SDK clients routinely stop reading at the terminal frame. Marking later would report those perfectly normal requests as abandoned. All five stream guards are covered so the family cannot drift: chat's `CompleteOnDrop`, both Anthropic guards in messages.rs, `ResponsesUsageGuard`, and the cross-provider `CompleteOnDrop` in responses_bridge.rs. Non-streaming constructions of `ResponseUsage` set `reached_end: true` — reaching that code means the response was received in full. The event is still emitted either way: the upstream work happened and may have been billed, so only the outcome differs. `streaming_chat_telemetry_fires_on_client_disconnect` now pins 499, and a new counterpart pins that a fully consumed stream stays 200 — verified to fail when the marker is removed, which is what would otherwise let every streamed request be silently reported as abandoned.
jarvis9443
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/aisix-proxy/src/responses.rs (1)
1894-1946: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCross-provider
/v1/responsesstreaming never actually reports 499 —reached_endis populated but unused.
ResponseUsage.reached_endis correctly threaded through from the bridge (reached_end: comp.reached_end), but thestatusvalue that's actually passed toemit_usage_event(Line 1910, unchanged by this PR) is computed purely fromcomp.guardrail_blocked:let status = if comp.guardrail_blocked { 422 } else { 200 };
emit_usage_eventnever readsusage.reached_endeither (it only forwards thestatus_codeparameter it's given). So an abandoned Anthropic/cross-provider/v1/responsesstream will always be reported as200(or422if blocked), never499— unlike the verbatim OpenAI streaming path 400 lines above (Lines 1469-1477), which correctly branches onusage.reached_end. This defeats the PR's stated goal specifically for this one dispatch path.🐛 Proposed fix
- let status = if comp.guardrail_blocked { 422 } else { 200 }; + let status = if comp.guardrail_blocked { + 422 + } else if comp.reached_end { + 200 + } else { + crate::CLIENT_CLOSED_REQUEST + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/responses.rs` around lines 1894 - 1946, Update the status calculation in the cross-provider streaming completion path before emit_usage_event so an ended-incompletely stream reports 499, while preserving 422 for guardrail_blocked responses and 200 for clean completions. Use comp.reached_end (or the already-populated usage.reached_end) in the same precedence/order as the sibling verbatim streaming path, and continue passing the resulting status to metrics and emit_usage_event.crates/aisix-proxy/src/messages.rs (2)
2357-2364: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEarly
returnon a mid-stream upstream error skipsreached_end = true.This
Err(e)arm (and the window hold-back overflow arm at Lines 2338-2341)returns the generator directly, before Line 2373 has a chance to setguard.comp().reached_end = true. A genuine upstream error mid-stream is the generator concluding on its own — not a client cancellation — yet withreached_endleftfalseit now gets reported asCLIENT_CLOSED_REQUEST(499) instead of the pre-PR200, contradicting the stated "non-cancellation metrics remain unchanged" goal for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/messages.rs` around lines 2357 - 2364, Update the mid-stream error handling around the Err(e) arm and the window hold-back overflow arm so they mark guard.comp().reached_end = true before returning the error frame. Preserve the existing error-frame response and ensure genuine upstream errors are distinguished from client cancellation and retain the prior non-cancellation completion status.
3325-3356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame early-return gap in the passthrough path.
Both the hold-back overflow branch (Lines 3325-3341) and
if errored && hold_policy.is_some() { return; }(Lines 3354-3356) exit the generator before Line 3361 setsreached_end = true, so a genuine upstream mid-stream failure/overflow here is also misreported as a client cancel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/messages.rs` around lines 3325 - 3356, The passthrough error and hold-back overflow early returns bypass the generator’s end-state bookkeeping, causing failures to be reported as client cancellation. Update the relevant streaming generator logic around the hold-back overflow branch and the `errored && hold_policy.is_some()` return so these exits set `reached_end = true` before returning, while preserving the existing fail-closed response and error forwarding behavior.crates/aisix-proxy/src/responses_bridge.rs (1)
1120-1128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame early-return-before-
reached_endgap as messages.rs.This
Err(e)armreturns immediately, before Line 1151 setsguard.comp().reached_end = true. A mid-stream upstream decode/connection error here will be reported asCLIENT_CLOSED_REQUESTinstead of the prior200, even though the client never disconnected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/responses_bridge.rs` around lines 1120 - 1128, The Err(e) handling in the response stream returns before the completion guard records reached_end, causing upstream errors to be misclassified as client disconnects. Update this error path to mark guard.comp().reached_end = true before returning, while preserving the existing error frame emission and early return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-proxy/src/chat.rs`:
- Around line 4102-4113: Move the `guard.comp().reached_end = true` assignment
in the chat stream completion flow to immediately after the forwarding loop and
before the post-loop output-guardrail scan. Mirror the ordering used by
`build_anthropic_sse_stream`, `build_anthropic_passthrough_stream`, and
`build_responses_bridge_stream`, preserving the flag’s contract when the
consumer disconnects during awaited guardrail checks.
---
Outside diff comments:
In `@crates/aisix-proxy/src/messages.rs`:
- Around line 2357-2364: Update the mid-stream error handling around the Err(e)
arm and the window hold-back overflow arm so they mark guard.comp().reached_end
= true before returning the error frame. Preserve the existing error-frame
response and ensure genuine upstream errors are distinguished from client
cancellation and retain the prior non-cancellation completion status.
- Around line 3325-3356: The passthrough error and hold-back overflow early
returns bypass the generator’s end-state bookkeeping, causing failures to be
reported as client cancellation. Update the relevant streaming generator logic
around the hold-back overflow branch and the `errored && hold_policy.is_some()`
return so these exits set `reached_end = true` before returning, while
preserving the existing fail-closed response and error forwarding behavior.
In `@crates/aisix-proxy/src/responses_bridge.rs`:
- Around line 1120-1128: The Err(e) handling in the response stream returns
before the completion guard records reached_end, causing upstream errors to be
misclassified as client disconnects. Update this error path to mark
guard.comp().reached_end = true before returning, while preserving the existing
error frame emission and early return behavior.
In `@crates/aisix-proxy/src/responses.rs`:
- Around line 1894-1946: Update the status calculation in the cross-provider
streaming completion path before emit_usage_event so an ended-incompletely
stream reports 499, while preserving 422 for guardrail_blocked responses and 200
for clean completions. Use comp.reached_end (or the already-populated
usage.reached_end) in the same precedence/order as the sibling verbatim
streaming path, and continue passing the resulting status to metrics and
emit_usage_event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39b534ef-ec1c-42a6-ae43-2d99e64b6986
📒 Files selected for processing (5)
crates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/responses_bridge.rs
The chat path set the flag after the end-of-stream output-guardrail block, unlike the three sibling streams which set it right after the forwarding loop. That block awaits remote guardrail calls whenever an output guardrail is attached, and a consumer dropping during that await — SDK clients close on the terminal frame, which the siblings' comments already call out as routine — left the flag unset. A fully delivered response was then reported as 499, contradicting the documented contract on the field itself. Move it to upstream EOF, matching messages.rs and responses_bridge.rs. It stays ahead of the final `[DONE]` yield either way, which was the original reason for the old placement. Adds a fully-consumed test with an output guardrail attached, the shape that puts an awaiting scan between the last chunk and `[DONE]`. It pins the placement contract but cannot reproduce the race: a keyword guardrail scans locally, so its await resolves immediately and no consumer can be dropped inside it.
2 participants
Problem
A caller that gives up on a request left the gateway reporting it two different ways depending on when it went away, and one of those ways was nothing at all.
Before the response head — axum drops the handler future, so none of the per-endpoint tail code runs (29
emit_access_logcall sites across 12 modules). The request was absent from the access log, the usage events and the metrics simultaneously. That made "the client says it sent N requests but the gateway logged M" unaccountable, and it hid exactly the case operators care about: a caller giving up during a long time-to-first-token.Mid-stream — the per-stream
Dropguard did emit a usage event, but marked it200, claiming a delivery that never finished.What changed
Both are now recorded as
499, nginx's client-closed-request convention. LiteLLM reports the same event the same way (ClientDisconnected), so an operator running both products reads one number.Head-phase cancel — a cancelled future is only observable from
Drop, so one layer arms a guard, disarms it when the inner service yields a response, and emits fromDropwhen it is still armed:status=499anderror_kind="client_disconnected"aisix_proxy_client_cancelled_requests_total{endpoint}Doing it in a single layer rather than at 29 call sites also keeps the endpoint family from drifting the way the request-id header did before
ensure_request_id. It sits outsiderecord_in_flight_requestso a hang-up during body upload is captured too, and insideensure_request_idso the line carries the request id the caller was handed.A panicking handler drops the guard mid-unwind with
armedstill set, which is indistinguishable from a cancel at theDropsite. Guarded withstd::thread::panicking(): recording it would invent a disconnect that never happened and bury the panic under a benign 499, and emitting from aDropduring an unwind risks a double panic. A panic already has its own signal.Mid-stream abandon — each streaming path carries
reached_endon its completion payload, set when the upstream stream ends and read by the telemetry closure to pick 200 or 499. All five guards are covered so the family cannot drift: chat'sCompleteOnDrop, both Anthropic guards,ResponsesUsageGuard, and the cross-providerCompleteOnDrop.The marker is set at upstream EOF rather than after the end-of-stream guardrail scan, and on the chat path before the final
[DONE]yield.async_stream::stream!resumes the body only when the consumer pulls again, and SDK clients routinely stop reading at the terminal frame — marking later would report those perfectly normal requests as abandoned. Non-streaming constructions ofResponseUsageset ittrue: reaching that code means the response was received in full.Behaviour change
499instead of200. The event is still emitted either way — the upstream work happened and may have been billed — only the outcome differs. Dashboards that count streamed successes bystatus_code == 200will see these move out of the success bucket, which is the point.Tests
client_cancel_before_response_head_is_recorded— counter andendpointlabel exist after a cancel. Verified to fail when the layer is unmounted.cancel_guard_stays_silent_during_unwind— a panicking handler is not miscounted. Verified to fail when thepanicking()check is removed.completed_request_is_not_counted_as_client_cancel/mid_stream_disconnect_is_not_counted_as_head_phase_cancel— no false positives, no double counting.streaming_chat_telemetry_fires_on_client_disconnectnow pins 499; a newstreaming_chat_telemetry_reports_200_when_fully_consumedpins the other direction. Verified to fail when the marker is removed — without it every streamed request would silently be reported as abandoned.768 tests pass.
Follow-up
A usage event for a head-phase cancel is not emitted yet. The agreed rule is to emit one when the request already reached the upstream (it may have incurred cost there) and to rely on the access log otherwise. That needs the dispatch context threaded to the guard and is left to its own PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes