Repository navigation
fix(resilience): stop counting client aborts as provider failures (#7907) - #7934
Long-Feeds wants to merge 2 commits into
Conversation
…eaper-inference#7907) abort(reason) rejects the in-flight upstream fetch with the raw abort reason. A string reason (streamHandler's "request_signal_aborted" default, "Client disconnected: ..." variants) carries no error name and no status, so the chatCore failure path fell through to the HTTP 502 default - cooling down the healthy serving connection via markAccountUnavailable and accruing whole-provider circuit-breaker failures for what is purely a client lifecycle event (combo race losers, model switches, tab close, user interrupt). Fix: - add isClientAbortError classifier next to isLocalStreamLifecycleError (cheaper-inference#4602): recognizes AbortError names, undici UND_ERR_ABORTED / ABORT_ERR codes, and string-or-message abort reasons, while treating any error carrying a real upstream HTTP status as NOT a client abort - chatCore catch path now classifies all client-abort shapes as 499, so shouldSkipConnDisable (cheaper-inference#7177) and the provider breaker's PROVIDER_BREAKER_FAILURE_STATUSES gate both see them correctly - exclude client aborts from the provider breaker isFailure predicate in chat.ts / chatHelpers.ts, matching the existing cheaper-inference#4602 exclusion Test: tests/unit/client-abort-not-provider-failure-7907.test.ts (failed before the fix - missing classifier meant string aborts were 502s; passes after, and the breaker still opens on genuine 502s). Fixes cheaper-inference#7907 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Thanks for tracking down #7907 — the root cause is exactly right: One thing has to be cleaned up before this lands: the commit carries a |
…aper-inference#7907) Integration-level companion to the cheaper-inference#7907 unit tests: drive handleChatCore with a responseFactory that throws the bare string "request_signal_aborted" (the exact leak shape) and assert the failure maps to 499 / "Request aborted" instead of the 502 provider-failure default. Verified failing (actual 502) against the pre-fix classification, passing after.
|
Thanks for the thorough review! Noted on the metadata rule — I've removed the footer from the PR description and the new commit follows the rule; will keep it in mind for future PRs. Took the optional follow-up as well: |
|
Obrigado por atacar #7907, @Long-Feeds — o problema é real e a correção está agora em Por que #7908 e não este, em detalhe para não soar como dispensa:
A ideia central — client abort não é falha de provider — era a mesma nos dois, e você chegou nela de forma independente. Se topar, um PR focado numa parte que #7908 não cobriu é muito bem-vindo. Valeu pela contribuição. |
|
Totally fair — #7908 landing first and reusing the existing plumbing makes sense, and One small note for the record: this PR's diff didn't touch Since #7908 intentionally keeps the surfaced status as-is, a client's own abort still logs and returns |
Summary
Fixes #7907 — client-initiated aborts were counted as provider failures, cooling down healthy accounts and accruing whole-provider circuit-breaker failures.
Root cause
abort(reason)rejects the in-flight upstream fetch with the raw abort reason. Onlyerror.name === "AbortError"was mapped to 499 in the chatCore failure path. A string abort reason — the stream controller's"request_signal_aborted"default, or"Client disconnected: ..."variants — carries nonameand nostatus, so it fell through to theHTTP_STATUS.BAD_GATEWAY(502) default. A 502:shouldSkipConnDisableguard (fix(combo): reject known context overflow without exhausting providers #7177, which only matches status 499 /client_disconnected), somarkAccountUnavailablecools down the healthy serving connection, andPROVIDER_BREAKER_FAILURE_STATUSES, so the whole-provider breaker accrues failures for what is purely a client lifecycle event (combo race losers, model switches, tab close, user interrupt).Fix
src/shared/utils/circuitBreaker.ts— newisClientAbortErrorclassifier next to the existingisLocalStreamLifecycleError([BUG] codexTransport=websocket: /v1/responses fails instantly (~300ms) with "Invalid state: Controller is already closed" (502); SSE/HTTP path works #4602). RecognizesAbortErrornames, undiciUND_ERR_ABORTED/ABORT_ERRcodes, and string-or-message abort reasons; any error carrying a real upstream HTTP status is never treated as a client abort.open-sse/handlers/chatCore.ts— the catch path now classifies all client-abort shapes as 499 (both thefailureStatus/failureMessagemapping and the early return), so the existing fix(combo): reject known context overflow without exhausting providers #7177 guard and the breaker status gate both see them correctly.src/sse/handlers/chat.ts/src/sse/handlers/chatHelpers.ts— provider breakerisFailurepredicate now also excludes client aborts, matching the existing [BUG] codexTransport=websocket: /v1/responses fails instantly (~300ms) with "Invalid state: Controller is already closed" (502); SSE/HTTP path works #4602 exclusion.Tests
tests/unit/client-abort-not-provider-failure-7907.test.ts— written first and verified failing (missing classifier → string aborts became 502s), then passing after the fix:AbortErrorshapes, undici codes, and string abort reasonsstatus)Verification run:
circuit-breaker-*,combo-499-abort,combo-breaker-429,stream-handler-client-disconnect,proxyfetch-caller-abort-log-suppression,skip-provider-breaker-consumer-2743,resilience-settings-provider-breaker): 63/63 passnpm run typecheck:core+ eslint on changed files: cleancheck:changelog-integrity: passFollow-up (review)
Added an integration-level test in
tests/unit/chatcore-translation-paths.test.tsthat driveshandleChatCorewith aresponseFactorythrowing the bare string"request_signal_aborted"— the exact leak shape — asserting 499 / "Request aborted". Verified failing (actual 502) against the pre-fix classification, passing after.