Compare commits

..

1 Commits

Author SHA1 Message Date
Markus Hartung
4d2abc313e fix(sse): cap streaming headers-wait timeout to a client-realistic ceiling (#11526)
The fetch-start (headers-wait) phase inherited the flat, non-adaptive
FETCH_TIMEOUT_MS (default 600_000ms) with no ceiling comparable to a real
client's patience, unlike the body-phase readiness watchdog which already
adapts per payload shape. Codex's own hard client-abort window for a
stalled turn is ~120s, 5x shorter than the old default — so when an
upstream never returned any response at all (not even headers, e.g. a
stalled NVIDIA target behind a tool-heavy Responses->Chat translation),
OmniRoute kept the connection open on keepalives only, guaranteeing the
client gave up first with an opaque 499 instead of OmniRoute detecting and
failing the stall fast.

Adds resolveFetchStartTimeout() (open-sse/utils/fetchStartTimeoutPolicy.ts)
that caps the headers-wait timeout to 110s for STREAMING requests only,
leaving non-streaming requests on the existing flat default. Wired into
BaseExecutor.execute(). The existing TimeoutError classification path
(chatCore.ts) already maps this to a 504, so no change was needed there.

Regression test: tests/unit/issue-11526-repro.test.ts.
2026-08-26 13:13:51 -03:00
9 changed files with 138 additions and 127 deletions

View File

@@ -1 +0,0 @@
- **fix(combos):** the combo builder's precision-select, global-model-search, and manual-entry flows now serialize a model step's `model` string using the provider's already-computed routing-alias prefix (e.g. `oc/`) instead of rebuilding it from the raw canonical `providerId`, fixing the no-auth "OpenCode Free" provider (`opencode`) being routed to the unrelated paid "OpenCode Zen" provider (`opencode-zen`) because `opencode` doubles as a manual routing-prefix override ([#11433](https://github.com/diegosouzapw/OmniRoute/issues/11433)).

View File

@@ -0,0 +1 @@
- **fix(sse):** cap the upstream headers-wait phase for STREAMING requests to a client-realistic ceiling (110s, under Codex's own ~120s hard client-abort window) instead of the flat 10-minute `FETCH_TIMEOUT_MS` default — that default was 5x longer than the body-phase readiness watchdog's own adaptive bound, so a request whose upstream never returned any response at all (not even headers, e.g. a stalled NVIDIA target behind a tool-heavy Responses→Chat translation) kept the client connection alive on keepalives only, guaranteeing the client's own patience ran out first with an opaque 499 instead of OmniRoute detecting and failing the stall fast. Non-streaming requests are unaffected — they keep the existing flat default (`open-sse/utils/fetchStartTimeoutPolicy.ts`) (#11526)

View File

@@ -1,5 +1,6 @@
import { HTTP_STATUS, FETCH_TIMEOUT_MS } from "../config/constants.ts";
import { getRegistryEntry } from "../config/providerRegistry.ts";
import { resolveFetchStartTimeout } from "../utils/fetchStartTimeoutPolicy.ts";
import {
resolveAlternateFormat,
type AlternateFormat,
@@ -902,9 +903,24 @@ export class BaseExecutor {
clampNestedThinkingBudget(transformedBody, thinkingBudgetClampedMax);
}
// Timeout only covers response start; stream stalls are handled downstream.
// #11526: streaming requests cap the headers-wait phase to a client-realistic
// ceiling (see fetchStartTimeoutPolicy.ts) — non-streaming keeps the flat default.
// Declared outside the try/catch below so the catch's TIMEOUT log (on the
// error path) reports the same effective value the fetch actually used.
const fetchStartTimeoutPolicy = resolveFetchStartTimeout({
baseTimeoutMs: this.getTimeoutMs(),
stream,
});
const fetchStartTimeoutMs = fetchStartTimeoutPolicy.timeoutMs;
if (fetchStartTimeoutPolicy.capped) {
log?.debug?.(
"TIMEOUT",
`fetch-start timeout capped ${fetchStartTimeoutPolicy.baseTimeoutMs}ms -> ${fetchStartTimeoutMs}ms (streaming)`
);
}
try {
// Timeout only covers response start; stream stalls are handled downstream.
const fetchStartTimeoutMs = this.getTimeoutMs();
const fetchWithStartTimeout = async (requestUrl: string, requestOptions: RequestInit) => {
// GHSA-4f49: guard here (not only next to the first buildUrl) so retries
// and fallback URLs are validated too, before any bytes leave the host.
@@ -1713,7 +1729,7 @@ export class BaseExecutor {
// Distinguish timeout errors from other abort errors
const err = error instanceof Error ? error : new Error(String(error));
if (err.name === "TimeoutError") {
log?.warn?.("TIMEOUT", `Fetch timeout after ${this.getTimeoutMs()}ms on ${url}`);
log?.warn?.("TIMEOUT", `Fetch timeout after ${fetchStartTimeoutMs}ms on ${url}`);
}
lastError = err;
if (!skipUpstreamRetry && urlIndex + 1 < fallbackCount) {

View File

@@ -0,0 +1,52 @@
// #11526: the fetch-start (headers-wait) phase had no ceiling comparable to a
// real client's patience for STREAMING requests — it inherited the flat,
// non-adaptive FETCH_TIMEOUT_MS (default 600_000ms / 10 minutes), five times
// longer than Codex's own ~120s hard client-abort window. When an upstream
// never returns a response at all (not even headers), OmniRoute kept the
// connection open with nothing but keepalives, guaranteeing the client gave
// up first with an opaque 499 instead of OmniRoute detecting the stall and
// failing fast/over within a client-realistic window.
//
// This mirrors the adaptive philosophy of streamReadinessPolicy.ts's
// resolveStreamReadinessTimeout (which already protects the BODY phase, after
// headers arrive) but inverted: instead of bumping a small base timeout up for
// heavy payloads, it caps an oversized base timeout down for the HEADERS
// phase of streaming requests specifically. Non-streaming requests are left
// on the existing flat default — providers that are legitimately slow to
// accept a connection (but not streaming SSE) are unaffected.
export type FetchStartTimeoutPolicyInput = {
baseTimeoutMs: number;
/** Only streaming requests are capped — non-streaming keeps the flat default. */
stream?: boolean | null;
capMs?: number;
};
export type FetchStartTimeoutPolicyResult = {
timeoutMs: number;
baseTimeoutMs: number;
/** True when the base timeout was reduced by the streaming cap. */
capped: boolean;
};
// Codex's documented hard client-abort window for a stalled turn (nothing but
// keepalives in flight) is ~120s. Keep the cap safely under that so OmniRoute's
// own headers-phase watchdog always fires before the client gives up on its own.
export const CODEX_CLIENT_ABORT_MS = 120_000;
export const DEFAULT_FETCH_START_TIMEOUT_CAP_MS = 110_000;
export function resolveFetchStartTimeout(
input: FetchStartTimeoutPolicyInput
): FetchStartTimeoutPolicyResult {
const baseTimeoutMs = Math.max(0, Math.floor(input.baseTimeoutMs || 0));
if (baseTimeoutMs <= 0 || !input.stream) {
return { timeoutMs: baseTimeoutMs, baseTimeoutMs, capped: false };
}
const capMs = Math.max(0, Math.floor(input.capMs ?? DEFAULT_FETCH_START_TIMEOUT_CAP_MS));
if (capMs <= 0 || baseTimeoutMs <= capMs) {
return { timeoutMs: baseTimeoutMs, baseTimeoutMs, capped: false };
}
return { timeoutMs: capMs, baseTimeoutMs, capped: true };
}

View File

@@ -2177,9 +2177,6 @@ function ComboFormModal({ isOpen, combo, onClose, onSave, activeProviders, combo
builderConnectionId !== COMBO_BUILDER_AUTO_CONNECTION ? builderConnectionId : null,
connectionLabel: selectedBuilderConnection?.label || null,
allowedConnectionIds: builderEffectiveAllowedConnectionIds,
// #11433: use the already-corrected routing prefix (e.g. "oc" for
// OpenCode Free) instead of letting it default to the raw providerId.
modelPrefix: parseQualifiedModel(selectedBuilderModel.qualifiedModel)?.providerId,
})
: null;
const builderHasDuplicate =
@@ -2504,9 +2501,6 @@ function ComboFormModal({ isOpen, combo, onClose, onSave, activeProviders, combo
builderConnectionId !== COMBO_BUILDER_AUTO_CONNECTION ? builderConnectionId : null,
connectionLabel: selectedBuilderConnection?.label || null,
allowedConnectionIds: builderEffectiveAllowedConnectionIds,
// #11433: use the already-corrected routing prefix (e.g. "oc" for
// OpenCode Free) instead of letting it default to the raw providerId.
modelPrefix: parseQualifiedModel(selectedBuilderModel.qualifiedModel)?.providerId,
});
if (hasExactModelStepDuplicate(models, nextStep)) {

View File

@@ -83,7 +83,6 @@ export function buildPrecisionComboModelStep({
connectionLabel,
allowedConnectionIds = null,
weight = 0,
modelPrefix,
}: {
providerId: string;
modelId: string;
@@ -92,22 +91,9 @@ export function buildPrecisionComboModelStep({
/** #3266: account allowlist scoping round-robin to a subset of connections. */
allowedConnectionIds?: string[] | null;
weight?: number;
/**
* #11433: the routing-prefix segment to serialize into `model` (e.g. "oc"
* for the no-auth OpenCode Free provider), when it differs from the
* canonical `providerId`. Some canonical provider ids collide with an
* unrelated manual `ALIAS_TO_PROVIDER_ID` routing override (`opencode` →
* `opencode-zen`), so reconstructing `model` from the raw `providerId`
* alone can round-trip to the wrong provider on request routing. Falls
* back to `providerId` when omitted/blank. `step.providerId` always stays
* the canonical id regardless, so routing/duplicate-detection identity is
* unaffected.
*/
modelPrefix?: string | null;
}): ComboModelStep {
const normalizedProviderId = toTrimmedString(providerId) || "provider";
const normalizedModelId = toTrimmedString(modelId) || "model";
const normalizedModelPrefix = toTrimmedString(modelPrefix) || normalizedProviderId;
const normalizedConnectionId = toTrimmedString(connectionId);
const normalizedConnectionLabel = toTrimmedString(connectionLabel);
// A pinned single connection wins over an allowlist, so only carry the allowlist
@@ -124,7 +110,7 @@ export function buildPrecisionComboModelStep({
return {
kind: "model",
providerId: normalizedProviderId,
model: `${normalizedModelPrefix}/${normalizedModelId}`,
model: `${normalizedProviderId}/${normalizedModelId}`,
...(normalizedConnectionId ? { connectionId: normalizedConnectionId } : {}),
...(normalizedConnectionLabel ? { label: normalizedConnectionLabel } : {}),
...(normalizedAllowed.length > 0 ? { allowedConnectionIds: normalizedAllowed } : {}),
@@ -174,15 +160,10 @@ export function buildManualComboModelStep({
const providerId = resolveComboBuilderProviderId(parsed.providerId, providers);
if (!providerId) return null;
// #11433: preserve the user-typed prefix (e.g. "oc") as the routing prefix
// instead of letting buildPrecisionComboModelStep rebuild `model` from the
// resolved canonical providerId, which can collide with an unrelated
// manual alias override (e.g. "opencode" -> "opencode-zen").
return buildPrecisionComboModelStep({
providerId,
modelId: parsed.modelId,
weight,
modelPrefix: parsed.providerId,
});
}
@@ -244,7 +225,7 @@ type ComboBuilderGlobalProvider = {
displayName?: unknown;
connectionCount?: unknown;
connections?: unknown[];
models?: Array<{ id?: unknown; name?: unknown; qualifiedModel?: unknown }>;
models?: Array<{ id?: unknown; name?: unknown }>;
};
/**
@@ -271,18 +252,12 @@ export function buildGlobalModelList(
const modelId = toTrimmedString(model?.id);
if (!modelId) return;
const modelName = toTrimmedString(model?.name) || modelId;
// #11433: derive the routing prefix from the model's already-corrected
// `qualifiedModel` (e.g. "oc/<model>" for the OpenCode Free provider)
// instead of defaulting to the raw providerId, which can collide with
// an unrelated manual alias override.
const modelPrefix = parseQualifiedModel(model?.qualifiedModel)?.providerId || providerId;
const step = buildPrecisionComboModelStep({
providerId,
modelId,
connectionId: null,
connectionLabel: null,
allowedConnectionIds: [],
modelPrefix,
});
list.push({
providerId,

View File

@@ -42,12 +42,6 @@ test("buildPrecisionComboModelStep preserves provider/model/account triple", ()
});
test("buildManualComboModelStep resolves provider aliases and uses dynamic account", () => {
// #11433: `providerId` resolves to the canonical id ("codex") for
// duplicate-detection/routing identity, but the serialized `model` string
// now preserves the user-typed prefix ("cx/") verbatim instead of
// collapsing back to the canonical id — some canonical ids (e.g.
// "opencode") collide with an unrelated manual routing-alias override, so
// rebuilding `model` from the canonical id alone can silently misroute.
assert.deepEqual(
builderDraft.buildManualComboModelStep({
value: "cx/gpt-5.5",
@@ -56,7 +50,7 @@ test("buildManualComboModelStep resolves provider aliases and uses dynamic accou
{
kind: "model",
providerId: "codex",
model: "cx/gpt-5.5",
model: "codex/gpt-5.5",
weight: 0,
}
);

View File

@@ -1,83 +0,0 @@
import { test } from "node:test";
import assert from "node:assert/strict";
import {
buildPrecisionComboModelStep,
buildGlobalModelList,
buildManualComboModelStep,
} from "../../src/lib/combos/builderDraft.ts";
import { resolveProviderAlias, parseModel } from "../../open-sse/services/model.ts";
// Issue #11433: the combo builder's precision-select path builds a step's
// `model` string as `${providerId}/${modelId}` using the CANONICAL provider id.
// For the no-auth "opencode" (OpenCode Free) provider this produces
// `model: "opencode/<modelId>"`, but `opencode` is ALSO a manual routing-prefix
// override (`ALIAS_TO_PROVIDER_ID["opencode"] = "opencode-zen"`) intended only
// for user-typed `opencode/` prefixes referring to the OpenCode Zen (api-key)
// tier. Parsing the step's own `model` string therefore resolves to a
// DIFFERENT provider than the one recorded in `step.providerId`.
test('sanity: resolveProviderAlias("opencode") is the manual override causing the collision', () => {
// Documents the root cause directly: the manual alias override in
// open-sse/services/model.ts unconditionally rewrites "opencode" to
// "opencode-zen", even though "opencode" is also a registered canonical
// provider id (src/shared/constants/providers/noauth.ts).
assert.equal(resolveProviderAlias("opencode"), "opencode-zen");
});
test("issue #11433 fix: buildPrecisionComboModelStep honors an explicit modelPrefix override", () => {
// The combo builder call sites now thread through the already-computed
// routing-alias prefix (e.g. "oc") instead of letting the step default to
// the raw providerId, so the serialized `model` field round-trips to the
// correct provider.
const step = buildPrecisionComboModelStep({
providerId: "opencode",
modelId: "big-pickle",
modelPrefix: "oc",
});
assert.equal(step.providerId, "opencode");
assert.equal(step.model, "oc/big-pickle");
const parsed = parseModel(step.model);
assert.equal(parsed.provider, step.providerId);
});
test("issue #11433 fix: buildGlobalModelList derives modelPrefix from qualifiedModel for the no-auth OpenCode Free provider", () => {
// Mirrors what src/lib/combos/builderOptions.ts::rewriteQualifiedModelPrefix
// produces for the no-auth "opencode" provider entry: `qualifiedModel` is
// already rewritten to the "oc/" alias prefix, but (pre-fix)
// buildGlobalModelList ignored it and rebuilt `model` from the raw
// providerId, producing "opencode/big-pickle" which parses back to the
// wrong provider ("opencode-zen").
const [entry] = buildGlobalModelList([
{
providerId: "opencode",
displayName: "OpenCode Free",
connectionCount: 0,
connections: [],
models: [{ id: "big-pickle", name: "Big Pickle", qualifiedModel: "oc/big-pickle" }],
},
]);
assert.equal(entry.step.providerId, "opencode");
assert.equal(entry.step.model, "oc/big-pickle");
assert.equal(parseModel(entry.step.model).provider, entry.step.providerId);
});
test("issue #11433 fix: buildManualComboModelStep preserves a user-typed oc/<model> prefix", () => {
// buildManualComboModelStep resolves the typed alias ("oc") back to the
// canonical providerId ("opencode") before building the step. Pre-fix, it
// then handed that canonical id straight to buildPrecisionComboModelStep,
// which rebuilt `model` from it and collapsed "oc/<model>" back down to
// "opencode/<model>" — reproducing the same collision for manual entry.
const step = buildManualComboModelStep({
value: "oc/big-pickle",
providers: [{ providerId: "opencode", alias: "oc" }],
});
assert.ok(step);
assert.equal(step?.providerId, "opencode");
assert.equal(step?.model, "oc/big-pickle");
assert.equal(parseModel(step!.model).provider, step!.providerId);
});

View File

@@ -0,0 +1,63 @@
import test from "node:test";
import assert from "node:assert/strict";
import { resolveStreamReadinessTimeout } from "../../open-sse/utils/streamReadinessPolicy.ts";
import {
resolveFetchStartTimeout,
CODEX_CLIENT_ABORT_MS,
} from "../../open-sse/utils/fetchStartTimeoutPolicy.ts";
import { getUpstreamTimeoutConfig } from "../../src/shared/utils/runtimeTimeouts.ts";
function items(count: number): Array<{ role: string; content: string }> {
return Array.from({ length: count }, (_, index) => ({
role: "user",
content: `message ${index}`,
}));
}
function tools(count: number): Array<{ type: string; name: string }> {
return Array.from({ length: count }, (_, index) => ({ type: "function", name: `tool_${index}` }));
}
test("issue #11526: body-phase readiness watchdog stays comfortably under Codex's ~120s patience for the reported tool-heavy payload shape", () => {
const result = resolveStreamReadinessTimeout({
baseTimeoutMs: 80_000,
provider: "nvidia",
model: "some-nvidia-model",
body: { input: items(68), tools: tools(16) },
});
assert.ok(
result.timeoutMs < CODEX_CLIENT_ABORT_MS,
`body-phase watchdog (${result.timeoutMs}ms) must stay under Codex's ~120s patience`
);
});
test("issue #11526 (fixed): headers-phase watchdog for STREAMING requests is bounded under Codex's ~120s patience", () => {
const { fetchTimeoutMs } = getUpstreamTimeoutConfig({});
// Default FETCH_TIMEOUT_MS (600000ms) is still the flat non-streaming baseline —
// the fix does not touch that default, it caps how much of it a STREAMING
// request's headers-wait phase is allowed to consume.
assert.equal(fetchTimeoutMs, 600_000);
const streaming = resolveFetchStartTimeout({ baseTimeoutMs: fetchTimeoutMs, stream: true });
assert.ok(
streaming.timeoutMs <= CODEX_CLIENT_ABORT_MS,
`headers-phase watchdog for streaming requests (${streaming.timeoutMs}ms) must not exceed a realistic client abort window (${CODEX_CLIENT_ABORT_MS}ms)`
);
assert.ok(streaming.capped, "expected the oversized default to be capped for streaming requests");
});
test("issue #11526 scope guard: non-streaming requests keep the flat FETCH_TIMEOUT_MS default", () => {
const { fetchTimeoutMs } = getUpstreamTimeoutConfig({});
const nonStreaming = resolveFetchStartTimeout({ baseTimeoutMs: fetchTimeoutMs, stream: false });
assert.equal(nonStreaming.timeoutMs, fetchTimeoutMs);
assert.equal(nonStreaming.capped, false);
});
test("issue #11526 scope guard: a base timeout already under the cap is left untouched for streaming requests", () => {
const result = resolveFetchStartTimeout({ baseTimeoutMs: 30_000, stream: true });
assert.equal(result.timeoutMs, 30_000);
assert.equal(result.capped, false);
});