fix(resilience): release admission lease on client abort after SSE response (#14456) - #14457
Merged
diegosouzapw merged 2 commits intoSep 22, 2026
Conversation
…ouzapw#14456) releaseChatAdmissionWhenDone wired lease release into three consumer-driven paths only: pull-to-done, pull-throws, and cancel. A client that disconnects mid-stream may stop pulling and never cancel, so none of them run and the heavyweight slot is charged for the lifetime of the process. Once activeHeavy reaches OMNIROUTE_CHAT_MAX_HEAVY_IN_FLIGHT every heavy request is shed with 503 chat_admission_busy, with waiting=0 and no real load. Observe the request signal as a fallback release path and route all five call sites through it. Release is idempotent so a late abort after a clean finish cannot double-decrement.
check:file-size freezes chatBodyAdmission.ts at 1206 lines and the fix pushed it to 1242. Move the release binding into its own module and re-export it from the original path, so every existing import site is unchanged. File is now 1159 lines; check:file-size passes.
Contributor
Author
Scope correction (2026-09-22)The current PR is a partial lifecycle fix, not a claim that all production 504 saturation is resolved. Using the exact exported module at head
The timeout/lifetime discrepancy observed in deployment therefore remains open. The next fix must reproduce the real route with timestamps for admission, combo deadline, request abort, handler settlement, upstream cancellation, and lease release. Decrementing a counter without terminating the underlying work would not be a complete fix. |
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.
Related to #14456 — partial lifecycle fix; does not close the production incident.
Scope clarification
This PR releases an admission lease when the supplied inbound request signal aborts after the handler has produced an SSE
Response. It does not currently enforce a combo deadline or clean up a handler promise that remains pending before returning a response.The initial description overstated this as the root cause and complete fix for production saturation. The incident recurred in the unpatched deployment after increasing the heavy cap to 200; long-duration 504 log rows require separate investigation. They have not been causally matched to individual held leases. See the corrected observations and open questions in #14456.
Implementation
releaseChatAdmissionWhenDone./v1/responses, both in/v1/chat/completions, and the sharedwithChatAdmissionmiddleware.src/shared/middleware/chatAdmissionRelease.tsand re-export them fromchatBodyAdmission.ts. This keeps existing import paths while satisfying the frozen file-size gate without changing its baseline.Uncovered phase: handler has not returned a Response
At head
7ed95a5952b98383e65b848ca82c4c25c8bc9c50:releaseChatAdmissionAfterHandlerawaits handler settlement before installing the new abort hook. If the client aborts whileresponsePromiseis still pending, the lease remains held until settlement. If that promise never settles, this PR alone does not release the lease.This is relevant to the reported timeout/lifetime discrepancy but is not proof of its full production cause. Returning capacity while the underlying operation is still running can itself defeat resource protection; a follow-up needs to verify actual cancellation/cleanup, not just decrement the counter.
Verification at the current head
Original regression suite and CI
The four added helper tests cover abort after wrapping, an already-aborted signal passed to the response wrapper, clean completion followed by late abort, and explicit reader cancellation. They passed locally, together with the related admission suites (39 tests reported in the original run).
The behavioral sabotage run failed the abort assertions (
actual: 1,expected: 0) with the abort hook removed. This establishes helper-level fix sensitivity; it does not reproduce the deployed HTTP disconnect chain or demonstrate that all observed 499s leaked slots.GitHub checks for
7ed95a5952b98383e65b848ca82c4c25c8bc9c50: 13 successful, 1 skipped (Build (advisory)), 2 neutral (Mergify). The earlier candidate-owned file-size failure was resolved by extraction. These are check results, not a claim of merge, deployment or incident resolution.Follow-up characterization using the actual PR module
A bounded local Node v26.8.2 probe imported the exact exported
chatAdmissionRelease.ts, with fake leases and handler promises but no copied production implementation:Those last two cases characterize a gap, not passing acceptance criteria for a complete fix. The probe is not a live Next.js/Codex/upstream reproduction. The repository tests currently do not assert cancellation while the handler promise remains pending.
Production evidence limitations and corrections
180000mstimeout message alongside a much largercall_logs.durationwarrants investigation, but does not alone prove when the timeout fired, when cleanup ended, or which admission lease was held.waiting=0, a count of long-duration historical rows equal to the cap, and a socket snapshot are not sufficient causal evidence.docker restartresets process-local counters. Recreating the container is required for changed environment values, not to reset a module-level singleton.Local lint baseline note
A plain ESLint invocation previously reported an unused
callCloudWithMachineIdimport at line 3 ofsrc/app/api/v1/chat/completions/route.ts, reproduced on the clean base2b8f89a652b5b67ac9182d2cf0c858027327a781. The repository's base-relativeNo new ESLint warningscheck passed. No baseline was widened or finding suppressed.Related work
#13648 / #13676 cover queue-wait policy; #12135 covers SSE occupancy; #13621 investigates retained backing strings; #14430 adds an in-flight byte ledger. They do not establish this deployment's complete root cause. This narrow change can be folded into related lifecycle work if that is preferable.
Still required before claiming #14456 resolved: real request-path reproduction of the pending-handler/timeout behavior, verified cancellation/settlement, lease-baseline recovery across repeated requests, and deployment verification.