fix(resilience): admission lease cleanup is incomplete while the handler is still pending (#14456) - #14566
Merged
Conversation
…ler is still pending (#14456)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14456
Root cause
releaseChatAdmissionAfterHandler(src/shared/middleware/chatAdmissionRelease.ts) onlyinstalled its abort-aware release wrapper after
await responsePromiseresolved. If theclient aborted while the handler promise (e.g. an in-flight/stuck upstream LLM call) was still
pending, nothing observed the abort during that window, so the heavyweight admission slot stayed
held until the handler eventually settled — which can be very late or effectively never under a
stuck upstream. PR #14457 fixed the case where an SSE
Responseobject already exists and theclient then disconnects; it did not cover this pending-handler phase.
Fix
releaseChatAdmissionAfterHandlernow attaches theoptions.signalabort listener beforeawaiting
responsePromise, releasing the lease immediately (idempotently, via the existinglease.releasedguard) if the client aborts during the pending phase. The listener is detachedonce the promise settles either way, and if the abort-triggered release already fired during the
pending phase, the redundant stream-wrapping work in
releaseChatAdmissionWhenDoneis skipped.This fix is scoped to the admission slot only — it does not cancel the underlying upstream
fetch/handler work still in flight; confirming that upstream work actually terminates remains a
separate, larger change per the issue's own "work remaining" list. Not closing that broader
follow-up scope; this PR only closes the specific admission-lease-cleanup defect.
Regression test
tests/unit/chat-admission-pending-handler-abort-14456.test.ts(new file, promoted from theplan-file's TDD probe):
an aborted client must not hold a heavyweight slot while the handler is still pending—AssertionError: 1 !== 0(activeHeavystayed at 1 after abort).releases exactly once with no listener leak, and an already-aborted signal releases immediately.
Existing tests
Ran the full touched-area suite (58 tests, 0 failures):
tests/unit/chat-body-admission.test.ts,tests/unit/chat-admission-abandoned-stream-lease.test.ts(the #14457 coverage, for contrast),tests/unit/chat-admission-wrapper.test.ts,tests/unit/responses-route-early-keepalive-wiring.test.ts.Gates run
npm run typecheck:core→ exit 0npx eslint --suppressions-location config/quality/eslint-suppressions.json <changed files>→ exit 0node scripts/check/check-file-size.mjs→ OKnode scripts/check/check-complexity-ratchets.mjs --base-ref origin/release/v3.8.51→ OK, 0 violations in the 1 changed filenode scripts/check/check-changelog-integrity.mjs→ OKnode scripts/check/check-mutation-test-coverage.mjs --strict→ pre-existing drift, unrelated to this PR (see below)check-mutation-test-coverage.mjs --strictreports 8 covering test(s) missing fromstryker.conf.jsonacrossopen-sse/services/accountFallback.ts,src/sse/services/auth.ts,open-sse/services/combo/comboStructure.tsandopen-sse/services/combo/quotaScoring.ts— noneof which this PR touches (this PR's only source file is
src/shared/middleware/chatAdmissionRelease.ts, which is not in the 31-module mutation scope atall). Inherited base drift, not introduced by this change.
Plan-file:
_tasks/pipeline/bugs/2-implementing/14456-fix-resilience-admission-lease-cleanup-is-incomplete-while-the.plan.md