From 5ad9ec044215ab1b48f052d956d14120e1baf450 Mon Sep 17 00:00:00 2001 From: Abhishek Sharma Date: Mon, 28 Sep 2026 19:26:48 -0700 Subject: [PATCH] fix(chat-admission): derive the queue wait from the turn it must bridge (#13676) The chat-admission queue wait is derived from the turn it must bridge (RATE_LIMIT_MAX_WAIT_MS), with the end-to-end aux-call test from #13675 carried with credit. Boarded on the release/v3.8.51 tip (3a2678f) with the b3 release-drain batch (26 PRs): typecheck:core and the open-sse typecheck are clean, ESLint is clean on every changed file, migration numbering OK, and the focused tests of the whole board pass 611/611 (51 files). File-size ceiling growth is reconciled in the wave follow-up. Thank you @abhisheksharma2411! --- .env.example | 12 ++-- .../fixes/13676-chat-admission-queue-wait.md | 1 + docs/architecture/admission-lanes.md | 2 +- docs/guides/TROUBLESHOOTING.md | 4 +- src/shared/middleware/chatBodyAdmission.ts | 13 +++- tests/unit/chat-body-admission-queue.test.ts | 62 ++++++++++++++++++- 6 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 changelog.d/fixes/13676-chat-admission-queue-wait.md diff --git a/.env.example b/.env.example index 6289780e31..93ff922af2 100644 --- a/.env.example +++ b/.env.example @@ -502,8 +502,10 @@ ALLOW_API_KEY_REVEAL=false # DISABLE_CONTEXT_WINDOW_CHECKS=false # How long a heavy request waits for heavyweight capacity before a retryable 503. # A short bounded wait serializes agent bursts instead of an instant 503; 0 = instant. -# Default 2000 (2s). -# OMNIROUTE_CHAT_ADMISSION_QUEUE_MS=2000 +# Defaults to RATE_LIMIT_MAX_WAIT_MS (see that var elsewhere in this file for its +# current value), the same budget the resilience layer gives the upstream turn that +# holds the slot — a shorter wait cannot bridge it. +# OMNIROUTE_CHAT_ADMISSION_QUEUE_MS=20000 # example override; default tracks RATE_LIMIT_MAX_WAIT_MS # Queued-bytes budget for the admission wait: bounds total buffered body bytes parked # per lane so the wait cannot amplify the heap (#4380). Over-budget waits 503 immediately. # Default 4194304 (4 MB). @@ -3390,8 +3392,10 @@ QUOTA_STORE_DRIVER=sqlite # Heavy chat admission queue wait before returning retryable 503. Set 0 for the # legacy immediate rejection. Used by: src/shared/middleware/chatBodyAdmission.ts. -# Default: 5000 (5 seconds) -# OMNIROUTE_CHAT_ADMISSION_QUEUE_MS=5000 +# Documented once above, under the chat admission block — this entry said 5000 +# while the code said 2000, so it is a pointer now rather than a second answer. +# Default: RATE_LIMIT_MAX_WAIT_MS (see that var elsewhere in this file for its +# current value) # Timeout for /api/jobs/:id/run-now while it waits for an in-flight run. # Used by: src/app/api/jobs/[id]/run-now/route.ts. Default: 30000 (30 seconds) diff --git a/changelog.d/fixes/13676-chat-admission-queue-wait.md b/changelog.d/fixes/13676-chat-admission-queue-wait.md new file mode 100644 index 0000000000..f7546c21fd --- /dev/null +++ b/changelog.d/fixes/13676-chat-admission-queue-wait.md @@ -0,0 +1 @@ +- **fix(chat-admission):** derive the heavyweight queue wait from `requestQueue.maxWaitMs` (`RATE_LIMIT_MAX_WAIT_MS`, 15 s) instead of a fixed 2 s — the single default heavyweight slot is held for the whole upstream turn, so a 2 s wait expired inside every occupancy it exists to bridge and a lone large-context session shed its own next request (`reason:"queue_timeout", activeHeavy:1, waiting:0`) with nothing to protect against; an explicit `OMNIROUTE_CHAT_ADMISSION_QUEUE_MS` still wins, `0` included, and `.env.example` no longer documents the variable twice with different defaults ([#13648](https://github.com/diegosouzapw/OmniRoute/issues/13648)) (#13676 — thanks @pacocartones) diff --git a/docs/architecture/admission-lanes.md b/docs/architecture/admission-lanes.md index eb3859f1da..a4bf32d4c2 100644 --- a/docs/architecture/admission-lanes.md +++ b/docs/architecture/admission-lanes.md @@ -46,7 +46,7 @@ complementary; operators should know which one they are looking at. - **Tuning:** - `OMNIROUTE_CHAT_MAX_INFLIGHT_BYTES` — override for the auto-derived byte budget - `OMNIROUTE_CHAT_MAX_HEAVY_IN_FLIGHT` — legacy request-count cap, opt-in only - - `OMNIROUTE_CHAT_ADMISSION_QUEUE_MS` — queue-wait before 503 (default 2000) + - `OMNIROUTE_CHAT_ADMISSION_QUEUE_MS` — queue-wait before 503 (defaults to `RATE_LIMIT_MAX_WAIT_MS`) - `OMNIROUTE_CHAT_ADMISSION_MAX_QUEUED_BYTES` — queued-bytes heap valve (default 4 MB) - `OMNIROUTE_CHAT_VIRTUAL_TTL_MS` / `OMNIROUTE_CHAT_VIRTUAL_MAX_SESSIONS` — deprecated no-ops since #10110 (accepted for config compatibility, ignored) diff --git a/docs/guides/TROUBLESHOOTING.md b/docs/guides/TROUBLESHOOTING.md index 4e949d8a79..b9324a0206 100644 --- a/docs/guides/TROUBLESHOOTING.md +++ b/docs/guides/TROUBLESHOOTING.md @@ -54,7 +54,7 @@ Common problems and solutions for OmniRoute. ```bash export OMNIROUTE_ROTATE_ON_400=true # hop to another model/provider on 400/401 (skips broken passthrough models) export OMNIROUTE_CHAT_MAX_HEAVY_IN_FLIGHT=4 # explicit heavyweight admission ceiling (unset by default: no request-count cap, see note below) -export OMNIROUTE_CHAT_ADMISSION_QUEUE_MS=5000 # longer bounded wait for heavyweight capacity instead of an immediate retryable 503 +export OMNIROUTE_CHAT_ADMISSION_QUEUE_MS=20000 # raise the bounded wait past the RATE_LIMIT_MAX_WAIT_MS default for slow upstreams ``` Set these in the OmniRoute process environment (the daemon, e.g. via the LaunchAgent plist or `systemctl edit`), then restart OmniRoute. The rotation flag is the single highest-leverage lever: it converts a hard failure into a transparent retry against a healthy provider in the pool. @@ -615,7 +615,7 @@ heavy request arrived at once. The old count cap (`OMNIROUTE_CHAT_MAX_HEAVY_IN_F still honored, but only if you explicitly set it. When capacity is busy, a heavyweight request first waits up to -`OMNIROUTE_CHAT_ADMISSION_QUEUE_MS` (default `2000`, `0` disables the wait) for a slot to free up +`OMNIROUTE_CHAT_ADMISSION_QUEUE_MS` (defaults to `RATE_LIMIT_MAX_WAIT_MS`; `0` disables the wait) for a slot to free up before answering the retryable `503`. The bounded wait exists so agent-style clients (OpenCode, Claude Code, Cursor) that fan out heavy sub-requests concurrently serialize the burst instead of burning their whole retry budget on immediate rejections and dying mid-task. diff --git a/src/shared/middleware/chatBodyAdmission.ts b/src/shared/middleware/chatBodyAdmission.ts index 9efe0a83b3..aee5c9d0e8 100644 --- a/src/shared/middleware/chatBodyAdmission.ts +++ b/src/shared/middleware/chatBodyAdmission.ts @@ -16,6 +16,7 @@ */ import { createLogger } from "../utils/logger"; +import { DEFAULT_REQUEST_QUEUE_MAX_WAIT_MS } from "../../lib/resilience/settings"; import v8 from "node:v8"; import { trackRequest } from "../../lib/gracefulShutdown"; import { resolveIngestByteBudget, type IngestBudgetSource } from "./admissionBudget"; @@ -74,10 +75,20 @@ export const CHAT_MAX_HEAVY_IN_FLIGHT = parsePositiveInt( * that routinely land on the admission gate together; an immediate 503 makes the * client burn its retry budget in seconds and the agent dies mid-task. A short * bounded wait serializes the burst instead. `0` (legacy) rejects immediately. + * + * The default tracks `requestQueue.maxWaitMs` rather than carrying a number of its + * own, because a wait shorter than the occupancy it must bridge cannot serialize + * anything. The single default heavyweight slot is held for the whole upstream + * turn, which the resilience layer already budgets at that value (15s), so the old + * fixed 2s expired inside every one of them: a single large-context session shed + * itself with `reason:"queue_timeout", activeHeavy:1, waiting:0` — one holder, no + * contention, nothing for the 503 to protect against (#13648). Tying the two + * together also means an operator who raises `RATE_LIMIT_MAX_WAIT_MS` for slow + * upstreams does not have to discover this second knob to keep the gate coherent. */ export const CHAT_ADMISSION_QUEUE_MAX_MS = parseNonNegativeInt( process.env.OMNIROUTE_CHAT_ADMISSION_QUEUE_MS, - 2000 + DEFAULT_REQUEST_QUEUE_MAX_WAIT_MS ); /** diff --git a/tests/unit/chat-body-admission-queue.test.ts b/tests/unit/chat-body-admission-queue.test.ts index b80a11436d..7640bb0a86 100644 --- a/tests/unit/chat-body-admission-queue.test.ts +++ b/tests/unit/chat-body-admission-queue.test.ts @@ -12,6 +12,7 @@ const { CHAT_ADMISSION_MAX_QUEUED_BYTES, CHAT_LARGE_BODY_BYTES, } = admissionModule; +const { DEFAULT_REQUEST_QUEUE_MAX_WAIT_MS } = await import("../../src/lib/resilience/settings.ts"); function chatRequest(body: string, contentLength: string | null = String(body.length)): Request { const headers: Record = { "content-type": "application/json" }; @@ -418,9 +419,17 @@ test("structural admission enforces the queued-bytes cap end-to-end", async () = assert.equal(controller.activeHeavy, 0); }); -test("queue-wait defaults are bounded (2s wait, 4MB queued-bytes budget)", () => { +test("queue-wait defaults are bounded, and the wait tracks the turn it must bridge", () => { if (process.env.OMNIROUTE_CHAT_ADMISSION_QUEUE_MS === undefined) { - assert.equal(CHAT_ADMISSION_QUEUE_MAX_MS, 2_000); + // Was a fixed 2_000. A wait shorter than the occupancy it bridges cannot + // serialize a burst — it just 503s the holder's own next request (#13648). + // Asserted against the constant rather than 15_000 so the two cannot drift: + // an operator who raises RATE_LIMIT_MAX_WAIT_MS moves both together. + assert.equal(CHAT_ADMISSION_QUEUE_MAX_MS, DEFAULT_REQUEST_QUEUE_MAX_WAIT_MS); + assert.ok( + CHAT_ADMISSION_QUEUE_MAX_MS >= 15_000, + "the shipped default must be able to bridge a default heavyweight turn" + ); } if (process.env.OMNIROUTE_CHAT_ADMISSION_MAX_QUEUED_BYTES === undefined) { assert.equal(CHAT_ADMISSION_MAX_QUEUED_BYTES, 4 * 1024 * 1024); @@ -546,3 +555,52 @@ test("aborting the signal cancels a structural queue-wait", async () => { held.release(); assert.equal(controller.activeHeavy, 0); }); + +// Ported from #13675 (Co-authored-by: oyi77), with queueMs derived from +// DEFAULT_REQUEST_QUEUE_MAX_WAIT_MS instead of a hardcoded 15_000 so the +// end-to-end proof cannot drift out of sync with the resilience layer's +// default the way the doc references in #13676 briefly did. +test("#13648: a resilience-scale occupancy bridges the default queue instead of self-shedding", async () => { + // The reported failure: a single large-context agent holds the one heavyweight + // slot for the resilience layer's default turn budget while its own aux call + // arrives — with the old fixed 2s default the aux call always shed + // `queue_timeout` and the session died. The default queue must bridge a full + // resilience-scale occupancy, whatever that default currently is. + const controller = new ChatAdmissionController(1); + const held = controller.tryAcquireHeavy(); + assert.ok(held); + + const queueMs = DEFAULT_REQUEST_QUEUE_MAX_WAIT_MS; + const pending = admitChatStructure( + { + messages: [ + { role: "user", content: "one" }, + { role: "user", content: "two" }, + ], + }, + null, + { + controller, + maxMessages: 10, + heavyMessages: 2, + heavyTools: 10, + heavyTokens: 10_000, + queueMs, + heapPressureCheck: () => true, + } + ); + + let settled = false; + void pending.then(() => { + settled = true; + }); + // Old default (2s) would have shed by now; the bridged wait must still park. + await new Promise((resolve) => setTimeout(resolve, 2_500)); + assert.equal(settled, false, "aux call must still wait past the old 2s shed point"); + + held.release(); + const result = await pending; + assert.equal(result.admit, true, "aux call acquires the freed slot instead of self-shedding"); + if (result.admit) result.lease?.release(); + assert.equal(controller.activeHeavy, 0); +});