fix(codex): fail fast and release per-account Responses WS leases - #12911
Merged
diegosouzapw merged 4 commits intoSep 17, 2026
Merged
diegosouzapw merged 4 commits into
diegosouzapw merged 4 commits into
Conversation
Owner
|
Solid fix, and the |
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
# Conflicts: # src/app/api/internal/codex-responses-ws/route.ts
diegosouzapw
merged commit Sep 17, 2026
36df9e5
into
diegosouzapw:release/v3.8.51
3 of 7 checks passed
diegosouzapw
added a commit
that referenced
this pull request
Sep 18, 2026
…the write, drain the 09-18 base-reds (#14101) * fix(quality): drain the 09-18 base-reds, part 1 — thinking gate parity, inventory, webpack externals Reproduced on the clean tip 7cc454d before touching anything. Five of the failures trace to one commit, #12905 (b7192b7): it gated thinking-block emission on `requestedThinking === true` in the streaming translator, while its own non-streaming path documents `undefined` as the legacy caller shape that keeps "always a thinking block". The two paths disagreed on the same input, and the streaming side also synthesized the reasoning into a TEXT block for that legacy shape. chatCore always resolves a boolean, so production never sends `undefined` — but every direct caller and the older #5786 suites do. Aligned the streaming gate to the documented tri-state: `false` suppresses, `true` and `undefined` relay, and the fix-B text synthesis fires only on an explicit opt-out. The #12905 test that asserted suppression used a bare createState() (`undefined`) to mean "client did not request thinking"; it now passes `requestedThinking: false`, which is what that sentence resolves to in production. The whole thinking family — dsml, adapter, translator, non-stream parity, #13620, #5786, markdown boundary — is 77/77. #12864 added requestRejectedFailure.ts with a getProviderConnectionById read that seeds the refusal streak across restarts; inventoried as a connection state read next to the family-cooldown site it resembles. #13909 made machineToken.ts import ./dataPaths; the isolated webpack compile has no repo tree, so it joins the sibling externals. The free-tier budget card SVG was one wave behind again (482 -> 491 models). Refs #13866 * fix(sse): restore maxQueueDepth=0 as unbounded, sanitize refusals at the write, drain the rest Part 2 of the 09-18 base-red drain. Two of the remaining failures were not stale tests but production defects the tests had caught. #12911 taught accountSemaphore to read `maxQueueSize: 0` as "reject when the slot is busy", which is what its Codex WS lease wants. But chatCore forwards `resilienceSettings.requestQueue.maxQueueDepth` into that option, and that setting's documented default since #6593 is `0 = disabled`. Under default settings every request that found its account slot occupied was answered 429 "Semaphore queue full (0)" instead of waiting — the managed-lease routing test saw exactly that. `0` (and any non-positive value) is unbounded again; the lease gets an explicit `failFast` option and its four tests stay green, so the #12911 behaviour is preserved where it was meant to apply. A contract test pins the #6593 semantics on the semaphore itself. #12864 moved two providerFailure persistence branches out of chatCore into requestRejectedFailure.ts and the sanitization did not travel with them: three `lastError` writes stored the message as received. The only caller already hands in the projected persistentMessage, so nothing leaks today, but a persistence branch must be safe at its own write (docs/security/ ERROR_SANITIZATION.md) rather than trust whoever calls it. The module now sanitizes on entry, and the public-boundary guard — which caught this by counting sanitized writes in chatCore and coming up two short — covers the extracted module too, verified by mutating one write back to raw. The rest are tests that had fallen behind legitimate changes: - #12905 inserted `requestedThinking` as the 14th positional argument of createSSETransformStreamWithLogger; two tests passed customToolNames or the buffer budget at their old positions. Both production callers were already correct. - #12754 added a per-connection reset-card fetch after the quota fetch; the spacing test now marks a chunk at the quota request only. - #13910 renamed `error` to `errorMetadata` in the timeout classification; the probe matches the identifier with a backreference and still fails when BodyTimeoutError is removed from both sites. Refs #13866 * fix(test): pin the opt-out thinking cases to requestedThinking=false; keep acquireMany under the complexity ceiling The #12905 gate-restore suite encoded 'requestedThinking absent' as opt-out, the same undefined-means-false shape its non-streaming twin documents the other way and that the two-month-old #5786 suites contradict. The three opt-out cases now set the flag explicitly, which is what chatCore resolves for an opted-out client; the two opt-in cases already did. Both suites pass together (27/27). The failFast branch pushed acquireMany over the complexity ceiling it already sat on; the admission policy (fail-fast / bounded / unbounded queue) moves to findQueueRejection() and the new-code ratchet is back at its base. Refs #13866 * fix(test): suspend the #14110 redaction assertion inline; refresh the budget card The 57 commits merged since the previous validation moved two things. #13295 changed how an unknown-root path with an ambiguous tail is answered: where `Provider failed at /custom/internal secret directory` used to become `Provider failed at <path>` it now ships verbatim. The #12506 boundary guard caught it. Two candidate fixes were tried and each breaks one of the two live contracts — #12506's fail-closed swallow, or #13144's rule that a route in prose must survive — so the choice is the owner's (#14110). The one contested assertion is suspended inline with the exact line and the issue; the other nine stay active. The isolated-child harness requires tests == pass, which is why it is a comment and not a todo. The free-tier budget card was one wave behind again (491 -> 489 models). Refs #13866, #14110 * fix(providers): type the TinyCMS DOM stub global as a loose record #13957 typed the mock global as `typeof globalThis & Record<string, unknown>`. The api-route typecheck loads lib.dom, so that intersection carries the real Window / HTMLCanvasElement / document signatures — every stub assignment fails against a DOM constructor, and `delete g.window` narrows the object to `never` (13 diagnostics, the API Route Typecheck base-red on the tip). The function exists to overwrite those globals with stubs; it is now typed as the plain record it manipulates. 29/29 tinycms tests unchanged. Refs #13866 * fix(test): pin the last opt-out thinking sibling to requestedThinking=false translator-reasoning-gate-502-repro is the third #12905 test that encoded a bare state as opt-out; the previous sweep matched files by glob and missed it. The family is now enumerated by grep on requestedThinking (7 files) plus the two pre-#12905 suites: 83/83 together. Refs #13866
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…egosouzapw#12911) * fix(codex): fail fast and release per-account Responses WS leases * chore(changelog): add fragment for Codex WS lease fail-fast fix Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> * fix(codex): carry the reasoning-rule context through the leased WS path --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…the write, drain the 09-18 base-reds (diegosouzapw#14101) * fix(quality): drain the 09-18 base-reds, part 1 — thinking gate parity, inventory, webpack externals Reproduced on the clean tip 7784784 before touching anything. Five of the failures trace to one commit, diegosouzapw#12905 (8b5f4dd): it gated thinking-block emission on `requestedThinking === true` in the streaming translator, while its own non-streaming path documents `undefined` as the legacy caller shape that keeps "always a thinking block". The two paths disagreed on the same input, and the streaming side also synthesized the reasoning into a TEXT block for that legacy shape. chatCore always resolves a boolean, so production never sends `undefined` — but every direct caller and the older diegosouzapw#5786 suites do. Aligned the streaming gate to the documented tri-state: `false` suppresses, `true` and `undefined` relay, and the fix-B text synthesis fires only on an explicit opt-out. The diegosouzapw#12905 test that asserted suppression used a bare createState() (`undefined`) to mean "client did not request thinking"; it now passes `requestedThinking: false`, which is what that sentence resolves to in production. The whole thinking family — dsml, adapter, translator, non-stream parity, diegosouzapw#13620, diegosouzapw#5786, markdown boundary — is 77/77. diegosouzapw#12864 added requestRejectedFailure.ts with a getProviderConnectionById read that seeds the refusal streak across restarts; inventoried as a connection state read next to the family-cooldown site it resembles. diegosouzapw#13909 made machineToken.ts import ./dataPaths; the isolated webpack compile has no repo tree, so it joins the sibling externals. The free-tier budget card SVG was one wave behind again (482 -> 491 models). Refs diegosouzapw#13866 * fix(sse): restore maxQueueDepth=0 as unbounded, sanitize refusals at the write, drain the rest Part 2 of the 09-18 base-red drain. Two of the remaining failures were not stale tests but production defects the tests had caught. diegosouzapw#12911 taught accountSemaphore to read `maxQueueSize: 0` as "reject when the slot is busy", which is what its Codex WS lease wants. But chatCore forwards `resilienceSettings.requestQueue.maxQueueDepth` into that option, and that setting's documented default since diegosouzapw#6593 is `0 = disabled`. Under default settings every request that found its account slot occupied was answered 429 "Semaphore queue full (0)" instead of waiting — the managed-lease routing test saw exactly that. `0` (and any non-positive value) is unbounded again; the lease gets an explicit `failFast` option and its four tests stay green, so the diegosouzapw#12911 behaviour is preserved where it was meant to apply. A contract test pins the diegosouzapw#6593 semantics on the semaphore itself. diegosouzapw#12864 moved two providerFailure persistence branches out of chatCore into requestRejectedFailure.ts and the sanitization did not travel with them: three `lastError` writes stored the message as received. The only caller already hands in the projected persistentMessage, so nothing leaks today, but a persistence branch must be safe at its own write (docs/security/ ERROR_SANITIZATION.md) rather than trust whoever calls it. The module now sanitizes on entry, and the public-boundary guard — which caught this by counting sanitized writes in chatCore and coming up two short — covers the extracted module too, verified by mutating one write back to raw. The rest are tests that had fallen behind legitimate changes: - diegosouzapw#12905 inserted `requestedThinking` as the 14th positional argument of createSSETransformStreamWithLogger; two tests passed customToolNames or the buffer budget at their old positions. Both production callers were already correct. - diegosouzapw#12754 added a per-connection reset-card fetch after the quota fetch; the spacing test now marks a chunk at the quota request only. - diegosouzapw#13910 renamed `error` to `errorMetadata` in the timeout classification; the probe matches the identifier with a backreference and still fails when BodyTimeoutError is removed from both sites. Refs diegosouzapw#13866 * fix(test): pin the opt-out thinking cases to requestedThinking=false; keep acquireMany under the complexity ceiling The diegosouzapw#12905 gate-restore suite encoded 'requestedThinking absent' as opt-out, the same undefined-means-false shape its non-streaming twin documents the other way and that the two-month-old diegosouzapw#5786 suites contradict. The three opt-out cases now set the flag explicitly, which is what chatCore resolves for an opted-out client; the two opt-in cases already did. Both suites pass together (27/27). The failFast branch pushed acquireMany over the complexity ceiling it already sat on; the admission policy (fail-fast / bounded / unbounded queue) moves to findQueueRejection() and the new-code ratchet is back at its base. Refs diegosouzapw#13866 * fix(test): suspend the diegosouzapw#14110 redaction assertion inline; refresh the budget card The 57 commits merged since the previous validation moved two things. diegosouzapw#13295 changed how an unknown-root path with an ambiguous tail is answered: where `Provider failed at /custom/internal secret directory` used to become `Provider failed at <path>` it now ships verbatim. The diegosouzapw#12506 boundary guard caught it. Two candidate fixes were tried and each breaks one of the two live contracts — diegosouzapw#12506's fail-closed swallow, or diegosouzapw#13144's rule that a route in prose must survive — so the choice is the owner's (diegosouzapw#14110). The one contested assertion is suspended inline with the exact line and the issue; the other nine stay active. The isolated-child harness requires tests == pass, which is why it is a comment and not a todo. The free-tier budget card was one wave behind again (491 -> 489 models). Refs diegosouzapw#13866, diegosouzapw#14110 * fix(providers): type the TinyCMS DOM stub global as a loose record diegosouzapw#13957 typed the mock global as `typeof globalThis & Record<string, unknown>`. The api-route typecheck loads lib.dom, so that intersection carries the real Window / HTMLCanvasElement / document signatures — every stub assignment fails against a DOM constructor, and `delete g.window` narrows the object to `never` (13 diagnostics, the API Route Typecheck base-red on the tip). The function exists to overwrite those globals with stubs; it is now typed as the plain record it manipulates. 29/29 tinycms tests unchanged. Refs diegosouzapw#13866 * fix(test): pin the last opt-out thinking sibling to requestedThinking=false translator-reasoning-gate-502-repro is the third diegosouzapw#12905 test that encoded a bare state as opt-out; the previous sweep matched files by glob and missed it. The family is now enumerated by grep on requestedThinking (7 files) plus the two pre-diegosouzapw#12905 suites: 83/83 together. Refs diegosouzapw#13866
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
2 participants
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.
fix(codex): fail fast and release per-account Responses WS leases
Summary
Improve account-slot management for the Codex Responses WebSocket bridge (
/api/internal/codex-responses-ws):src/sse/services/codexWsLease.ts(acquireCodexWsLease,maxQueueSize: 0non-queued lease) attempts to acquire the slot immediately; on failure, add that account toexcludeConnectionIdsand select another eligible account (up to 8 attempts).catch) or the access token is missing — fail fast. The normal path releases when the session ends.accountSemaphore maxQueueSize=0: InacquireMany, move the queue-full check after the immediate-acquire check (do not reject when immediate acquisition is possible) + supportmaxQueueSize >= 0as valid (0 = no queuing). Under the existingmaxQueueSize > 0condition, 0 behaved like an unlimited queue.scripts/dev/responses-ws-proxy.mjs(51 lines) for development WS proxy consistency.Error responses go through
sanitizeErrorMessage()(Hard Rule #12 compliance).Related Issues
Validation
node --import tsx/esm --test tests/unit/codex-ws-lease-contract.test.ts tests/unit/codex-ws-load-balancer.test.ts tests/unit/accountSemaphore.test.ts+node --test tests/unit/responses-ws-proxy.test.mjs(all pass)npx tsc --pretty false -p tsconfig.typecheck-core.json— 0 errorsTests Added Or Updated
tests/unit/codex-ws-lease-contract.test.tstests/unit/codex-ws-load-balancer.test.tstests/unit/accountSemaphore.test.tstests/unit/responses-ws-proxy.test.mjsCoverage Notes
Full test coverage of 2 new files (
codexWsLease.ts,route.tsrewrite) + theaccountSemaphore acquireManypath.clearCodexWsLeasesForTest()isolates tests.Reviewer Notes
d50888e45/ patch0023. Authored in fork (initguru)release/v3.8.51.changelog.d/fixes/12911-codex-ws-lease-fail-fast.md