From edfcb8be17720f8e6e28b6db882591bc5a928f78 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:14 +0700 Subject: [PATCH 01/19] fix(test): resolve the WebDAV handler path with fileURLToPath (#13196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct: `URL.pathname` is a URL path, so on Windows it yields `/C:/...` and `path.resolve` produces the doubled `C:\C:\` prefix. `fileURLToPath` is the right decoder and also un-escapes `%20`. All 37 WebDAV tests green here. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- changelog.d/fixes/webdav-test-windows-path.md | 1 + tests/unit/webdav-server-3485.test.ts | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changelog.d/fixes/webdav-test-windows-path.md diff --git a/changelog.d/fixes/webdav-test-windows-path.md b/changelog.d/fixes/webdav-test-windows-path.md new file mode 100644 index 0000000000..2ba7400611 --- /dev/null +++ b/changelog.d/fixes/webdav-test-windows-path.md @@ -0,0 +1 @@ +- **fix(test):** resolve the WebDAV handler path with `fileURLToPath` so the suite's 37 WebDAV tests run on Windows instead of failing with a doubled `C:\C:\` drive prefix diff --git a/tests/unit/webdav-server-3485.test.ts b/tests/unit/webdav-server-3485.test.ts index f30d5824e3..291d25dc03 100644 --- a/tests/unit/webdav-server-3485.test.ts +++ b/tests/unit/webdav-server-3485.test.ts @@ -30,14 +30,19 @@ import path from "node:path"; import http from "node:http"; import { EventEmitter } from "node:events"; import { createCipheriv, randomBytes, scryptSync } from "node:crypto"; -import { pathToFileURL } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; // ───────────────────────────────────────────────────────────────────────────── // Helpers // ───────────────────────────────────────────────────────────────────────────── +// `URL.pathname` is a URL path, not an OS path: on Windows it yields +// "/C:/..." — a leading slash before the drive letter. `path.resolve` does not +// treat that as absolute, so it prepends the CWD and produces "C:\C:\...", +// which fails to import. `fileURLToPath` decodes to a real OS path on every +// platform (it also un-escapes %20 in paths containing spaces). const HANDLER_PATH = path.resolve( - path.dirname(new URL(import.meta.url).pathname), + path.dirname(fileURLToPath(import.meta.url)), "../../scripts/dev/webdav-handler.mjs" ); From 178d25250a5361f94c0867159c69ae0d58d9895c Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:18 +0700 Subject: [PATCH 02/19] fix(test): cap local unit-test concurrency at 4 to avoid exhausting commit charge (#13187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligns the two hand-typed local scripts with `test:unit:ci`, which already ran at concurrency 4; `--test-force-exit` was likewise the one flag `test` was missing. CI scripts are untouched. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- changelog.d/fixes/local-test-concurrency.md | 1 + package.json | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) create mode 100644 changelog.d/fixes/local-test-concurrency.md diff --git a/changelog.d/fixes/local-test-concurrency.md b/changelog.d/fixes/local-test-concurrency.md new file mode 100644 index 0000000000..70a46e3984 --- /dev/null +++ b/changelog.d/fixes/local-test-concurrency.md @@ -0,0 +1 @@ +- **fix(test):** run the local `test` and `test:unit` scripts at concurrency 4 so a full-suite run no longer exhausts the machine's commit charge and kills unrelated processes diff --git a/package.json b/package.json index 2d5ae328ae..ccebb40b09 100644 --- a/package.json +++ b/package.json @@ -125,8 +125,8 @@ "electron:build:mac": "npm run build && cd electron && npm run build:mac", "electron:build:linux": "npm run build && cd electron && npm run build:linux", "electron:smoke:packaged": "node scripts/dev/smoke-electron-packaged.mjs", - "test": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-concurrency=20 tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-concurrency=20 \"tests/unit/dashboard/**/*.test.ts\"", - "test:unit": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=20 tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=20 \"tests/unit/dashboard/**/*.test.ts\" && npm run test:unit:serial", + "test": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 \"tests/unit/dashboard/**/*.test.ts\"", + "test:unit": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 \"tests/unit/dashboard/**/*.test.ts\" && npm run test:unit:serial", "test:unit:ci": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 \"tests/unit/dashboard/**/*.test.ts\" && npm run test:unit:serial", "test:unit:ci:shard": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 --test-shard=$TEST_SHARD tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=4 --test-shard=$TEST_SHARD \"tests/unit/dashboard/**/*.test.ts\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-concurrency=1 --test-shard=$TEST_SHARD \"tests/unit/serial/**/*.test.ts\"", "test:unit:fast": "cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-isolation=none tests/unit/*.test.ts \"tests/unit/{api,auth,authz,build,cli,cli-helper,combo,compression,correctness,cors,db,db-adapters,docs,gamification,guardrails,lib,mcp,memory,runtime,security,services,settings,shared,translator,ui,usage}/**/*.test.ts\" \"tests/unit/**/*.test.mjs\" && cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=8192 --import tsx --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test --test-force-exit --test-isolation=none \"tests/unit/dashboard/**/*.test.ts\" && npm run test:unit:serial", From a1b9b02d5d2ac41c8f8646761c15104de15f0edd Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:23 +0700 Subject: [PATCH 03/19] fix(telegram): authenticate webhook deliveries with the Telegram secret token (#13175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real abuse vector: the bot-webhook branch reached `proxyChat()` — which mints an API key and spends upstream quota — with nothing proving the caller was Telegram. Fail-closed 503 when the secret is unset is the right default. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .env.example | 5 ++ .../fixes/13172-telegram-webhook-secret.md | 1 + docs/reference/ENVIRONMENT.md | 1 + src/app/api/telegram/update/route.ts | 46 +++++++++++- src/lib/telegram/botApi.ts | 17 ++++- src/lib/telegram/config.ts | 24 +++++++ .../telegram-webhook-secret-13172.test.ts | 72 +++++++++++++++++++ 7 files changed, 162 insertions(+), 4 deletions(-) create mode 100644 changelog.d/fixes/13172-telegram-webhook-secret.md create mode 100644 tests/unit/telegram-webhook-secret-13172.test.ts diff --git a/.env.example b/.env.example index 67097b7104..f259d4d854 100644 --- a/.env.example +++ b/.env.example @@ -3070,6 +3070,11 @@ QUOTA_STORE_DRIVER=sqlite # Telegram Mini App bridge. The update endpoint remains disabled while the bot # token is unset. Used by: src/lib/telegram/* and src/app/api/telegram/update/route.ts. # TELEGRAM_BOT_TOKEN= +# Shared secret registered with setWebhook and echoed back by Telegram as the +# X-Telegram-Bot-Api-Secret-Token header. REQUIRED for the webhook path: without +# it the webhook is rejected with 503, because an unauthenticated update lets any +# caller mint API keys and spend upstream quota. The Mini App path does not use it. +# TELEGRAM_WEBHOOK_SECRET= # TELEGRAM_DEFAULT_MODEL=auto/chat # TELEGRAM_BOT_API_BASE=https://api.telegram.org # TELEGRAM_WEBHOOK_TIMEOUT_MS=60000 diff --git a/changelog.d/fixes/13172-telegram-webhook-secret.md b/changelog.d/fixes/13172-telegram-webhook-secret.md new file mode 100644 index 0000000000..49f14b55fb --- /dev/null +++ b/changelog.d/fixes/13172-telegram-webhook-secret.md @@ -0,0 +1 @@ +- **fix(telegram):** authenticate webhook deliveries with Telegram's `secret_token` so an unauthenticated caller can no longer mint API keys or spend upstream quota ([#13172](https://github.com/diegosouzapw/OmniRoute/issues/13172)) diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index f357f73584..13b70dd4e3 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -1623,6 +1623,7 @@ These settings were introduced after the previous environment-contract snapshot. | `ADOBE_FIREFLY_CHROME_HEADLESS` | `0` | `open-sse/services/adobeFireflyBrowserLogin.ts` | Debug-only true-headless mode; Adobe colligo normally rejects the resulting risk session. | | `CHROME_PATH` | auto-detect | `open-sse/executors/cloudflare-playground.ts`, `open-sse/executors/chatgpt-web-codex.ts` | Optional absolute Chrome executable used by the browser-driven executors when platform auto-detection is insufficient. | | `TELEGRAM_BOT_TOKEN` | _(unset)_ | `src/lib/telegram/config.ts` | BotFather token that enables the inbound webhook and signs Mini App `initData`. | +| `TELEGRAM_WEBHOOK_SECRET` | _(unset)_ | `src/lib/telegram/config.ts` | Shared secret registered via `setWebhook` and verified against the `X-Telegram-Bot-Api-Secret-Token` header on every webhook delivery. Required for the webhook path; unset means webhook deliveries are refused with 503. | | `TELEGRAM_DEFAULT_MODEL` | `auto/chat` | `src/lib/telegram/chatProxy.ts` | Model used for Telegram chat replies. | | `TELEGRAM_BOT_API_BASE` | `https://api.telegram.org` | `src/lib/telegram/config.ts` | Bot API base URL override for proxies or self-hosted Bot API servers. | | `TELEGRAM_WEBHOOK_TIMEOUT_MS` | `60000` | `src/lib/telegram/config.ts` | Timeout in milliseconds for outbound Bot API calls. | diff --git a/src/app/api/telegram/update/route.ts b/src/app/api/telegram/update/route.ts index 1fd3582da9..c2194a5895 100644 --- a/src/app/api/telegram/update/route.ts +++ b/src/app/api/telegram/update/route.ts @@ -13,12 +13,18 @@ * 3. Handles /start (returns the Mini App deep link) and everything else * as a chat prompt proxied through the OmniRoute pipeline. */ +import { timingSafeEqual } from "node:crypto"; import { NextResponse } from "next/server"; import { z } from "zod"; import { validateBody, isValidationFailure } from "@/shared/validation/helpers"; import type { TelegramUpdate } from "@/lib/telegram/botApi"; import { extractChatMessage, sendTelegramMessage } from "@/lib/telegram/botApi"; -import { getTelegramBotToken, isTelegramEnabled } from "@/lib/telegram/config"; +import { + getTelegramBotToken, + getTelegramWebhookSecret, + isTelegramEnabled, + isTelegramWebhookSecretConfigured, +} from "@/lib/telegram/config"; import { verifyInitData, parseInitData } from "@/lib/telegram/initData"; import { proxyChat } from "@/lib/telegram/chatProxy"; import { formatTelegramGatewayError } from "@/lib/telegram/errorMessage"; @@ -33,7 +39,12 @@ import { resolveOmniRouteBaseUrl } from "@/shared/utils/resolveOmniRouteBaseUrl" const telegramBodySchema = z .object({ initData: z.string().optional(), - message: z.string().optional(), + // `message` is a STRING on the Mini App path ({ initData, message }) and an + // OBJECT on the webhook path (a Telegram update). Constraining it to a + // string rejected every real webhook delivery with 400 before any auth or + // routing ran, so accept either shape here and let each branch validate the + // shape it actually needs. + message: z.union([z.string(), z.record(z.string(), z.unknown())]).optional(), update_id: z.number().optional(), // allow unknown update fields }) @@ -103,6 +114,21 @@ export async function POST(request: Request) { } // ── Bot webhook path: TelegramUpdate ───────────────────────────────────── + // Unlike the Mini App branch above (which verifies the initData HMAC), a + // webhook body carries no proof of origin: `chat.id` is attacker-chosen and + // reaches proxyChat(), which mints a real API key and spends upstream quota. + // Telegram's `secret_token` echo is the only authentication available here. + if (!isTelegramWebhookSecretConfigured()) { + return NextResponse.json( + { ok: false, error: "Telegram webhook secret not configured" }, + { status: 503 } + ); + } + const presentedSecret = request.headers.get("x-telegram-bot-api-secret-token") || ""; + if (!webhookSecretMatches(presentedSecret, getTelegramWebhookSecret())) { + return NextResponse.json({ ok: false, error: "Unauthorized" }, { status: 401 }); + } + const update = body as unknown as TelegramUpdate; const chat = extractChatMessage(update); if (!chat) { @@ -117,6 +143,22 @@ export async function POST(request: Request) { return NextResponse.json({ ok: true }); } +/** + * Constant-time comparison of the presented webhook secret against the + * configured one. A plain `===` short-circuits on the first differing byte and + * leaks the shared-prefix length through response timing; `timingSafeEqual` + * does not. It requires equal-length buffers, so a length mismatch is rejected + * up front (the length itself is not secret). + * + * Exported as a test seam only — not part of the route contract. + */ +export function webhookSecretMatches(presented: string, expected: string): boolean { + const a = Buffer.from(presented); + const b = Buffer.from(expected); + if (a.length !== b.length) return false; + return timingSafeEqual(a, b); +} + async function handleAndReply(chatId: number, text: string, messageId?: number): Promise { try { const trimmed = text.trim(); diff --git a/src/lib/telegram/botApi.ts b/src/lib/telegram/botApi.ts index 4bdc50071a..c1a0e5e048 100644 --- a/src/lib/telegram/botApi.ts +++ b/src/lib/telegram/botApi.ts @@ -5,7 +5,12 @@ * replies and setWebhook for webhook registration. Streaming is emulated * by the caller via progressive edits (sendMessage / editMessageText). */ -import { getTelegramBotApiBase, getTelegramBotToken, getTelegramWebhookTimeoutMs } from "./config"; +import { + getTelegramBotApiBase, + getTelegramBotToken, + getTelegramWebhookTimeoutMs, + getTelegramWebhookSecret, +} from "./config"; export interface TelegramSendMessageParams { chat_id: number | string; @@ -92,7 +97,15 @@ export async function setTelegramWebhook( opts: { dropPending?: boolean } = {} ): Promise<{ url: string; pending_update_count?: number }> { if (url) { - return botFetch("setWebhook", { url, drop_pending_updates: opts.dropPending ?? true }); + // Register the shared secret so Telegram echoes it back as + // X-Telegram-Bot-Api-Secret-Token on every delivery; the webhook route + // rejects deliveries that do not carry it (#13172). + const secret = getTelegramWebhookSecret(); + return botFetch("setWebhook", { + url, + drop_pending_updates: opts.dropPending ?? true, + ...(secret ? { secret_token: secret } : {}), + }); } return botFetch("deleteWebhook", { drop_pending_updates: opts.dropPending ?? true }); } diff --git a/src/lib/telegram/config.ts b/src/lib/telegram/config.ts index 421739ef5e..817641e9be 100644 --- a/src/lib/telegram/config.ts +++ b/src/lib/telegram/config.ts @@ -25,6 +25,30 @@ export function getTelegramWebhookTimeoutMs(): number { return Number.isInteger(parsed) && parsed > 0 ? parsed : DEFAULT_WEBHOOK_TIMEOUT_MS; } +/** + * Shared secret for authenticating Telegram webhook deliveries. + * + * Telegram echoes the `secret_token` passed to `setWebhook` back on every + * delivery in the `X-Telegram-Bot-Api-Secret-Token` header, which is the only + * way to prove a webhook POST actually came from Telegram. Kept in the + * environment alongside the bot token so it is never stored in the DB. + */ +export function getTelegramWebhookSecret(): string { + return process.env.TELEGRAM_WEBHOOK_SECRET || ""; +} + +/** + * Whether webhook deliveries are authenticated. + * + * When no secret is configured the webhook path is rejected outright rather + * than served unauthenticated: an open path mints API keys and spends upstream + * quota for any caller (see #13172). The Mini App path is unaffected — it + * authenticates with the initData HMAC and does not use this secret. + */ +export function isTelegramWebhookSecretConfigured(): boolean { + return getTelegramWebhookSecret().length > 0; +} + export function getTelegramBotApiBase(): string { return process.env.TELEGRAM_BOT_API_BASE || "https://api.telegram.org"; } diff --git a/tests/unit/telegram-webhook-secret-13172.test.ts b/tests/unit/telegram-webhook-secret-13172.test.ts new file mode 100644 index 0000000000..f525ee745a --- /dev/null +++ b/tests/unit/telegram-webhook-secret-13172.test.ts @@ -0,0 +1,72 @@ +/** + * Regression test for #13172: the Telegram webhook path must authenticate. + * + * Telegram echoes the `secret_token` given to `setWebhook` back on every + * delivery as `X-Telegram-Bot-Api-Secret-Token`. Without checking it, any + * caller can POST a synthetic update with an arbitrary `chat.id`, which reaches + * proxyChat() and mints a real API key plus upstream spend. + * + * The Mini App branch authenticates separately (initData HMAC) and must keep + * working without a webhook secret. + */ +import { describe, test, before, after } from "node:test"; +import assert from "node:assert/strict"; + +const BOT_TOKEN = "123456:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; +const SECRET = "s3cret-webhook-token"; + +let POST: (req: Request) => Promise; +let webhookSecretMatches: (a: string, b: string) => boolean; +const proxied: number[] = []; + +before(async () => { + process.env.TELEGRAM_BOT_TOKEN = BOT_TOKEN; + process.env.TELEGRAM_WEBHOOK_SECRET = SECRET; + + const mod = await import("../../src/app/api/telegram/update/route.ts"); + POST = mod.POST as typeof POST; + webhookSecretMatches = mod.webhookSecretMatches as typeof webhookSecretMatches; +}); + +after(() => { + delete process.env.TELEGRAM_WEBHOOK_SECRET; +}); + +function webhookRequest(headers: Record = {}): Request { + return new Request("https://example.test/api/telegram/update", { + method: "POST", + headers: { "content-type": "application/json", ...headers }, + // A realistic Telegram update: `message` is an object here, whereas the + // Mini App path sends it as a string. Both shapes must reach their branch. + body: JSON.stringify({ + update_id: 1, + message: { chat: { id: 999 }, text: "hi", message_id: 5 }, + }), + }); +} + +describe("telegram webhook authentication (#13172)", () => { + test("rejects a delivery with no secret header", async () => { + const res = await POST(webhookRequest()); + assert.equal(res.status, 401, "unauthenticated webhook must be rejected"); + assert.deepEqual(proxied, [], "no chat should be proxied"); + }); + + test("rejects a delivery with a wrong secret", async () => { + const res = await POST( + webhookRequest({ "x-telegram-bot-api-secret-token": "wrong-token-value" }) + ); + assert.equal(res.status, 401, "a mismatched secret must be rejected"); + }); + + test("accepts a delivery carrying the configured secret", async () => { + const res = await POST(webhookRequest({ "x-telegram-bot-api-secret-token": SECRET })); + assert.equal(res.status, 200, "a correctly authenticated delivery must be accepted"); + }); + + test("comparison is length-safe and value-correct", () => { + assert.equal(webhookSecretMatches(SECRET, SECRET), true); + assert.equal(webhookSecretMatches("short", SECRET), false, "length mismatch must not throw"); + assert.equal(webhookSecretMatches("", ""), true, "equal empties compare equal"); + }); +}); From 658153c7b02808c994c08974b21f7a373f7b4a47 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:26 +0700 Subject: [PATCH 04/19] fix(stream): release the upstream body when the JSON-to-SSE sniff times out (#13171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sniff loop abandoned the upstream reader when `withBodyTimeout()` rejected. The `handedOff` flag correctly spares the two success paths from cancellation; the second test guards that direction. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../fixes/13169-jsonbody-sniff-reader-leak.md | 1 + open-sse/handlers/chatCore/jsonBodyToSse.ts | 61 ++++++----- .../jsonbody-sniff-reader-leak-13169.test.ts | 101 ++++++++++++++++++ 3 files changed, 139 insertions(+), 24 deletions(-) create mode 100644 changelog.d/fixes/13169-jsonbody-sniff-reader-leak.md create mode 100644 tests/unit/jsonbody-sniff-reader-leak-13169.test.ts diff --git a/changelog.d/fixes/13169-jsonbody-sniff-reader-leak.md b/changelog.d/fixes/13169-jsonbody-sniff-reader-leak.md new file mode 100644 index 0000000000..b0ad5629c3 --- /dev/null +++ b/changelog.d/fixes/13169-jsonbody-sniff-reader-leak.md @@ -0,0 +1 @@ +- **fix(stream):** cancel the upstream response body when the JSON-to-SSE sniff unwinds on a body timeout, so a stalled upstream no longer pins the connection ([#13169](https://github.com/diegosouzapw/OmniRoute/issues/13169)) diff --git a/open-sse/handlers/chatCore/jsonBodyToSse.ts b/open-sse/handlers/chatCore/jsonBodyToSse.ts index 66c9688807..97bf840ee9 100644 --- a/open-sse/handlers/chatCore/jsonBodyToSse.ts +++ b/open-sse/handlers/chatCore/jsonBodyToSse.ts @@ -88,33 +88,46 @@ async function sniffJsonBodyForSse( let sniffed = ""; let sniffedBytes = 0; const maxSniffBytes = 4096; - while (sniffedBytes < maxSniffBytes) { - const chunk = await deps.withBodyTimeout>(reader.read()); - if (chunk.done || !chunk.value) break; - bufferedChunks.push(chunk.value); - sniffedBytes += chunk.value.byteLength; - sniffed += decoder.decode(chunk.value, { stream: true }); + // The two success paths below hand this still-open reader to + // prependBufferedChunks(), so the reader must NOT be cancelled on the happy + // path. Any other unwind (notably a withBodyTimeout rejection on a stalled + // upstream) would otherwise abandon the body with no cancellation, pinning + // the connection for the lifetime of the socket. + let handedOff = false; + try { + while (sniffedBytes < maxSniffBytes) { + const chunk = await deps.withBodyTimeout>(reader.read()); + if (chunk.done || !chunk.value) break; + bufferedChunks.push(chunk.value); + sniffedBytes += chunk.value.byteLength; + sniffed += decoder.decode(chunk.value, { stream: true }); - if (classifyBodyPrefix(sniffed) === "sse") { - const rebuiltHeaders = new Headers(providerResponse.headers); - rebuiltHeaders.delete("content-length"); - rebuiltHeaders.set("content-type", "text/event-stream"); - ctx.log?.debug?.( - "STREAM", - `Upstream returned SSE bytes with application/json content-type — preserving streaming body (${ctx.provider}/${ctx.model})` - ); - return { - sseResponse: new Response(prependBufferedChunks(bufferedChunks, reader), { - status: providerResponse.status, - statusText: providerResponse.statusText, - headers: rebuiltHeaders, - }), - jsonBody: new Response(null), - }; + if (classifyBodyPrefix(sniffed) === "sse") { + const rebuiltHeaders = new Headers(providerResponse.headers); + rebuiltHeaders.delete("content-length"); + rebuiltHeaders.set("content-type", "text/event-stream"); + ctx.log?.debug?.( + "STREAM", + `Upstream returned SSE bytes with application/json content-type — preserving streaming body (${ctx.provider}/${ctx.model})` + ); + handedOff = true; + return { + sseResponse: new Response(prependBufferedChunks(bufferedChunks, reader), { + status: providerResponse.status, + statusText: providerResponse.statusText, + headers: rebuiltHeaders, + }), + jsonBody: new Response(null), + }; + } } - } - return { jsonBody: new Response(prependBufferedChunks(bufferedChunks, reader)) }; + handedOff = true; + return { jsonBody: new Response(prependBufferedChunks(bufferedChunks, reader)) }; + } finally { + // Cancellation is best-effort: the body may already be errored or closed. + if (!handedOff) void reader.cancel().catch(() => {}); + } } export async function maybeConvertJsonBodyToSse( diff --git a/tests/unit/jsonbody-sniff-reader-leak-13169.test.ts b/tests/unit/jsonbody-sniff-reader-leak-13169.test.ts new file mode 100644 index 0000000000..7518f4087c --- /dev/null +++ b/tests/unit/jsonbody-sniff-reader-leak-13169.test.ts @@ -0,0 +1,101 @@ +/** + * Regression test for #13169: the JSON-to-SSE sniff must release the upstream + * body when it unwinds abnormally. + * + * `sniffJsonBodyForSse()` reads the upstream body under `withBodyTimeout()`. + * On a stalled upstream that rejects, an un-cancelled reader keeps the + * connection pinned. The upstream stream declares an explicit `cancel()` hook, + * so the assertions observe real cancellation rather than an incidental close. + */ +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; + +import { maybeConvertJsonBodyToSse } from "../../open-sse/handlers/chatCore/jsonBodyToSse.ts"; + +type Deps = Parameters[2]; + +/** Upstream that serves `first` and then stalls forever, tracking cancellation. */ +function stallingUpstream(first: string) { + const state = { cancelled: false }; + let pulls = 0; + const body = new ReadableStream({ + pull(controller) { + pulls += 1; + if (pulls === 1) { + controller.enqueue(new TextEncoder().encode(first)); + return; + } + return new Promise(() => {}); + }, + cancel() { + state.cancelled = true; + }, + }); + return { body, state }; +} + +function timeoutDeps(ms: number): Deps { + return { + withBodyTimeout: ((p: Promise) => + Promise.race([ + p, + new Promise((_, reject) => + setTimeout(() => { + const err = new Error(`Response body read timeout after ${ms}ms`); + err.name = "BodyTimeoutError"; + reject(err); + }, ms) + ), + ])) as Deps["withBodyTimeout"], + synthesizeOpenAiSseFromJson: () => null, + } as Deps; +} + +describe("jsonBodyToSse upstream body release (#13169)", () => { + test("cancels the upstream body when the sniff times out", async () => { + const { body, state } = stallingUpstream('{"choices":['); + const providerResponse = new Response(body, { + status: 200, + headers: { "content-type": "application/json" }, + }); + + await assert.rejects( + () => + maybeConvertJsonBodyToSse(providerResponse, { provider: "p", model: "m" }, timeoutDeps(50)), + (err: Error) => err.name === "BodyTimeoutError" + ); + + // Let any async cancellation settle before observing. + await new Promise((r) => setTimeout(r, 50)); + + assert.equal(state.cancelled, true, "upstream body should be cancelled after the timeout"); + }); + + test("does NOT cancel the body on the success path", async () => { + // A complete SSE-looking body: the sniff hands the reader onward, so + // cancelling here would truncate a healthy stream. + const state = { cancelled: false }; + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("data: {}\n\n")); + controller.close(); + }, + cancel() { + state.cancelled = true; + }, + }); + const providerResponse = new Response(body, { + status: 200, + headers: { "content-type": "application/json" }, + }); + + const out = await maybeConvertJsonBodyToSse( + providerResponse, + { provider: "p", model: "m" }, + timeoutDeps(5000) + ); + + assert.ok(out instanceof Response, "sniff should return a Response"); + assert.equal(state.cancelled, false, "a healthy body must not be cancelled by the sniff"); + }); +}); From 30c96d43a50f66c91a3eb70537d3a8b80a63dcfa Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:30 +0700 Subject: [PATCH 05/19] fix(telegram): bound the per-user API key cache (#13166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resolveUserApiKey()` keyed an uncapped Map on an id taken straight from the webhook body. The LRU's recency test is what keeps this from regressing into a clear-when-full cache. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../13165-telegram-keycache-unbounded.md | 1 + src/lib/telegram/chatProxy.ts | 26 +++- .../telegram-keycache-bounded-13165.test.ts | 123 ++++++++++++++++++ 3 files changed, 147 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fixes/13165-telegram-keycache-unbounded.md create mode 100644 tests/unit/telegram-keycache-bounded-13165.test.ts diff --git a/changelog.d/fixes/13165-telegram-keycache-unbounded.md b/changelog.d/fixes/13165-telegram-keycache-unbounded.md new file mode 100644 index 0000000000..2672fb93be --- /dev/null +++ b/changelog.d/fixes/13165-telegram-keycache-unbounded.md @@ -0,0 +1 @@ +- **fix(telegram):** bound the per-user API key cache in the Telegram chat proxy so a burst of distinct chat ids can no longer grow the process heap without limit ([#13165](https://github.com/diegosouzapw/OmniRoute/issues/13165)) diff --git a/src/lib/telegram/chatProxy.ts b/src/lib/telegram/chatProxy.ts index d2b136954e..724ccd2b00 100644 --- a/src/lib/telegram/chatProxy.ts +++ b/src/lib/telegram/chatProxy.ts @@ -21,11 +21,31 @@ const DEFAULT_MODEL = process.env.TELEGRAM_DEFAULT_MODEL || "auto/chat"; * Resolve (and lazily mint) an OmniRoute API key for a Telegram user. * Returns the plaintext key value, cached per user id. */ +// Bounded LRU. The webhook path passes a caller-supplied chat id, so the key +// space is not limited to the real user population and an uncapped Map would +// grow for the lifetime of the process. Insertion order is the recency order: +// a hit re-inserts, and the oldest entry is dropped once the cap is reached. +const KEY_CACHE_MAX_ENTRIES = 1000; const keyCache = new Map(); +function rememberUserApiKey(telegramUserId: number, key: string): void { + // Re-insert so this id becomes the most recently used entry. + keyCache.delete(telegramUserId); + keyCache.set(telegramUserId, key); + while (keyCache.size > KEY_CACHE_MAX_ENTRIES) { + const oldest = keyCache.keys().next(); + if (oldest.done) break; + keyCache.delete(oldest.value); + } +} + export async function resolveUserApiKey(telegramUserId: number): Promise { const cached = keyCache.get(telegramUserId); - if (cached) return cached; + if (cached) { + // Refresh recency so an active user is not evicted by a burst of new ids. + rememberUserApiKey(telegramUserId, cached); + return cached; + } const machineId = (await getConsistentMachineId().catch(() => null)) || "0000000000000000"; @@ -39,12 +59,12 @@ export async function resolveUserApiKey(telegramUserId: number): Promise ); const matchKey = (match as { key?: string } | undefined)?.key; if (typeof matchKey === "string" && matchKey.length > 0) { - keyCache.set(telegramUserId, matchKey); + rememberUserApiKey(telegramUserId, matchKey); return matchKey; } const created = await createApiKey(`telegram:${telegramUserId}`, machineId); - keyCache.set(telegramUserId, created.key); + rememberUserApiKey(telegramUserId, created.key); return created.key; } diff --git a/tests/unit/telegram-keycache-bounded-13165.test.ts b/tests/unit/telegram-keycache-bounded-13165.test.ts new file mode 100644 index 0000000000..46305744c5 --- /dev/null +++ b/tests/unit/telegram-keycache-bounded-13165.test.ts @@ -0,0 +1,123 @@ +/** + * Regression test for #13165: the Telegram per-user key cache must stay bounded. + * + * `resolveUserApiKey()` is reachable from the webhook path of + * POST /api/telegram/update with a caller-supplied chat id, so an uncapped Map + * grows for the lifetime of the process. The cache is module-private, so this + * asserts the observable LRU contract: a cold id is re-minted after a burst of + * distinct ids (proving eviction), while a recently used id survives it. + * + * Runner: node:test (tests/unit/*.test.ts), so DB access is stubbed through a + * module mock rather than vi.mock. + */ +import { test, describe, before, beforeEach } from "node:test"; +import assert from "node:assert/strict"; +import { register } from "node:module"; +import { pathToFileURL } from "node:url"; + +const CAP = 1000; + +/** Names passed to createApiKey — one entry per real mint (i.e. per cache miss). */ +const minted: string[] = []; + +let resolveUserApiKey: (id: number) => Promise; + +before(async () => { + // Stub the DB + machine-id modules so nothing touches SQLite. The loader + // matches the specifiers used by chatProxy.ts. The stub must export every + // name the real module exports: chatProxy pulls in the chat handler, which + // imports other members of this module, and a missing export is a module-load + // SyntaxError that would look like a failing assertion. + const dbExports = [ + "clearApiKeyCaches", + "deleteApiKey", + "getApiKeyById", + "getApiKeyMetadata", + "getApiKeysCount", + "getExclusiveLeaseConnectionIds", + "isModelAllowedForKey", + "pickApiKeyForInternalUse", + "regenerateApiKey", + "resetApiKeyState", + "revokeApiKey", + "setApiKeyExpiry", + "updateApiKeyPermissions", + "validateApiKey", + ]; + + const dbStub = ` + export async function getApiKeys() { return []; } + export async function createApiKey(name) { + globalThis.__mintedKeys.push(name); + return { key: "sk-omni-" + "x".repeat(32) + "-" + name }; + } + ${dbExports.map((n) => `export async function ${n}() { return null; }`).join("\n")} + `; + const machineStub = ` + export async function getConsistentMachineId() { return "0000000000000000"; } + `; + + (globalThis as Record).__mintedKeys = minted; + + const loader = ` + export async function resolve(spec, ctx, next) { + if (spec.includes("db/apiKeys")) { + return { url: "data:text/javascript,${encodeURIComponent(dbStub)}", shortCircuit: true }; + } + if (spec.includes("machineId")) { + return { url: "data:text/javascript,${encodeURIComponent(machineStub)}", shortCircuit: true }; + } + return next(spec, ctx); + } + `; + register("data:text/javascript," + encodeURIComponent(loader), pathToFileURL("./")); + + ({ resolveUserApiKey } = await import("../../src/lib/telegram/chatProxy.ts")); +}); + +describe("telegram keyCache bounding (#13165)", () => { + beforeEach(() => { + minted.length = 0; + }); + + test("evicts a cold id once the cap is exceeded", async () => { + const victim = 7_000_001; + const beforeFirstResolve = minted.length; + await resolveUserApiKey(victim); + assert.equal(minted.length - beforeFirstResolve, 1, "first resolve should mint exactly once"); + + // Never touch `victim` again: it must fall out of a CAP-sized cache. + for (let i = 0; i < CAP + 50; i++) await resolveUserApiKey(600_000 + i); + + // Measure the victim's own resolve in isolation. Comparing against the + // running total would be dominated by the burst's own mints and would pass + // even with an unbounded cache. + const beforeVictimResolve = minted.length; + await resolveUserApiKey(victim); + const mintedForVictim = minted.length - beforeVictimResolve; + + // Evicted => cache miss => exactly one fresh mint for this id. + assert.equal( + mintedForVictim, + 1, + `expected victim to be re-minted after eviction, got ${mintedForVictim} mint(s)` + ); + }); + + test("keeps a recently used id alive across a burst of new ids", async () => { + const active = 8_000_001; + const first = await resolveUserApiKey(active); + + // Touch the active id throughout the burst so it stays most-recently-used. + for (let i = 0; i < CAP * 2; i++) { + await resolveUserApiKey(500_000 + i); + if (i % 100 === 0) await resolveUserApiKey(active); + } + + const mintsBefore = minted.length; + const again = await resolveUserApiKey(active); + + assert.equal(again, first, "active id should keep its cached key"); + assert.equal(minted.length, mintsBefore, "active id should not be re-minted"); + }); +}); From a3fa6cf524d8c2235b15e3e4a0f9bc5dab7c1dcb Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:34 +0700 Subject: [PATCH 06/19] fix(traffic-inspector): release WS subscriber and ping timer on a dead socket (#13155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaching `close`/`error` before subscribing closes the window where a resource is held with no live cleanup path, and the destroyed-socket re-check after the handshake covers the in-flight case. `write()` not throwing synchronously is exactly why the old `try/catch` never fired. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- ...52-traffic-inspector-ws-subscriber-leak.md | 1 + .../api/tools/traffic-inspector/ws/route.ts | 69 ++++++--- ...inspector-ws-subscriber-leak-13152.test.ts | 134 ++++++++++++++++++ 3 files changed, 186 insertions(+), 18 deletions(-) create mode 100644 changelog.d/fixes/13152-traffic-inspector-ws-subscriber-leak.md create mode 100644 tests/unit/traffic-inspector-ws-subscriber-leak-13152.test.ts diff --git a/changelog.d/fixes/13152-traffic-inspector-ws-subscriber-leak.md b/changelog.d/fixes/13152-traffic-inspector-ws-subscriber-leak.md new file mode 100644 index 0000000000..4c5249f4fd --- /dev/null +++ b/changelog.d/fixes/13152-traffic-inspector-ws-subscriber-leak.md @@ -0,0 +1 @@ +- **fix(traffic-inspector):** the WebSocket route no longer leaks a traffic-buffer subscriber and a 30s ping timer when the client socket is already closed at handler time — listeners are attached before any resource is acquired, a destroyed socket bails out early, and the ping interval stops on a dead socket where `write()` never throws ([#13155](https://github.com/diegosouzapw/OmniRoute/pull/13155)) diff --git a/src/app/api/tools/traffic-inspector/ws/route.ts b/src/app/api/tools/traffic-inspector/ws/route.ts index b32a5d7cc5..a9545e221f 100644 --- a/src/app/api/tools/traffic-inspector/ws/route.ts +++ b/src/app/api/tools/traffic-inspector/ws/route.ts @@ -96,6 +96,14 @@ export async function GET(request: Request): Promise { } const acceptHeader = acceptKey(clientKey); + + // The client can vanish during the upgrade round trip. `close` has then + // ALREADY fired, so the listeners below would never run and every resource + // acquired past this point would be held with no path to release it. + if (socket.destroyed) { + return new Response(null, { status: 101 }); + } + socket.write( [ "HTTP/1.1 101 Switching Protocols", @@ -106,21 +114,17 @@ export async function GET(request: Request): Promise { ].join("\r\n") ); - const unsubscribe = globalTrafficBuffer.subscribe((ev) => { - sendText(socket, ev); - }); - - const pingTimer = setInterval(() => { - try { - socket.write(encodeWsFrame(0x09)); // ping - } catch { - cleanup(); - } - }, PING_INTERVAL_MS); + let unsubscribe: (() => void) | null = null; + let pingTimer: ReturnType | null = null; + let cleanedUp = false; function cleanup(): void { - clearInterval(pingTimer); - unsubscribe(); + if (cleanedUp) return; + cleanedUp = true; + if (pingTimer) clearInterval(pingTimer); + pingTimer = null; + unsubscribe?.(); + unsubscribe = null; try { socket.destroy(); } catch { @@ -128,14 +132,43 @@ export async function GET(request: Request): Promise { } } - socket.once("close", cleanup); - socket.once("error", cleanup); - - // Never resolve — the socket is the response channel. - await new Promise((resolve) => { + // Attached BEFORE any resource is acquired, so there is no window in which a + // subscriber or timer exists without a live path to cleanup(). + const settled = new Promise((resolve) => { socket.once("close", resolve); socket.once("error", resolve); }); + socket.once("close", cleanup); + socket.once("error", cleanup); + + // Re-check: `close` may have fired while we were writing the handshake, in + // which case the listeners above already ran and cleanup() is a no-op we + // still must not skip. + if (socket.destroyed) { + cleanup(); + return new Response(null, { status: 101 }); + } + + unsubscribe = globalTrafficBuffer.subscribe((ev) => { + sendText(socket, ev); + }); + + pingTimer = setInterval(() => { + // `socket.write()` does NOT throw synchronously on a destroyed socket, so + // the destroyed check — not the catch — is what stops a dead interval. + if (socket.destroyed) { + cleanup(); + return; + } + try { + socket.write(encodeWsFrame(0x09)); // ping + } catch { + cleanup(); + } + }, PING_INTERVAL_MS); + + // Never resolve — the socket is the response channel. + await settled; cleanup(); return new Response(null, { status: 101 }); diff --git a/tests/unit/traffic-inspector-ws-subscriber-leak-13152.test.ts b/tests/unit/traffic-inspector-ws-subscriber-leak-13152.test.ts new file mode 100644 index 0000000000..230a3f28d2 --- /dev/null +++ b/tests/unit/traffic-inspector-ws-subscriber-leak-13152.test.ts @@ -0,0 +1,134 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import net from "node:net"; +import type { AddressInfo } from "node:net"; + +import { GET } from "@/app/api/tools/traffic-inspector/ws/route"; +import { globalTrafficBuffer } from "@/mitm/inspector/buffer"; + +const DEAD_UPGRADES = 6; + +function armedTimers(): number { + return process.getActiveResourcesInfo().filter((r) => r === "Timeout").length; +} + +function upgradeRequest(socket: net.Socket): Request { + const req = new Request("http://127.0.0.1/api/tools/traffic-inspector/ws", { + headers: { + upgrade: "websocket", + "sec-websocket-key": "dGhlIHNhbXBsZSBub25jZQ==", + }, + }); + Object.defineProperty(req, "socket", { value: socket, configurable: true }); + return req; +} + +async function deadSocket(port: number): Promise { + const sock = net.connect(port, "127.0.0.1"); + await new Promise((r) => sock.once("connect", () => r())); + sock.on("error", () => {}); + sock.destroy(); + await new Promise((r) => setTimeout(r, 20)); + return sock; +} + +test("an already-closed socket leaves no subscriber and no ping timer", async () => { + const accepted: net.Socket[] = []; + const server = net.createServer((c) => { + accepted.push(c); + c.on("error", () => {}); + }); + await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); + const { port } = server.address() as AddressInfo; + + try { + const timersBefore = armedTimers(); + const subsBefore = globalTrafficBuffer.subscriberCount(); + + const handlers: Promise[] = []; + for (let i = 0; i < DEAD_UPGRADES; i++) { + // Catch at creation time: the route answers a hijacked upgrade with a 101 + // Response, which undici rejects off a real server. Left unattached, that + // rejection would sit through the next await and trip Node's unhandled + // rejection detection. Either settlement proves the handler released its + // resources instead of hanging, which is what this test measures. + handlers.push(GET(upgradeRequest(await deadSocket(port))).catch(() => undefined)); + } + + // Own the race timer so it can be cleared before measuring; otherwise the + // test's own armed timeout is counted as a leaked one. + let raceTimer: ReturnType | undefined; + const outcome = await Promise.race([ + Promise.all(handlers).then(() => "settled"), + new Promise((r) => { + raceTimer = setTimeout(() => r("hung"), 2000); + }), + ]); + if (raceTimer) clearTimeout(raceTimer); + assert.equal( + outcome, + "settled", + "each handler must return instead of hanging forever on a dead socket" + ); + + const timersAfter = armedTimers(); + assert.ok( + timersAfter <= timersBefore, + `${DEAD_UPGRADES} dead upgrades retained ${timersAfter - timersBefore} ping timer(s)` + ); + + // Measure the subscriber set directly; counting fan-out to our own probe + // says nothing about whether the dead sockets stayed subscribed. + assert.equal( + globalTrafficBuffer.subscriberCount(), + subsBefore, + `${DEAD_UPGRADES} dead upgrades left ${globalTrafficBuffer.subscriberCount() - subsBefore} subscriber(s) behind` + ); + } finally { + // close() only fires once every accepted connection is gone. + for (const c of accepted) c.destroy(); + await new Promise((r) => server.close(() => r())); + } +}); + +test("a live socket keeps its subscription until the socket closes", async () => { + const accepted: net.Socket[] = []; + const server = net.createServer((c) => { + accepted.push(c); + c.on("error", () => {}); + }); + await new Promise((r) => server.listen(0, "127.0.0.1", () => r())); + const { port } = server.address() as AddressInfo; + + const sock = net.connect(port, "127.0.0.1"); + await new Promise((r) => sock.once("connect", () => r())); + sock.on("error", () => {}); + + try { + const subsBefore = globalTrafficBuffer.subscriberCount(); + + const handler = GET(upgradeRequest(sock)).catch(() => undefined); + await new Promise((r) => setTimeout(r, 100)); + + assert.equal( + globalTrafficBuffer.subscriberCount(), + subsBefore + 1, + "a live upgrade must register exactly one traffic subscriber" + ); + + // Closing the socket resolves the handler's `settled` promise, which is the + // only path that releases the subscriber. + sock.destroy(); + await handler; + + assert.equal( + globalTrafficBuffer.subscriberCount(), + subsBefore, + "closing the socket must release the subscriber" + ); + } finally { + // close() only fires once every accepted connection is gone. + for (const c of accepted) c.destroy(); + await new Promise((r) => server.close(() => r())); + } +}); From 66330cc7241295e51e263e1f50eb197bb0e40709 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:38 +0700 Subject: [PATCH 07/19] fix(compression): pass a URL object when spawning the LLMLingua worker (#13093) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `node:worker_threads` only treats a `URL` instance as a `file:` URL — a string must be a relative path. The silent `catch {}` in `pump()` made this degrade compression to a passthrough while still reporting success, which is the worst shape for it. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- changelog.d/fixes/llmlingua-worker-spawn.md | 1 + .../compression/engines/llmlingua/worker.ts | 7 +- .../llmlingua-worker-spawn-12822.test.ts | 77 +++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/llmlingua-worker-spawn.md create mode 100644 tests/unit/compression/llmlingua-worker-spawn-12822.test.ts diff --git a/changelog.d/fixes/llmlingua-worker-spawn.md b/changelog.d/fixes/llmlingua-worker-spawn.md new file mode 100644 index 0000000000..865f19a32e --- /dev/null +++ b/changelog.d/fixes/llmlingua-worker-spawn.md @@ -0,0 +1 @@ +- fix(compression): spawn the LLMLingua worker with a file URL object so compression actually runs instead of silently failing open on Node diff --git a/open-sse/services/compression/engines/llmlingua/worker.ts b/open-sse/services/compression/engines/llmlingua/worker.ts index 00ba3e1255..0994e1a935 100644 --- a/open-sse/services/compression/engines/llmlingua/worker.ts +++ b/open-sse/services/compression/engines/llmlingua/worker.ts @@ -234,7 +234,12 @@ function ensureWorker(): Worker { const { workerFile, execArgv } = resolveWorkerFile(); const absoluteWorkerFile = path.resolve(workerFile); - const w = new Worker(pathToFileURL(absoluteWorkerFile).href, { execArgv }); + // Pass the URL OBJECT, not `.href`. `new Worker()` treats a plain string as a + // filesystem path, so a "file://..." string is looked up literally and throws + // ERR_WORKER_PATH (a string arg must start with ./ or ../). Only a URL instance + // is interpreted as a file: URL. Spawn failures are swallowed by pump()'s catch, + // so getting this wrong silently disables compression instead of erroring. + const w = new Worker(pathToFileURL(absoluteWorkerFile), { execArgv }); w.on("message", (reply: WorkerReply) => { const entry = pending.get(reply.id); diff --git a/tests/unit/compression/llmlingua-worker-spawn-12822.test.ts b/tests/unit/compression/llmlingua-worker-spawn-12822.test.ts new file mode 100644 index 0000000000..360a1f2037 --- /dev/null +++ b/tests/unit/compression/llmlingua-worker-spawn-12822.test.ts @@ -0,0 +1,77 @@ +/** + * Regression guard for #12822: the LLMLingua worker must actually spawn on Node. + * + * Root cause: `new Worker(pathToFileURL(file).href, ...)` passes a STRING. Node treats a + * string argument as a filesystem path (it must start with ./ or ../), so a "file://..." + * string is looked up literally and throws ERR_WORKER_PATH. Only a URL INSTANCE is + * interpreted as a file: URL. + * + * Why it was invisible: pump() wraps ensureWorker() in `catch {}` and fails open, so the + * spawn crash silently degraded every compression call to a passthrough instead of erroring. + * + * This test asserts the Node contract directly against a real Worker, so it fails on the + * old `.href` spelling and passes on the URL object. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { Worker } from "node:worker_threads"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const WORKER_SRC = path.resolve( + here, + "../../../open-sse/services/compression/engines/llmlingua/worker.ts" +); + +function spawnWith(arg: string | URL): Promise { + return new Promise((resolve, reject) => { + let w: Worker; + try { + w = new Worker(arg, {}); + } catch (err) { + reject(err); + return; + } + w.on("error", reject); + w.on("exit", () => resolve()); + }); +} + +test("a file: URL STRING is rejected by node:worker_threads (the #12822 crash)", async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-worker-")); + const child = path.join(dir, "child.mjs"); + fs.writeFileSync(child, "process.exit(0);\n"); + + await assert.rejects( + () => spawnWith(pathToFileURL(child).href), + (err: NodeJS.ErrnoException) => err.code === "ERR_WORKER_PATH", + "passing .href must fail — this is exactly what shipped and was swallowed by the fail-open catch" + ); + + fs.rmSync(dir, { recursive: true, force: true }); +}); + +test("a file: URL OBJECT spawns cleanly", async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-worker-")); + const child = path.join(dir, "child.mjs"); + fs.writeFileSync(child, "process.exit(0);\n"); + + await spawnWith(pathToFileURL(child)); + + fs.rmSync(dir, { recursive: true, force: true }); +}); + +test("worker.ts passes the URL object, not .href", () => { + const code = fs.readFileSync(WORKER_SRC, "utf8"); + assert.ok( + /new Worker\(\s*pathToFileURL\([A-Za-z0-9_]+\)\s*,/.test(code), + "ensureWorker must pass the URL instance to new Worker()" + ); + assert.ok( + !/new Worker\(\s*pathToFileURL\([A-Za-z0-9_]+\)\.href/.test(code), + "ensureWorker must not pass pathToFileURL(...).href — that throws ERR_WORKER_PATH" + ); +}); From b1733d3c83d8241542ae3ea2f7fa638fe4667df1 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:42 +0700 Subject: [PATCH 08/19] fix(plugins): make SIGKILL escalation idempotent per child (#13092) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `once` only detaches when exit actually fires, so a plugin trapping SIGTERM accumulated one listener and one timer per hook timeout. Keying idempotence on the child via a `WeakSet` is right — a second SIGKILL timer would only re-signal a corpse. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../fixes/plugin-sigkill-listener-leak.md | 1 + src/lib/plugins/loader.ts | 46 +++++--- ...lugins-sigkill-listener-leak-12819.test.ts | 100 ++++++++++++++++++ 3 files changed, 135 insertions(+), 12 deletions(-) create mode 100644 changelog.d/fixes/plugin-sigkill-listener-leak.md create mode 100644 tests/unit/plugins-sigkill-listener-leak-12819.test.ts diff --git a/changelog.d/fixes/plugin-sigkill-listener-leak.md b/changelog.d/fixes/plugin-sigkill-listener-leak.md new file mode 100644 index 0000000000..a4baa47066 --- /dev/null +++ b/changelog.d/fixes/plugin-sigkill-listener-leak.md @@ -0,0 +1 @@ +- fix(plugins): stop leaking an exit listener per plugin hook timeout, which triggered MaxListenersExceededWarning on plugins that ignore SIGTERM diff --git a/src/lib/plugins/loader.ts b/src/lib/plugins/loader.ts index d4bcd2739d..71e9223b00 100644 --- a/src/lib/plugins/loader.ts +++ b/src/lib/plugins/loader.ts @@ -9,6 +9,7 @@ */ import { spawn } from "child_process"; +import type { ChildProcess } from "child_process"; import { writeFile, readFile } from "fs/promises"; import { rmSync } from "fs"; import { join } from "path"; @@ -105,6 +106,37 @@ function forwardChildOutput( * against process exit — under `node --test --test-force-exit` the runner exits * before the promise settles, leaking one temp .mjs per plugin load. */ +/** Children already escalating to SIGKILL. Prevents re-arming a second timer + listener + * for a child that is already being killed. */ +const escalating = new WeakSet(); + +/** + * SIGTERM has already been sent; escalate to SIGKILL if the child ignores it. + * + * Must be idempotent per child. Every hook timeout hits this path, and a plugin that + * traps SIGTERM keeps taking calls, so re-arming would add one exit listener plus one + * killTimer closure per timeout — Node starts printing MaxListenersExceededWarning at 11. + * One pending kill per child is also all that is useful: SIGKILL cannot be ignored, so a + * second timer would only re-signal a corpse. (#12819) + */ +function escalateToSigkill(child: ChildProcess): void { + if (escalating.has(child)) return; + escalating.add(child); + + const onExit = () => { + clearTimeout(killTimer); + escalating.delete(child); + }; + const killTimer = setTimeout(() => { + child.removeListener("exit", onExit); + escalating.delete(child); + try { + child.kill("SIGKILL"); + } catch {} + }, SIGKILL_GRACE_MS); + child.once("exit", onExit); +} + function removeHostScript(path: string): void { try { rmSync(path, { force: true }); @@ -293,12 +325,7 @@ export async function loadPlugin( } child.kill("SIGTERM"); // Escalate to SIGKILL if plugin ignores SIGTERM - const killTimer = setTimeout(() => { - try { - child.kill("SIGKILL"); - } catch {} - }, SIGKILL_GRACE_MS); - child.once("exit", () => clearTimeout(killTimer)); + escalateToSigkill(child); reject(new Error(`Plugin hook '${hook}' timed out after ${timeout}ms`)); }, timeout); @@ -399,12 +426,7 @@ export async function loadPlugin( const cleanup = () => { child.kill("SIGTERM"); // Escalate to SIGKILL after grace period - const killTimer = setTimeout(() => { - try { - child.kill("SIGKILL"); - } catch {} - }, SIGKILL_GRACE_MS); - child.once("exit", () => clearTimeout(killTimer)); + escalateToSigkill(child); removeHostScript(hostScriptPath); log.info("loader.cleanup", { name: manifest.name }); }; diff --git a/tests/unit/plugins-sigkill-listener-leak-12819.test.ts b/tests/unit/plugins-sigkill-listener-leak-12819.test.ts new file mode 100644 index 0000000000..df6a39638a --- /dev/null +++ b/tests/unit/plugins-sigkill-listener-leak-12819.test.ts @@ -0,0 +1,100 @@ +// Regression test for #12819 — loadPlugin() leaked one "exit" listener per hook timeout. +// +// Root cause: on the SIGTERM→SIGKILL escalation path the loader attached a fresh +// `child.once("exit", () => clearTimeout(killTimer))`. `once` only detaches when exit +// actually FIRES, so a plugin that ignores SIGTERM leaves the listener (and its killTimer +// closure) attached on every hook timeout. Node then prints MaxListenersExceededWarning +// once 11 accumulate. +// +// The plugin below traps SIGTERM and keeps running, which is exactly the condition the +// bug needs. We drive several hook timeouts and assert the listener count stays bounded. +import { test, describe, after } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +const { loadPlugin } = await import("../../src/lib/plugins/loader.ts"); + +const dirs: string[] = []; +after(() => { + for (const d of dirs) rmSync(d, { recursive: true, force: true }); +}); + +/** A plugin that ignores SIGTERM and never answers a hook, forcing the escalation path. */ +function writeStubbornPlugin(): string { + const dir = mkdtempSync(join(tmpdir(), "omniroute-plugin-12819-")); + dirs.push(dir); + const entry = join(dir, "index.mjs"); + writeFileSync( + entry, + [ + // Trap SIGTERM so the loader has to escalate to SIGKILL. + 'process.on("SIGTERM", () => {});', + "export default {", + " // Never resolves → every call hits the hook timeout.", + " onRequest: () => new Promise(() => {}),", + "};", + "", + ].join("\n") + ); + return entry; +} + +describe("plugin loader SIGKILL escalation (#12819)", () => { + test("does not accumulate an exit listener per hook timeout", async () => { + const entryPoint = writeStubbornPlugin(); + const loaded = await loadPlugin( + entryPoint, + { + name: "sigkill-listener-leak", + version: "1.0.0", + license: "MIT", + main: "index.mjs", + source: "local", + tags: [], + requires: { permissions: [] }, + hooks: { onRequest: true, onResponse: false, onError: false }, + skills: [], + enabledByDefault: false, + configSchema: {}, + } as never, + { hookTimeoutMs: 120 } + ); + + const onRequest = ( + loaded.plugin as unknown as { + onRequest?: (ctx: unknown) => Promise; + } + ).onRequest; + assert.ok(onRequest, "onRequest hook should be registered"); + + // `child` is private to the loader, so observe the leak the way a user does: Node + // itself emits MaxListenersExceededWarning once an emitter passes 10 listeners. + const warnings: string[] = []; + const onWarning = (w: Error) => { + if (w.name === "MaxListenersExceededWarning") warnings.push(w.message); + }; + process.on("warning", onWarning); + + try { + // 12 timeouts: comfortably past Node's default limit of 10, so the pre-fix code + // trips the warning while the fixed code stays flat. + for (let i = 0; i < 12; i++) { + await onRequest({ body: {} }).catch(() => undefined); + } + // Warnings are delivered on the next tick; let them land before asserting. + await new Promise((r) => setTimeout(r, 50)); + } finally { + process.removeListener("warning", onWarning); + } + + assert.deepEqual( + warnings, + [], + `hook timeouts must not accumulate exit listeners (#12819): ${warnings[0] ?? ""}` + ); + + loaded.cleanup?.(); + }); +}); From 85a5126dba7f3843c9d1a5ad0d22d18bd2917522 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:27:45 +0700 Subject: [PATCH 09/19] fix(compression): terminate idle workers on eviction (#13091) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remove(slot, false)` dropped the slot without `terminate()`, leaving the OS thread and its heap alive — invisible to RSS, which is why 55 orphaned `MessagePort`s took 16 h to surface. Removing the parameter rather than keeping it is the correct call: the pool was its only owner. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../fixes/compression-pool-idle-terminate.md | 1 + .../compression/compressionWorkerPool.ts | 15 ++-- .../worker-pool-idle-eviction-12812.test.ts | 69 +++++++++++++++++++ 3 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 changelog.d/fixes/compression-pool-idle-terminate.md create mode 100644 tests/unit/compression/worker-pool-idle-eviction-12812.test.ts diff --git a/changelog.d/fixes/compression-pool-idle-terminate.md b/changelog.d/fixes/compression-pool-idle-terminate.md new file mode 100644 index 0000000000..d2a280f1ba --- /dev/null +++ b/changelog.d/fixes/compression-pool-idle-terminate.md @@ -0,0 +1 @@ +- fix(compression): terminate idle worker threads on eviction so long-running instances stop leaking OS threads and MessagePorts diff --git a/open-sse/services/compression/compressionWorkerPool.ts b/open-sse/services/compression/compressionWorkerPool.ts index 352aabd39c..7402294c39 100644 --- a/open-sse/services/compression/compressionWorkerPool.ts +++ b/open-sse/services/compression/compressionWorkerPool.ts @@ -131,7 +131,7 @@ export class CompressionWorkerPool { } async close(): Promise { for (const job of this.queue.splice(0)) job.resolve(unchanged(job.originalBody)); - await Promise.all([...this.workers].map((slot) => this.remove(slot, true))); + await Promise.all([...this.workers].map((slot) => this.remove(slot))); } private spawn(): PoolWorker { const slot: PoolWorker = { @@ -185,7 +185,10 @@ export class CompressionWorkerPool { slot.timeout = null; slot.job = null; job.resolve(result); - slot.idle = setTimeout(() => void this.remove(slot, false), this.idleMs); + // Idle eviction MUST terminate. Dropping the slot from the set only releases our + // reference - the thread, its MessagePort and its private heap outlive the pool + // for the whole process lifetime, invisible to process.memoryUsage(). (#12812) + slot.idle = setTimeout(() => void this.remove(slot), this.idleMs); slot.idle.unref(); this.dispatch(); } @@ -193,13 +196,15 @@ export class CompressionWorkerPool { const job = slot.job; if (job) job.resolve(unchanged(job.originalBody)); slot.job = null; - void this.remove(slot, true).finally(() => this.dispatch()); + void this.remove(slot).finally(() => this.dispatch()); } - private async remove(slot: PoolWorker, terminate: boolean): Promise { + /** Drop a slot and release its OS thread. Removal always terminates: a pooled worker + * has no other owner, so skipping terminate() strands the thread permanently. */ + private async remove(slot: PoolWorker): Promise { if (!this.workers.delete(slot)) return; if (slot.timeout) clearTimeout(slot.timeout); if (slot.idle) clearTimeout(slot.idle); - if (terminate) await slot.worker.terminate().catch(() => undefined); + await slot.worker.terminate().catch(() => undefined); } } diff --git a/tests/unit/compression/worker-pool-idle-eviction-12812.test.ts b/tests/unit/compression/worker-pool-idle-eviction-12812.test.ts new file mode 100644 index 0000000000..c95eb181ed --- /dev/null +++ b/tests/unit/compression/worker-pool-idle-eviction-12812.test.ts @@ -0,0 +1,69 @@ +/** + * Regression guard for #12812: idle eviction must terminate the worker thread. + * + * Root cause: finish() scheduled `remove(slot, false)`, so the idle timer dropped the slot + * from the pool WITHOUT calling worker.terminate(). The OS thread, its MessagePort and its + * private heap then survived for the whole process lifetime. Nothing in + * process.memoryUsage() reports that, which is why a 16h instance showed rss=660MB while + * holding 5.7GB of commit charge. + * + * The assertion measures the real thing: a worker that was evicted must no longer be able + * to run code. A live-but-unreferenced thread still responds; a terminated one cannot. + */ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; +import type { Worker } from "node:worker_threads"; +import { CompressionWorkerPool } from "../../../open-sse/services/compression/compressionWorkerPool.ts"; + +const body = { + model: "gpt-test", + messages: [{ role: "user", content: "please kindly actually simplify this text ".repeat(40) }], +}; + +/** Reach into the pool's private slot set — the leak is only observable there. */ +function slotsOf(pool: CompressionWorkerPool): Set<{ worker: Worker }> { + return (pool as unknown as { workers: Set<{ worker: Worker }> }).workers; +} + +describe("compression worker pool idle eviction (#12812)", () => { + it("terminates the worker thread when the idle timer fires", async () => { + // Idle window short enough to fire during the test. + const pool = new CompressionWorkerPool({ size: 1, idleMs: 50 }); + + await pool.run(body, "stacked", undefined, undefined); + + const slots = [...slotsOf(pool)]; + assert.equal(slots.length, 1, "one worker should have been spawned"); + const { worker } = slots[0]; + + // The observable difference between 'evicted' and 'terminated' is the exit event: + // a leaked thread stays alive and never emits it. Arm the listener BEFORE the idle + // window so we cannot miss the event. + const exited = new Promise((resolve) => { + worker.once("exit", () => resolve(true)); + setTimeout(() => resolve(false), 3_000).unref?.(); + }); + + await new Promise((r) => setTimeout(r, 400)); + assert.equal(slotsOf(pool).size, 0, "slot should be evicted from the pool"); + + assert.equal( + await exited, + true, + "idle eviction must terminate the thread, not just drop the reference (#12812)" + ); + + await pool.close(); + }); + + it("close() terminates every pooled worker", async () => { + const pool = new CompressionWorkerPool({ size: 2, idleMs: 60_000 }); + await Promise.all([ + pool.run(body, "stacked", undefined, undefined), + pool.run(body, "stacked", undefined, undefined), + ]); + assert.ok(slotsOf(pool).size >= 1, "pool should hold workers before close"); + await pool.close(); + assert.equal(slotsOf(pool).size, 0, "close() must drain the pool"); + }); +}); From 67618978b0eb828fb1a1126b05c1c0244c35d095 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:30:27 +0700 Subject: [PATCH 10/19] fix(gamification): close the badge SSE stream when the signal is already aborted (#13106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct: an `abort` listener registered on an already-aborted signal never fires, and `safeEnqueue` can't save it because enqueuing into an unread stream only buffers. The abort-later test earning its keep as a guard on the healthy path is the right instinct. I reformatted the changelog fragment to the `changelog.d` convention (`- **fix(scope):** …`) before merging — `check:changelog-integrity` rejects a fragment that does not start with a markdown bullet, which is the same gate your #13158 was about. Wording is yours, unchanged in substance. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../fixes/13103-badge-sse-aborted-signal.md | 1 + src/lib/gamification/notifications.ts | 7 ++ .../badge-sse-aborted-signal-13103.test.ts | 79 +++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 changelog.d/fixes/13103-badge-sse-aborted-signal.md create mode 100644 tests/unit/badge-sse-aborted-signal-13103.test.ts diff --git a/changelog.d/fixes/13103-badge-sse-aborted-signal.md b/changelog.d/fixes/13103-badge-sse-aborted-signal.md new file mode 100644 index 0000000000..000e3ddb45 --- /dev/null +++ b/changelog.d/fixes/13103-badge-sse-aborted-signal.md @@ -0,0 +1 @@ +- **fix(gamification):** close the badge notification SSE stream when the request signal is already aborted before the stream starts — a client that disconnects while the route is still awaiting auth used to leave both the 2s unlock poll and the 15s heartbeat running for the lifetime of the process. diff --git a/src/lib/gamification/notifications.ts b/src/lib/gamification/notifications.ts index 4226cb9977..ef3daf1dab 100644 --- a/src/lib/gamification/notifications.ts +++ b/src/lib/gamification/notifications.ts @@ -110,6 +110,13 @@ export function createBadgeNotificationStream( } }; + // A client that disconnects while the route is still awaiting auth + // arrives here already aborted, and "abort" will never fire again -- + // the timers above would then run for the lifetime of the process. + if (signal?.aborted) { + cleanup(); + return; + } if (signal) { signal.addEventListener("abort", cleanup); } diff --git a/tests/unit/badge-sse-aborted-signal-13103.test.ts b/tests/unit/badge-sse-aborted-signal-13103.test.ts new file mode 100644 index 0000000000..fbe5a91ed7 --- /dev/null +++ b/tests/unit/badge-sse-aborted-signal-13103.test.ts @@ -0,0 +1,79 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { createBadgeNotificationStream } = + await import("../../src/lib/gamification/notifications.ts"); + +/** + * Count timers created while `fn` runs and are still armed afterwards. + * The stream owns its handles privately, so this is the only way to observe them. + */ +async function withTimerAccounting( + fn: () => Promise | T +): Promise<{ result: T; live: number }> { + const live = new Set(); + const realSet = globalThis.setInterval; + const realClear = globalThis.clearInterval; + + globalThis.setInterval = ((...args: Parameters) => { + const handle = realSet(...args); + live.add(handle); + return handle; + }) as typeof realSet; + + globalThis.clearInterval = ((handle: Parameters[0]) => { + if (handle !== undefined) live.delete(handle); + return realClear(handle); + }) as typeof realClear; + + try { + const result = await fn(); + // Let any pending abort/microtask cleanup run. + await new Promise((r) => setTimeout(r, 50)); + // Stop whatever survived so a failing test cannot hang the runner. + for (const handle of live) realClear(handle as Parameters[0]); + return { result, live: live.size }; + } finally { + globalThis.setInterval = realSet; + globalThis.clearInterval = realClear; + } +} + +test("aborting after the stream starts clears both intervals (#13103)", async () => { + const controller = new AbortController(); + const { live } = await withTimerAccounting(async () => { + createBadgeNotificationStream("key-normal", controller.signal); + controller.abort(); + }); + assert.equal(live, 0, "the normal lifecycle must clean up (baseline for the next test)"); +}); + +test("a signal already aborted before start() must not leave timers running (#13103)", async () => { + const controller = new AbortController(); + // The route awaits auth before building the stream, so a client that + // disconnects during that round-trip arrives here already aborted. + controller.abort(); + + const { live } = await withTimerAccounting(() => { + createBadgeNotificationStream("key-preaborted", controller.signal); + }); + + assert.equal( + live, + 0, + `an already-aborted signal left ${live} interval(s) running for the lifetime of the process` + ); +}); + +test("an already-aborted stream is closed rather than left enqueuing (#13103)", async () => { + const controller = new AbortController(); + controller.abort(); + + const stream = createBadgeNotificationStream("key-closed", controller.signal); + const reader = stream.getReader(); + + // enqueue() into an unread stream only buffers -- it does not throw -- so a + // stream left open here would keep filling its queue with nobody draining it. + const { done } = await reader.read(); + assert.equal(done, true, "the stream must be closed when the signal was already aborted"); +}); From 99fb441434055d402bba4364f49f84478de4702a Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:30:32 +0700 Subject: [PATCH 11/19] fix(db): release process listeners when a node:sqlite adapter closes (#13109) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Right precedent — #7494 fixed exactly this for the sql.js adapter and the `node:sqlite` one never got the same treatment, even though it is the default driver whenever better-sqlite3 is unavailable. `/api/db-backups/import` opening a throwaway adapter per request makes it reachable. I reformatted the changelog fragment to the `changelog.d` convention (`- **fix(scope):** …`) before merging — `check:changelog-integrity` rejects a fragment that does not start with a markdown bullet, which is the same gate your #13158 was about. Wording is yours, unchanged in substance. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../13108-nodesqlite-process-listener-leak.md | 1 + src/lib/db/adapters/nodeSqliteAdapter.ts | 30 +++++---- ...sqlite-process-listener-leak-13108.test.ts | 63 +++++++++++++++++++ 3 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 changelog.d/fixes/13108-nodesqlite-process-listener-leak.md create mode 100644 tests/unit/nodesqlite-process-listener-leak-13108.test.ts diff --git a/changelog.d/fixes/13108-nodesqlite-process-listener-leak.md b/changelog.d/fixes/13108-nodesqlite-process-listener-leak.md new file mode 100644 index 0000000000..93875c856f --- /dev/null +++ b/changelog.d/fixes/13108-nodesqlite-process-listener-leak.md @@ -0,0 +1 @@ +- **fix(db):** release the `beforeExit`/`SIGINT`/`SIGTERM` handlers when a `node:sqlite` adapter closes, so a closed adapter and its database handle are no longer pinned to `process` for the lifetime of the run — the same treatment #7494 gave the sql.js adapter. diff --git a/src/lib/db/adapters/nodeSqliteAdapter.ts b/src/lib/db/adapters/nodeSqliteAdapter.ts index 73c3aeee60..a422d1c8c5 100644 --- a/src/lib/db/adapters/nodeSqliteAdapter.ts +++ b/src/lib/db/adapters/nodeSqliteAdapter.ts @@ -35,26 +35,34 @@ export async function createNodeSqliteAdapter(filePath: string): Promise { + adapter.close(); + }; + const onSignal = () => { + adapter.close(); + process.exit(0); + }; + function gracefulClose() { clearInterval(checkpointTimer as unknown as NodeJS.Timeout); try { db.exec("PRAGMA wal_checkpoint(TRUNCATE)"); } catch {} + process.removeListener("beforeExit", onBeforeExit); + process.removeListener("SIGINT", onSignal); + process.removeListener("SIGTERM", onSignal); } const adapter = createNodeSqliteAdapterFromDatabase(db, filePath, gracefulClose); - process.once("beforeExit", () => { - adapter.close(); - }); - process.once("SIGINT", () => { - adapter.close(); - process.exit(0); - }); - process.once("SIGTERM", () => { - adapter.close(); - process.exit(0); - }); + process.once("beforeExit", onBeforeExit); + process.once("SIGINT", onSignal); + process.once("SIGTERM", onSignal); return adapter; } diff --git a/tests/unit/nodesqlite-process-listener-leak-13108.test.ts b/tests/unit/nodesqlite-process-listener-leak-13108.test.ts new file mode 100644 index 0000000000..500461a0c1 --- /dev/null +++ b/tests/unit/nodesqlite-process-listener-leak-13108.test.ts @@ -0,0 +1,63 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { tmpdir } from "node:os"; +import { mkdtempSync, rmSync } from "node:fs"; +import { join } from "node:path"; + +const { createNodeSqliteAdapter } = await import("../../src/lib/db/adapters/nodeSqliteAdapter.ts"); + +const SIGNALS = ["beforeExit", "SIGINT", "SIGTERM"] as const; + +function counts(): Record { + return Object.fromEntries(SIGNALS.map((s) => [s, process.listenerCount(s)])); +} + +function delta(before: Record, after: Record) { + return Object.fromEntries(SIGNALS.map((s) => [s, after[s] - before[s]])); +} + +test("closing a node:sqlite adapter releases its process listeners (#13108)", async () => { + const dir = mkdtempSync(join(tmpdir(), "omniroute-dbleak-")); + const before = counts(); + + try { + // Short-lived adapters are a real pattern: POST /api/db-backups/import + // opens one per request purely to validate the uploaded file. + const N = 12; + for (let i = 0; i < N; i++) { + const adapter = await createNodeSqliteAdapter(join(dir, `probe-${i}.sqlite`)); + adapter.close(); + } + + const leaked = delta(before, counts()); + for (const signal of SIGNALS) { + assert.equal( + leaked[signal], + 0, + `${N} open+close cycles retained ${leaked[signal]} "${signal}" listener(s) on process` + ); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("an open node:sqlite adapter keeps its shutdown listeners registered (#13108)", async () => { + const dir = mkdtempSync(join(tmpdir(), "omniroute-dbleak-open-")); + const before = counts(); + let adapter: Awaited> | null = null; + + try { + adapter = await createNodeSqliteAdapter(join(dir, "open.sqlite")); + + // The fix must not detach eagerly: these handlers are what checkpoint the + // WAL on Ctrl-C, so they have to stay armed for as long as the db is open. + const armed = delta(before, counts()); + for (const signal of SIGNALS) { + assert.equal(armed[signal], 1, `an open adapter must keep its "${signal}" handler`); + } + } finally { + adapter?.close(); + rmSync(dir, { recursive: true, force: true }); + } +}); From d61b1727cd4551c488c34dd2a6fc205391a051cc Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:30:38 +0700 Subject: [PATCH 12/19] fix(cli-helper): clear the log stream timeout on the abort path (#13114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stop()` is the normal lifecycle for a `follow: true` stream, not an edge case, so the `signal.aborted` early return skipping `clearTimeout` leaked one armed timer per stop. Cancelling the reader on the early loop exit closes the second half. I reformatted the changelog fragment to the `changelog.d` convention (`- **fix(scope):** …`) before merging — `check:changelog-integrity` rejects a fragment that does not start with a markdown bullet, which is the same gate your #13158 was about. Wording is yours, unchanged in substance. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../fixes/13113-logstream-timer-leak.md | 1 + src/lib/cli-helper/log-streamer.ts | 23 +++-- tests/unit/logstream-timer-leak-13113.test.ts | 98 +++++++++++++++++++ 3 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 changelog.d/fixes/13113-logstream-timer-leak.md create mode 100644 tests/unit/logstream-timer-leak-13113.test.ts diff --git a/changelog.d/fixes/13113-logstream-timer-leak.md b/changelog.d/fixes/13113-logstream-timer-leak.md new file mode 100644 index 0000000000..6bd6ced117 --- /dev/null +++ b/changelog.d/fixes/13113-logstream-timer-leak.md @@ -0,0 +1 @@ +- **fix(cli-helper):** clear the `createLogStream` timeout on the abort path — `stop()` aborts the in-flight fetch and returned through the `signal.aborted` branch, which skipped `clearTimeout` and left an armed timer per stopped stream. The stream reader is now also cancelled when the read loop exits early. diff --git a/src/lib/cli-helper/log-streamer.ts b/src/lib/cli-helper/log-streamer.ts index 1fdc151848..06dbffbb2d 100644 --- a/src/lib/cli-helper/log-streamer.ts +++ b/src/lib/cli-helper/log-streamer.ts @@ -38,30 +38,37 @@ export function createLogStream(options: LogStreamOptions = {}): LogStream { if (!response.ok) { controller.error(new Error(`HTTP ${response.status}: ${response.statusText}`)); - clearTimeout(timeoutId); return; } if (!response.body) { controller.error(new Error("Response body is null")); - clearTimeout(timeoutId); return; } const reader = response.body.getReader(); - while (true) { - const { done, value } = await reader.read(); - if (done) break; - if (signal.aborted) break; - controller.enqueue(value); + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + if (signal.aborted) break; + controller.enqueue(value); + } + } finally { + // Leaving the loop early (abort/throw) otherwise keeps the body locked + // and its socket held until GC. + await reader.cancel().catch(() => {}); } controller.close(); - clearTimeout(timeoutId); } catch (err) { if (signal.aborted) return; // Expected stop controller.error(err instanceof Error ? err : new Error(String(err))); + } finally { + // `stop()` aborts mid-fetch and returns through the `signal.aborted` + // branch above, so clearing the timer on the individual exit paths + // misses the one path stop() is built to take. clearTimeout(timeoutId); } }, diff --git a/tests/unit/logstream-timer-leak-13113.test.ts b/tests/unit/logstream-timer-leak-13113.test.ts new file mode 100644 index 0000000000..111ad34c9c --- /dev/null +++ b/tests/unit/logstream-timer-leak-13113.test.ts @@ -0,0 +1,98 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; + +import { createLogStream } from "../../src/lib/cli-helper/log-streamer.ts"; + +function armedTimers(): number { + return process.getActiveResourcesInfo().filter((r) => r === "Timeout").length; +} + +async function startServer(): Promise<{ port: number; close: () => Promise }> { + const open: http.ServerResponse[] = []; + const server = http.createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "text/plain" }); + res.write("log line\n"); + // Deliberately left open: stop() must land while the stream is still live. + open.push(res); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { + port, + close: async () => { + for (const res of open) res.end(); + await new Promise((resolve) => server.close(() => resolve())); + }, + }; +} + +test("stop() clears the stream timeout timer", async () => { + const server = await startServer(); + try { + const before = armedTimers(); + + const streams = Array.from({ length: 8 }, () => + createLogStream({ + baseUrl: `http://127.0.0.1:${server.port}`, + follow: true, + // Long enough that a leaked timer is still armed when we measure. + timeout: 120_000, + }) + ); + + // Begin consuming so start() runs and the fetch is in flight. + for (const s of streams) { + void s.stream + .getReader() + .read() + .catch(() => {}); + } + await new Promise((r) => setTimeout(r, 300)); + + for (const s of streams) s.stop(); + await new Promise((r) => setTimeout(r, 500)); + + const after = armedTimers(); + assert.ok( + after <= before, + `stopping 8 streams retained ${after - before} armed timer(s) ` + + `(before=${before} after=${after}); stop() must clear the timeout` + ); + } finally { + await server.close(); + } +}); + +test("a stream that ends normally still clears its timer", async () => { + const finished = http.createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "text/plain" }); + res.end("done\n"); + }); + await new Promise((resolve) => finished.listen(0, "127.0.0.1", resolve)); + const { port } = finished.address() as AddressInfo; + + try { + const before = armedTimers(); + const { stream } = createLogStream({ + baseUrl: `http://127.0.0.1:${port}`, + follow: false, + timeout: 120_000, + }); + + const reader = stream.getReader(); + while (true) { + const { done } = await reader.read(); + if (done) break; + } + await new Promise((r) => setTimeout(r, 200)); + + assert.ok( + armedTimers() <= before, + "a normally-completed stream must not leave its timeout armed" + ); + } finally { + await new Promise((resolve) => finished.close(() => resolve())); + } +}); From e1cfdb5e48a9d9fcdd3699585f35156077d72634 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:30:44 +0700 Subject: [PATCH 13/19] fix(acp): release listeners, timers and sessions on every sendPrompt outcome (#13096) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `acpManager` being a module-level singleton is what turns this from a per-call leak into unbounded growth — the `MaxListenersExceededWarning` at 11 is the visible symptom. Routing every outcome through one `settle()` is the right shape, and deleting the session on the child's own exit fixes the map growth that `getActiveSessions()`'s `alive` filter was hiding. I reformatted the changelog fragment to the `changelog.d` convention (`- **fix(scope):** …`) before merging — `check:changelog-integrity` rejects a fragment that does not start with a markdown bullet, which is the same gate your #13158 was about. Wording is yours, unchanged in substance. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- .../13095-acp-sendprompt-listener-leak.md | 1 + src/lib/acp/manager.ts | 34 +++--- .../acp-manager-sendprompt-leak-13095.test.ts | 101 ++++++++++++++++++ 3 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 changelog.d/fixes/13095-acp-sendprompt-listener-leak.md create mode 100644 tests/unit/acp-manager-sendprompt-leak-13095.test.ts diff --git a/changelog.d/fixes/13095-acp-sendprompt-listener-leak.md b/changelog.d/fixes/13095-acp-sendprompt-listener-leak.md new file mode 100644 index 0000000000..56473cc8c2 --- /dev/null +++ b/changelog.d/fixes/13095-acp-sendprompt-listener-leak.md @@ -0,0 +1 @@ +- **fix(acp):** release the `stdout`/`exit` listeners and the idle timer that a `sendPrompt` timeout used to leave attached to the `acpManager` singleton, and drop sessions that exited on their own from the session map instead of keeping them forever. diff --git a/src/lib/acp/manager.ts b/src/lib/acp/manager.ts index 85bc05e720..85725fd5fc 100644 --- a/src/lib/acp/manager.ts +++ b/src/lib/acp/manager.ts @@ -90,6 +90,10 @@ export class AcpManager extends EventEmitter { child.on("exit", (code, signal) => { session.alive = false; + // Only kill() used to remove entries, so any agent that exited on its own + // stayed in the map forever. getActiveSessions() filters on `alive`, which + // hid the growth from callers. + this.sessions.delete(sessionId); this.emit("exit", { sessionId, code, signal }); }); @@ -129,31 +133,35 @@ export class AcpManager extends EventEmitter { // Wait for response (collect until process goes idle or timeout) return new Promise((resolve, reject) => { - const timer = setTimeout(() => { - reject(new Error(`ACP timeout after ${timeoutMs}ms`)); - }, timeoutMs); + let idleTimer: ReturnType | undefined; - let idleTimer: ReturnType; + // Every outcome -- idle, exit, or timeout -- has to release the same + // resources. `acpManager` is a module-level singleton, so a branch that + // skips this leaks a listener per call for the lifetime of the process. + const settle = (finish: () => void) => { + clearTimeout(timer); + clearTimeout(idleTimer); + this.removeListener("stdout", onData); + this.removeListener("exit", onExit); + finish(); + }; + + const timer = setTimeout(() => { + settle(() => reject(new Error(`ACP timeout after ${timeoutMs}ms`))); + }, timeoutMs); const onData = ({ sessionId: sid }: { sessionId: string }) => { if (sid !== sessionId) return; // Reset idle timer on new data clearTimeout(idleTimer); idleTimer = setTimeout(() => { - clearTimeout(timer); - this.removeListener("stdout", onData); - this.removeListener("exit", onExit); - resolve(session.stdoutBuffer); + settle(() => resolve(session.stdoutBuffer)); }, 2000); // 2s idle = response complete }; const onExit = ({ sessionId: sid }: { sessionId: string }) => { if (sid !== sessionId) return; - clearTimeout(timer); - clearTimeout(idleTimer); - this.removeListener("stdout", onData); - this.removeListener("exit", onExit); - resolve(session.stdoutBuffer); + settle(() => resolve(session.stdoutBuffer)); }; this.on("stdout", onData); diff --git a/tests/unit/acp-manager-sendprompt-leak-13095.test.ts b/tests/unit/acp-manager-sendprompt-leak-13095.test.ts new file mode 100644 index 0000000000..d3b5dce291 --- /dev/null +++ b/tests/unit/acp-manager-sendprompt-leak-13095.test.ts @@ -0,0 +1,101 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { AcpManager } = await import("../../src/lib/acp/manager.ts"); +const { setCustomAgents } = await import("../../src/lib/acp/registry.ts"); + +// A registered agent whose binary is just node running a script that stays quiet, +// so sendPrompt() reliably hits its timeout instead of resolving on data/exit. +const AGENT_ID = "acp-leak-probe"; +setCustomAgents([ + { + id: AGENT_ID, + name: "ACP leak probe", + binary: process.execPath, + description: "test-only agent", + }, +]); + +function spawnIdleSession(manager) { + // Keeps stdin open and never writes to stdout: the prompt can only time out. + return manager.spawn(AGENT_ID, process.execPath, [ + "-e", + "process.stdin.resume(); setTimeout(() => {}, 60_000);", + ]); +} + +test("sendPrompt timeout does not leak listeners on the manager (#13095)", async () => { + const manager = new AcpManager(); + const session = spawnIdleSession(manager); + + try { + const before = { + stdout: manager.listenerCount("stdout"), + exit: manager.listenerCount("exit"), + }; + + // Each of these must reject on the timeout path. + for (let i = 0; i < 12; i++) { + await assert.rejects( + () => manager.sendPrompt(session.id, "ping", 15), + /ACP timeout after 15ms/, + `attempt ${i + 1} should time out` + ); + } + + // The timeout branch has to tear down both listeners it registered. Before the + // fix these grew by one per timed-out prompt and were never released, which + // matters because `acpManager` is a module-level singleton. + assert.equal( + manager.listenerCount("stdout"), + before.stdout, + "stdout listeners must return to the pre-prompt count" + ); + assert.equal( + manager.listenerCount("exit"), + before.exit, + "exit listeners must return to the pre-prompt count" + ); + } finally { + manager.killAll(); + } +}); + +test("sendPrompt timeout clears its idle timer so the process can settle (#13095)", async () => { + const manager = new AcpManager(); + const session = spawnIdleSession(manager); + + try { + await assert.rejects( + () => manager.sendPrompt(session.id, "ping", 15), + /ACP timeout after 15ms/ + ); + + // A leaked idle timer keeps a 2s handle (and the captured session) alive after + // the promise already rejected. Nothing should be pending on the manager. + assert.equal(manager.listenerCount("stdout"), 0); + assert.equal(manager.listenerCount("exit"), 0); + } finally { + manager.killAll(); + } +}); + +test("exited sessions are removed from the session map (#13095)", async () => { + const manager = new AcpManager(); + // Exits immediately on its own; nothing calls kill() for it. + const session = manager.spawn(AGENT_ID, process.execPath, ["-e", "process.exit(0)"]); + + await new Promise((resolve) => { + manager.on("exit", ({ sessionId }) => { + if (sessionId === session.id) resolve(); + }); + }); + // Let the exit handler finish its bookkeeping. + await new Promise((resolve) => setTimeout(resolve, 50)); + + assert.equal( + manager.getSession(session.id), + undefined, + "a session that exited on its own must not stay in the map" + ); +}); From 20abd89d7cf33d469f97286f3bfc8e17a31137d9 Mon Sep 17 00:00:00 2001 From: anhtahaylove <37265396+anhtahaylove@users.noreply.github.com> Date: Fri, 11 Sep 2026 23:32:51 +0700 Subject: [PATCH 14/19] fix(acp): bound session output buffers and reset stderr per prompt (#13100) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keeping the tail is the right direction — `sendPrompt` resolves with the stdout collected since the prompt was written and stderr is read for diagnostics after a failure, so the newest output is what callers actually use. The `[...output truncated...]` marker keeps it from being silent. Resetting `stderrBuffer` alongside `stdoutBuffer` fixes the subtler half: diagnostics for one prompt were carrying stale output from every earlier one. Verified on the tree that actually ships — your branch merged onto the current tip, which already carries #13096: `appendCapped()` and `settle()` coexist cleanly and all 7 assertions across both ACP test files pass together. I reformatted the changelog fragment to the `changelog.d` convention (`- **fix(scope):** …`) before merging — `check:changelog-integrity` rejects a fragment that does not start with a markdown bullet, which is the same gate your #13158 was about. Wording is yours, unchanged in substance. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them. - `typecheck:core` clean - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 71 focused assertions green across the 13 test files this batch adds or touches ⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff. Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit. --- changelog.d/fixes/13095-acp-buffer-cap.md | 1 + src/lib/acp/manager.ts | 37 ++++- .../unit/acp-manager-buffer-cap-13095.test.ts | 143 ++++++++++++++++++ 3 files changed, 178 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fixes/13095-acp-buffer-cap.md create mode 100644 tests/unit/acp-manager-buffer-cap-13095.test.ts diff --git a/changelog.d/fixes/13095-acp-buffer-cap.md b/changelog.d/fixes/13095-acp-buffer-cap.md new file mode 100644 index 0000000000..61473dca6d --- /dev/null +++ b/changelog.d/fixes/13095-acp-buffer-cap.md @@ -0,0 +1 @@ +- **fix(acp):** bound the ACP session output buffers — `stdoutBuffer` and `stderrBuffer` now cap at 1 MiB keeping the most recent output behind a visible `[...output truncated...]` marker, and `stderrBuffer` is reset per prompt instead of accumulating for the lifetime of the session. diff --git a/src/lib/acp/manager.ts b/src/lib/acp/manager.ts index 85725fd5fc..099239e9b1 100644 --- a/src/lib/acp/manager.ts +++ b/src/lib/acp/manager.ts @@ -30,6 +30,34 @@ export interface AcpSession { createdAt: Date; } +/** + * Upper bound for each per-session output buffer. + * + * Both buffers grow on every chunk a CLI agent writes and are only reset when + * the next prompt starts, so a chatty or looping agent can grow them without + * limit while the session stays alive. 1 MiB is far above a realistic agent + * response while keeping a stuck session's footprint bounded. + */ +const MAX_BUFFER_CHARS = 1_048_576; + +const TRUNCATION_NOTICE = "\n[...output truncated...]\n"; + +/** + * Append to a buffer, keeping the most recent output when the cap is exceeded. + * + * The tail is what callers care about: `sendPrompt` resolves with the stdout + * collected since the prompt was written, and stderr is read for diagnostics + * after a failure. Dropping from the front keeps both useful. + */ +function appendCapped(buffer: string, chunk: string): string { + const combined = buffer + chunk; + if (combined.length <= MAX_BUFFER_CHARS) return combined; + + const keep = MAX_BUFFER_CHARS - TRUNCATION_NOTICE.length; + if (keep <= 0) return combined.slice(-MAX_BUFFER_CHARS); + return TRUNCATION_NOTICE + combined.slice(-keep); +} + /** * ACP Session Manager * @@ -79,12 +107,12 @@ export class AcpManager extends EventEmitter { }; child.stdout?.on("data", (chunk: Buffer) => { - session.stdoutBuffer += chunk.toString(); + session.stdoutBuffer = appendCapped(session.stdoutBuffer, chunk.toString()); this.emit("stdout", { sessionId, data: chunk.toString() }); }); child.stderr?.on("data", (chunk: Buffer) => { - session.stderrBuffer += chunk.toString(); + session.stderrBuffer = appendCapped(session.stderrBuffer, chunk.toString()); this.emit("stderr", { sessionId, data: chunk.toString() }); }); @@ -125,8 +153,11 @@ export class AcpManager extends EventEmitter { const session = this.sessions.get(sessionId); if (!session?.alive) throw new Error(`Session ${sessionId} is not alive`); - // Clear buffer before sending + // Clear buffers before sending. stderr is reset too: it was previously only + // ever appended to, so diagnostics for one prompt carried stale output from + // every earlier prompt in the session. session.stdoutBuffer = ""; + session.stderrBuffer = ""; // Send prompt this.sendInput(sessionId, prompt + "\n"); diff --git a/tests/unit/acp-manager-buffer-cap-13095.test.ts b/tests/unit/acp-manager-buffer-cap-13095.test.ts new file mode 100644 index 0000000000..31ac053625 --- /dev/null +++ b/tests/unit/acp-manager-buffer-cap-13095.test.ts @@ -0,0 +1,143 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { AcpManager } = await import("../../src/lib/acp/manager.ts"); +const { setCustomAgents } = await import("../../src/lib/acp/registry.ts"); + +const AGENT_ID = "buffer-cap-probe"; +const CAP = 1_048_576; + +/** + * Spawn a node process that writes `bytes` of stdout (or stderr) and stays alive, + * so the buffers can be inspected while the session is still running. + */ +function makeAgent(stream: "stdout" | "stderr", bytes: number) { + setCustomAgents([ + { + id: AGENT_ID, + name: "Buffer cap probe", + binary: process.execPath, + acpSpawnable: true, + }, + ]); + const script = ` + const chunk = "x".repeat(64 * 1024); + let written = 0; + const target = ${bytes}; + while (written < target) { + process.${stream}.write(chunk); + written += chunk.length; + } + setInterval(() => {}, 1000); + `; + return ["-e", script]; +} + +async function waitForOutput(session: { stdoutBuffer: string; stderrBuffer: string }) { + // Give the child time to flush everything it intends to write. + for (let i = 0; i < 60; i++) { + await new Promise((r) => setTimeout(r, 50)); + if (session.stdoutBuffer.length > CAP / 2 || session.stderrBuffer.length > CAP / 2) break; + } + await new Promise((r) => setTimeout(r, 300)); +} + +test("stdout buffer stays bounded when an agent floods it (#13095)", async () => { + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, makeAgent("stdout", 4 * CAP)); + try { + await waitForOutput(session); + assert.ok( + session.stdoutBuffer.length > 0, + "precondition: the probe agent must have written something" + ); + assert.ok( + session.stdoutBuffer.length <= CAP, + `stdoutBuffer grew to ${session.stdoutBuffer.length} chars, above the ${CAP} cap` + ); + } finally { + mgr.kill(session.id); + } +}); + +test("stderr buffer stays bounded when an agent floods it (#13095)", async () => { + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, makeAgent("stderr", 4 * CAP)); + try { + await waitForOutput(session); + assert.ok( + session.stderrBuffer.length > 0, + "precondition: the probe agent must have written something" + ); + assert.ok( + session.stderrBuffer.length <= CAP, + `stderrBuffer grew to ${session.stderrBuffer.length} chars, above the ${CAP} cap` + ); + } finally { + mgr.kill(session.id); + } +}); + +test("truncation keeps the most recent output, not the oldest (#13095)", async () => { + setCustomAgents([ + { + id: AGENT_ID, + name: "Buffer cap probe", + binary: process.execPath, + acpSpawnable: true, + }, + ]); + const script = ` + const chunk = "x".repeat(64 * 1024); + let written = 0; + while (written < ${2 * CAP}) { process.stdout.write(chunk); written += chunk.length; } + process.stdout.write("FINAL-MARKER"); + setInterval(() => {}, 1000); + `; + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, ["-e", script]); + try { + await waitForOutput(session); + // The tail is the part callers use: sendPrompt resolves with stdout, and + // stderr is read for diagnostics after a failure. + assert.ok( + session.stdoutBuffer.endsWith("FINAL-MARKER"), + "the newest output must survive truncation" + ); + assert.ok(session.stdoutBuffer.length <= CAP, "buffer must still respect the cap"); + } finally { + mgr.kill(session.id); + } +}); + +test("stderr is reset between prompts so diagnostics are per-prompt (#13095)", async () => { + setCustomAgents([ + { + id: AGENT_ID, + name: "Buffer cap probe", + binary: process.execPath, + acpSpawnable: true, + }, + ]); + // Echoes stdin back on stdout, and writes a fixed line to stderr per prompt. + const script = ` + process.stdin.on("data", (d) => { + process.stderr.write("warn:" + d.toString().trim() + "\\n"); + process.stdout.write("ok\\n"); + }); + setInterval(() => {}, 1000); + `; + const mgr = new AcpManager(); + const session = mgr.spawn(AGENT_ID, process.execPath, ["-e", script]); + try { + await mgr.sendPrompt(session.id, "first", 6000); + await mgr.sendPrompt(session.id, "second", 6000); + assert.ok( + !session.stderrBuffer.includes("warn:first"), + `stderr from an earlier prompt leaked into the next one: ${JSON.stringify(session.stderrBuffer)}` + ); + assert.ok(session.stderrBuffer.includes("warn:second"), "current prompt's stderr must be kept"); + } finally { + mgr.kill(session.id); + } +}); From 9f0d54a48f40de83c99dba74e71c4488a4948c4e Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:51:13 +0200 Subject: [PATCH 15/19] fix(combo): parse numeric-epoch rateLimitedUntil in hasFutureRateLimitUntil (#13141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct and well-traced: `rate_limited_until` is TEXT but one write path stores a bare epoch that SQLite coerces to `"1781696905131.0"`, which `new Date()` alone reads as `NaN` — so a still-cooling connection looked available and combo fed it traffic that could only come back 429. Routing both readers through the existing tolerant normalizer is the minimal fix, and keeping unreadable values fail-open is the right default. The #3954/#3995 lineage explains exactly why this function never inherited the normalization. This PR also carries the batch's file-size rebaseline, since it merges first and the ceiling has to cover every intermediate state. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them. - `typecheck:core` clean; `check:changelog-integrity` OK - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests - `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on #13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch. ⚠️ base-red inherited: #12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs. Thanks @maxmad64bis. --- .../fixes/13141-breaker-epoch-cooldown.md | 1 + config/quality/file-size-baseline.json | 9 +-- open-sse/services/accountFallback.ts | 3 +- open-sse/services/combo/comboPredicates.ts | 10 ++- .../combo-predicates-epoch-cooldown.test.ts | 63 +++++++++++++++++++ 5 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 changelog.d/fixes/13141-breaker-epoch-cooldown.md create mode 100644 tests/unit/combo-predicates-epoch-cooldown.test.ts diff --git a/changelog.d/fixes/13141-breaker-epoch-cooldown.md b/changelog.d/fixes/13141-breaker-epoch-cooldown.md new file mode 100644 index 0000000000..d66876f65e --- /dev/null +++ b/changelog.d/fixes/13141-breaker-epoch-cooldown.md @@ -0,0 +1 @@ +- **fix(combo):** parse numeric-epoch `rate_limited_until` in the combo cooldown read path ([#13141](https://github.com/diegosouzapw/OmniRoute/pull/13141)) — thanks @maxmad64bis diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index fea4476daa..bbd36cfa5c 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -1,4 +1,5 @@ { + "_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode": "/merge-batch 2026-09-11 (v3.8.51), PRs #13141, #13146 and #12975 by maxmad64bis. Final combined values for the batch, set here on the first PR merged so every intermediate merge state is covered too. src/sse/services/auth.ts 3450->3488 (+38): #13146 adds the narrow ruleScope===model branch to markAccountUnavailable (gated on status 400; every other status keeps its path) plus the HONORS_RULE_LOCK_SCOPE_PROVIDERS opencode entry, and #12975 adds buildExhaustionOptions so the exhaustion log lines carry the request correlationId. open-sse/executors/base.ts 1751->1757 (+6): #12975 adds the optional ExecuteInput.correlationId field with its doc comment (+2); the other +4 is prettier splitting the cliFingerprints import, a 103-char line the tip left unformatted, which lint-staged rewrites on any commit touching the file. src/sse/handlers/chat.ts 2458->2460 (+2): #12975 threads correlationId through the three executor call sites (+2) and prettier splits a 168-char comboTargetPassesKeyModelPolicy condition (+8), same unformatted-tip cause; the tip itself sits 9 lines under its own freeze, which absorbs the rest. open-sse/services/accountFallback.ts 2467->2468 (+1): #13141 routes hasFutureRateLimitUntil through the tolerant epoch normalizer; #13146 is net zero there (+16/-16). No new branching beyond the two guarded branches named above. open-sse/utils/stream.ts is deliberately NOT rebaselined: it is already 3115 > 3098 on the pure tip with zero contribution from this batch (base-red #12732, owned by /sweep-reds). Covered by tests/unit/combo-predicates-epoch-cooldown.test.ts, opencode-400-model-unavailable.test.ts, agentrouter-error-rules.test.ts, opencode-transient-rotation.test.ts and chat-correlation-id-exhaustion.test.ts.", "_rebaseline_2026_09_10_mergebatch_v3851_greenpt_eurouter": "/merge-batch 2026-09-10 (v3.8.51), PRs #13024 (GreenPT, closes #12986) and #13025 (EURouter, closes #12985) by ntdatt812: src/shared/constants/providers/apikey/gateways.ts 1462->1502 (+40 = two APIKEY_PROVIDERS_GATEWAYS catalog entries, declarative data only: id/alias/name/icon/color/website plus the hasFree=false rationale comments and the apiHint copy each PR verified). No logic and no new branching. Same god-file no-split rationale as every prior gateways.ts rebaseline (#11786 seekai, #10987 logfare, #10668 tabitoken, #10531 freebuff, #11631 1min.ai): the file header says it is pure data merged by apikey/index.ts via spread, and it is already split into 6 family files under apikey/, so splitting a catalog for two entries would violate the semantic-families rule rather than help. Both entries are deliberately conservative (models: [] with passthroughModels, no tool/vision capability declared, hasFree false), so the growth is the entry itself, not claims. EURouter is in AGGREGATOR_PROVIDER_IDS because it routes to third-party upstreams; GreenPT is not because it serves its own inference. Covered by tests/unit/greenpt-provider.test.ts and tests/unit/eurouter-provider.test.ts.", "_rebaseline_2026_09_10_12828_translate_usage_chunk": "PR #12828 own growth: open-sse/utils/stream.ts 3072->3080 (+8). Translate-mode streams now send the estimated usage as the canonical trailing usage-only chunk before [DONE] when the upstream stays silent (parity with the #12151 passthrough flush), with a latch so a finish chunk that already carried the estimate is not doubled. The chunk builder is shared with the passthrough flush in open-sse/utils/usageOnlyChunk.ts (under cap); what remains is the flush-site wiring. Covered by tests/unit/stream-translate-usage-trailing.test.ts.", "_rebaseline_2026_09_10_12715_queue_budget": "PR #12715 own growth: open-sse/handlers/chatCore.ts 6021->6036 (+15). Hierarchical admission now resolves the per-connection queue budget before the gates and hands withRateLimit the remaining budget, the correlation id and the executor timeout context, so gate wait, provider slot and Bottleneck queue share one bound instead of stacking. Error shaping lives in open-sse/handlers/chatCore/queueBudget.ts (under cap); what remains is irreducible call-site wiring. Covered by tests/unit/rate-limit-remaining-budget.test.ts, rate-limit-manager-queue-bound.test.ts and chatcore-hierarchical-admission.test.ts.", @@ -422,7 +423,7 @@ "_rebaseline_pr1043_minimax_tts": "Upstream port decolua/9router#1043 (toanalien) own growth: audioSpeech.ts 965->1061 (+96). Adds MiniMax T2A v2 TTS dispatch (handleMinimaxSpeech + hexToBytes helper) — provider entry was already in audioRegistry (format: minimax-tts) but no handler existed, falling through to the OpenAI-compatible default that fails (T2A has custom shape + hex-encoded audio + base_resp envelope). New branch sits next to the other inline provider branches (xiaomi-mimo, coqui, tortoise, aws-polly) — extracting would just create indirection. Covered by tests/unit/minimax-tts-1043.test.ts (3 tests, GREEN: success, base_resp error, invalid-hex).", "_rebaseline_pr4592_exclude_exhausted_auto": "Reconcile #4592 already-merged growth: combo.ts 2991->3036 (+45, terminal-status quota-cutoff exclusion in buildAutoCandidates + opt-in gate). Fast-gate PR->release does not run check:file-size.", "open-sse/executors/antigravity.ts": 1665, - "open-sse/executors/base.ts": 1751, + "open-sse/executors/base.ts": 1757, "open-sse/executors/chatgpt-web.ts": 5056, "open-sse/executors/codex.ts": 1505, "open-sse/executors/cursor.ts": 1759, @@ -433,7 +434,7 @@ "open-sse/handlers/search.ts": 1789, "open-sse/mcp-server/schemas/tools.ts": 1621, "open-sse/mcp-server/server.ts": 1572, - "open-sse/services/accountFallback.ts": 2467, + "open-sse/services/accountFallback.ts": 2468, "open-sse/services/adobeFireflyBrowserLogin.ts": 1401, "open-sse/services/combo.ts": 4080, "open-sse/services/combo/executeTargetAttempt.ts": 1205, @@ -468,8 +469,8 @@ "src/shared/components/RequestLoggerV2.tsx": 1718, "src/shared/constants/providers/apikey/gateways.ts": 1502, "src/shared/services/cliRuntime.ts": 1296, - "src/sse/handlers/chat.ts": 2458, - "src/sse/services/auth.ts": 3450, + "src/sse/handlers/chat.ts": 2460, + "src/sse/services/auth.ts": 3488, "tests/unit/account-fallback-service.test.ts": 2453, "tests/unit/provider-validation-specialty.test.ts": 4656, "open-sse/services/autoCombo/virtualFactory.ts": 1219, diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index c6465321f9..2f05ffc6e7 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -2321,7 +2321,8 @@ export function formatRetryAfter( rateLimitedUntil: string | number | Date | null | undefined ): string { if (!rateLimitedUntil) return ""; - const diffMs = new Date(rateLimitedUntil).getTime() - Date.now(); + const diffMs = cooldownUntilMs(rateLimitedUntil) - Date.now(); + if (!Number.isFinite(diffMs)) return ""; if (diffMs <= 0) return "reset after 0s"; const totalSec = Math.ceil(diffMs / 1000); const h = Math.floor(totalSec / 3600); diff --git a/open-sse/services/combo/comboPredicates.ts b/open-sse/services/combo/comboPredicates.ts index 6359607cac..4a7b6a20e1 100644 --- a/open-sse/services/combo/comboPredicates.ts +++ b/open-sse/services/combo/comboPredicates.ts @@ -16,7 +16,11 @@ import { isLocalExecutionError, isModelCapacityOverloadError, } from "@/shared/utils/circuitBreaker"; -import { CONTEXT_OVERFLOW_PATTERNS, MODEL_ACCESS_DENIED_PATTERNS } from "../accountFallback.ts"; +import { + CONTEXT_OVERFLOW_PATTERNS, + MODEL_ACCESS_DENIED_PATTERNS, + cooldownUntilMs, +} from "../accountFallback.ts"; import { isResourceNotFoundResponse } from "../errorClassifier.ts"; import { getTrustedLocalRateLimitResponse } from "../rateLimitManager/errors.ts"; import type { ResolvedComboTarget } from "./types.ts"; @@ -476,7 +480,9 @@ export function normalizeConnectionStatus(value: unknown): string { export function hasFutureRateLimitUntil(value: unknown): boolean { if (value == null || value === "") return false; - const time = new Date(String(value)).getTime(); + if (typeof value !== "string" && typeof value !== "number" && !(value instanceof Date)) + return false; + const time = cooldownUntilMs(value); return Number.isFinite(time) && time > Date.now(); } diff --git a/tests/unit/combo-predicates-epoch-cooldown.test.ts b/tests/unit/combo-predicates-epoch-cooldown.test.ts new file mode 100644 index 0000000000..a038ce43ca --- /dev/null +++ b/tests/unit/combo-predicates-epoch-cooldown.test.ts @@ -0,0 +1,63 @@ +/** + * Regression: `hasFutureRateLimitUntil` parses with `new Date(String(value))` + * alone, so a numeric-epoch string from the TEXT `rate_limited_until` column + * (e.g. a `${Date.now()}.0`-shaped value, cf. #3954) yields NaN and the + * still-cooling connection is never skipped (fail-open → guaranteed upstream + * 429). `formatRetryAfter` has the same blind spot and renders + * "reset after NaNs". + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +const { hasFutureRateLimitUntil } = + await import("../../open-sse/services/combo/comboPredicates.ts"); +const { formatRetryAfter } = await import("../../open-sse/services/accountFallback.ts"); + +const HOUR = 3_600_000; + +test("hasFutureRateLimitUntil: future numeric-epoch string is future", () => { + assert.equal(hasFutureRateLimitUntil(`${Date.now() + HOUR}.0`), true); +}); + +test("hasFutureRateLimitUntil: future numeric epoch number is future", () => { + assert.equal(hasFutureRateLimitUntil(Date.now() + HOUR), true); +}); + +test("hasFutureRateLimitUntil: past numeric-epoch string is not future", () => { + assert.equal(hasFutureRateLimitUntil(String(Date.now() - HOUR)), false); +}); + +test("hasFutureRateLimitUntil: future ISO string is future (unchanged)", () => { + assert.equal(hasFutureRateLimitUntil(new Date(Date.now() + HOUR).toISOString()), true); +}); + +test("hasFutureRateLimitUntil: empty/null/undefined/blank is not future (unchanged)", () => { + assert.equal(hasFutureRateLimitUntil(""), false); + assert.equal(hasFutureRateLimitUntil(null), false); + assert.equal(hasFutureRateLimitUntil(undefined), false); + assert.equal(hasFutureRateLimitUntil(" "), false); +}); + +test("hasFutureRateLimitUntil: garbage is not future (unchanged)", () => { + assert.equal(hasFutureRateLimitUntil("abc"), false); +}); + +test("hasFutureRateLimitUntil: non-string values never throw (narrowing)", () => { + assert.equal(hasFutureRateLimitUntil(true), false); + assert.equal(hasFutureRateLimitUntil({}), false); + assert.equal(hasFutureRateLimitUntil([]), false); +}); + +test("formatRetryAfter: future numeric-epoch string renders a duration", () => { + const rendered = formatRetryAfter(`${Date.now() + HOUR}.0`); + assert.match(rendered, /^reset after \d/); + assert.doesNotMatch(rendered, /NaN/); +}); + +test("formatRetryAfter: past numeric-epoch string renders reset after 0s", () => { + assert.equal(formatRetryAfter(String(Date.now() - HOUR)), "reset after 0s"); +}); + +test("formatRetryAfter: garbage renders empty (unknown, not expired)", () => { + assert.equal(formatRetryAfter("abc"), ""); +}); From 3156643f6c88a99d75d4efffec1de7af4a140d5a Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:51:17 +0200 Subject: [PATCH 16/19] fix(opencode): require an http(s) baseURL in both OpenCode plugins (#13142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real and nasty precisely because it is silent: `z.string().url()` accepts `localhost:20128` as scheme `localhost:` plus a path, every model gets published with an unusable api url, and the failure happens inside the client so the gateway logs show nothing. Backing the option schema, the publish boundary and the snapshot filter with one `isHttpUrl` in v2 is the right call — those three cannot drift apart. Duplicating the predicate in v1 rather than sharing it is also correct, since the two packages ship independently. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them. - `typecheck:core` clean; `check:changelog-integrity` OK - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests - `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on #13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch. ⚠️ base-red inherited: #12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs. Thanks @maxmad64bis. --- @omniroute/opencode-plugin-v2/src/cache.ts | 14 ++-- @omniroute/opencode-plugin-v2/src/catalog.ts | 10 +++ @omniroute/opencode-plugin-v2/src/options.ts | 7 +- .../src/shared/models-map.ts | 16 +++++ .../opencode-plugin-v2/tests/options.test.ts | 32 +++++++++ .../tests/snapshot-stale-entries.test.ts | 70 +++++++++++++++++-- @omniroute/opencode-plugin/src/index.ts | 22 +++++- .../tests/options-schema.test.ts | 20 ++++++ .../fixes/13142-plugin-v2-model-api-url.md | 1 + docs/guides/OPENCODE-V2-PLUGIN.md | 2 +- 10 files changed, 182 insertions(+), 12 deletions(-) create mode 100644 changelog.d/fixes/13142-plugin-v2-model-api-url.md diff --git a/@omniroute/opencode-plugin-v2/src/cache.ts b/@omniroute/opencode-plugin-v2/src/cache.ts index 58aca429e4..0f7ad948bd 100644 --- a/@omniroute/opencode-plugin-v2/src/cache.ts +++ b/@omniroute/opencode-plugin-v2/src/cache.ts @@ -10,6 +10,7 @@ import type { OmniRouteRawCombo, OmniRouteRawModelEntry, } from "./shared/index.js"; +import { isHttpUrl } from "./shared/index.js"; export const DEFAULT_MODEL_CACHE_TTL_MS = 300_000 as const; @@ -34,8 +35,9 @@ export const SNAPSHOT_FORMAT_VERSION = 2 as const; /** * A raw snapshot entry is stale when it cannot be mapped to a publishable - * model: no string `id` (unroutable) or a pre-mapped `api` block without a - * valid `npm` package (the runner would reject it as `Unsupported package`). + * model: no string `id` (unroutable), or a pre-mapped `api` block missing a + * valid `npm` package (the runner would reject it as `Unsupported package`) + * or a usable `url` (the host would reach the AI SDK with no baseURL). * Plain `/v1/models` entries carry no `api` block -- it is synthesized at * publish time -- so only a present-but-invalid block drops the entry. */ @@ -47,7 +49,11 @@ export function isStaleSnapshotModel(entry: unknown): boolean { if (api === undefined) return false; if (!api || typeof api !== "object") return true; const npm = (api as { npm?: unknown }).npm; - return typeof npm !== "string" || npm.length === 0; + if (typeof npm !== "string" || npm.length === 0) return true; + // Same requirement as `npm`, and the same predicate the options schema + // applies to `baseURL`: a pre-mapped block without a callable `url` publishes + // a model the host cannot route -- see `legacyApiToInfoApi`. + return !isHttpUrl((api as { url?: unknown }).url); } interface DiskSnapshotV2 { @@ -145,7 +151,7 @@ export async function readDiskSnapshot( (entry) => !isStaleSnapshotModel(entry) ); if (stale > 0) { - logger?.warn(`[omniroute-v2] dropping ${stale} stale snapshot entries without api block`); + logger?.warn(`[omniroute-v2] dropping ${stale} stale snapshot entries with an unusable api block`); } if (models.length === 0) return undefined; return { diff --git a/@omniroute/opencode-plugin-v2/src/catalog.ts b/@omniroute/opencode-plugin-v2/src/catalog.ts index 73c3f4ab70..6766b49b7c 100644 --- a/@omniroute/opencode-plugin-v2/src/catalog.ts +++ b/@omniroute/opencode-plugin-v2/src/catalog.ts @@ -3,6 +3,7 @@ import { type HostContract, detectHostContract, emitsLegacyFields } from "./comp import type { Model as LegacyModelV2 } from "@opencode-ai/sdk/v2"; import type { ModelV2Info, ProviderV2Info } from "@opencode-ai/sdk/v2/types"; import { + isHttpUrl, type ApiFormatV2, type LogLevel, type Logger, @@ -142,6 +143,15 @@ export function legacyApiToInfoApi(api: LegacyModelV2["api"]): ModelV2Info["api" "[omniroute-v2] refusing to publish a model without an api block (missing api.npm)" ); } + // The host reads `api.url` in `prepareOptions` and never falls back to the + // provider's own, so a model published without one reaches the AI SDK with no + // baseURL and fails at call time with a bare `Invalid URL` — no request on the + // wire, nothing in the gateway logs, no model named. + if (!isHttpUrl(api.url)) { + throw new Error( + "[omniroute-v2] refusing to publish a model whose api block carries no http(s) url" + ); + } return { id: api.id, type: "aisdk", package: api.npm, url: api.url }; } diff --git a/@omniroute/opencode-plugin-v2/src/options.ts b/@omniroute/opencode-plugin-v2/src/options.ts index 9782ca74e6..9f9f23041c 100644 --- a/@omniroute/opencode-plugin-v2/src/options.ts +++ b/@omniroute/opencode-plugin-v2/src/options.ts @@ -1,5 +1,7 @@ import { z } from "zod"; +import { isHttpUrl } from "./shared/models-map.js"; + const apiFormatSchema = z .object({ allowAnthropic: z.boolean().optional(), @@ -28,7 +30,10 @@ const pluginOptionsSchema = z .regex(/^[A-Za-z0-9._-]+$/, "providerId may only contain letters, digits, '.', '_' and '-'") .refine((v) => v !== "." && v !== "..", "providerId cannot be a path segment") .default("omniroute"), - baseURL: z.string().url(), + baseURL: z + .string() + .trim() + .refine(isHttpUrl, "baseURL must be an http(s) URL, for example http://localhost:20128"), apiKey: z.string().optional(), displayName: z.string().optional(), managementReadToken: z.string().optional(), diff --git a/@omniroute/opencode-plugin-v2/src/shared/models-map.ts b/@omniroute/opencode-plugin-v2/src/shared/models-map.ts index 625e02f232..a750ef2e0b 100644 --- a/@omniroute/opencode-plugin-v2/src/shared/models-map.ts +++ b/@omniroute/opencode-plugin-v2/src/shared/models-map.ts @@ -111,6 +111,22 @@ function trimTrailingSlashes(value: string): string { * (it appends `/v1/messages` automatically), so callers should branch on * format first. */ +/** + * A url the AI SDK can actually call. `new URL()` alone is not enough: it + * parses `localhost:20128` as the scheme `localhost:` and `ftp://host` as ftp, + * both of which reach `fetch` and fail there. Mirrors the `isHttpUrl` guard the + * settings schema applies to `headroomUrl`. + */ +export function isHttpUrl(value: unknown): boolean { + if (typeof value !== "string") return false; + try { + const { protocol } = new URL(value); + return protocol === "http:" || protocol === "https:"; + } catch { + return false; + } +} + export function ensureV1Suffix(url: string): string { const trimmed = trimTrailingSlashes(url); return trimmed.endsWith("/v1") ? trimmed : `${trimmed}/v1`; diff --git a/@omniroute/opencode-plugin-v2/tests/options.test.ts b/@omniroute/opencode-plugin-v2/tests/options.test.ts index 6a09ade0cc..cf36b1331d 100644 --- a/@omniroute/opencode-plugin-v2/tests/options.test.ts +++ b/@omniroute/opencode-plugin-v2/tests/options.test.ts @@ -29,6 +29,38 @@ describe("parsePluginOptions", () => { it("requires baseURL", () => { assert.throws(() => parsePluginOptions({}), /baseURL/); }); + it("rejects a baseURL that is not an http(s) URL", () => { + // `new URL()` reads "localhost:20128" as the scheme "localhost:" followed + // by a path, so a gateway address typed without "http://" parses. Every + // model would then be published with "localhost:20128/v1" as its api url + // and every call would fail in the client on an unknown scheme, with no + // request on the wire and nothing in the gateway logs. + for (const baseURL of [ + "localhost:20128", + "localhost:20128/v1", + "ftp://gw.example.com/v1", + "gw.example.com/v1", + ]) { + assert.throws( + () => parsePluginOptions({ baseURL }), + /baseURL must be an http\(s\) URL/, + `expected ${baseURL} to be rejected` + ); + } + }); + it("accepts http and https baseURLs, with or without a port or path", () => { + for (const baseURL of [ + "http://localhost:20128/v1", + "http://localhost:20128", + "https://gw.example.com/v1", + "https://gw.example.com/omniroute/v1", + ]) { + assert.equal(parsePluginOptions({ baseURL }).baseURL, baseURL); + // Padding a copied address is trimmed rather than rejected, matching the + // treatment `headroomUrl` already gets in the settings schema. + assert.equal(parsePluginOptions({ baseURL: ` ${baseURL} ` }).baseURL, baseURL); + } + }); it("rejects unknown top-level keys (strict)", () => { assert.throws(() => parsePluginOptions({ baseURL: "https://gw.example.com", bogus: 1 })); }); diff --git a/@omniroute/opencode-plugin-v2/tests/snapshot-stale-entries.test.ts b/@omniroute/opencode-plugin-v2/tests/snapshot-stale-entries.test.ts index 3d08cc5d09..1bf90eff63 100644 --- a/@omniroute/opencode-plugin-v2/tests/snapshot-stale-entries.test.ts +++ b/@omniroute/opencode-plugin-v2/tests/snapshot-stale-entries.test.ts @@ -5,7 +5,11 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { createHash } from "node:crypto"; import plugin from "../src/index.js"; -import { diskSnapshotPath, snapshotIdentityFingerprint } from "../src/cache.js"; +import { + diskSnapshotPath, + isStaleSnapshotModel, + snapshotIdentityFingerprint, +} from "../src/cache.js"; import { legacyApiToInfoApi } from "../src/catalog.js"; function isolateDisk(): { dir: string; restore: () => void } { @@ -97,7 +101,7 @@ function downFetch(): typeof fetch { const fingerprint = snapshotIdentityFingerprint("https://gw.example.com", "k-snapfix", "k-snapfix"); describe("plugin-v2 snapshot stale-entry filter", () => { - it("snapshot with 2 entries without api block + 1 valid: only the valid one is published + warn emitted", async () => { + it("snapshot with 3 unusable pre-mapped entries + 1 valid: only the valid one is published + warn emitted", async () => { const disk = isolateDisk(); const providerId = "snapfix-mixed"; mkdirSync(join(disk.dir, "plugins"), { recursive: true }); @@ -106,11 +110,15 @@ describe("plugin-v2 snapshot stale-entry filter", () => { JSON.stringify({ v: 2, identityFingerprint: fingerprint, - // Two pre-mapped entries with a broken api block (missing npm) plus - // one plain raw entry (no api block: synthesized at publish time). + // Three pre-mapped entries with an unusable api block — missing npm, + // empty npm, and a well-formed npm with no url (the shape a snapshot + // written by an older build carries, and the one that reaches the host + // as a bare `Invalid URL`) — plus one plain raw entry, which has no api + // block at all and gets one synthesized at publish time. models: [ { id: "stale-a", api: {} }, { id: "stale-b", api: { npm: "" } }, + { id: "stale-c", api: { id: "openai-compatible", npm: "@ai-sdk/openai-compatible" } }, { id: "good-1", context_length: 128000 }, ], combos: [], @@ -137,7 +145,7 @@ describe("plugin-v2 snapshot stale-entry filter", () => { ); }); assert.ok( - warns.some((w) => w.includes("dropping 2 stale snapshot entries without api block")), + warns.some((w) => w.includes("dropping 3 stale snapshot entries with an unusable api block")), `expected stale-drop warn, got: ${JSON.stringify(warns)}` ); } finally { @@ -216,4 +224,56 @@ describe("plugin-v2 snapshot stale-entry filter", () => { // Sanity: sha256 helper used above matches the plugin identity scheme. assert.equal(createHash("sha256").update("x").digest("hex").length, 64); }); + + it("legacyApiToInfoApi throws unless api.url is an http(s) url", () => { + const npm = "@ai-sdk/openai-compatible"; + for (const api of [ + { id: "openai-compatible", npm }, + { id: "openai-compatible", npm, url: "" }, + { id: "openai-compatible", npm, url: " " }, + // Non-empty but uncallable: the AI SDK reaches `fetch` and fails there. + { id: "openai-compatible", npm, url: "/v1" }, + { id: "openai-compatible", npm, url: "gw.example.com/v1" }, + { id: "openai-compatible", npm, url: "ftp://gw.example.com/v1" }, + ]) { + assert.throws( + () => legacyApiToInfoApi(api as unknown as { id: string; npm: string; url: string }), + /api block carries no http\(s\) url/, + `expected a publish-time refusal for ${JSON.stringify(api)}` + ); + } + // A complete block still publishes unchanged. + assert.deepEqual( + legacyApiToInfoApi({ + id: "openai-compatible", + npm: "@ai-sdk/openai-compatible", + url: "https://gw.example.com/v1", + }), + { + id: "openai-compatible", + type: "aisdk", + package: "@ai-sdk/openai-compatible", + url: "https://gw.example.com/v1", + } + ); + }); + + it("isStaleSnapshotModel drops a pre-mapped entry whose api.url is unusable", () => { + const npm = "@ai-sdk/openai-compatible"; + // Present-but-unusable url: stale, for the same reason a missing npm is. + for (const url of [undefined, "", " ", "/v1", "gw.example.com/v1", "ftp://gw/v1"]) { + assert.equal( + isStaleSnapshotModel({ id: "a/b", api: { id: "x", npm, ...(url === undefined ? {} : { url }) } }), + true, + `expected ${JSON.stringify(url)} to be treated as stale` + ); + } + // Complete block: publishable. + assert.equal( + isStaleSnapshotModel({ id: "a/b", api: { id: "x", npm, url: "https://gw/v1" } }), + false + ); + // No api block at all stays publishable: it is synthesized at publish time. + assert.equal(isStaleSnapshotModel({ id: "a/b" }), false); + }); }); diff --git a/@omniroute/opencode-plugin/src/index.ts b/@omniroute/opencode-plugin/src/index.ts index 4738c0e422..39e3c6f274 100644 --- a/@omniroute/opencode-plugin/src/index.ts +++ b/@omniroute/opencode-plugin/src/index.ts @@ -220,7 +220,11 @@ const optionsSchema = z * to 60000. Default when unset: 300000. */ autoSyncIntervalMs: z.number().int().nonnegative().optional(), - baseURL: z.string().url().optional(), + baseURL: z + .string() + .trim() + .refine(isHttpUrl, "baseURL must be an http(s) URL, for example http://localhost:20128") + .optional(), managementReadToken: z.string().min(1).optional(), features: featuresSchema.optional(), }) @@ -482,6 +486,22 @@ export const DEFAULT_ANTHROPIC_PREFIXES = ["cc", "claude", "anthropic", "kiro", * (it appends `/v1/messages` automatically), so callers should branch on * format first. */ +/** + * A url the AI SDK can actually call. `new URL()` alone is not enough: it + * parses `localhost:20128` as the scheme `localhost:` and `ftp://host` as ftp, + * both of which reach `fetch` and fail there. Mirrors the `isHttpUrl` guard the + * settings schema applies to `headroomUrl`. + */ +export function isHttpUrl(value: unknown): boolean { + if (typeof value !== "string") return false; + try { + const { protocol } = new URL(value); + return protocol === "http:" || protocol === "https:"; + } catch { + return false; + } +} + export function ensureV1Suffix(url: string): string { const trimmed = trimTrailingSlashes(url); return trimmed.endsWith("/v1") ? trimmed : `${trimmed}/v1`; diff --git a/@omniroute/opencode-plugin/tests/options-schema.test.ts b/@omniroute/opencode-plugin/tests/options-schema.test.ts index 435363946c..e941276247 100644 --- a/@omniroute/opencode-plugin/tests/options-schema.test.ts +++ b/@omniroute/opencode-plugin/tests/options-schema.test.ts @@ -59,6 +59,26 @@ test("parseOmniRoutePluginOptions: invalid baseURL (not a URL) → throws", () = assert.throws(() => parseOmniRoutePluginOptions({ baseURL: "not-a-url" }), /baseURL/i); }); +test("parseOmniRoutePluginOptions: baseURL without an http(s) scheme → throws", () => { + // `new URL()` reads "localhost:20128" as the scheme "localhost:" followed by + // a path, so the address parses and the models are published with an api url + // no client can call. + for (const baseURL of ["localhost:20128", "localhost:20128/v1", "ftp://or.example.com", "or.example.com"]) { + assert.throws( + () => parseOmniRoutePluginOptions({ baseURL }), + /baseURL must be an http\(s\) URL/, + `expected ${baseURL} to be rejected` + ); + } +}); + +test("parseOmniRoutePluginOptions: http and https baseURLs are accepted, padding trimmed", () => { + for (const baseURL of ["http://localhost:20128", "https://or.example.com/v1"]) { + assert.equal(parseOmniRoutePluginOptions({ baseURL }).baseURL, baseURL); + assert.equal(parseOmniRoutePluginOptions({ baseURL: ` ${baseURL} ` }).baseURL, baseURL); + } +}); + test("parseOmniRoutePluginOptions: unknown key → throws (strict mode catches typos)", () => { assert.throws( () => diff --git a/changelog.d/fixes/13142-plugin-v2-model-api-url.md b/changelog.d/fixes/13142-plugin-v2-model-api-url.md new file mode 100644 index 0000000000..34f1c9a172 --- /dev/null +++ b/changelog.d/fixes/13142-plugin-v2-model-api-url.md @@ -0,0 +1 @@ +- **fix(opencode):** both OpenCode plugins now reject a gateway address typed without `http://` at configuration time, instead of publishing every model with an api url no client can call, and the v2 plugin no longer publishes a model card whose api url is blank or relative ([#13142](https://github.com/diegosouzapw/OmniRoute/pull/13142)) — thanks @maxmad64bis diff --git a/docs/guides/OPENCODE-V2-PLUGIN.md b/docs/guides/OPENCODE-V2-PLUGIN.md index c79c31dac6..cc27ab54f2 100644 --- a/docs/guides/OPENCODE-V2-PLUGIN.md +++ b/docs/guides/OPENCODE-V2-PLUGIN.md @@ -77,7 +77,7 @@ naming the endpoint and what was lost — so a degraded picker is never a myster | Key | Default | Notes | | -------------------------------- | ---------------------------------------------- | ------------------------------------------------------------------------------------------------------ | | `providerId` | `"omniroute"` | Provider id, integration id, and the prefix models appear under | -| `baseURL` | required | Gateway root; the `/v1` suffix is added where needed | +| `baseURL` | required | Gateway root, `http(s)` only; the `/v1` suffix is added where needed | | `apiKey` | connected credential, then `OMNIROUTE_API_KEY` | Chat key for `/v1/*` | | `managementReadToken` | falls back to `apiKey` | Key for `/api/*` — usually **not** the same one | | `displayName` | `"OmniRoute"` | Provider name in the picker | From cc4f7ed1c4b368f46994759289a79d1dfcc42dab Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:51:21 +0200 Subject: [PATCH 17/19] fix(logging): prefer the pipeline over the raw bodies in call-log storage and detail enrichment (#13147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both halves are right, and shipping them together is justified: the ladder dropping `pipeline` first threw away the upstream answer while keeping a prompt it already had a copy of, and `resolvePreviousResponseState` rebuilds continuation history out of exactly that field. The detail-panel fix has to ride along because the size-limit placeholder is a non-empty string, so fixing the ladder alone would let it overwrite a good payload. Re-checking emptiness per side after reading is the actual bug — `responseBody` being one value for both sides is what let a provider payload show as the client response. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them. - `typecheck:core` clean; `check:changelog-integrity` OK - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests - `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on #13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch. ⚠️ base-red inherited: #12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs. Thanks @maxmad64bis. --- .../fixes/13147-bodies-first-artifact.md | 1 + src/lib/usage/callLogArtifacts.ts | 104 ++++++----- src/lib/usage/completedRequestDetails.ts | 35 ++-- .../call-log-artifact-bodies-first.test.ts | 172 ++++++++++++++++++ ...mpleted-detail-pipeline-precedence.test.ts | 100 ++++++++++ 5 files changed, 358 insertions(+), 54 deletions(-) create mode 100644 changelog.d/fixes/13147-bodies-first-artifact.md create mode 100644 tests/unit/call-log-artifact-bodies-first.test.ts create mode 100644 tests/unit/completed-detail-pipeline-precedence.test.ts diff --git a/changelog.d/fixes/13147-bodies-first-artifact.md b/changelog.d/fixes/13147-bodies-first-artifact.md new file mode 100644 index 0000000000..f197b0f657 --- /dev/null +++ b/changelog.d/fixes/13147-bodies-first-artifact.md @@ -0,0 +1 @@ +- **fix(logging):** keep the provider exchange rather than the raw client bodies when a call log exceeds its size budget, and show that recovered payload in the request-detail panel instead of replacing it with the stored response body ([#13147](https://github.com/diegosouzapw/OmniRoute/pull/13147)) — thanks @maxmad64bis diff --git a/src/lib/usage/callLogArtifacts.ts b/src/lib/usage/callLogArtifacts.ts index d0193b26a1..1fe14b98e7 100644 --- a/src/lib/usage/callLogArtifacts.ts +++ b/src/lib/usage/callLogArtifacts.ts @@ -17,6 +17,17 @@ const OMITTED_FOR_SIZE_LIMIT = "[omitted: call log artifact size limit exceeded] const STREAM_CHUNKS_OMITTED_FOR_SIZE_LIMIT = "[stream chunks omitted: call log artifact size limit exceeded]"; +/** + * True for a placeholder a size-limit fallback wrote in place of a real + * payload. Consumers that fall back from one artifact field to another + * (`maybeEnrichCompletedDetail`) must treat a marker as absent: it is a + * non-empty string, so a bare truthiness check happily "recovers" it and + * overwrites the real value it was meant to stand in for. + */ +export function isSizeLimitOmissionMarker(value: unknown): boolean { + return value === OMITTED_FOR_SIZE_LIMIT || value === STREAM_CHUNKS_OMITTED_FOR_SIZE_LIMIT; +} + // The error is the only field that says *why* a request failed, and it is // typically ~90 bytes next to the multi-hundred-KB bodies that trip the cap. // Dropping it made a size-limited row undiagnosable: a provider outage, a local @@ -182,33 +193,49 @@ function buildMinimalArtifactForSizeLimit(artifact: CallLogArtifact) { }; } -function serializeFinalSizeLimitFallback(artifact: CallLogArtifact, maxBytes: number): string { - const withSummary = JSON.stringify(buildMinimalArtifactForSizeLimit(artifact)); - if (Buffer.byteLength(withSummary) <= maxBytes) { - return withSummary; - } - - // The summary alone exceeded the cap (pathological). Keep the error so the - // row stays diagnosable, drop everything else including the summary body. - const errorOnly = JSON.stringify({ - schemaVersion: artifact.schemaVersion, - _omniroute_truncated: true, - reason: SIZE_LIMIT_EXCEEDED_REASON, +/** + * Fallback ladder for an artifact that does not fit its byte budget, ordered + * from "keeps the most" to "keeps the least": the first stage that fits wins. + * + * Ordering rule: drop the payload that most plausibly tripped the cap, and + * drop a payload that is *duplicated elsewhere in the artifact* before one + * that is unique. `pipeline` carries both sides of the exchange already + * translated (`clientRawRequest`/`providerRequest`/`providerResponse`/ + * `clientResponse`), so evicting it to keep `requestBody` traded the whole + * upstream exchange -- including the only record of what the provider + * actually answered -- for a raw client prompt the pipeline already holds a + * translated copy of. Bodies go first now, and `pipeline` survives one stage + * longer; the previous order is still reached when dropping the bodies alone + * is not enough. + * + * Two consumers depend on that ordering, not just human diagnosis: + * `resolvePreviousResponseState` (db/responsesContinuationStore.ts) rebuilds + * `previous_response_id` history from `pipeline.clientRawRequest` / + * `pipeline.clientResponse` and returns null -- forcing the client to resend + * full history -- for any artifact whose pipeline was omitted; and + * `maybeEnrichCompletedDetail` (usage/completedRequestDetails.ts) reads + * `pipeline.providerResponse` in preference to `responseBody`. + */ +function buildSizeLimitStages(artifact: CallLogArtifact): Array<() => unknown> { + const omitBodies = (value: T) => ({ + ...value, + requestBody: OMITTED_FOR_SIZE_LIMIT, + responseBody: OMITTED_FOR_SIZE_LIMIT, error: preserveErrorForSizeLimit(artifact.error), }); - if (Buffer.byteLength(errorOnly) <= maxBytes) { - return errorOnly; - } - // Last resort: even the error-only payload did not fit. The error still - // rides along -- without it this row says only "something was too big", - // which is the state this change exists to remove. - return JSON.stringify({ - schemaVersion: artifact.schemaVersion, - _omniroute_truncated: true, - reason: SIZE_LIMIT_EXCEEDED_REASON, - error: preserveErrorForSizeLimit(artifact.error), - }); + return [ + () => truncateArtifactForStorage(artifact), + // Bodies alone: worth a stage only when there is a pipeline to keep in + // exchange. Without one it produces the same bytes as the stage two lines + // below, so it is left out rather than costing a redundant stringify. + ...(artifact.pipeline ? [() => omitBodies(artifact)] : []), + () => omitOversizedPipeline(artifact), + () => omitBodies(omitOversizedPipeline(artifact)), + // The summary alone exceeded the cap (pathological). Keep the error so the + // row stays diagnosable, drop everything else including the summary body. + () => buildMinimalArtifactForSizeLimit(artifact), + ]; } function serializeArtifactForStorage(artifact: CallLogArtifact): string { @@ -227,27 +254,22 @@ function serializeArtifactForStorage(artifact: CallLogArtifact): string { return serialized; } - const truncated = JSON.stringify(truncateArtifactForStorage(artifact)); - if (Buffer.byteLength(truncated) <= maxBytes) { - return truncated; + for (const buildStage of buildSizeLimitStages(artifact)) { + const candidate = JSON.stringify(buildStage()); + if (Buffer.byteLength(candidate) <= maxBytes) { + return candidate; + } } - const withoutPipeline = JSON.stringify(omitOversizedPipeline(artifact)); - if (Buffer.byteLength(withoutPipeline) <= maxBytes) { - return withoutPipeline; - } - - const minimal = JSON.stringify({ - ...omitOversizedPipeline(artifact), - requestBody: OMITTED_FOR_SIZE_LIMIT, - responseBody: OMITTED_FOR_SIZE_LIMIT, + // Last resort: not even the summary fit. The error still rides along -- + // without it this row says only "something was too big", which is the state + // the size-limit fallbacks exist to remove. + return JSON.stringify({ + schemaVersion: artifact.schemaVersion, + _omniroute_truncated: true, + reason: SIZE_LIMIT_EXCEEDED_REASON, error: preserveErrorForSizeLimit(artifact.error), }); - if (Buffer.byteLength(minimal) <= maxBytes) { - return minimal; - } - - return serializeFinalSizeLimitFallback(artifact, maxBytes); } export function writeCallArtifact( diff --git a/src/lib/usage/completedRequestDetails.ts b/src/lib/usage/completedRequestDetails.ts index b9d06649bf..ac91b736e2 100644 --- a/src/lib/usage/completedRequestDetails.ts +++ b/src/lib/usage/completedRequestDetails.ts @@ -50,13 +50,14 @@ export function clearCompletedDetails() { completedDetails.clear(); } +function isUnset(value: unknown): boolean { + return value === undefined || value === null; +} + export function maybeEnrichCompletedDetail(updated: PendingRequestDetail, connectionId: string) { void (async () => { try { - const missingProvider = - updated.providerResponse === undefined || updated.providerResponse === null; - const missingClient = updated.clientResponse === undefined || updated.clientResponse === null; - if (!missingProvider && !missingClient) return; + if (!isUnset(updated.providerResponse) && !isUnset(updated.clientResponse)) return; const db = getDbInstance(); const sinceIso = new Date(Date.now() - 30_000).toISOString(); @@ -67,24 +68,32 @@ export function maybeEnrichCompletedDetail(updated: PendingRequestDetail, connec .all(connectionId, updated.model, sinceIso) as Array<{ artifact_relpath: string | null }>; for (const row of rows) { if (!row.artifact_relpath) continue; - const { readCallArtifact } = await import("./callLogArtifacts"); + const { readCallArtifact, isSizeLimitOmissionMarker } = await import("./callLogArtifacts"); const art = readCallArtifact(row.artifact_relpath); if (art.state !== "ready" || !art.artifact) continue; const pipeline = art.artifact.pipeline as | { providerResponse?: unknown; clientResponse?: unknown } | undefined; - if (missingProvider && pipeline?.providerResponse) { + // pipeline.* first: it is the translated payload of one specific side. + // `responseBody` is a single coarse value handed to both sides, so it + // may only fill a side still empty AFTER the pipeline had its turn -- + // testing emptiness once before the loop let it overwrite the payload + // just recovered, showing a provider payload as the client response. + if (isUnset(updated.providerResponse) && pipeline?.providerResponse) { updated.providerResponse = pipeline.providerResponse; } - if (missingClient && pipeline?.clientResponse) { + if (isUnset(updated.clientResponse) && pipeline?.clientResponse) { updated.clientResponse = pipeline.clientResponse; } - if ( - (missingProvider && art.artifact.responseBody) || - (missingClient && art.artifact.responseBody) - ) { - if (missingProvider) updated.providerResponse = art.artifact.responseBody; - if (missingClient) updated.clientResponse = art.artifact.responseBody; + // A size-limited artifact stores an omission marker string in place of + // the body. It is truthy, so recovering it here overwrites a real + // payload with "[omitted: ...]". + const responseBody = isSizeLimitOmissionMarker(art.artifact.responseBody) + ? null + : art.artifact.responseBody; + if (responseBody) { + if (isUnset(updated.providerResponse)) updated.providerResponse = responseBody; + if (isUnset(updated.clientResponse)) updated.clientResponse = responseBody; } if (updated.providerResponse || updated.clientResponse) { if (completedDetails.has(updated.id)) storeCompletedDetail(updated); diff --git a/tests/unit/call-log-artifact-bodies-first.test.ts b/tests/unit/call-log-artifact-bodies-first.test.ts new file mode 100644 index 0000000000..a957b5d48e --- /dev/null +++ b/tests/unit/call-log-artifact-bodies-first.test.ts @@ -0,0 +1,172 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { useDecollidedMigrationsDir } from "./helpers/decollidedMigrationsDir.ts"; + +useDecollidedMigrationsDir(); +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-call-log-bodies-first-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { writeCallArtifact, readCallArtifact, isSizeLimitOmissionMarker } = await import( + "../../src/lib/usage/callLogArtifacts.ts" +); + +const OMITTED = "[omitted: call log artifact size limit exceeded]"; +const PIPELINE_MARKER = { + error: { + _omniroute_truncated: true, + reason: "call_log_artifact_size_limit_exceeded", + }, +}; + +// Pin the budget env for determinism (save/restore idiom per +// call-log-cap.test.ts:32/43-51); never hardcode bytes near 512 KB. +const ORIGINAL_PIPELINE_MAX = process.env.CALL_LOG_PIPELINE_MAX_SIZE_KB; +test.beforeEach(() => { + process.env.CALL_LOG_PIPELINE_MAX_SIZE_KB = "512"; +}); +test.afterEach(() => { + if (ORIGINAL_PIPELINE_MAX === undefined) delete process.env.CALL_LOG_PIPELINE_MAX_SIZE_KB; + else process.env.CALL_LOG_PIPELINE_MAX_SIZE_KB = ORIGINAL_PIPELINE_MAX; +}); + +function artifact(overrides: Record = {}) { + return { + schemaVersion: 5 as const, + summary: { + id: `bodies-first-${Math.random().toString(16).slice(2)}`, + timestamp: new Date().toISOString(), + method: "POST", + path: "/v1/messages", + status: 200, + model: "openai/gpt-4.1", + requestedModel: null, + }, + error: null, + ...overrides, + } as never; +} + +function roundTrip(input: ReturnType) { + const relativePath = `bodies-first/${(input as { summary: { id: string } }).summary.id}.json`; + assert.ok(writeCallArtifact(input, relativePath), "artifact should be written"); + const { artifact: stored, state } = readCallArtifact(relativePath); + assert.equal(state, "ready"); + assert.ok(stored, "artifact should be readable"); + return stored as unknown as Record; +} + +test("artifact bodies-first eviction", async (t) => { + await t.test("body overflow keeps pipeline.providerResponse", async () => { + // Fixture mirrors the observed shape (not a 900KB/tiny toy alone): + // requestBody O(200KB) next to a pipeline sized so the TOTAL just + // exceeds the cap. The bodies are what tripped the cap, so they go + // first and the pipeline survives. + const providerResponse = { + status: 200, + body: { data: "p".repeat(330 * 1024) }, + }; + const stored = roundTrip( + artifact({ + requestBody: "r".repeat(200 * 1024), + responseBody: { output: "response" }, + pipeline: { + providerRequest: { url: "https://provider.example/v1/messages", method: "POST" }, + providerResponse, + }, + }) + ); + + assert.equal(stored.requestBody, OMITTED); + assert.equal(stored.responseBody, OMITTED); + // camelCase per requestLogger.ts:19. + assert.deepEqual( + (stored.pipeline as Record).providerResponse, + providerResponse + ); + }); + + await t.test("pipeline-only overflow keeps current behavior", async () => { + // Small bodies, huge pipeline: the pipeline is what tripped the cap, + // so it is replaced by the marker while the bodies are kept verbatim + // (same contract as call-log-cap.test.ts:597). + const requestBody = { payload: "request" }; + const responseBody = { output: "response" }; + const stored = roundTrip( + artifact({ + requestBody, + responseBody, + pipeline: { + providerRequest: { body: "x".repeat(300 * 1024) }, + providerResponse: { body: "y".repeat(300 * 1024) }, + }, + }) + ); + + assert.deepEqual(stored.requestBody, requestBody); + assert.deepEqual(stored.responseBody, responseBody); + assert.deepEqual(stored.pipeline, PIPELINE_MARKER); + }); + + await t.test("both-large falls through to current minimal", async () => { + // Body AND pipeline each over budget: omitting the bodies alone still + // leaves the pipeline over budget, so the stored form is bodies + // omitted plus the pipeline marker. + const stored = roundTrip( + artifact({ + requestBody: "r".repeat(600 * 1024), + responseBody: { output: "response" }, + pipeline: { + providerRequest: { body: "x".repeat(600 * 1024) }, + providerResponse: { body: "y".repeat(600 * 1024) }, + }, + }) + ); + + assert.equal(stored.requestBody, OMITTED); + assert.equal(stored.responseBody, OMITTED); + assert.deepEqual(stored.pipeline, PIPELINE_MARKER); + }); + await t.test("no pipeline: the stage is skipped, storage is unchanged", async () => { + // Without a pipeline there is nothing for the new stage to save, and its + // output would be byte-identical to the minimal stage below it -- it must + // not fire at all, so an artifact that never had a pipeline keeps exactly + // the shape it had before this change. + const stored = roundTrip( + artifact({ + requestBody: "r".repeat(600 * 1024), + responseBody: { output: "response" }, + error: { message: "upstream 500" }, + }) + ); + + assert.equal(stored.requestBody, OMITTED); + assert.equal(stored.responseBody, OMITTED); + assert.deepEqual(stored.error, { message: "upstream 500" }); + assert.equal(stored.pipeline, undefined); + }); + + await t.test("an omitted body is detectable by consumers, not just truthy", async () => { + // maybeEnrichCompletedDetail (usage/completedRequestDetails.ts) falls back + // from pipeline.providerResponse to responseBody. The marker is a + // non-empty string, so a truthiness check "recovers" it and overwrites the + // pipeline payload this change exists to keep; the shared predicate is the + // contract that stops it. + const stored = roundTrip( + artifact({ + requestBody: "r".repeat(200 * 1024), + responseBody: { output: "response" }, + pipeline: { providerResponse: { status: 200, body: { data: "p".repeat(330 * 1024) } } }, + }) + ); + + assert.ok(stored.responseBody, "the marker is truthy -- that is the trap"); + assert.equal(isSizeLimitOmissionMarker(stored.responseBody), true); + assert.equal(isSizeLimitOmissionMarker(stored.requestBody), true); + assert.equal(isSizeLimitOmissionMarker({ output: "response" }), false); + assert.equal(isSizeLimitOmissionMarker(null), false); + }); +}); diff --git a/tests/unit/completed-detail-pipeline-precedence.test.ts b/tests/unit/completed-detail-pipeline-precedence.test.ts new file mode 100644 index 0000000000..ccbb118891 --- /dev/null +++ b/tests/unit/completed-detail-pipeline-precedence.test.ts @@ -0,0 +1,100 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { useDecollidedMigrationsDir } from "./helpers/decollidedMigrationsDir.ts"; + +useDecollidedMigrationsDir(); +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-completed-detail-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const { writeCallArtifact } = await import("../../src/lib/usage/callLogArtifacts.ts"); +const { maybeEnrichCompletedDetail } = await import( + "../../src/lib/usage/completedRequestDetails.ts" +); + +type PipelinePayloads = { providerResponse?: unknown; clientResponse?: unknown }; + +function seedRow(id: string, connectionId: string, pipeline: PipelinePayloads | undefined) { + const relativePath = `precedence/${id}.json`; + const written = writeCallArtifact( + { + schemaVersion: 5, + summary: { id, timestamp: new Date().toISOString(), model: "openai/gpt-4.1" }, + requestBody: { payload: "request" }, + responseBody: { from: "responseBody" }, + error: null, + ...(pipeline ? { pipeline } : {}), + } as never, + relativePath + ); + assert.ok(written, "artifact should be written"); + + core + .getDbInstance() + .prepare( + `INSERT INTO call_logs (id, timestamp, method, path, status, model, provider, connection_id, detail_state, artifact_relpath) + VALUES (@id, @timestamp, 'POST', '/v1/chat/completions', 200, 'openai/gpt-4.1', 'openai', @connectionId, 'ready', @artifact)` + ) + .run({ id, timestamp: new Date().toISOString(), connectionId, artifact: relativePath }); +} + +// maybeEnrichCompletedDetail is fire-and-forget (`void (async () => …)`), so the +// assertion waits on the mutation instead of on a returned promise. +async function enrich(id: string, connectionId: string) { + const detail = { + id, + model: "openai/gpt-4.1", + provider: "openai", + connectionId, + startedAt: Date.now(), + providerResponse: null, + clientResponse: null, + }; + maybeEnrichCompletedDetail(detail as never, connectionId); + const deadline = Date.now() + 5000; + while (Date.now() < deadline && detail.providerResponse === null) { + await new Promise((resolve) => setTimeout(resolve, 20)); + } + return detail; +} + +test("completed-detail enrichment prefers the pipeline over the body", async (t) => { + await t.test("a body does not overwrite a payload the pipeline already supplied", async () => { + // pipeline.* is the translated, per-side payload; responseBody is one coarse + // value assigned to BOTH sides. Reading the pipeline first and then letting + // the body overwrite it handed the panel the wrong side of the exchange -- + // a provider payload shown as the client response, and vice versa. + const providerResponse = { from: "pipeline.providerResponse" }; + const clientResponse = { from: "pipeline.clientResponse" }; + seedRow("precedence-both", "conn-both", { providerResponse, clientResponse }); + + const detail = await enrich("precedence-both", "conn-both"); + + assert.deepEqual(detail.providerResponse, providerResponse); + assert.deepEqual(detail.clientResponse, clientResponse); + }); + + await t.test("the body still fills a side the pipeline left empty", async () => { + // The fallback itself must survive: with no pipeline at all, responseBody is + // the only payload the artifact carries and both sides take it. + seedRow("precedence-body-only", "conn-body-only", undefined); + + const detail = await enrich("precedence-body-only", "conn-body-only"); + + assert.deepEqual(detail.providerResponse, { from: "responseBody" }); + assert.deepEqual(detail.clientResponse, { from: "responseBody" }); + }); + + await t.test("a half-filled pipeline keeps its side and the body fills the other", async () => { + seedRow("precedence-half", "conn-half", { providerResponse: { from: "pipeline.provider" } }); + + const detail = await enrich("precedence-half", "conn-half"); + + assert.deepEqual(detail.providerResponse, { from: "pipeline.provider" }); + assert.deepEqual(detail.clientResponse, { from: "responseBody" }); + }); +}); From a19bb2227faff22441fb15e4bd1817c1adc87cf9 Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:51:24 +0200 Subject: [PATCH 18/19] fix(providers): lock opencode model on upstream 400 model-unavailable (#13146) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scoping the lock to the MODEL rather than the connection is the right layer for a `400 "Model is unavailable"` — a multi-day upstream outage on one model should not darken the account. Good discipline on the two allowlist-adjacent changes: gating the `markAccountUnavailable` branch on `ruleScope === "model"` AND `status === 400` leaves every other status on its existing path, and deliberately not widening `FULL_TEXT_RULE_PROVIDERS` keeps the #10880 egress-bucketed 429 classification intact. Reading the cooldown from the rule instead of a literal at the call site is what makes it self-healing. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them. - `typecheck:core` clean; `check:changelog-integrity` OK - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests - `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on #13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch. ⚠️ base-red inherited: #12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs. Thanks @maxmad64bis. --- .../fixes/13146-opencode-400-model-lock.md | 1 + open-sse/config/providerErrorRules.ts | 42 ++++- open-sse/services/accountFallback.ts | 32 ++-- open-sse/services/combo/targetExhaustion.ts | 14 +- src/sse/services/auth.ts | 63 ++++++- tests/unit/agentrouter-error-rules.test.ts | 8 +- .../opencode-400-model-unavailable.test.ts | 162 ++++++++++++++++++ 7 files changed, 289 insertions(+), 33 deletions(-) create mode 100644 changelog.d/fixes/13146-opencode-400-model-lock.md create mode 100644 tests/unit/opencode-400-model-unavailable.test.ts diff --git a/changelog.d/fixes/13146-opencode-400-model-lock.md b/changelog.d/fixes/13146-opencode-400-model-lock.md new file mode 100644 index 0000000000..3666287873 --- /dev/null +++ b/changelog.d/fixes/13146-opencode-400-model-lock.md @@ -0,0 +1 @@ +- **fix(providers):** lock opencode model on upstream 400 model-unavailable ([#13146](https://github.com/diegosouzapw/OmniRoute/pull/13146)) — thanks @maxmad64bis diff --git a/open-sse/config/providerErrorRules.ts b/open-sse/config/providerErrorRules.ts index 6f00d2c5f1..4c6e4e094c 100644 --- a/open-sse/config/providerErrorRules.ts +++ b/open-sse/config/providerErrorRules.ts @@ -32,7 +32,7 @@ export type ProviderErrorRuleMatch = { /** * Intended lock scope. #10334: for a BUILT-IN catalog rule, this field is * CONSUMED end-to-end only for providers in `HONORS_RULE_LOCK_SCOPE_PROVIDERS` - * (agentrouter-exclusive today, gated by `honorsRuleLockScope()`) — for those, + * (agentrouter + the opencode family, gated by `honorsRuleLockScope()`) — for * `checkFallbackError` surfaces it as `ruleScope` on its return value for the * persistence layer to honor instead of re-deriving scope from * `hasPerModelQuota()`. For every other built-in-rule provider it remains @@ -155,6 +155,19 @@ function buildOpencodeRules(): ProviderErrorRule[] { return null; }, }, + { + id: "opencode-400-model-unavailable", + match: ({ status, body }) => { + if (status !== 400) return null; + const text = JSON.stringify(body ?? "").toLowerCase(); + if (!text.includes("upstream request failed: model is unavailable.")) return null; + return { + reason: "model_capacity", + scope: "model", + cooldownMs: 3_600_000, + }; + }, + }, ]; } @@ -290,15 +303,16 @@ function buildAgentrouterRules(): ProviderErrorRule[] { ]; } +/** Providers sharing the opencode upstream envelope, hence the opencode catalog rules. */ +const OPENCODE_RULE_FAMILY = ["opencode", "opencode-zen", "opencode-go", "opencode-cli"]; + /** * Global registry. Provider name → ordered list of rules (first match wins). * Add new providers here; the matcher in classifyError will pick them up * automatically. */ export const providerRuleRegistry = new Map([ - ["opencode", buildOpencodeRules()], - ["opencode-go", buildOpencodeRules()], - ["opencode-cli", buildOpencodeRules()], + ...OPENCODE_RULE_FAMILY.map((id): [string, ProviderErrorRule[]] => [id, buildOpencodeRules()]), ["minimax", buildMinimaxRules()], ["minimax-passthrough", buildMinimaxRules()], ["cloudflare-ai", buildCloudflareAiRules()], @@ -323,7 +337,7 @@ export const providerRuleRegistry = new Map([ * mechanism (#11104) silently inert for every provider except the ones listed * below. See `hasOperatorRuleForProvider`. */ -const HONORS_RULE_LOCK_SCOPE_PROVIDERS = new Set(["agentrouter"]); +const HONORS_RULE_LOCK_SCOPE_PROVIDERS = new Set(["agentrouter", ...OPENCODE_RULE_FAMILY]); export function honorsRuleLockScope(provider: string | null | undefined): boolean { if (!provider) return false; @@ -509,3 +523,21 @@ export function parseResetCountdownMs(text: string): number | null { return null; } } + +/** + * Opencode-family "Upstream request failed: Model is unavailable." 400: the rule's + * model-scope match, or null for any other provider, status or rule. Takes the raw + * error text so it stays independent of FULL_TEXT_RULE_PROVIDERS (#10880). + */ +export function getOpencodeModelUnavailableMatch( + provider: string | null | undefined, + status: number, + headers: Headers | Record | null | undefined, + errorText: unknown +): ProviderErrorRuleMatch | null { + if (status !== 400 || !provider || !OPENCODE_RULE_FAMILY.includes(provider.toLowerCase())) { + return null; + } + const match = getProviderErrorRuleMatch(provider, status, headers, errorText); + return match?.scope === "model" && match.reason === "model_capacity" ? match : null; +} diff --git a/open-sse/services/accountFallback.ts b/open-sse/services/accountFallback.ts index 2f05ffc6e7..bf66815a5d 100644 --- a/open-sse/services/accountFallback.ts +++ b/open-sse/services/accountFallback.ts @@ -16,6 +16,7 @@ import { isNimFunctionDegraded, } from "../config/errorConfig.ts"; import { + getOpencodeModelUnavailableMatch, getProviderErrorRuleMatch, resolveRuleMatchBody, honorsRuleLockScope, @@ -1792,6 +1793,18 @@ export function checkFallbackError( return profile?.useUpstreamRetryHints ? detectRetryHint() : null; } + function ruleScopedResult(match: NonNullable>) { + const scaled = getScaledBaseCooldown(match.reason as RateLimitReasonValue, backoffLevel); + return { + shouldFallback: true, + cooldownMs: match.cooldownMs ?? scaled.cooldownMs, + baseCooldownMs: match.cooldownMs ?? scaled.baseCooldownMs, + configuredCooldownMs: match.cooldownMs, + newBackoffLevel: match.cooldownMs !== undefined ? 0 : scaled.newBackoffLevel, + reason: match.reason, + ruleScope: match.scope, + }; + } function getScaledBaseCooldown(reason: RateLimitReasonValue, level = backoffLevel) { void reason; const baseCooldownMs = @@ -2065,22 +2078,7 @@ export function checkFallbackError( headers, resolveRuleMatchBody(provider, structuredError ?? null, errorStr) ); - if (forbiddenMatch) { - const scaled = getScaledBaseCooldown( - forbiddenMatch.reason as RateLimitReasonValue, - backoffLevel - ); - const ruleCooldownMs = forbiddenMatch.cooldownMs; - return { - shouldFallback: true, - cooldownMs: ruleCooldownMs ?? scaled.cooldownMs, - baseCooldownMs: ruleCooldownMs ?? scaled.baseCooldownMs, - configuredCooldownMs: ruleCooldownMs, - newBackoffLevel: ruleCooldownMs !== undefined ? 0 : scaled.newBackoffLevel, - reason: forbiddenMatch.reason, - ruleScope: forbiddenMatch.scope, - }; - } + if (forbiddenMatch) return ruleScopedResult(forbiddenMatch); } if ( @@ -2199,6 +2197,8 @@ export function checkFallbackError( // 400 — context overflow / malformed request / model access denied if (status === HTTP_STATUS.BAD_REQUEST) { + const modelUnavailable = getOpencodeModelUnavailableMatch(provider, status, headers, errorStr); + if (modelUnavailable) return ruleScopedResult(modelUnavailable); // Check structured error codes first (more reliable, no false positives) // OpenAI: error.code === "model_not_found" // Anthropic: error.type === "not_found_error" / "permission_error" diff --git a/open-sse/services/combo/targetExhaustion.ts b/open-sse/services/combo/targetExhaustion.ts index 11e8efa654..7e636e69d0 100644 --- a/open-sse/services/combo/targetExhaustion.ts +++ b/open-sse/services/combo/targetExhaustion.ts @@ -27,9 +27,11 @@ import { import { RateLimitReason } from "../../config/constants.ts"; import { isProviderCircuitOpenResult, isRequestScopedUpstreamFailure } from "./comboPredicates.ts"; import { isCloudflareFingerprintRejection } from "../errorClassifier.ts"; -// #10334 — agentrouter-exclusive predicate shared with the persistence layer +// #10334 — connection-scope predicate shared with the persistence layer // (markAccountUnavailable) so the same-request combo skip and the persisted // connection cooldown agree on exactly which fallbackResult shapes qualify. +// Exclusive in practice to agentrouter's "额度不足" rule: no opencode-family +// rule matches 403 today, so only agentrouter reaches this predicate via 403. import { isAgentrouterConnectionQuotaScope } from "@/sse/services/auth"; import type { ComboLogger, ResolvedComboTarget } from "./types.ts"; @@ -84,9 +86,9 @@ export type ComboExhaustionSets = { export type ApplyComboTargetExhaustionOptions = { result: { status: number; headers?: Headers | null }; fallbackResult: Parameters[0] & { - /** #10334 — agentrouter-exclusive; see isAgentrouterConnectionQuotaScope + /** #10334 — agentrouter + opencode family; see isAgentrouterConnectionQuotaScope * (src/sse/services/auth.ts). Populated only for providers in - * HONORS_RULE_LOCK_SCOPE_PROVIDERS (today: agentrouter only). */ + * HONORS_RULE_LOCK_SCOPE_PROVIDERS (agentrouter + opencode family). */ ruleScope?: "model" | "provider" | "connection"; permanent?: boolean; }; @@ -115,7 +117,8 @@ export function applyComboTargetExhaustion( const { result, sets, log, tag, errorText, structuredError } = opts; const provider = target.provider; - // #10334: agentrouter-exclusive account-wide quota exhaustion ("额度不足") + // #10334: connection-scope account-wide quota exhaustion (agentrouter "额度不足"; + // exclusive in practice — no opencode-family rule matches 403 today) // must skip remaining SAME-CONNECTION targets within THIS request too, not // just via the persisted cooldown markAccountUnavailable applies for // whichever leg runs next. agentrouter is a passthroughModels provider @@ -341,7 +344,8 @@ function markAuthLevelExhaustion( } /** - * #10334: agentrouter-exclusive connection-scope account quota exhaustion. Mirrors + * #10334: connection-scope account quota exhaustion (agentrouter-exclusive in + * practice — see above). Mirrors * markAuthLevelExhaustion's connectionId-present/absent split — when the target carries a * connectionId, only that connection's account is exhausted (sibling agentrouter connections * for the same user may still have quota); fall back to whole-provider exhaustion only when no diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index 79fc31dfec..f9d0fad58c 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -2401,9 +2401,12 @@ export async function getProviderCredentialsWithQuotaPreflight( } /** - * #10334 — Guard for the agentrouter-exclusive "connection scope" quota - * cooldown branch in markAccountUnavailable. The "never terminal" invariant of - * that branch is NOT structurally guaranteed by `ruleScope === "connection"` + * #10334 — Guard for the "connection scope" quota cooldown branch in + * markAccountUnavailable (agentrouter-exclusive in practice: no opencode-family + * rule matches 403 today, so only agentrouter's "额度不足" rule reaches this + * predicate via 403 — but opencode-family 429 header-quota hits also qualify + * via the 429 path). The "never terminal" invariant of that branch is NOT + * structurally guaranteed by `ruleScope === "connection"` * alone — it also depends on the provider rule table only ever pairing scope * "connection" with a genuinely transient reason. Today * (`buildAgentrouterRules()` in providerErrorRules.ts) that is true: the only @@ -2727,8 +2730,10 @@ export async function markAccountUnavailable( const isPerModelQuotaProvider = hasPerModelQuota(provider, model, connectionPassthroughModels); - // #10334 — agentrouter EXCLUSIVE: the matched provider rule declared scope - // "connection" for account-wide quota exhaustion ("额度不足"). agentrouter is + // #10334 — connection-scope branch: the matched provider rule declared scope + // "connection" for account-wide quota exhaustion (agentrouter "额度不足"; + // exclusive in practice — no opencode-family rule matches 403 today). + // agentrouter is // a passthroughModels provider (isPerModelQuotaProvider === true), so without // this branch the next `if` would treat it like any other passthrough 429 and // lock a SINGLE model — leaving combo routing to burn one upstream call per @@ -2754,6 +2759,15 @@ export async function markAccountUnavailable( // of cooldown" ends up producing a LONGER effective block for this one rule. // Not addressed here; flagged for a future #2997 follow-up if it proves to be // a real operator complaint. + // + // HONORS note: since the opencode family joined HONORS, an opencode-family + // 429 carrying upstream quota headers (x-ratelimit-remaining-*) also lands + // here with ruleScope "connection" — before the #10880 egress branch below, + // so sibling cooling is skipped on that path. Latent today: the only + // request-path caller forwarding headers is chat.ts:2383 (chat completions), + // and opencode upstreams rarely send those headers on 429 (the observed + // envelope is the headers-less "monthly usage limit" body, which keeps + // flowing to the egress block with ruleScope undefined). if (ruleScopeIsConnection && provider && !disableCooling) { const connectionCooldownMs = fallbackResult.cooldownMs > 0 ? fallbackResult.cooldownMs : COOLDOWN_MS.rateLimit; @@ -2848,6 +2862,45 @@ export async function markAccountUnavailable( const isNvidiaModelGone = provider === "nvidia" && status === 410; const modelLockoutOptions = { maxCooldownMs: effectiveProviderProfile?.maxCooldownMs }; + // Same persisted reason the agentrouter 403 model-scope branch hard-codes + // ("forbidden"): the lock key is the getModelLockKey tuple shared with the + // combo path, and the declared 1h (same order as that combo lock) is + // operator-clamped by recordModelLockoutFailure to mlSettings.maxCooldownMs + // (~30min default) — the verbatim 1h never escapes operator control. + // Narrow scope: status === 400 only (never a 403/429 rule), adjacent to + // :2843's per-model-quota status set (which excludes 400) — malformed 400s + // carry no ruleScope and fall through unchanged. + if (model && provider && status === 400 && fallbackResult.ruleScope === "model") { + // Single source of truth: the rule's own cooldownMs (surfaced on + // fallbackResult by the 400 pre-check in checkFallbackError). The literal + // is only the fallback for a rule that declares no cooldown — editing + // the rule's cooldownMs takes effect without touching this call site. + const ruleCooldownMs = + typeof fallbackResult.cooldownMs === "number" && fallbackResult.cooldownMs > 0 + ? fallbackResult.cooldownMs + : 3_600_000; + const lockout = recordModelLockoutFailure( + provider, + connectionId, + model, + "model_capacity", + 400, + ruleCooldownMs, + effectiveProviderProfile, + { exactCooldownMs: ruleCooldownMs, maxCooldownMs: mlSettings.maxCooldownMs } + ); + updateProviderConnection(connectionId, { + lastErrorType: "model_capacity", + lastError: `Model ${model} model_capacity`, + lastErrorAt: new Date().toISOString(), + errorCode: status, + }).catch(() => {}); + log.info( + "AUTH", + `Model-only lockout for ${provider}:${model} — ${status} model_capacity ${Math.ceil(lockout.cooldownMs / 1000)}s (rule scope=model, connection stays active)` + ); + return { shouldFallback: true, cooldownMs: lockout.cooldownMs }; + } if ( isPerModelQuotaProvider && provider && diff --git a/tests/unit/agentrouter-error-rules.test.ts b/tests/unit/agentrouter-error-rules.test.ts index 272d21993b..498c31d911 100644 --- a/tests/unit/agentrouter-error-rules.test.ts +++ b/tests/unit/agentrouter-error-rules.test.ts @@ -168,10 +168,14 @@ test("A13: exclusivity — ruleScope stays undefined for other providers", () => assert.equal(openrouter.ruleScope, undefined); }); -test("A14: honorsRuleLockScope allowlist is agentrouter-only", async () => { +test("A14: honorsRuleLockScope allowlist is agentrouter + opencode family", async () => { const { honorsRuleLockScope } = await import("../../open-sse/config/providerErrorRules.ts"); assert.equal(honorsRuleLockScope("agentrouter"), true); assert.equal(honorsRuleLockScope("AgentRouter"), true); - assert.equal(honorsRuleLockScope("opencode"), false); + assert.equal(honorsRuleLockScope("opencode"), true); + assert.equal(honorsRuleLockScope("opencode-zen"), true); + assert.equal(honorsRuleLockScope("opencode-go"), true); + assert.equal(honorsRuleLockScope("opencode-cli"), true); + assert.equal(honorsRuleLockScope("openrouter"), false); assert.equal(honorsRuleLockScope(null), false); }); diff --git a/tests/unit/opencode-400-model-unavailable.test.ts b/tests/unit/opencode-400-model-unavailable.test.ts new file mode 100644 index 0000000000..278c2e35e9 --- /dev/null +++ b/tests/unit/opencode-400-model-unavailable.test.ts @@ -0,0 +1,162 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { + checkFallbackError, + recordModelLockoutFailure, + isModelLocked, + clearAllModelLockouts, +} from "../../open-sse/services/accountFallback.ts"; +import { isModelScoped400 } from "../../open-sse/services/combo/comboPredicates.ts"; +import { providerRuleRegistry } from "../../open-sse/config/providerErrorRules.ts"; + +// checkFallbackError is positional: (status, errorText, backoffLevel = 0, +// _model = null, provider = null, headers = null, profileOverride = null, +// structuredError?, …). ruleScope IS on the return type (accountFallback.ts:1686, +// #10334) but always undefined for non-allowlisted providers until the fenced +// pre-check + HONORS widening land — RED fails on values alone; the cast is +// convenience, not necessity. +const VERBATIM_BODY = `{"type":"server_error","message":"Error from provider (Console): Upstream request failed: Model is unavailable."}`; + +test("opencode 400 model-unavailable", async (t) => { + await t.test("locks the model on the pinned verbatim (opencode)", () => { + const r = checkFallbackError(400, VERBATIM_BODY, 0, null, "opencode"); + assert.equal(r.shouldFallback, true); + assert.equal((r as { ruleScope?: string }).ruleScope, "model"); + assert.equal(r.reason, "model_capacity"); + }); + + await t.test( + "locks the model on the pinned verbatim (opencode-zen, distinctly registered)", + () => { + assert.ok(providerRuleRegistry.get("opencode-zen"), "zen key registered"); + const r = checkFallbackError(400, VERBATIM_BODY, 0, null, "opencode-zen"); + assert.equal(r.shouldFallback, true); + assert.equal((r as { ruleScope?: string }).ruleScope, "model"); + } + ); + + await t.test("malformed 400 does NOT take the model lock (zero-cooldown guard preserved)", () => { + // #2101 infinite-loop guard (accountFallback.ts:2231-2237, re-pinned by + // accountfallback-ratelimit-400-4976.test.ts:38-44): a malformed 400 stays + // {shouldFallback:true, cooldownMs:0, reason:model_capacity} — "terminal" + // MEANS zero-cooldown, not shouldFallback:false. The new model-lock branch + // must not fire here: no ruleScope, no persisted lock. + const r = checkFallbackError( + 400, + `{"type":"invalid_request","message":"improperly formed request: invalid message format"}`, + 0, + null, + "opencode" + ); + assert.equal(r.shouldFallback, true); + assert.equal(r.cooldownMs, 0); + assert.equal(r.reason, "model_capacity"); + assert.equal((r as { ruleScope?: string }).ruleScope, undefined); + }); + + await t.test("model-unavailable write persists a readable model lock", () => { + // Direct round-trip on the same getModelLockKey tuple both paths share + // (exact-model key for these inputs): the auth.ts model branch calls + // recordModelLockoutFailure with the same (provider, connectionId, model, + // "model_capacity", 400) tuple, and combo routing reads it via isModelLocked. + clearAllModelLockouts(); + recordModelLockoutFailure( + "opencode", + "conn-test-400", + "deepseek-v4-flash-free", + "model_capacity", + 400, + 0, + null, + { exactCooldownMs: 3_600_000, maxCooldownMs: 1_800_000 } + ); + assert.equal(isModelLocked("opencode", "conn-test-400", "deepseek-v4-flash-free"), true); + clearAllModelLockouts(); + }); + + await t.test( + "headers-only quota rule still surfaces connection scope (pre-existing, HONORS now honors it)", + () => { + // The quota-exhausted-headers rule keys on headers alone, so it matched + // before this PR too — but ruleScope stayed undefined (opencode not in + // HONORS). Widening HONORS surfaces the rule's declared connection scope + // on header-passing paths (accountFallback 429 branch, combo executors). + // Body markers stay inert without FULL_TEXT (separate assert below). + // HONORS side effect (documented in the PR body): the pre-existing 429 + // headers rule now yields scope=connection for the whole opencode family, + // where the persistence layer previously re-derived scope via + // hasPerModelQuota(). opencode is not per-model-quota (no passthrough in + // either registry), so both derivations agree on connection — pinned here + // for all four family members plus the monthly-quota body rule, which + // keeps its exact verbatim cooldown (13 days, not the scaled default). + for (const provider of ["opencode", "opencode-zen", "opencode-go", "opencode-cli"]) { + const r = checkFallbackError(429, "rate limit reached, slow down", 0, null, provider, { + "x-ratelimit-remaining-requests": "0", + }); + assert.equal(r.reason, "quota_exhausted", provider); + assert.equal((r as { ruleScope?: string }).ruleScope, "connection", provider); + // Same body without headers: no rule fires, scope stays undefined. + const r2 = checkFallbackError( + 429, + "rate limit reached, slow down", + 0, + null, + provider, + null + ); + assert.equal((r2 as { ruleScope?: string }).ruleScope, undefined, provider); + } + // Pins parser day-granularity (parseResetCountdownMs), not this PR's code: + // relax to a range if the parser ever learns hour/minute residuals. + const monthly = checkFallbackError( + 429, + "[429] Monthly usage limit reached. Resets in 13 days.", + 0, + null, + "opencode", + null + ); + assert.equal(monthly.reason, "quota_exhausted"); + assert.ok( + monthly.cooldownMs >= 13 * 24 * 60 * 60 * 1000 && + monthly.cooldownMs < 14 * 24 * 60 * 60 * 1000 + ); + assert.equal((monthly as { ruleScope?: string }).ruleScope, undefined); + } + ); + + await t.test("quota-body markers stay inert without FULL_TEXT", () => { + // FULL_TEXT_RULE_PROVIDERS is still agentrouter-only: quota-body markers + // (organization_quota_exceeded, plan_limit_reached, account_quota_exceeded) + // must NOT surface a rule scope — the #10880 egress block stays reachable. + for (const marker of [ + "organization_quota_exceeded", + "plan_limit_reached", + "account_quota_exceeded", + ]) { + const r = checkFallbackError( + 429, + `{"error":{"message":"${marker}"}}`, + 0, + null, + "opencode", + null + ); + assert.equal(r.reason, "rate_limit_exceeded", marker); + assert.equal((r as { ruleScope?: string }).ruleScope, undefined, marker); + } + }); + + await t.test("verbatim stays terminal on non-family providers", () => { + // The new model-lock branch is fenced on OPENCODE_FAMILY: the verbatim + // under any other provider must stay shouldFallback:false (generic 400). + for (const provider of ["agentrouter", "openrouter", "minimax", "mimocode", "unknown-vendor"]) { + const r = checkFallbackError(400, VERBATIM_BODY, 0, null, provider); + assert.equal(r.shouldFallback, false, provider); + } + }); + + await t.test("combo model-scope classifier still matches (regression)", () => { + assert.equal(isModelScoped400(VERBATIM_BODY), true); + }); +}); From cfa2fc754878ae8759fb600788603044dec0ee1f Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:57:28 +0200 Subject: [PATCH 19/19] fix(sse): rotate opencode accounts on transient 5xx (#12975) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciled and merged. This branch was stacked on #12941, which has since landed, so it read as 1484 additions across 16 files and CONFLICTING. I merged the current `release/v3.8.51` into it rather than rewriting your branch: `open-sse/executors/opencode.ts` conflicted in six places where your side was a strict superset of the squashed #12941, and the tip had touched that file through nothing but #12941, so your side was taken whole. The PR now reads as its real 12 files, +839/-25. The change itself is right: a flapping upstream 5xx aborting the whole agentic chain is exactly the case where rotating to the next healthy account is safe, and keeping it a separate arm from the 400-empty branch matters because that one has to clone-read the body while this one never touches it. Threading `correlationId` through so interleaved requests stay attributable — and never fabricating one when absent — is the right discipline. Both geo-block regression suites pass alongside the new ones (43/43 across the five opencode test files), which is what proves the conflict resolution preserved #12941's behaviour. I also tightened the batch's file-size rebaseline here: `src/sse/handlers/chat.ts` needed no bump at all (it lands at 2452, under its existing 2458 freeze) and `open-sse/executors/base.ts` needed only your +2. An earlier measurement had included a local prettier reformat that is not part of this branch. --- Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the rest of this batch — zero conflicts between them. - `typecheck:core` clean; `check:changelog-integrity` OK - complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline - 86 focused assertions green across the batch's 10 unit test files, plus 16/16 on the v1 plugin option schema and 16/16 on the v2 option tests - `check-file-size` rebaselined for this batch's real growth (annotation `_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode`, landed on #13141). `open-sse/utils/stream.ts` was deliberately left frozen: it is already 3115 > 3098 on the pure tip with zero contribution from this batch. ⚠️ base-red inherited: #12732 — `Docs Gates`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and the `stream.ts` freeze above). None of them touch these diffs. Thanks @maxmad64bis. --- .../12975-opencode-transient-5xx-rotation.md | 1 + config/quality/file-size-baseline.json | 7 +- open-sse/executors/base.ts | 2 + open-sse/executors/opencode.ts | 57 +- .../executors/opencodeTransientFailure.ts | 17 + open-sse/handlers/chatCore.ts | 3 + src/sse/handlers/chat.ts | 12 +- src/sse/handlers/chatHelpers.ts | 12 +- src/sse/services/auth.ts | 26 +- .../chat-correlation-id-exhaustion.test.ts | 177 ++++++ ...encode-transient-failure-predicate.test.ts | 36 ++ .../unit/opencode-transient-rotation.test.ts | 514 ++++++++++++++++++ 12 files changed, 839 insertions(+), 25 deletions(-) create mode 100644 changelog.d/fixes/12975-opencode-transient-5xx-rotation.md create mode 100644 open-sse/executors/opencodeTransientFailure.ts create mode 100644 tests/unit/chat-correlation-id-exhaustion.test.ts create mode 100644 tests/unit/opencode-transient-failure-predicate.test.ts create mode 100644 tests/unit/opencode-transient-rotation.test.ts diff --git a/changelog.d/fixes/12975-opencode-transient-5xx-rotation.md b/changelog.d/fixes/12975-opencode-transient-5xx-rotation.md new file mode 100644 index 0000000000..36c2b21012 --- /dev/null +++ b/changelog.d/fixes/12975-opencode-transient-5xx-rotation.md @@ -0,0 +1 @@ +- **fix(sse):** transient opencode upstream failures rotate to the next account proxy instead of failing, so one flapping egress no longer aborts the whole chain ([#12975](https://github.com/diegosouzapw/OmniRoute/pull/12975)) — thanks @maxmad64bis diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index bbd36cfa5c..f855229516 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -1,5 +1,6 @@ { - "_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode": "/merge-batch 2026-09-11 (v3.8.51), PRs #13141, #13146 and #12975 by maxmad64bis. Final combined values for the batch, set here on the first PR merged so every intermediate merge state is covered too. src/sse/services/auth.ts 3450->3488 (+38): #13146 adds the narrow ruleScope===model branch to markAccountUnavailable (gated on status 400; every other status keeps its path) plus the HONORS_RULE_LOCK_SCOPE_PROVIDERS opencode entry, and #12975 adds buildExhaustionOptions so the exhaustion log lines carry the request correlationId. open-sse/executors/base.ts 1751->1757 (+6): #12975 adds the optional ExecuteInput.correlationId field with its doc comment (+2); the other +4 is prettier splitting the cliFingerprints import, a 103-char line the tip left unformatted, which lint-staged rewrites on any commit touching the file. src/sse/handlers/chat.ts 2458->2460 (+2): #12975 threads correlationId through the three executor call sites (+2) and prettier splits a 168-char comboTargetPassesKeyModelPolicy condition (+8), same unformatted-tip cause; the tip itself sits 9 lines under its own freeze, which absorbs the rest. open-sse/services/accountFallback.ts 2467->2468 (+1): #13141 routes hasFutureRateLimitUntil through the tolerant epoch normalizer; #13146 is net zero there (+16/-16). No new branching beyond the two guarded branches named above. open-sse/utils/stream.ts is deliberately NOT rebaselined: it is already 3115 > 3098 on the pure tip with zero contribution from this batch (base-red #12732, owned by /sweep-reds). Covered by tests/unit/combo-predicates-epoch-cooldown.test.ts, opencode-400-model-unavailable.test.ts, agentrouter-error-rules.test.ts, opencode-transient-rotation.test.ts and chat-correlation-id-exhaustion.test.ts.", + "_rebaseline_2026_09_10_12975_rotation_correlation_id": "PR #12975 own growth: open-sse/executors/base.ts 1751->1753 (+2) and open-sse/handlers/chatCore.ts 6021->6024 (+3). The opencode rotation lines carry the request correlationId: one optional ExecuteInput field and one correlationId argument at each of the three executor.execute call sites in handleChatCore. Irreducible plumbing at existing call sites; the rotation logic itself lives in open-sse/executors/opencode.ts and the new leaf predicates (under cap). Covered by tests/unit/opencode-transient-rotation.test.ts and tests/unit/chat-correlation-id-exhaustion.test.ts.", + "_rebaseline_2026_09_11_mergebatch_v3851_maxmad_opencode": "/merge-batch 2026-09-11 (v3.8.51), PRs #13141, #13146 and #12975 by maxmad64bis. src/sse/services/auth.ts 3450->3488 (+38): #13146 adds the narrow ruleScope===model branch to markAccountUnavailable (gated on status 400; every other status keeps its path) plus the HONORS_RULE_LOCK_SCOPE_PROVIDERS opencode entry, taking it to 3464; #12975 then adds buildExhaustionOptions so the exhaustion log lines carry the request correlationId (+24). open-sse/services/accountFallback.ts 2467->2468 (+1): #13141 routes hasFutureRateLimitUntil through the tolerant epoch normalizer; #13146 is net zero there (+16/-16). open-sse/executors/base.ts 1751->1753 (+2): #12975 adds the optional ExecuteInput.correlationId field with its doc comment. src/sse/handlers/chat.ts is NOT rebaselined: #12975 threads correlationId through the three executor call sites (+2) but the file lands at 2452, still under its existing 2458 freeze. open-sse/utils/stream.ts is deliberately NOT rebaselined either: it is already 3115 > 3098 on the pure tip with zero contribution from this batch (base-red #12732, owned by /sweep-reds). No new branching beyond the two guarded branches named above. Covered by tests/unit/combo-predicates-epoch-cooldown.test.ts, opencode-400-model-unavailable.test.ts, agentrouter-error-rules.test.ts, opencode-transient-rotation.test.ts and chat-correlation-id-exhaustion.test.ts.", "_rebaseline_2026_09_10_mergebatch_v3851_greenpt_eurouter": "/merge-batch 2026-09-10 (v3.8.51), PRs #13024 (GreenPT, closes #12986) and #13025 (EURouter, closes #12985) by ntdatt812: src/shared/constants/providers/apikey/gateways.ts 1462->1502 (+40 = two APIKEY_PROVIDERS_GATEWAYS catalog entries, declarative data only: id/alias/name/icon/color/website plus the hasFree=false rationale comments and the apiHint copy each PR verified). No logic and no new branching. Same god-file no-split rationale as every prior gateways.ts rebaseline (#11786 seekai, #10987 logfare, #10668 tabitoken, #10531 freebuff, #11631 1min.ai): the file header says it is pure data merged by apikey/index.ts via spread, and it is already split into 6 family files under apikey/, so splitting a catalog for two entries would violate the semantic-families rule rather than help. Both entries are deliberately conservative (models: [] with passthroughModels, no tool/vision capability declared, hasFree false), so the growth is the entry itself, not claims. EURouter is in AGGREGATOR_PROVIDER_IDS because it routes to third-party upstreams; GreenPT is not because it serves its own inference. Covered by tests/unit/greenpt-provider.test.ts and tests/unit/eurouter-provider.test.ts.", "_rebaseline_2026_09_10_12828_translate_usage_chunk": "PR #12828 own growth: open-sse/utils/stream.ts 3072->3080 (+8). Translate-mode streams now send the estimated usage as the canonical trailing usage-only chunk before [DONE] when the upstream stays silent (parity with the #12151 passthrough flush), with a latch so a finish chunk that already carried the estimate is not doubled. The chunk builder is shared with the passthrough flush in open-sse/utils/usageOnlyChunk.ts (under cap); what remains is the flush-site wiring. Covered by tests/unit/stream-translate-usage-trailing.test.ts.", "_rebaseline_2026_09_10_12715_queue_budget": "PR #12715 own growth: open-sse/handlers/chatCore.ts 6021->6036 (+15). Hierarchical admission now resolves the per-connection queue budget before the gates and hands withRateLimit the remaining budget, the correlation id and the executor timeout context, so gate wait, provider slot and Bottleneck queue share one bound instead of stacking. Error shaping lives in open-sse/handlers/chatCore/queueBudget.ts (under cap); what remains is irreducible call-site wiring. Covered by tests/unit/rate-limit-remaining-budget.test.ts, rate-limit-manager-queue-bound.test.ts and chatcore-hierarchical-admission.test.ts.", @@ -423,7 +424,7 @@ "_rebaseline_pr1043_minimax_tts": "Upstream port decolua/9router#1043 (toanalien) own growth: audioSpeech.ts 965->1061 (+96). Adds MiniMax T2A v2 TTS dispatch (handleMinimaxSpeech + hexToBytes helper) — provider entry was already in audioRegistry (format: minimax-tts) but no handler existed, falling through to the OpenAI-compatible default that fails (T2A has custom shape + hex-encoded audio + base_resp envelope). New branch sits next to the other inline provider branches (xiaomi-mimo, coqui, tortoise, aws-polly) — extracting would just create indirection. Covered by tests/unit/minimax-tts-1043.test.ts (3 tests, GREEN: success, base_resp error, invalid-hex).", "_rebaseline_pr4592_exclude_exhausted_auto": "Reconcile #4592 already-merged growth: combo.ts 2991->3036 (+45, terminal-status quota-cutoff exclusion in buildAutoCandidates + opt-in gate). Fast-gate PR->release does not run check:file-size.", "open-sse/executors/antigravity.ts": 1665, - "open-sse/executors/base.ts": 1757, + "open-sse/executors/base.ts": 1753, "open-sse/executors/chatgpt-web.ts": 5056, "open-sse/executors/codex.ts": 1505, "open-sse/executors/cursor.ts": 1759, @@ -469,7 +470,7 @@ "src/shared/components/RequestLoggerV2.tsx": 1718, "src/shared/constants/providers/apikey/gateways.ts": 1502, "src/shared/services/cliRuntime.ts": 1296, - "src/sse/handlers/chat.ts": 2460, + "src/sse/handlers/chat.ts": 2458, "src/sse/services/auth.ts": 3488, "tests/unit/account-fallback-service.test.ts": 2453, "tests/unit/provider-validation-specialty.test.ts": 4656, diff --git a/open-sse/executors/base.ts b/open-sse/executors/base.ts index b18c27e20c..5d07ef73c3 100644 --- a/open-sse/executors/base.ts +++ b/open-sse/executors/base.ts @@ -211,6 +211,8 @@ export type ExecuteInput = { ) => Promise | void; /** When true, skip the intra-URL 429 retry in execute() so the caller handles fallback. */ skipUpstreamRetry?: boolean; + /** Request-scoped id for log attribution; absent off the chat path, never fabricated. */ + correlationId?: string | null; /** Delegated Context Editing (Claude only): when enabled, attach the * `context_management.clear_tool_uses` strategy so the provider clears stale * tool-use blocks server-side. Honored only on the genuine `claude` path. */ diff --git a/open-sse/executors/opencode.ts b/open-sse/executors/opencode.ts index 3431b5c5e6..c4a3206d09 100644 --- a/open-sse/executors/opencode.ts +++ b/open-sse/executors/opencode.ts @@ -29,6 +29,7 @@ import { extractChatcmplId, } from "./accountRotation.ts"; import { isOpencodeGeoBlocked, proxyKeyOf } from "./opencodeGeoBlock.ts"; +import { isRetriableUpstreamFailure } from "./opencodeTransientFailure.ts"; import { isNetworkRotationSharedEgressGuardEnabled } from "@/shared/utils/featureFlags"; /** @@ -504,6 +505,10 @@ export class OpencodeExecutor extends BaseExecutor { this.syncAccountsFromCredentials(input.credentials); const { log } = input; + // Request-scoped attribution prefix for rotation logs: message head, + // empty when absent (never n/a/none/fabricated). The existing motif + // stays byte-identical after the prefix. + const cid = input.correlationId ? `correlationId=${input.correlationId} ` : ""; const hasProxies = this.accounts.some((a) => a.proxy !== null); // Fast path: no multi-account proxy wiring configured → original behavior, @@ -533,7 +538,7 @@ export class OpencodeExecutor extends BaseExecutor { const chatcmplId = extractChatcmplId(bodyText); log?.warn?.( "OPENCODE", - `upstream empty rejection on direct account (${chatcmplId}), retrying once…` + `${cid}upstream empty rejection on direct account (${chatcmplId}), retrying once…` ); return this.normalizeMuseSparkResponse(input, await super.execute(input)); } @@ -567,8 +572,9 @@ export class OpencodeExecutor extends BaseExecutor { // through the accounts is the retry). Avoids an unbounded loop on a // persistently malformed upstream. const emptyRejectionBudget = this.accounts.length === 1 ? 1 : 0; - // 403-geo tried set: proxy keys already proven geo-blocked for this - // request's model. Request-local only — nothing persists past execute(). + // Tried set: proxy keys already proven unusable for this request's + // model (geo-blocked, or transient 5xx). Request-local only — nothing + // persists past execute(). const geoTriedProxyKeys = new Set(); let directTried = false; @@ -594,15 +600,19 @@ export class OpencodeExecutor extends BaseExecutor { } const lastStatus = lastResult !== null ? lastResult.response.status : null; const lastWasGeo = lastStatus === 403 || lastStatus === 451; + const lastWasTransient = lastStatus !== null && lastStatus >= 500 && lastStatus < 600; + const isMonoRetryOwed = this.accounts.length === 1 && lastWasTransient; if ( + !isMonoRetryOwed && lastResult !== null && geoTriedProxyKeys.size > 0 && !isProxiedCandidate(account) && !(account.proxy === null && !directTried) ) { // Geo exhaustion (last was 403/451) → surface as-is, no success mark. + // Transient exhaustion (last was 5xx) → same: surface last as-is. // Any other last status (e.g. 429 after 403s) → skip without a call. - if (lastWasGeo) break; + if (lastWasGeo || lastWasTransient) break; continue; } // Commit the last-resort direct attempt so a later exclusion breaks @@ -614,7 +624,7 @@ export class OpencodeExecutor extends BaseExecutor { if (sharedEgressGuardEnabled && sharedEgressDown && !account.proxy) { log?.warn?.( "OPENCODE", - `skipping account ${masked} (no dedicated proxy, shared egress already down this request)` + `${cid}skipping account ${masked} (no dedicated proxy, shared egress already down this request)` ); continue; } @@ -625,7 +635,7 @@ export class OpencodeExecutor extends BaseExecutor { // Token stays masked — never log the full account id. log?.info?.( "OPENCODE", - `dispatch via account ${masked} (idx ${attempt + 1}/${this.accounts.length})` + + `${cid}dispatch via account ${masked} (idx ${attempt + 1}/${this.accounts.length})` + (account.proxy ? ` through proxy ${account.proxy.host}:${account.proxy.port}` : " direct") @@ -657,20 +667,20 @@ export class OpencodeExecutor extends BaseExecutor { lastSharedEgressError = err; log?.warn?.( "OPENCODE", - `network error on account ${masked} (no dedicated proxy, shared egress), cooldown applied — trying next available account… (${reason})` + `${cid}network error on account ${masked} (no dedicated proxy, shared egress), cooldown applied — trying next available account… (${reason})` ); continue; } log?.warn?.( "OPENCODE", - `network error on account ${masked} (no dedicated proxy, shared egress) — not rotating (${reason})` + `${cid}network error on account ${masked} (no dedicated proxy, shared egress) — not rotating (${reason})` ); throw err; } this.markCooldown(account); log?.warn?.( "OPENCODE", - `network error on account ${masked}, rotating to next… (${reason})` + `${cid}network error on account ${masked}, rotating to next… (${reason})` ); continue; } @@ -679,7 +689,28 @@ export class OpencodeExecutor extends BaseExecutor { const status = result.response.status; if (status === 429) { this.markCooldown(account); - log?.warn?.("OPENCODE", `Rate limited (429) on account ${masked}, rotating to next…`); + log?.warn?.( + "OPENCODE", + `${cid}Rate limited (429) on account ${masked}, rotating to next…` + ); + continue; + } + + if (isRetriableUpstreamFailure(status)) { + const key = proxyKeyOf(account.proxy); + if (key !== null) geoTriedProxyKeys.add(key); + else directTried = true; + log?.warn?.( + "OPENCODE", + `${cid}transient upstream ${status} on account ${masked} (proxy ${key ?? "direct"}), rotating to next…` + ); + // Deliberately a separate branch from the 400-empty arm below, + // not one merged `if`: this arm never touches the body, the 400 + // arm must clone-read it. Both share the predicate + tried-set. + // Single proxied account: one retry via the existing budget (a + // proxy-less single account takes the fast path, never the loop). + // Transient is not deterministic like geo: upstream may recover. + // No 0-retry guard here (it stays geo-only). continue; } @@ -696,7 +727,7 @@ export class OpencodeExecutor extends BaseExecutor { else directTried = true; log?.warn?.( "OPENCODE", - `geo-blocked on account ${masked} (proxy ${key ?? "direct"}), rotating to next…` + `${cid}geo-blocked on account ${masked} (proxy ${key ?? "direct"}), rotating to next…` ); // Single account with a proxy: 0 retries (same egress = dead latency). // (The fast path above already covers single-without-proxy; here length===1 WITH proxy.) @@ -719,11 +750,11 @@ export class OpencodeExecutor extends BaseExecutor { } catch { log?.debug?.("OPENCODE", "body read failed on empty rejection check"); } - if (bodyText !== null && isEmptyUpstreamRejection(400, bodyText)) { + if (bodyText !== null && isRetriableUpstreamFailure(400, bodyText)) { const chatcmplId = extractChatcmplId(bodyText); log?.warn?.( "OPENCODE", - `upstream empty rejection on account ${masked} (${chatcmplId}), rotating to next…` + `${cid}upstream empty rejection on account ${masked} (${chatcmplId}), rotating to next…` ); continue; } diff --git a/open-sse/executors/opencodeTransientFailure.ts b/open-sse/executors/opencodeTransientFailure.ts new file mode 100644 index 0000000000..9af52a1fa4 --- /dev/null +++ b/open-sse/executors/opencodeTransientFailure.ts @@ -0,0 +1,17 @@ +/** + * opencodeTransientFailure.ts — retriable-upstream predicate for the opencode + * executor loop. + * + * Leaf module: one internal import only (isEmptyUpstreamRejection, same + * executors layer — no registry, no DB). 5xx short-circuits on status alone; + * the 400 arm delegates to the existing empty-rejection classifier. + */ + +import { isEmptyUpstreamRejection } from "./accountRotation.ts"; + +export function isRetriableUpstreamFailure(status: number, bodyText?: string): boolean { + if (status >= 500 && status < 600) return true; + if (status !== 400) return false; + if (typeof bodyText !== "string" || bodyText === "") return false; + return isEmptyUpstreamRejection(status, bodyText); +} diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index f622da986c..5d1206f541 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -3167,6 +3167,7 @@ export async function handleChatCore({ onCredentialsRefreshed, skipUpstreamRetry, contextEditing: { enabled: contextEditingEnabled }, + correlationId, }) ), }); @@ -3353,6 +3354,7 @@ export async function handleChatCore({ onCredentialsRefreshed, skipUpstreamRetry, contextEditing: { enabled: contextEditingEnabled }, + correlationId, }) ), }); @@ -4009,6 +4011,7 @@ export async function handleChatCore({ onCredentialsRefreshed, skipUpstreamRetry: isCombo, contextEditing: { enabled: contextEditingEnabled }, + correlationId, }) ) ); diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index 54afa13c8c..0a5b3fafbe 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -12,6 +12,7 @@ import { resolveRoutingModel, RoutingModelOps } from "./resolveRoutingModel"; import { getProviderCredentialsWithQuotaPreflight, markAccountUnavailable, + buildExhaustionOptions, extractApiKey, isValidApiKey, extractSessionAffinityKey, @@ -1781,7 +1782,8 @@ async function handleSingleModelChat( lastStatus, candidateAliases, isCombo, - shadowedNode + shadowedNode, + runtimeOptions?.correlationId ?? null ); const lastFailedConnectionId = excludedConnectionIds.size > 0 @@ -2093,7 +2095,7 @@ async function handleSingleModelChat( provider, model, providerProfile, - { isCombo } + buildExhaustionOptions(runtimeOptions.correlationId ?? null, { isCombo }) ); if (shouldFallback && !hasForcedConnection) { @@ -2142,7 +2144,7 @@ async function handleSingleModelChat( provider, model, providerProfile, - { isCombo } + buildExhaustionOptions(runtimeOptions.correlationId ?? null, { isCombo }) ); if (shouldFallback && !hasForcedConnection) { @@ -2387,7 +2389,7 @@ async function handleSingleModelChat( provider, model, providerProfile, - { + buildExhaustionOptions(runtimeOptions.correlationId ?? null, { persistUnavailableState: !( isCombo && result.status === 429 && @@ -2395,7 +2397,7 @@ async function handleSingleModelChat( ), isCombo, headers: result.response.headers, - } + }) ); // An explicit pin (combo step `connectionId` / `x-omniroute-connection`) is an diff --git a/src/sse/handlers/chatHelpers.ts b/src/sse/handlers/chatHelpers.ts index b98ee96542..17e3a0f08a 100644 --- a/src/sse/handlers/chatHelpers.ts +++ b/src/sse/handlers/chatHelpers.ts @@ -3,7 +3,11 @@ import { getComboForModel, getModelInfoOrRetirementResponse, } from "../services/model"; -import { clearAccountError, markAccountUnavailable } from "../services/auth"; +import { + clearAccountError, + markAccountUnavailable, + buildExhaustionOptions, +} from "../services/auth"; import { connectionHasExtraKeys } from "@omniroute/open-sse/services/apiKeyRotator.ts"; import { createBuiltinAutoCombo } from "@omniroute/open-sse/services/autoCombo/builtinCatalog.ts"; import * as log from "../utils/logger"; @@ -555,7 +559,7 @@ export async function executeChatWithBreaker({ provider, model, providerProfile, - { isCombo } + buildExhaustionOptions(correlationId ?? null, { isCombo }) ); }, }) @@ -731,7 +735,8 @@ export function handleNoCredentials( lastStatus: number | null, candidateAliases?: readonly string[], isCombo: boolean = false, - shadowedNode: ShadowedProviderNode | null = null + shadowedNode: ShadowedProviderNode | null = null, + correlationId?: string | null ) { if (credentials?.allRateLimited) { const errorMsg = lastError || credentials.lastError || "Unavailable"; @@ -772,6 +777,7 @@ export function handleNoCredentials( provider, model, lastStatus, + ...(correlationId ? { correlationId } : {}), }); return errorResponse(lastStatus, lastError); } diff --git a/src/sse/services/auth.ts b/src/sse/services/auth.ts index f9d0fad58c..d0fff53479 100644 --- a/src/sse/services/auth.ts +++ b/src/sse/services/auth.ts @@ -2550,6 +2550,26 @@ async function applyEgressIpLockout( } } +/** Build the options for markAccountUnavailable on the chat exhaustion path. + * Single place that forwards the request id so no chat sender can forget it: + * every chat caller passes its in-scope id through here. */ +export function buildExhaustionOptions( + correlationId: string | null, + rest: { + persistUnavailableState?: boolean; + /** Caller is the combo engine — it records its own model-level lockouts. */ + isCombo?: boolean; + headers?: Headers | Record | null; + } = {} +): { + persistUnavailableState?: boolean; + isCombo?: boolean; + headers?: Headers | Record | null; + correlationId: string | null; +} { + return { ...rest, correlationId }; +} + /** Persist exponential-backoff state for an unavailable provider connection. */ export async function markAccountUnavailable( connectionId: string, @@ -2563,6 +2583,7 @@ export async function markAccountUnavailable( /** Caller is the combo engine — it records its own model-level lockouts. */ isCombo?: boolean; headers?: Headers | Record | null; + correlationId?: string | null; } = {} ) { const currentMutex = markMutexes.get(connectionId) || Promise.resolve(); @@ -2931,7 +2952,10 @@ export async function markAccountUnavailable( }).catch(() => {}); log.info( "AUTH", - `Server error for ${provider}:${model} — ${status} ${reason} (no model lockout, connection stays active for sibling models)` + `Server error for ${provider}:${model} — ${status} ${reason} (no model lockout, connection stays active for sibling models)`, + { + ...(options.correlationId ? { correlationId: options.correlationId } : {}), + } ); return { shouldFallback: true, cooldownMs: 0 }; } diff --git a/tests/unit/chat-correlation-id-exhaustion.test.ts b/tests/unit/chat-correlation-id-exhaustion.test.ts new file mode 100644 index 0000000000..1875017e17 --- /dev/null +++ b/tests/unit/chat-correlation-id-exhaustion.test.ts @@ -0,0 +1,177 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-exhaustion-id-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const providersDb = await import("../../src/lib/db/providers.ts"); +const auth = await import("../../src/sse/services/auth.ts"); +const chatHelpers = await import("../../src/sse/handlers/chatHelpers.ts"); + +async function resetStorage() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +async function createConnection(provider = "opencode-test") { + const conn = await providersDb.createProviderConnection({ + provider, + authType: "oauth", + accessToken: "access-token", + refreshToken: "refresh-token", + isActive: true, + testStatus: "active", + }); + return String(conn.id); +} + +async function driveExhaustionViaBare500( + connId: string, + options?: { correlationId?: string | null } +) { + // Bare 500 takes the status === 500 early branch: no model lockout, the + // request-scoped id lands on the exhaustion line. + return auth.markAccountUnavailable( + connId, + 500, + "transient upstream 500", + "opencode-test", + "test-model", + null, + options ?? {} + ); +} + +function readSource(rel: string) { + return fs.readFileSync(new URL(rel, import.meta.url), "utf8"); +} + +test("exhaustion lines carry the request id", async (t) => { + await t.test("chat sender forwards the id (sender side)", async () => { + // A receiver-only test (options hand-set at the auth call) would + // still pass if a chat sender stopped forwarding the id. This test reads + // the sender call sites directly: every chat sender must pass its + // in-scope request id via options. If any of the four senders drops the + // field, the count/asserts below fail. + const chatSource = readSource("../../src/sse/handlers/chat.ts"); + const helpersSource = readSource("../../src/sse/handlers/chatHelpers.ts"); + + const chatSenders = [ + ...chatSource.matchAll(/buildExhaustionOptions\(runtimeOptions\.correlationId \?\? null,/g), + ]; + assert.equal( + chatSenders.length, + 3, + "chat.ts must pass runtimeOptions.correlationId at all three markAccountUnavailable senders (:2089/:2138/:2383)" + ); + assert.match( + helpersSource, + /buildExhaustionOptions\(correlationId \?\? null,/, + "chatHelpers.ts onStreamFailure must pass its in-scope correlationId via options" + ); + // The fallback-path sender (:2383) carries the full options literal — + // persist flag, combo flag, headers AND the id together. + assert.match( + chatSource, + /buildExhaustionOptions\(runtimeOptions\.correlationId \?\? null, \{\s*persistUnavailableState: !\([\s\S]*?headers: result\.response\.headers,\s*\}\)/, + "chat.ts:2383 fallback sender must forward the id alongside the existing options literal" + ); + // The exhaustion caller passes the id positionally (10th arg), not a bare + // request id from another scope. + assert.match( + chatSource, + /handleNoCredentials\(\s*credentials,[\s\S]*?shadowedNode,\s*runtimeOptions\?\.correlationId \?\? null\s*\)/, + "chat.ts:1775 must pass runtimeOptions?.correlationId ?? null as the trailing handleNoCredentials arg" + ); + + // The pure helper itself forwards the exact id the sender passes in. + assert.deepEqual(auth.buildExhaustionOptions("trace-123", { isCombo: true }), { + isCombo: true, + correlationId: "trace-123", + }); + assert.deepEqual(auth.buildExhaustionOptions(null, { isCombo: false }), { + isCombo: false, + correlationId: null, + }); + }); + + await t.test("auth.ts emits structured id meta on the exhaustion line", async () => { + const authSource = readSource("../../src/sse/services/auth.ts"); + assert.match( + authSource, + /\.\.\.\(options\.correlationId \? \{ correlationId: options\.correlationId \} : \{\}\)/, + "auth.ts:2868 must spread correlationId into the log meta only when truthy" + ); + + await resetStorage(); + const withId = await createConnection(); + const resWithId = await driveExhaustionViaBare500( + withId, + auth.buildExhaustionOptions("trace-123") + ); + // Bare 500: no model lockout, connection stays active, fallback allowed. + assert.equal(resWithId.shouldFallback, true); + const withAfter = await providersDb.getProviderConnectionById(withId); + assert.equal( + (withAfter as unknown as { lastErrorType?: string })?.lastErrorType, + "server_error" + ); + + await resetStorage(); + const withoutId = await createConnection(); + const resWithoutId = await driveExhaustionViaBare500( + withoutId, + auth.buildExhaustionOptions(null) + ); + assert.equal(resWithoutId.shouldFallback, true); + }); + + await t.test("handleNoCredentials emits structured id meta", async () => { + const helpersSource = readSource("../../src/sse/handlers/chatHelpers.ts"); + assert.match( + helpersSource, + /\.\.\.\(correlationId \? \{ correlationId \} : \{\}\)/, + "chatHelpers.ts:771 must spread correlationId into the log meta only when truthy" + ); + + // Exhaustion with an id returns the upstream error; without an id the + // response shape is unchanged. + const withId = chatHelpers.handleNoCredentials( + null, + "conn-1", + "opencode-test", + "test-model", + "upstream 500", + 500, + undefined, + false, + null, + "trace-123" + ); + assert.equal(withId.status, 500); + const withBody = (await withId.json()) as { error?: { message?: string } }; + assert.equal(withBody?.error?.message, "upstream 500"); + + const withoutId = chatHelpers.handleNoCredentials( + null, + "conn-1", + "opencode-test", + "test-model", + "upstream 500", + 500 + ); + assert.equal(withoutId.status, 500); + const withoutBody = (await withoutId.json()) as { error?: { message?: string } }; + assert.equal(withoutBody?.error?.message, "upstream 500"); + }); +}); diff --git a/tests/unit/opencode-transient-failure-predicate.test.ts b/tests/unit/opencode-transient-failure-predicate.test.ts new file mode 100644 index 0000000000..a5dee38e64 --- /dev/null +++ b/tests/unit/opencode-transient-failure-predicate.test.ts @@ -0,0 +1,36 @@ +import { describe, it } from "node:test"; +import assert from "node:assert"; +import { isRetriableUpstreamFailure } from "../../open-sse/executors/opencodeTransientFailure.ts"; + +const EMPTY_400_BODY = JSON.stringify({ + id: "chatcmpl-abc123", + choices: [{ message: {}, finish_reason: null }], +}); +const REAL_400_BODY = JSON.stringify({ error: { message: "bad request" } }); + +describe("isRetriableUpstreamFailure", () => { + it("matches 500/502/503/504 by status alone, no body needed", () => { + assert.strictEqual(isRetriableUpstreamFailure(500), true); + assert.strictEqual(isRetriableUpstreamFailure(502), true); + assert.strictEqual(isRetriableUpstreamFailure(503), true); + assert.strictEqual(isRetriableUpstreamFailure(504), true); + }); + it("matches 500 even with a body present (status short-circuits first)", () => { + assert.strictEqual(isRetriableUpstreamFailure(500, "Internal server error"), true); + }); + it("matches empty 400 with body", () => { + assert.strictEqual(isRetriableUpstreamFailure(400, EMPTY_400_BODY), true); + }); + it("rejects real-error 400", () => { + assert.strictEqual(isRetriableUpstreamFailure(400, REAL_400_BODY), false); + }); + it("rejects 400 without body (absent = non-empty = no retry)", () => { + assert.strictEqual(isRetriableUpstreamFailure(400), false); + assert.strictEqual(isRetriableUpstreamFailure(400, ""), false); + }); + it("rejects 403/429/200", () => { + assert.strictEqual(isRetriableUpstreamFailure(403), false); + assert.strictEqual(isRetriableUpstreamFailure(429), false); + assert.strictEqual(isRetriableUpstreamFailure(200), false); + }); +}); diff --git a/tests/unit/opencode-transient-rotation.test.ts b/tests/unit/opencode-transient-rotation.test.ts new file mode 100644 index 0000000000..76aab6c11f --- /dev/null +++ b/tests/unit/opencode-transient-rotation.test.ts @@ -0,0 +1,514 @@ +import { describe, it, beforeEach, afterEach, before, after } from "node:test"; +import assert from "node:assert"; +import net from "node:net"; +import { OpencodeExecutor } from "../../open-sse/executors/opencode.ts"; +import type { ExecutorLog, ProviderCredentials } from "../../open-sse/executors/base.ts"; +import { resolveProxyForRequest } from "../../open-sse/utils/proxyFetch.ts"; + +const log: ExecutorLog = { debug() {}, info() {}, warn() {}, error() {} }; + +const FP_A = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const FP_B = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; +const FP_C = "cccccccccccccccccccccccccccccccc"; + +let serverA: net.Server; +let serverB: net.Server; +let serverC: net.Server; +let portA = 0; +let portB = 0; +let portC = 0; + +function listen(server: net.Server): Promise { + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + resolve((server.address() as net.AddressInfo).port); + }); + }); +} + +before(async () => { + serverA = net.createServer((s) => s.destroy()); + serverB = net.createServer((s) => s.destroy()); + serverC = net.createServer((s) => s.destroy()); + portA = await listen(serverA); + portB = await listen(serverB); + portC = await listen(serverC); +}); + +after(() => { + serverA?.close(); + serverB?.close(); + serverC?.close(); +}); + +function portFor(fp: string): number { + if (fp === FP_A) return portA; + if (fp === FP_B) return portB; + return portC; +} + +function credentialsFor(fingerprints: string[]): ProviderCredentials { + return { + apiKey: null, + accessToken: null, + connectionId: "noauth", + providerSpecificData: { + fingerprints, + accountProxies: fingerprints.map((fp) => ({ + fingerprint: fp, + proxy: { type: "http", host: "127.0.0.1", port: portFor(fp) }, + })), + }, + }; +} + +describe("OpencodeExecutor transient-failure rotation", () => { + let originalFetch: typeof globalThis.fetch; + let observed: string[]; + + beforeEach(() => { + originalFetch = globalThis.fetch; + observed = []; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + class CloneCountingResponse extends Response { + static clones = 0; + clone(): Response { + CloneCountingResponse.clones++; + return super.clone(); + } + } + + function installFetch(plan: Array<{ status: number; body?: string }>) { + let call = 0; + CloneCountingResponse.clones = 0; + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + const resolved = resolveProxyForRequest(url); + observed.push(resolved.proxyUrl ? new URL(resolved.proxyUrl).port : "direct"); + const step = plan[Math.min(call, plan.length - 1)]; + call++; + return new CloneCountingResponse(step.body ?? JSON.stringify({ ok: step.status === 200 }), { + status: step.status, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof globalThis.fetch; + } + + it("rotates past a 500 to the healthy proxy without cooldown", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + installFetch([{ status: 500 }, { status: 200 }]); + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B]), + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 200); + assert.strictEqual(observed.length, 2); + assert.strictEqual(observed[0], String(portA)); + assert.strictEqual( + CloneCountingResponse.clones, + 1, + "only success-path normalize clones; 500 branch reads no body" + ); + }); + + it("rotates on 502/503/504 like on 500", async () => { + for (const status of [502, 503, 504]) { + observed = []; + const exec = new OpencodeExecutor("opencode-zen"); + installFetch([{ status }, { status: 200 }]); + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B]), + log, + }); + + assert.strictEqual( + (result as { response: Response }).response.status, + 200, + `status ${status} must rotate` + ); + assert.strictEqual(observed.length, 2); + } + }); + + it("single account without proxy stays on fast path on 500 (propagates)", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + installFetch([{ status: 500 }]); + + const creds = credentialsFor([FP_A]); + (creds.providerSpecificData as Record).accountProxies = []; + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: creds, + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 500); + assert.strictEqual(observed.length, 1); + }); + + it("true mono-direct (no fingerprints) propagates 500 without success mark", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + installFetch([{ status: 500 }]); + + const creds: ProviderCredentials = { + apiKey: null, + accessToken: null, + connectionId: "noauth", + providerSpecificData: { fingerprints: [] }, + }; + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: creds, + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 500); + assert.strictEqual(observed.length, 1, "fast path: single call, no loop"); + }); + + it("propagates the last 500 after exhausting all proxies", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + installFetch([{ status: 200 }]); + await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B, FP_C]), + log, + }); + const warm = ( + exec as unknown as { accounts: Array<{ cooldownUntil: number; consecutiveFails: number }> } + ).accounts; + assert.strictEqual(warm.length, 3, "warm-up materialized all accounts"); + for (const a of warm) a.consecutiveFails = 2; + installFetch([{ status: 500 }, { status: 500 }, { status: 500 }]); + observed = []; + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B, FP_C]), + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 500); + assert.strictEqual(observed.length, 3, "every proxy tried exactly once"); + for (const port of [portA, portB, portC]) { + assert.ok(observed.includes(String(port)), `proxy ${port} tried`); + } + const after = ( + exec as unknown as { accounts: Array<{ cooldownUntil: number; consecutiveFails: number }> } + ).accounts; + for (const a of after) { + assert.strictEqual(a.cooldownUntil, 0, "no cooldown from 500 exhaustion"); + assert.strictEqual(a.consecutiveFails, 2, "500 exhaustion never marks success"); + } + }); + + it("never re-touches a proxy tried by either 500 or geo-403", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + const GEO_BODY = JSON.stringify({ + error: { type: "RegionError", message: "This model is not available in your country." }, + }); + installFetch([{ status: 500 }, { status: 403, body: GEO_BODY }, { status: 200 }]); + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B, FP_C]), + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 200); + assert.strictEqual(observed.length, 3); + assert.strictEqual( + observed.filter((p) => p === String(portA)).length, + 1, + "500-tried proxy A called exactly once" + ); + }); + + it("a 429 still cools down while a 500 rotates cleanly", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + installFetch([{ status: 500 }, { status: 429 }, { status: 200 }]); + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B, FP_C]), + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 200); + assert.strictEqual(observed.length, 3); + const state = (exec as unknown as { accounts: Array<{ cooldownUntil: number }> }).accounts; + const cooled = state.filter((a) => a.cooldownUntil > Date.now()); + assert.strictEqual(cooled.length, 1, "exactly the 429 account cooled down"); + }); + + it("single proxied account: one retry on 500, then last surfaces", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + const creds = credentialsFor([FP_A]); + installFetch([{ status: 500 }, { status: 500 }]); + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: creds, + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 500); + assert.strictEqual(observed.length, 2, "one retry via the mono budget, then stop"); + }); + + it("500 rotation never cools the account down", async () => { + const exec2 = new OpencodeExecutor("opencode-zen"); + installFetch([{ status: 200 }]); + await exec2.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B]), + log, + }); + const mid = ( + exec2 as unknown as { accounts: Array<{ cooldownUntil: number; consecutiveFails: number }> } + ).accounts; + assert.strictEqual(mid.length, 2, "warm-up materialized both accounts"); + for (const a of mid) a.consecutiveFails = 2; + installFetch([{ status: 500 }, { status: 200 }]); + await exec2.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B]), + log, + }); + const after = ( + exec2 as unknown as { accounts: Array<{ cooldownUntil: number; consecutiveFails: number }> } + ).accounts; + for (const a of after) { + assert.strictEqual(a.cooldownUntil, 0, "no cooldown from 500 rotation"); + } + assert.strictEqual( + after.filter((a) => a.consecutiveFails === 0).length, + 1, + "exactly the winning account resets via markSuccess" + ); + assert.strictEqual( + after.filter((a) => a.consecutiveFails === 2).length, + after.length - 1, + "blocked accounts keep prior fails" + ); + }); + + it("a 500 on the last-resort direct attempt surfaces cleanly", async () => { + const exec = new OpencodeExecutor("opencode-zen"); + const creds = credentialsFor([FP_A, FP_B]); + (creds.providerSpecificData as Record).accountProxies = [ + { fingerprint: FP_A, proxy: { type: "http", host: "127.0.0.1", port: portA } }, + ]; + installFetch([{ status: 500 }, { status: 500 }]); + + const result = await exec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: creds, + log, + }); + + assert.strictEqual((result as { response: Response }).response.status, 500); + assert.strictEqual(observed.length, 2, "one proxied + one direct, direct last"); + assert.strictEqual(observed[0], String(portA)); + assert.strictEqual(observed[1], "direct"); + }); + + it("executor rotation lines carry correlationId", async () => { + // Genuinely overlapped A/B: both execute() calls are in flight + // simultaneously on ONE shared executor (production shape — the registry + // caches one instance per provider). Each of the 4 upstream dispatches is + // a deferred promise resolved in a cross order (B1, A1, A2, B2), so a + // shared/module-level cid — or any cross-request bleed — would attribute + // at least one line to the wrong request and fail the per-id assertions. + const exec = new OpencodeExecutor("opencode-zen"); + const gates: Array<{ + resolve: (r: Response) => void; + url: string; + }> = []; + const gateFetchCalls: string[] = []; + globalThis.fetch = ((input: RequestInfo | URL) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + const resolved = resolveProxyForRequest(url); + gateFetchCalls.push(resolved.proxyUrl ? new URL(resolved.proxyUrl).port : "direct"); + return new Promise((resolve) => { + gates.push({ resolve, url }); + }); + }) as typeof globalThis.fetch; + const ok = () => + new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + const fail500 = () => + new Response(JSON.stringify({ ok: false }), { + status: 500, + headers: { "Content-Type": "application/json" }, + }); + + function runWithLines(id: string) { + const lines: string[] = []; + const spyLog: ExecutorLog = { + debug() {}, + info(tag, message) { + lines.push(`${tag} ${message}`); + }, + warn(tag, message) { + lines.push(`${tag} ${message}`); + }, + error() {}, + }; + const done = exec + .execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B]), + log: spyLog, + correlationId: id, + }) + .then((result) => { + assert.strictEqual( + (result as { response: Response }).response.status, + 200, + `request ${id} must rotate past its 500` + ); + return lines; + }); + return { id, lines, done }; + } + + const reqA = runWithLines("A"); + const reqB = runWithLines("B"); + // Let both first dispatches land before resolving anything: proves both + // requests are in flight simultaneously (the cross-talk window). + for (let i = 0; i < 50 && gates.length < 2; i++) { + await new Promise((r) => setImmediate(r)); + } + assert.strictEqual(gates.length, 2, "both requests must be in flight simultaneously"); + // Controllable cross order: B's 500 first, then A's 500, then A's 200, B's 200. + gates[1].resolve(fail500()); + for (let i = 0; i < 50 && gates.length < 3; i++) { + await new Promise((r) => setImmediate(r)); + } + gates[0].resolve(fail500()); + for (let i = 0; i < 50 && gates.length < 4; i++) { + await new Promise((r) => setImmediate(r)); + } + assert.strictEqual(gates.length, 4, "both rotations must dispatch a second attempt"); + gates[2].resolve(ok()); + gates[3].resolve(ok()); + const [linesA, linesB] = await Promise.all([reqA.done, reqB.done]); + + for (const [lines, id] of [ + [linesA, "A"], + [linesB, "B"], + ] as const) { + const rotation = lines.filter((l) => /rotating to next|dispatch via account/.test(l)); + assert.ok(rotation.length > 0, `request ${id} must emit rotation lines`); + for (const line of rotation) { + assert.ok( + line.startsWith(`OPENCODE correlationId=${id} `), + `line must start with correlationId=${id}: ${line}` + ); + } + } + assert.ok( + linesA.every((l) => !l.includes("correlationId=B")), + "no cross-talk: A's lines must never carry B's id" + ); + assert.ok( + linesB.every((l) => !l.includes("correlationId=A")), + "no cross-talk: B's lines must never carry A's id" + ); + + // Absent id leaves the line unchanged: no correlationId field, motif intact. + installFetch([{ status: 500 }, { status: 200 }]); + const plainExec = new OpencodeExecutor("opencode-zen"); + const plain: string[] = []; + const plainLog: ExecutorLog = { + debug() {}, + info(tag, message) { + plain.push(`${tag} ${message}`); + }, + warn(tag, message) { + plain.push(`${tag} ${message}`); + }, + error() {}, + }; + const plainResult = await plainExec.execute({ + model: "muse-spark-1.3-contributor-free", + body: { messages: [{ role: "user", content: "hi" }], stream: false }, + stream: false, + signal: null, + credentials: credentialsFor([FP_A, FP_B]), + log: plainLog, + }); + assert.strictEqual((plainResult as { response: Response }).response.status, 200); + const plainRotation = plain.filter((l) => /rotating to next|dispatch via account/.test(l)); + assert.ok(plainRotation.length > 0, "must emit rotation lines without an id"); + for (const line of plainRotation) { + assert.ok(!line.includes("correlationId"), `no id field when absent: ${line}`); + } + assert.ok( + plainRotation.some((l) => + /transient upstream 500 on account .* \(proxy .*\), rotating to next…/.test(l) + ), + "existing 5xx rotation motif byte-identical when no id is present" + ); + assert.ok( + plainRotation.some((l) => /dispatch via account .* \(idx \d+\/2\)/.test(l)), + "existing dispatch motif byte-identical when no id is present" + ); + }); +});