diff --git a/src/web-search/executor.ts b/src/web-search/executor.ts index 33b6a1eb6cc..e14288ebafa 100644 --- a/src/web-search/executor.ts +++ b/src/web-search/executor.ts @@ -49,10 +49,12 @@ export type SidecarOutcome = WebSearchResult & { error?: string }; * The forward backend throttles burst sidecar traffic, and without a replay the 429 becomes a * failed tool result that poisons the query for the whole turn (see failedQueries in loop.ts). * 1 initial send + 2 replays; Retry-After is honored as a lower bound and capped by - * RETRY_AFTER_CEILING_MS (an instruction past the ceiling ends with the 429 instead of - * parking the search). Each wait releases the unread 429 body first so sockets do not - * accumulate under a rate-limit storm. Abort or timeout ends the wait through the existing - * catch, exactly like an abort during the SSE parse. + * RETRY_AFTER_CEILING_MS and the remaining sidecar deadline (an instruction past either + * ends with the 429 instead of parking the search). Each wait releases the unread 429 body first so sockets do not + * accumulate under a rate-limit storm. The release itself may take up to a second, so a + * deadline landing during release or backoff ends with the 429 already in hand rather than + * a timeout; a caller abort still ends the wait through the shared catch, exactly like an + * abort during the SSE parse. */ const SIDECAR_429_MAX_ATTEMPTS = 3; const SIDECAR_429_BASE_DELAY_MS = 1_000; @@ -98,9 +100,10 @@ export async function runWebSearch( stream: true, }; const url = `${forwardProvider.baseUrl}/responses`; + // t0 precedes the deadline timer's start so the remaining-time check stays conservative. + const t0 = Date.now(); const linkedSignal = signalWithTimeout(settings.timeoutMs, abortSignal); const sidecarExit = sidecarEnter("web-search"); - const t0 = Date.now(); try { const sendOnce = () => fetchWithResetRetry( // Recovery nests INSIDE the version helper: applyUpstreamRecoveryInit then always receives a @@ -129,10 +132,19 @@ export async function runWebSearch( }); // A deadline, not a clamp: an instruction past the ceiling ends the search with the // 429 instead of parking it at a provider that already said it would refuse. - if (delay > RETRY_AFTER_CEILING_MS) break; + if (delay > RETRY_AFTER_CEILING_MS || delay >= settings.timeoutMs - (Date.now() - t0)) break; console.warn(`[web-search] sidecar HTTP 429 — retrying (${attempt + 2}/${SIDECAR_429_MAX_ATTEMPTS}) after ${delay}ms`); - await releaseResponseBodyBestEffort(res.body, linkedSignal.signal); - await sleepWithAbort(delay, linkedSignal.signal); + try { + await releaseResponseBodyBestEffort(res.body, linkedSignal.signal); + await sleepWithAbort(delay, linkedSignal.signal); + } catch (e) { + // The release above may consume up to 1s, so the sidecar deadline can land during + // cleanup or mid-backoff — before the replay is dispatched. The observed 429 is + // already in hand: end with it rather than laundering it into a timeout. A caller + // abort (or a non-deadline throw) still propagates to the shared catch below. + if (!linkedSignal.signal.aborted || linkedSignal.signal.reason === abortSignal?.reason) throw e; + break; + } res = await sendOnce(); } // Attach the body guard before ANY branch reads it. The success path guarded itself below, diff --git a/structure/providers-and-adapters.md b/structure/providers-and-adapters.md index de68ef9f7ae..c60705d4b57 100644 --- a/structure/providers-and-adapters.md +++ b/structure/providers-and-adapters.md @@ -183,6 +183,7 @@ turn show the destination one consistent conversation. The rewrite runs only for `call_id` the body already carries — leaves the replayed item untouched. Re-running the search or synthesizing result text is not a permitted recovery. The bridge finalizes request-scoped OpenAI sidecar authority on completion, failure, and client cancellation — cancellation releases immediately rather than waiting on an abandoned upstream read — so a recovery probe lease no search consumed is always returned. `tests/web-search/web-search-bridge-replay.test.ts` pins the restore and each of those refusals. +A forward OpenAI search sidecar retries a 429 only when the requested delay fits both its retry ceiling and the remaining overall sidecar deadline. A delay that cannot fit returns and records the original 429 so pool routing retains quota evidence. A leg whose upstream terminal is `response.failed` or `response.incomplete` runs no search at all and closes any cell it opened rather than leaving it in progress. Assistant text is not treated as a search diff --git a/structure/runtime.md b/structure/runtime.md index ec03516b8cc..f1827917741 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -378,6 +378,8 @@ Automatic Codex pool selection and account status share the [plan exclusion cont ### Empty forced search answers `src/web-search/loop.ts` makes at most one extra answer attempt after a clean forced-answer terminal with no visible output or tool call. The recovery has no tools and reuses gathered search results. Malformed calls fail before refusal/truncation passthrough, and well-formed recognized refusal/truncation terminals pass through unchanged, including empty or partial answers. The extra generation may incur provider usage. + +OpenAI sidecar 429 replays run only when their backoff fits the remaining sidecar deadline; otherwise the original 429 remains the routing-health outcome rather than becoming a timeout. ## Scoped provider quota for Combo selection `src/providers/quota/report-cache.ts` publishes routing evidence only when a producer explicitly supplies its diff --git a/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts b/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts index 41a61c21023..b63411dc20f 100644 --- a/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts +++ b/tests/adapters/openai/openai-chat-dangling-toolcalls.test.ts @@ -12,6 +12,10 @@ const provider: OcxProviderConfig = { baseUrl: "https://example.test/v1", apiKey: "sk-test", authMode: "key", + // The wire role folds to `system` unless a destination is recorded as accepting + // `developer`; these cases are about where the barrier lands, so the destination + // declares the role rather than asserting the unrecorded default. + foldDeveloperRoleToSystem: false, }; interface ChatMsg { diff --git a/tests/responses/chat-inline-document-bytes.test.ts b/tests/responses/chat-inline-document-bytes.test.ts index dd88fa05788..1bbb5f152d8 100644 --- a/tests/responses/chat-inline-document-bytes.test.ts +++ b/tests/responses/chat-inline-document-bytes.test.ts @@ -175,12 +175,19 @@ describe("inline document bytes reach a wire that can hold them", () => { stream: false, options: {}, } as unknown as OcxParsedRequest; - const outbound = JSON.parse(createOpenAIChatAdapter(chatProvider).buildRequest(parsed).body) as { - messages: Array<{ role: string; content: unknown }>; - }; - expect(outbound.messages).toEqual([{ + const buildBody = (provider: OcxProviderConfig) => JSON.parse( + createOpenAIChatAdapter(provider).buildRequest(parsed).body, + ) as { messages: Array<{ role: string; content: unknown }> }; + // Carrying a document must not demote the turn to `user`; which role the slot shows on the + // wire is the destination's recorded answer, so the accepting destination keeps `developer` + // and the unrecorded one folds to `system` in place. + expect(buildBody({ ...chatProvider, foldDeveloperRoleToSystem: false }).messages).toEqual([{ role: "developer", content: [{ type: "file", file: { file_data: PDF_DATA_URL, filename: "spec" } }], }]); + expect(buildBody(chatProvider).messages).toEqual([{ + role: "system", + content: [{ type: "file", file: { file_data: PDF_DATA_URL, filename: "spec" } }], + }]); }); }); diff --git a/tests/web-search/web-search-sidecar-429.test.ts b/tests/web-search/web-search-sidecar-429.test.ts index 6956b855333..dcfe530122a 100644 --- a/tests/web-search/web-search-sidecar-429.test.ts +++ b/tests/web-search/web-search-sidecar-429.test.ts @@ -31,14 +31,20 @@ describe("web-search sidecar 429 replays", () => { return new Response("data: [DONE]\n\n", { headers: { "content-type": "text/event-stream" } }); } - function searchWith(fetchImpl: () => Promise) { + function searchWith( + fetchImpl: () => Promise, + timeoutMs = 30_000, + recordOutcome?: (outcome: number | "connect_error" | "connect_neutral" | "timeout") => void, + ) { globalThis.fetch = fetchImpl as unknown as typeof fetch; return runOpenAiWebSearch( "current docs", { type: "web_search" }, sidecarProvider(), new Headers({ authorization: "Bearer selected-token" }), - { model: "gpt-5.6-luna", reasoning: "low", timeoutMs: 30_000 }, + { model: "gpt-5.6-luna", reasoning: "low", timeoutMs }, + undefined, + recordOutcome, ); } @@ -72,4 +78,42 @@ describe("web-search sidecar 429 replays", () => { expect(calls).toBe(1); expect(outcome.error).toContain("429"); }); + + test("a Retry-After that cannot fit the sidecar deadline preserves the 429", async () => { + let calls = 0; + const recorded: Array = []; + const outcome = await searchWith(async () => { + calls += 1; + return new Response("slow down", { status: 429, headers: { "retry-after": "0.1" } }); + }, 50, value => recorded.push(value)); + expect(calls).toBe(1); + expect(outcome.error).toContain("429"); + expect(recorded).toEqual([429]); + }); + + test("a deadline expiring during pre-retry body cleanup preserves the 429", async () => { + // The never-settling body is a worse leak than the other mocks leave behind: restore + // fetch so a later file's shared search loop does not inherit a 1s release per retry. + const originalFetch = globalThis.fetch; + try { + let calls = 0; + const recorded: Array = []; + const outcome = await searchWith(async () => { + calls += 1; + // A cancel() that never settles makes the bounded 1s release run to its cap; the + // remaining deadline then cannot fit the backoff, so the wait ends mid-sleep. The + // observed 429 must survive that expiry instead of being recorded as a timeout. + const body = new ReadableStream({ + start: controller => controller.enqueue(new TextEncoder().encode("rate limited")), + cancel: () => new Promise(() => {}), + }); + return new Response(body, { status: 429, headers: { "retry-after": "1" } }); + }, 1_500, value => recorded.push(value)); + expect(calls).toBe(1); + expect(outcome.error).toContain("429"); + expect(recorded).toEqual([429]); + } finally { + globalThis.fetch = originalFetch; + } + }); });