From 88c2e1a4a5048ee72657be3c81e98ac906b0135c Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:56:05 -0300 Subject: [PATCH 1/7] fix(cli): remove MITM DNS spoof entries before killing server process (port from 9router#1809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stopMitm() killed the spawned MITM server process first and only removed the /etc/hosts DNS-spoof entries afterward. During that window any client whose DNS still resolved a target host to 127.0.0.1 but whose MITM listener was already dead got connect ECONNREFUSED 127.0.0.1:443 — exactly the community-confirmed workaround (stop DNS before stopping the server) proves. Swap the two steps so DNS is always cleared first, mirroring the ordering already used by repairMitm() and handleExitCleanup(). Reported-by: dionisius95 (https://github.com/decolua/9router/issues/1809) --- .../fixes/1809-mitm-stop-dns-before-kill.md | 1 + src/mitm/manager.ts | 71 ++++++++++++---- .../mitm-stop-dns-before-kill-1809.test.ts | 82 +++++++++++++++++++ 3 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 changelog.d/fixes/1809-mitm-stop-dns-before-kill.md create mode 100644 tests/unit/mitm-stop-dns-before-kill-1809.test.ts diff --git a/changelog.d/fixes/1809-mitm-stop-dns-before-kill.md b/changelog.d/fixes/1809-mitm-stop-dns-before-kill.md new file mode 100644 index 000000000000..06e42e461ce9 --- /dev/null +++ b/changelog.d/fixes/1809-mitm-stop-dns-before-kill.md @@ -0,0 +1 @@ +- **fix(cli):** `stopMitm()` now removes /etc/hosts DNS-spoof entries before killing the MITM server process, closing the window where a client's DNS still resolved a target host to `127.0.0.1` while nothing was listening there — the cause of `connect ECONNREFUSED 127.0.0.1:443` right after stopping the MITM proxy (thanks @dionisius95). diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index 47765bf51565..ba743469db7c 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -57,6 +57,17 @@ export function interpretMitmStartupError(stderr: string, port: number): string let serverProcess: ChildProcess | null = null; let serverPid: number | null = null; +/** + * Test-only seam: install a fake server process (and pid) so stopMitm() can be + * exercised without spawning a real MITM child. Not part of the public API — + * only intended for unit tests that need to assert stopMitm()'s DNS/kill + * ordering (#1809). No-op in production code paths. + */ +export function __setServerProcessForTest(proc: ChildProcess | null, pid: number | null): void { + serverProcess = proc; + serverPid = pid; +} + // Set while startMitm() is in flight, from the guard check through spawn. // Guards a TOCTOU race: the "already running" check above only trips once // `serverProcess` is assigned by spawn() — ~130 lines and several awaits @@ -710,10 +721,51 @@ async function startMitmInternal( /** * Stop MITM proxy + * + * Ordering is deliberate and load-bearing (#1809 — "connect ECONNREFUSED + * 127.0.0.1:443" after stop). DNS entries MUST be removed BEFORE the server + * process is killed: if the process dies first, any client whose DNS still + * resolves the target host to 127.0.0.1 (from startMitm()'s spoof) but whose + * MITM listener is already dead gets ECONNREFUSED against a dead port for the + * whole window between the two steps. Removing DNS first closes that window — + * once /etc/hosts no longer points at 127.0.0.1, clients fall back to real + * resolution regardless of when the listener actually goes away. This mirrors + * the DNS-first ordering already used by repairMitm() and handleExitCleanup(). * @param {string} sudoPassword - Sudo password for DNS cleanup + * @param _depsOverride - optional dependency override, used in tests for DI. */ -export async function stopMitm(sudoPassword: string): Promise<{ running: false; pid: null }> { - // 1. Kill server process (in-memory or from PID file) +export async function stopMitm( + sudoPassword: string, + _depsOverride?: { + removeDNSEntry?: (sudoPassword: string) => Promise; + removeDNSEntries?: (hosts: string[], sudoPassword: string) => Promise; + collectManagedHosts?: () => string[]; + } +): Promise<{ running: false; pid: null }> { + const deps = { + removeDNSEntry: _depsOverride?.removeDNSEntry ?? removeDNSEntry, + removeDNSEntries: _depsOverride?.removeDNSEntries ?? removeDNSEntries, + collectManagedHosts: _depsOverride?.collectManagedHosts ?? collectManagedHosts, + }; + + // 1. Remove DNS entries FIRST — Antigravity defaults PLUS every agent + + // custom host that startMitm() may have spoofed. removeDNSEntries is + // idempotent, so over-inclusion is safe; under-inclusion leaks + // /etc/hosts lines that hijack resolution machine-wide after stop + // (Gap 8). Doing this before the process kill closes the ECONNREFUSED + // window described above (#1809). + log.info("Removing DNS entries..."); + await deps.removeDNSEntry(sudoPassword); + try { + const managed = deps.collectManagedHosts(); + if (managed.length > 0) { + await deps.removeDNSEntries(managed, sudoPassword); + } + } catch (err) { + log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); + } + + // 2. Kill server process (in-memory or from PID file) const proc = serverProcess; if (proc && !proc.killed) { log.info("Stopping MITM server..."); @@ -745,21 +797,6 @@ export async function stopMitm(sudoPassword: string): Promise<{ running: false; serverPid = null; } - // 2. Remove DNS entries — Antigravity defaults PLUS every agent + custom host - // that startMitm() may have spoofed. removeDNSEntries is idempotent, so - // over-inclusion is safe; under-inclusion leaks /etc/hosts lines that - // hijack resolution machine-wide after stop (Gap 8). - log.info("Removing DNS entries..."); - await removeDNSEntry(sudoPassword); - try { - const managed = collectManagedHosts(); - if (managed.length > 0) { - await removeDNSEntries(managed, sudoPassword); - } - } catch (err) { - log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); - } - // 3. Clean up clearCachedPassword(); // Clear password from memory when proxy stops try { diff --git a/tests/unit/mitm-stop-dns-before-kill-1809.test.ts b/tests/unit/mitm-stop-dns-before-kill-1809.test.ts new file mode 100644 index 000000000000..3dd92dcaad0a --- /dev/null +++ b/tests/unit/mitm-stop-dns-before-kill-1809.test.ts @@ -0,0 +1,82 @@ +/** + * Regression test for upstream issue #1809: "connect ECONNREFUSED 127.0.0.1:443" + * after stopping the MITM proxy. + * + * Root cause: stopMitm() killed the spawned MITM server process FIRST, and only + * removed the /etc/hosts DNS-spoof entries AFTER. During that window any client + * whose DNS still resolved the target host to 127.0.0.1 (from startMitm's spoof) + * but whose MITM listener was already dead got ECONNREFUSED — exactly the + * community-confirmed workaround ("stop DNS before stopping the server") proves. + * + * This test drives stopMitm() with real DI: a fake serverProcess standing in for + * the spawned MITM child, and dependency-injected DNS-removal functions that + * record the order in which they are invoked relative to the process kill. The + * fix must remove DNS entries before killing the server process so no window + * exists where DNS points at 127.0.0.1 with nothing listening there. + * + * Uses the project's DATA_DIR-tmp + resetDbInstance pattern so the Node native + * test runner does not hang on open SQLite handles (CLAUDE.md PII learning #3). + */ +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 { EventEmitter } from "node:events"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-mitm-stop-order-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const manager = await import("../../src/mitm/manager.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("stopMitm removes DNS entries before killing the MITM server process (#1809)", async () => { + const events: string[] = []; + + // Fake child process standing in for the spawned MITM server. + const fakeProc = new EventEmitter() as EventEmitter & { + killed: boolean; + kill: (signal?: string) => boolean; + }; + fakeProc.killed = false; + fakeProc.kill = (signal?: string) => { + events.push(`kill:${signal}`); + fakeProc.killed = true; + return true; + }; + + manager.__setServerProcessForTest(fakeProc as unknown as import("child_process").ChildProcess, 4242); + + const removeDNSEntry = async () => { + events.push("removeDNSEntry"); + }; + const removeDNSEntries = async () => { + events.push("removeDNSEntries"); + }; + const collectManagedHosts = () => ["fake.example.test"]; + + await manager.stopMitm("fake-sudo-password", { + removeDNSEntry, + removeDNSEntries, + collectManagedHosts, + }); + + const firstKillIndex = events.findIndex((e) => e.startsWith("kill:")); + const firstDnsIndex = events.findIndex( + (e) => e === "removeDNSEntry" || e === "removeDNSEntries" + ); + + assert.ok(firstKillIndex !== -1, "server process kill was never invoked"); + assert.ok(firstDnsIndex !== -1, "DNS removal was never invoked"); + assert.ok( + firstDnsIndex < firstKillIndex, + `DNS entries must be removed BEFORE the MITM server process is killed ` + + `(got order: ${JSON.stringify(events)}) — otherwise a client whose DNS still ` + + `points at 127.0.0.1 hits a dead listener and gets ECONNREFUSED (#1809)` + ); +}); From f7888c965c52d33c4c17552ff3a3cdde6c5d8ad4 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:57:05 -0300 Subject: [PATCH 2/7] fix(combos): reject oversized fusion panels before fan-out (port from 9router#1905) A fusion combo fans every panel model out in parallel and buffers each model's full response text in memory simultaneously. With the runtime heap capped by Dockerfile's OMNIROUTE_MEMORY_MB (default 1024MB), a large panel (reported: ~73 models via an 'auto' combo with strategy: fusion) with sizable concurrent responses can exceed the heap ceiling and OOM-crash the whole container instead of failing one request. handleFusionChat now rejects panels above a configurable hard cap (FUSION_DEFAULTS.maxPanel = 40, overridable per-combo via fusionTuning.maxPanel) with a clean 400 before fan-out begins. Reported-by: Phong Vu (@fontvu) (https://github.com/decolua/9router/issues/1905) --- Dockerfile | 6 ++ changelog.d/fixes/1905-fusion-panel-oom.md | 1 + open-sse/services/fusion.ts | 23 ++++++ src/shared/validation/schemas/combo.ts | 5 ++ tests/unit/fusion-panel-size-cap-1905.test.ts | 79 +++++++++++++++++++ 5 files changed, 114 insertions(+) create mode 100644 changelog.d/fixes/1905-fusion-panel-oom.md create mode 100644 tests/unit/fusion-panel-size-cap-1905.test.ts diff --git a/Dockerfile b/Dockerfile index adf3cf5e910d..98e0a3215b50 100644 --- a/Dockerfile +++ b/Dockerfile @@ -114,6 +114,12 @@ LABEL org.opencontainers.image.title="omniroute" \ ENV NODE_ENV=production ENV PORT=20128 ENV HOSTNAME=0.0.0.0 +# Runtime heap ceiling. 1024MB is enough for normal traffic but can be tight +# for large fusion-combo panels (many models fanned out in parallel, each +# response buffered in full — see open-sse/services/fusion.ts::FUSION_DEFAULTS +# .maxPanel, issue #1905). Override at `docker run` time with +# `-e OMNIROUTE_MEMORY_MB=2048` (or higher) if you raise fusionTuning.maxPanel +# above the default cap. ENV OMNIROUTE_MEMORY_MB=1024 ENV NODE_OPTIONS="--max-old-space-size=${OMNIROUTE_MEMORY_MB}" diff --git a/changelog.d/fixes/1905-fusion-panel-oom.md b/changelog.d/fixes/1905-fusion-panel-oom.md new file mode 100644 index 000000000000..68bc6dbfbc71 --- /dev/null +++ b/changelog.d/fixes/1905-fusion-panel-oom.md @@ -0,0 +1 @@ +- **fix(combos):** fusion combos now reject an oversized panel (>40 models by default, tunable via `fusionTuning.maxPanel`) with a clean 400 before fanning out, instead of buffering dozens of concurrent full responses in memory and OOM-crashing the whole container. (thanks @fontvu) diff --git a/open-sse/services/fusion.ts b/open-sse/services/fusion.ts index 000f5a21c517..d6e5f4fa1f29 100644 --- a/open-sse/services/fusion.ts +++ b/open-sse/services/fusion.ts @@ -27,12 +27,20 @@ export const FUSION_DEFAULTS = { minPanel: 2, // answers needed before stragglers get a grace window stragglerGraceMs: 8000, // wait this long for laggards once quorum is reached panelHardTimeoutMs: 90000, // absolute cap so one hung model can't stall forever + // Hard cap on panel size (issue #1905). Every panel member is fanned out in + // parallel and its full response text buffered in memory simultaneously — + // with the runtime heap capped (Dockerfile OMNIROUTE_MEMORY_MB, default + // 1024MB), a large panel (reported: ~73 models) with sizable concurrent + // responses can exceed the heap ceiling and OOM-crash the whole process. + // Reject oversized panels up front with a clean 400 instead. + maxPanel: 40, } as const; export type FusionTuning = { minPanel?: number; stragglerGraceMs?: number; panelHardTimeoutMs?: number; + maxPanel?: number; }; type Body = Record; @@ -246,6 +254,21 @@ export async function handleFusionChat({ return handleSingleModel(body, panel[0]); } + // Reject an oversized panel BEFORE fan-out (issue #1905): fanning out N + // parallel calls and buffering N full response bodies at once is what + // drives the process into an OOM crash, not any one call in isolation. + const maxPanel = tuning?.maxPanel ?? FUSION_DEFAULTS.maxPanel; + if (panel.length > maxPanel) { + log.warn( + "FUSION", + `Combo "${comboName ?? ""}" panel=${panel.length} exceeds maxPanel=${maxPanel} — rejecting before fan-out (#1905)` + ); + return errorResponse( + 400, + `Fusion panel too large (${panel.length} models, max ${maxPanel}) — reduce the combo's target count or raise fusionTuning.maxPanel` + ); + } + const cfg = { minPanel: tuning?.minPanel ?? FUSION_DEFAULTS.minPanel, stragglerGraceMs: tuning?.stragglerGraceMs ?? FUSION_DEFAULTS.stragglerGraceMs, diff --git a/src/shared/validation/schemas/combo.ts b/src/shared/validation/schemas/combo.ts index 5c00a1be08fa..fbe95188cd0c 100644 --- a/src/shared/validation/schemas/combo.ts +++ b/src/shared/validation/schemas/combo.ts @@ -227,6 +227,11 @@ export const comboRuntimeConfigSchema = z minPanel: z.coerce.number().int().min(1).max(50).optional(), stragglerGraceMs: z.coerce.number().int().min(0).max(120_000).optional(), panelHardTimeoutMs: z.coerce.number().int().min(1000).max(600_000).optional(), + // Hard cap on panel size (issue #1905) — see FUSION_DEFAULTS.maxPanel in + // open-sse/services/fusion.ts. Bounds how many models can be fanned out + // and buffered in memory concurrently before the container's heap ceiling + // is at risk. + maxPanel: z.coerce.number().int().min(1).max(200).optional(), }) .strict() .optional(), diff --git a/tests/unit/fusion-panel-size-cap-1905.test.ts b/tests/unit/fusion-panel-size-cap-1905.test.ts new file mode 100644 index 000000000000..6b9ac1b93565 --- /dev/null +++ b/tests/unit/fusion-panel-size-cap-1905.test.ts @@ -0,0 +1,79 @@ +/** + * Regression test for upstream issue decolua/9router#1905. + * + * Reported symptom: a fusion combo populated with ~70+ panel models fans every + * member out in parallel (`open-sse/services/fusion.ts::handleFusionChat` → + * `Promise.all`-style fan-out via `collectPanel`), buffering each model's full + * response text in memory at once. With the runtime heap capped at 1024MB + * (Dockerfile `OMNIROUTE_MEMORY_MB`), a large panel with sizable concurrent + * responses can exceed the heap ceiling and crash the whole container with + * "FATAL ERROR: Ineffective mark-compacts near heap limit — JavaScript heap + * out of memory" instead of failing one request gracefully. + * + * Fix: `handleFusionChat` now rejects panels above a configurable hard cap + * (`FUSION_DEFAULTS.maxPanel`, overridable via `fusionTuning.maxPanel`) with a + * clean 400 *before* fan-out, rather than let an unbounded panel size drive + * the process into an OOM crash. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { handleFusionChat, FUSION_DEFAULTS } from "../../open-sse/services/fusion.ts"; + +const noop = () => {}; +const log = { info: noop, warn: noop, debug: noop, error: noop }; + +type Body = Record; + +test("fusion #1905: an oversized panel (73 models) is rejected before fan-out instead of OOM-crashing", async () => { + let calls = 0; + const handleSingleModel = (_b: Body, _m: string) => { + calls++; + const body = JSON.stringify({ + choices: [{ message: { role: "assistant", content: "x".repeat(1000) } }], + }); + return Promise.resolve( + new Response(body, { status: 200, headers: { "Content-Type": "application/json" } }) + ); + }; + + const panel = Array.from({ length: 73 }, (_, i) => `provider/model-${i}`); + + const res = await handleFusionChat({ + body: { messages: [{ role: "user", content: "hi" }] }, + models: panel, + handleSingleModel, + log, + comboName: "auto", + }); + + assert.equal(res.status, 400); + // Must reject BEFORE fan-out — no per-model calls should have happened. + assert.equal(calls, 0, "panel fan-out must not start once the size cap is exceeded"); + + const json = (await res.json()) as { error?: { message?: string } }; + assert.match(json.error?.message ?? "", /panel/i); +}); + +test("fusion #1905: a panel at or under the cap still fans out normally", async () => { + const handleSingleModel = (_b: Body, _m: string) => { + const body = JSON.stringify({ + choices: [{ message: { role: "assistant", content: "ok" } }], + }); + return Promise.resolve( + new Response(body, { status: 200, headers: { "Content-Type": "application/json" } }) + ); + }; + + const panel = Array.from({ length: FUSION_DEFAULTS.maxPanel }, (_, i) => `provider/model-${i}`); + + const res = await handleFusionChat({ + body: { messages: [{ role: "user", content: "hi" }] }, + models: panel, + handleSingleModel, + log, + comboName: "auto", + }); + + assert.equal(res.status, 200); +}); From 38ebca0af1134dd295c4a99bd9285ed02f0b9b34 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:57:31 -0300 Subject: [PATCH 3/7] fix(combo): detect empty content_block in streaming SSE peek (port from 9router#1382) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bounded SSE peek in validateResponseQuality() treated ANY content_block_start/delta/stop event as proof of real output and stopped buffering immediately, without checking whether the block actually carried text/tool_use content. Some upstreams (reported: DeepSeek, GLM via claude→openai translation) can open and close a text content_block with empty text and no tool_use on tool-heavy requests — the gateway logged success and forwarded a client-visible empty completion, and combo routing never failed over to the next model. Track real content separately from 'a content_block_* event was seen': a tool_use/redacted_thinking block start is self-evidently real signal, a text/thinking block start is not (real content only confirmed via a subsequent delta carrying non-empty text/thinking, or an input_json_delta streaming tool arguments). A completed lifecycle (message_start + message_delta/stop) that never produced real content now fails validateResponseQuality(), matching the existing content_filter empty-stream detection path (#3685). Reported-by: heishen6 (https://github.com/decolua/9router/issues/1382) --- .../1382-streaming-empty-content-block.md | 1 + open-sse/services/combo/validateQuality.ts | 70 +++++++-- ...streaming-empty-content-block-1382.test.ts | 138 ++++++++++++++++++ 3 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixes/1382-streaming-empty-content-block.md create mode 100644 tests/unit/streaming-empty-content-block-1382.test.ts diff --git a/changelog.d/fixes/1382-streaming-empty-content-block.md b/changelog.d/fixes/1382-streaming-empty-content-block.md new file mode 100644 index 000000000000..c224cd299199 --- /dev/null +++ b/changelog.d/fixes/1382-streaming-empty-content-block.md @@ -0,0 +1 @@ +- **fix(combo):** streaming Claude responses whose content block opens (`content_block_start`) and closes with no usable text/tool_use — a shape some upstreams return for tool-heavy requests on HTTP 200 — are now detected by `validateResponseQuality`'s SSE peek and trigger combo failover instead of being forwarded to the client as a silent empty completion (thanks @heishen6). diff --git a/open-sse/services/combo/validateQuality.ts b/open-sse/services/combo/validateQuality.ts index 2b5614647952..e0bd3f7619ab 100644 --- a/open-sse/services/combo/validateQuality.ts +++ b/open-sse/services/combo/validateQuality.ts @@ -127,6 +127,16 @@ export async function validateResponseQuality( // SSE lifecycle state. let hasMessageStart = false; let hasContentBlock = false; + // #1382: hasContentBlock only means "a content_block_* event was observed" + // — it does NOT mean the block carried usable content. A content_block_start + // for a text/thinking block routinely opens with empty text (real content + // arrives via subsequent content_block_delta events); some upstreams + // (reported: DeepSeek/GLM via claude→openai translation on tool-heavy + // requests) open and close such a block without ever emitting a delta. + // hasRealContent tracks whether we've actually seen usable output: a + // tool_use/redacted_thinking block start (self-evidently real, even before + // any delta), or a delta carrying non-empty text/thinking/tool-input. + let hasRealContent = false; let hasLifecycleEnd = false; let anyContentFound = false; let sawAnyBytes = false; @@ -138,8 +148,9 @@ export async function validateResponseQuality( * flags in the closure. The last (potentially incomplete) line is kept in * `decodedSoFar` for the next iteration. * - * Returns true when a content_block_* event is detected — the caller - * should stop peeking and treat the stream as non-empty. + * Returns true once REAL content (not just an empty content_block_start) + * is detected — the caller should stop peeking and treat the stream as + * non-empty. */ function parseAccumulatedSse(): boolean { const lines = decodedSoFar.split(/\r?\n/); @@ -181,12 +192,47 @@ export async function validateResponseQuality( case "message_start": hasMessageStart = true; break; - case "content_block_start": - case "content_block_delta": + case "content_block_start": { + hasContentBlock = true; + const contentBlock = parsed.content_block; + const blockType = + contentBlock && typeof contentBlock === "object" + ? (contentBlock as Record).type + : undefined; + // tool_use / redacted_thinking blocks are real signal as soon as + // they start — a tool call is meaningful even before its + // input_json_delta arrives. text/thinking blocks routinely open + // empty; keep peeking for a delta that proves real content. + if (blockType === "tool_use" || blockType === "redacted_thinking") { + hasRealContent = true; + return true; + } + break; + } + case "content_block_delta": { + hasContentBlock = true; + const delta = parsed.delta; + if (delta && typeof delta === "object") { + const d = delta as Record; + const deltaType = typeof d.type === "string" ? d.type : ""; + const hasNonEmptyText = + (deltaType === "text_delta" || deltaType === "thinking_delta") && + typeof (d.text ?? d.thinking) === "string" && + ((d.text ?? d.thinking) as string).length > 0; + // input_json_delta carries tool-call argument fragments — any + // fragment (even an empty-string first chunk) is real signal + // that a tool_use block is actively streaming arguments. + const hasToolInput = deltaType === "input_json_delta"; + if (hasNonEmptyText || hasToolInput) { + hasRealContent = true; + return true; + } + } + break; + } case "content_block_stop": hasContentBlock = true; - // Signal caller to stop buffering immediately. - return true; + break; case "message_stop": hasLifecycleEnd = true; break; @@ -258,11 +304,17 @@ export async function validateResponseQuality( if (decodedSoFar.trim()) decodedSoFar += "\n\n"; parseAccumulatedSse(); - if (hasMessageStart && hasLifecycleEnd && !hasContentBlock) { - // Complete Claude lifecycle with zero content blocks → failover. + if (hasMessageStart && hasLifecycleEnd && !hasRealContent) { + // Complete Claude lifecycle with zero content blocks, or with + // content_block_start/stop pairs that never carried real text/ + // thinking/tool_use content (#1382 — tool-heavy claude→openai + // requests against upstreams like DeepSeek/GLM can "complete" a + // lifecycle around an empty block) → failover. log.warn?.( "COMBO", - "Streaming Claude response has complete lifecycle but zero content blocks (content_filter?) — marking as invalid for combo failover" + hasContentBlock + ? "Streaming Claude response has complete lifecycle but its content block(s) carried no usable text/tool_use — marking as invalid for combo failover" + : "Streaming Claude response has complete lifecycle but zero content blocks (content_filter?) — marking as invalid for combo failover" ); return { valid: false, reason: "streaming empty content block" }; } diff --git a/tests/unit/streaming-empty-content-block-1382.test.ts b/tests/unit/streaming-empty-content-block-1382.test.ts new file mode 100644 index 000000000000..750d7d673a8b --- /dev/null +++ b/tests/unit/streaming-empty-content-block-1382.test.ts @@ -0,0 +1,138 @@ +/** + * Issue #1382 (upstream decolua/9router) — a streaming Claude response that + * opens a `content_block_start` (type "text", initial text "") and then + * immediately `content_block_stop`s WITHOUT ever emitting a + * `content_block_delta` carrying real text/tool_use content must be treated + * as an empty/malformed response, not a valid completion. + * + * Before this fix, `validateResponseQuality`'s bounded SSE peek stopped + * buffering (and reported `valid: true`) as soon as ANY content_block_* + * event was observed — including a content_block_start whose block never + * carries usable text. Tool-heavy requests against backends that mishandle + * tool definitions (reported: DeepSeek, GLM via claude→openai translation) + * can emit exactly this shape: a lifecycle that "completes" successfully at + * the transport layer while the client receives no usable content. The + * combo loop never saw this as a failure, so no failover to the next model + * in the combo ever happened. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { validateResponseQuality } = await import("../../open-sse/services/combo.ts"); + +const encoder = new TextEncoder(); +const silentLog = { warn: () => {} }; + +function claudeSseStream(events: string[]): ReadableStream { + const body = events.join("\n") + "\n"; + return new ReadableStream({ + start(controller) { + controller.enqueue(encoder.encode(body)); + controller.close(); + }, + }); +} + +/** + * Build a mock Claude 200 streaming response with a content_block_start/stop + * pair carrying EMPTY text and no tool_use block — the shape reported in + * #1382 for tool-heavy claude→openai requests against DeepSeek/GLM. + */ +function makeEmptyTextBlockStream(): Response { + const events = [ + `event: message_start\ndata: ${JSON.stringify({ + type: "message_start", + message: { + id: "msg_test_1382", + type: "message", + role: "assistant", + model: "deepseek-v4-pro-max", + content: [], + stop_reason: null, + stop_sequence: null, + usage: { input_tokens: 19882, output_tokens: 0 }, + }, + })}`, + "", + `event: content_block_start\ndata: ${JSON.stringify({ + type: "content_block_start", + index: 0, + content_block: { type: "text", text: "" }, + })}`, + "", + `event: content_block_stop\ndata: ${JSON.stringify({ + type: "content_block_stop", + index: 0, + })}`, + "", + `event: message_delta\ndata: ${JSON.stringify({ + type: "message_delta", + delta: { stop_reason: "end_turn", stop_sequence: null }, + usage: { input_tokens: 0, output_tokens: 25 }, + })}`, + "", + `event: message_stop\ndata: ${JSON.stringify({ type: "message_stop" })}`, + "", + ]; + + return new Response(claudeSseStream(events), { + status: 200, + headers: { "content-type": "text/event-stream" }, + }); +} + +test("#1382 streaming Claude response with empty content_block (no text, no tool_use) is marked invalid", async () => { + const res = makeEmptyTextBlockStream(); + const out = await validateResponseQuality(res, true, silentLog); + assert.equal( + out.valid, + false, + `expected invalid for empty content_block stream, got valid=true (reason: ${out.reason})` + ); + assert.match(out.reason ?? "", /empty/i, `reason should mention 'empty', got: "${out.reason}"`); +}); + +test("#1382 streaming Claude response with a real tool_use content_block_start remains valid", async () => { + const events = [ + `event: message_start\ndata: ${JSON.stringify({ + type: "message_start", + message: { + id: "msg_test_1382_tool", + type: "message", + role: "assistant", + model: "deepseek-v4-pro-max", + content: [], + stop_reason: null, + stop_sequence: null, + usage: { input_tokens: 100, output_tokens: 0 }, + }, + })}`, + "", + `event: content_block_start\ndata: ${JSON.stringify({ + type: "content_block_start", + index: 0, + content_block: { type: "tool_use", id: "toolu_1", name: "Bash", input: {} }, + })}`, + "", + `event: content_block_stop\ndata: ${JSON.stringify({ + type: "content_block_stop", + index: 0, + })}`, + "", + `event: message_delta\ndata: ${JSON.stringify({ + type: "message_delta", + delta: { stop_reason: "tool_use", stop_sequence: null }, + usage: { input_tokens: 0, output_tokens: 12 }, + })}`, + "", + `event: message_stop\ndata: ${JSON.stringify({ type: "message_stop" })}`, + "", + ]; + const res = new Response(claudeSseStream(events), { + status: 200, + headers: { "content-type": "text/event-stream" }, + }); + const out = await validateResponseQuality(res, true, silentLog); + assert.equal(out.valid, true, `expected valid for tool_use stream, got invalid: ${out.reason}`); + assert.ok(out.clonedResponse, "clonedResponse must be present for valid streaming response"); +}); From 4d80e0a6692eb22bbf667f5dfcb283a3d6817dfc Mon Sep 17 00:00:00 2001 From: KooshaPari Date: Mon, 13 Jul 2026 19:49:31 -0700 Subject: [PATCH 4/7] fix(auto): use p95 fallback in speed factors --- .../autoCombo/__tests__/speedRanking.test.ts | 29 +++++++++++++++---- open-sse/services/autoCombo/speedRanking.ts | 10 +++++-- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/open-sse/services/autoCombo/__tests__/speedRanking.test.ts b/open-sse/services/autoCombo/__tests__/speedRanking.test.ts index 1599e36b82f3..e3a4ad85f241 100644 --- a/open-sse/services/autoCombo/__tests__/speedRanking.test.ts +++ b/open-sse/services/autoCombo/__tests__/speedRanking.test.ts @@ -165,15 +165,34 @@ describe("rankBySpeed — factor breakdown", () => { }); it("falls back to 0.5 per missing metric so new providers are not crushed", () => { - const ranked = rankBySpeed([candidate({ provider: "fresh", model: "m" })]); + const ranked = rankBySpeed([ + candidate({ + provider: "fresh", + model: "m", + p95LatencyMs: undefined, + latencyStdDev: undefined, + }), + ]); expect(ranked).toHaveLength(1); // No telemetry at all → weighted sum lands near 0.5 with reliability multiplier 1 expect(ranked[0].factors.reliability).toBe(1); expect(ranked[0].factors.health).toBe(1); expect(ranked[0].factors.ttft).toBe(0.5); - expect(ranked[0].factors.tps).toBe(0.5); - }); -}); + expect(ranked[0].factors.tps).toBe(0.5); + }); + + it("uses p95 latency when TTFT and E2E telemetry are unavailable", () => { + const ranked = rankBySpeed([ + candidate({ provider: "slow-tail", model: "m", p95LatencyMs: 4000 }), + candidate({ provider: "fast-tail", model: "m", p95LatencyMs: 1000 }), + ]); + const fast = ranked.find((entry) => entry.provider === "fast-tail"); + const slow = ranked.find((entry) => entry.provider === "slow-tail"); + + expect(fast?.factors.ttft).toBeGreaterThan(slow?.factors.ttft ?? 1); + expect(fast?.factors.e2e).toBeGreaterThan(slow?.factors.e2e ?? 1); + }); +}); describe("rankBySpeed — weight overrides", () => { it("respects caller weight overrides (e.g. heavy TTFT bias)", () => { @@ -223,4 +242,4 @@ describe("pickFastest", () => { const winner = pickFastest([slow, fast]); expect(winner?.provider).toBe("fast"); }); -}); \ No newline at end of file +}); diff --git a/open-sse/services/autoCombo/speedRanking.ts b/open-sse/services/autoCombo/speedRanking.ts index 514420a5a06d..a3d7d8102738 100644 --- a/open-sse/services/autoCombo/speedRanking.ts +++ b/open-sse/services/autoCombo/speedRanking.ts @@ -211,9 +211,15 @@ function speedFactorsFor( failureRate: number ): SpeedFactors { return { - ttft: lowerIsBetter(positiveFinite(candidate.avgTtftMs), maxima.ttft), + ttft: lowerIsBetter( + positiveFinite(candidate.avgTtftMs) ?? positiveFinite(candidate.p95LatencyMs), + maxima.ttft + ), tps: higherIsBetter(positiveFinite(candidate.avgTokensPerSecond), maxima.tps), - e2e: lowerIsBetter(positiveFinite(candidate.avgE2ELatencyMs), maxima.e2e), + e2e: lowerIsBetter( + positiveFinite(candidate.avgE2ELatencyMs) ?? positiveFinite(candidate.p95LatencyMs), + maxima.e2e + ), p95: lowerIsBetter(positiveFinite(candidate.p95LatencyMs), maxima.p95), health: healthScoreFor(candidate.circuitBreakerState), reliability: clamp01(1 - failureRate), From 8f918d7a7e81e564904de515c2039680981943f1 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 14 Jul 2026 21:57:00 -0300 Subject: [PATCH 5/7] refactor(combo): extract SSE lifecycle applier to keep the complexity ratchets at baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #1382 empty-content_block peek added a branchy switch inline in parseAccumulatedSse, pushing check:complexity to 2057 > baseline 2056. Move the switch to a module-level applySseLifecycleEvent() and hold the four lifecycle booleans in a single SseLifecycleFlags object threaded through it, so the closure no longer copies flags in and out per event. The per-event predicates (content_block_start / content_block_delta / message_delta) are split into small guard helpers, which keeps the applier flat — cognitive complexity punishes nesting, and an earlier switch-only extraction traded the cyclomatic ratchet for a cognitive regression at 891 > 890. Logic is unchanged; both ratchets are now green (complexity 2055, cognitive-complexity 890) and the #1382 regression tests still pass. --- open-sse/services/combo/validateQuality.ts | 165 ++++++++++++--------- 1 file changed, 97 insertions(+), 68 deletions(-) diff --git a/open-sse/services/combo/validateQuality.ts b/open-sse/services/combo/validateQuality.ts index e0bd3f7619ab..7f1a32b38a3a 100644 --- a/open-sse/services/combo/validateQuality.ts +++ b/open-sse/services/combo/validateQuality.ts @@ -54,6 +54,91 @@ function extractEnvelopeErrorText(json: Record): string | null return parts.length > 0 ? parts.join(" ") : null; } +/** Mutable lifecycle flags threaded through {@link applySseLifecycleEvent}. */ +interface SseLifecycleFlags { + hasMessageStart: boolean; + hasContentBlock: boolean; + hasRealContent: boolean; + hasLifecycleEnd: boolean; +} + +/** Read `parsed.` as a nested object bag, or null when absent/not an object. */ +function asObject(parsed: Record, key: string): Record | null { + const value = parsed[key]; + return value && typeof value === "object" ? (value as Record) : null; +} + +/** + * A content_block_start is real signal only for tool_use / redacted_thinking — + * a tool call is meaningful even before its input_json_delta arrives. text and + * thinking blocks routinely open empty; keep peeking for a delta instead. + */ +function contentBlockStartIsRealSignal(parsed: Record): boolean { + const blockType = asObject(parsed, "content_block")?.type; + return blockType === "tool_use" || blockType === "redacted_thinking"; +} + +/** + * A content_block_delta is real signal when it carries non-empty text/thinking, + * or any input_json_delta fragment — even an empty-string first chunk proves a + * tool_use block is actively streaming its arguments. + */ +function contentBlockDeltaIsRealSignal(parsed: Record): boolean { + const delta = asObject(parsed, "delta"); + if (!delta) return false; + const deltaType = typeof delta.type === "string" ? delta.type : ""; + if (deltaType === "input_json_delta") return true; + if (deltaType !== "text_delta" && deltaType !== "thinking_delta") return false; + const text = delta.text ?? delta.thinking; + return typeof text === "string" && text.length > 0; +} + +/** A message_delta closes the lifecycle once it carries a stop_reason. */ +function messageDeltaEndsLifecycle(parsed: Record): boolean { + return asObject(parsed, "delta")?.stop_reason != null; +} + +/** + * Apply a single parsed Claude SSE event to the peeked lifecycle `flags` + * (mutated in place). Extracted from `parseAccumulatedSse`'s inline switch to + * keep that function under the complexity/line ratchets — logic unchanged. + * + * Returns true once REAL content (not just an empty content_block_start) is + * detected — the caller should stop peeking and treat the stream as non-empty. + */ +function applySseLifecycleEvent( + eventType: string, + parsed: Record, + flags: SseLifecycleFlags +): boolean { + switch (eventType) { + case "message_start": + flags.hasMessageStart = true; + return false; + case "content_block_start": + flags.hasContentBlock = true; + if (!contentBlockStartIsRealSignal(parsed)) return false; + flags.hasRealContent = true; + return true; + case "content_block_delta": + flags.hasContentBlock = true; + if (!contentBlockDeltaIsRealSignal(parsed)) return false; + flags.hasRealContent = true; + return true; + case "content_block_stop": + flags.hasContentBlock = true; + return false; + case "message_stop": + flags.hasLifecycleEnd = true; + return false; + case "message_delta": + if (messageDeltaEndsLifecycle(parsed)) flags.hasLifecycleEnd = true; + return false; + default: + return false; + } +} + function responsesApiOutputHasContent(output: unknown): boolean { return ( Array.isArray(output) && @@ -125,8 +210,7 @@ export async function validateResponseQuality( let decodedSoFar = ""; // SSE lifecycle state. - let hasMessageStart = false; - let hasContentBlock = false; + // // #1382: hasContentBlock only means "a content_block_* event was observed" // — it does NOT mean the block carried usable content. A content_block_start // for a text/thinking block routinely opens with empty text (real content @@ -136,8 +220,12 @@ export async function validateResponseQuality( // hasRealContent tracks whether we've actually seen usable output: a // tool_use/redacted_thinking block start (self-evidently real, even before // any delta), or a delta carrying non-empty text/thinking/tool-input. - let hasRealContent = false; - let hasLifecycleEnd = false; + const sse: SseLifecycleFlags = { + hasMessageStart: false, + hasContentBlock: false, + hasRealContent: false, + hasLifecycleEnd: false, + }; let anyContentFound = false; let sawAnyBytes = false; const sseLineNormalizer = createSSEDataLineNormalizer(); @@ -188,67 +276,8 @@ export async function validateResponseQuality( return true; } - switch (eventType) { - case "message_start": - hasMessageStart = true; - break; - case "content_block_start": { - hasContentBlock = true; - const contentBlock = parsed.content_block; - const blockType = - contentBlock && typeof contentBlock === "object" - ? (contentBlock as Record).type - : undefined; - // tool_use / redacted_thinking blocks are real signal as soon as - // they start — a tool call is meaningful even before its - // input_json_delta arrives. text/thinking blocks routinely open - // empty; keep peeking for a delta that proves real content. - if (blockType === "tool_use" || blockType === "redacted_thinking") { - hasRealContent = true; - return true; - } - break; - } - case "content_block_delta": { - hasContentBlock = true; - const delta = parsed.delta; - if (delta && typeof delta === "object") { - const d = delta as Record; - const deltaType = typeof d.type === "string" ? d.type : ""; - const hasNonEmptyText = - (deltaType === "text_delta" || deltaType === "thinking_delta") && - typeof (d.text ?? d.thinking) === "string" && - ((d.text ?? d.thinking) as string).length > 0; - // input_json_delta carries tool-call argument fragments — any - // fragment (even an empty-string first chunk) is real signal - // that a tool_use block is actively streaming arguments. - const hasToolInput = deltaType === "input_json_delta"; - if (hasNonEmptyText || hasToolInput) { - hasRealContent = true; - return true; - } - } - break; - } - case "content_block_stop": - hasContentBlock = true; - break; - case "message_stop": - hasLifecycleEnd = true; - break; - case "message_delta": { - const delta = parsed.delta; - if ( - delta && - typeof delta === "object" && - (delta as Record).stop_reason != null - ) { - hasLifecycleEnd = true; - } - break; - } - default: - break; + if (applySseLifecycleEvent(eventType, parsed, sse)) { + return true; } } return false; @@ -304,7 +333,7 @@ export async function validateResponseQuality( if (decodedSoFar.trim()) decodedSoFar += "\n\n"; parseAccumulatedSse(); - if (hasMessageStart && hasLifecycleEnd && !hasRealContent) { + if (sse.hasMessageStart && sse.hasLifecycleEnd && !sse.hasRealContent) { // Complete Claude lifecycle with zero content blocks, or with // content_block_start/stop pairs that never carried real text/ // thinking/tool_use content (#1382 — tool-heavy claude→openai @@ -312,7 +341,7 @@ export async function validateResponseQuality( // lifecycle around an empty block) → failover. log.warn?.( "COMBO", - hasContentBlock + sse.hasContentBlock ? "Streaming Claude response has complete lifecycle but its content block(s) carried no usable text/tool_use — marking as invalid for combo failover" : "Streaming Claude response has complete lifecycle but zero content blocks (content_filter?) — marking as invalid for combo failover" ); @@ -325,7 +354,7 @@ export async function validateResponseQuality( // (an explicit `data: [DONE]`, ping/metadata events, an incomplete // Claude lifecycle) keep the pass-through contract (#3399/#3685): // those are handled by the stream-readiness timeout, not failover. - if (!anyContentFound && !hasContentBlock && !sawAnyBytes) { + if (!anyContentFound && !sse.hasContentBlock && !sawAnyBytes) { log.warn?.( "COMBO", "Streaming response ended with no recognized content — marking as invalid for combo failover" From 6eef6828382f91b7eccb71a0ba97e17b767a48c7 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 14 Jul 2026 21:59:17 -0300 Subject: [PATCH 6/7] refactor(mitm): extract repair planning out of manager to respect the file-size cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #1809 DNS-before-kill ordering fix pushed src/mitm/manager.ts to 813 lines, over the 800-line cap check:file-size enforces for non-frozen files. Move the pure repair-planning pieces (collectManagedHosts, the RepairPlan shape and its filesystem/cert/DNS sweep) into a sibling src/mitm/repair.ts. The in-memory session bookkeeping repairMitm() owns — cached sudo password, orphaned flag, PID file — deliberately stays in manager.ts, so the seam is "plan the repair" vs "own the session". manager.ts is now 731 lines; behavior is unchanged. The DNS-first ordering fix and its regression guard (tests/unit/mitm-stop-dns-before-kill-1809.ts) are untouched and still pass. --- src/mitm/manager.ts | 111 ++++++------------------------------------ src/mitm/repair.ts | 115 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 96 deletions(-) create mode 100644 src/mitm/repair.ts diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index ba743469db7c..99e5cef14da6 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -5,15 +5,22 @@ import { resolveMitmDataDir } from "./dataDir.ts"; import { removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts"; import { provisionDnsEntries } from "./dns/provision.ts"; import { generateCert } from "./cert/generate.ts"; -import { installCertResult, uninstallCert } from "./cert/install.ts"; +import { installCertResult } from "./cert/install.ts"; import { ALL_TARGETS } from "./targets/index.ts"; import { detectAgent } from "./detection/index.ts"; import type { AgentId, DetectionResult, MitmTarget } from "./types.ts"; import { getAllAgentBridgeStates } from "@/lib/db/agentBridgeState.ts"; -import { listCustomHosts } from "@/lib/db/inspectorCustomHosts.ts"; import { getUserBypassPatterns } from "@/lib/db/agentBridgeBypass.ts"; import { configureUpstreamCa } from "./upstreamTrust.ts"; import { createLogger } from "@/shared/utils/logger.ts"; +import { + buildRepairPlan, + collectManagedHosts, + performRepairSteps, + type RepairPlan, +} from "./repair.ts"; + +export { buildRepairPlan, collectManagedHosts, type RepairPlan }; const log = createLogger("mitm-manager"); @@ -230,108 +237,20 @@ function isProcessAlive(pid: number): boolean { } } -/** - * Enumerate every hostname OmniRoute may have written to /etc/hosts during - * startMitm(): the full agent-target registry plus all custom hosts. Removal - * via removeDNSEntries() is idempotent (absent entries are skipped), so this - * set is intentionally over-inclusive — a host that was never spoofed costs - * nothing to "remove", but a host we forget to list leaks machine-wide. - * (Gap 8 — clean-stop DNS leak.) - */ -export function collectManagedHosts(): string[] { - const hosts = new Set(); - for (const target of ALL_TARGETS) { - for (const h of target.hosts) hosts.add(h); - } - try { - for (const ch of listCustomHosts()) hosts.add(ch.host); - } catch (err) { - log.error({ err }, "collectManagedHosts: failed to read custom hosts (continuing)"); - } - return [...hosts]; -} - -export interface RepairPlan { - dnsHostsToRemove: string[]; - removeCert: boolean; - revertSystemProxy: boolean; -} - -/** - * Pure description of what a repair must undo. Separated from repairMitm() so - * the enumeration is unit-testable without touching the OS or requiring sudo. - * (Gap 7.) - */ -export function buildRepairPlan(): RepairPlan { - return { - dnsHostsToRemove: collectManagedHosts(), - removeCert: true, - revertSystemProxy: true, - }; -} - -/** - * Best-effort revert of an applied system proxy. The applied state lives - * in-memory (captureState), so this only succeeds within the same process that - * applied it; after a crash the previousState is gone and this is a no-op. DNS - * + cert teardown are always reversible because they read on-disk state. - */ -async function revertSystemProxyIfApplied(): Promise { - try { - const { getSystemProxyState, clearSystemProxy } = await import("@/lib/inspector/captureState"); - const state = getSystemProxyState(); - if (!state.applied || !state.previousState) return false; - const { revert } = await import("./inspector/systemProxyConfig.ts"); - await revert(state.previousState); - clearSystemProxy(); - return true; - } catch (err) { - log.error({ err }, "revertSystemProxyIfApplied failed (continuing)"); - return false; - } -} - /** * Undo every system mutation startMitm() may have made, WITHOUT requiring the * MITM server to be running. Safe to call when state is already clean (every * step is idempotent). Used by: the /repair route, the CLI cleanup subcommand, * and the stale-PID auto-repair on app startup. (Gap 7 — the application-layer - * analogue of ProxyBridge's destructor + `--cleanup`.) + * analogue of ProxyBridge's destructor + `--cleanup`.) Steps 1-3 (DNS, cert, + * system-proxy) are delegated to `./repair.ts::performRepairSteps()`; the PID + * file + in-memory session cleanup below stays here since it touches this + * module's private state. */ export async function repairMitm(sudoPassword: string): Promise<{ repaired: string[] }> { - const plan = buildRepairPlan(); - const repaired: string[] = []; - - // 1. DNS — remove every host we may have spoofed (idempotent, reads /etc/hosts). - try { - await removeDNSEntry(sudoPassword); - if (plan.dnsHostsToRemove.length > 0) { - await removeDNSEntries(plan.dnsHostsToRemove, sudoPassword); - } - repaired.push("dns"); - } catch (err) { - log.error({ err }, "repairMitm: DNS cleanup failed (continuing)"); - } - - // 2. Certificate — uninstall the MITM root CA from the trust store. - if (plan.removeCert) { - try { - const certPath = path.join(resolveMitmDataDir(), "mitm", "server.crt"); - if (fs.existsSync(certPath)) { - await uninstallCert(sudoPassword, certPath); - repaired.push("cert"); - } - } catch (err) { - log.error({ err }, "repairMitm: cert removal failed (continuing)"); - } - } - - // 3. System proxy — best-effort revert if applied in this process. - if (plan.revertSystemProxy) { - if (await revertSystemProxyIfApplied()) repaired.push("system-proxy"); - } + const repaired = await performRepairSteps(sudoPassword); - // 4. Stale PID file. + // Stale PID file. try { if (fs.existsSync(PID_FILE)) fs.unlinkSync(PID_FILE); } catch { diff --git a/src/mitm/repair.ts b/src/mitm/repair.ts new file mode 100644 index 000000000000..4ee2f160f281 --- /dev/null +++ b/src/mitm/repair.ts @@ -0,0 +1,115 @@ +import path from "path"; +import fs from "fs"; +import { resolveMitmDataDir } from "./dataDir.ts"; +import { removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts"; +import { uninstallCert } from "./cert/install.ts"; +import { ALL_TARGETS } from "./targets/index.ts"; +import { listCustomHosts } from "@/lib/db/inspectorCustomHosts.ts"; +import { createLogger } from "@/shared/utils/logger.ts"; + +const log = createLogger("mitm-repair"); + +/** + * Enumerate every hostname OmniRoute may have written to /etc/hosts during + * startMitm(): the full agent-target registry plus all custom hosts. Removal + * via removeDNSEntries() is idempotent (absent entries are skipped), so this + * set is intentionally over-inclusive — a host that was never spoofed costs + * nothing to "remove", but a host we forget to list leaks machine-wide. + * (Gap 8 — clean-stop DNS leak.) + */ +export function collectManagedHosts(): string[] { + const hosts = new Set(); + for (const target of ALL_TARGETS) { + for (const h of target.hosts) hosts.add(h); + } + try { + for (const ch of listCustomHosts()) hosts.add(ch.host); + } catch (err) { + log.error({ err }, "collectManagedHosts: failed to read custom hosts (continuing)"); + } + return [...hosts]; +} + +export interface RepairPlan { + dnsHostsToRemove: string[]; + removeCert: boolean; + revertSystemProxy: boolean; +} + +/** + * Pure description of what a repair must undo. Separated from repairMitm() so + * the enumeration is unit-testable without touching the OS or requiring sudo. + * (Gap 7.) + */ +export function buildRepairPlan(): RepairPlan { + return { + dnsHostsToRemove: collectManagedHosts(), + removeCert: true, + revertSystemProxy: true, + }; +} + +/** + * Best-effort revert of an applied system proxy. The applied state lives + * in-memory (captureState), so this only succeeds within the same process that + * applied it; after a crash the previousState is gone and this is a no-op. DNS + * + cert teardown are always reversible because they read on-disk state. + */ +async function revertSystemProxyIfApplied(): Promise { + try { + const { getSystemProxyState, clearSystemProxy } = await import("@/lib/inspector/captureState"); + const state = getSystemProxyState(); + if (!state.applied || !state.previousState) return false; + const { revert } = await import("./inspector/systemProxyConfig.ts"); + await revert(state.previousState); + clearSystemProxy(); + return true; + } catch (err) { + log.error({ err }, "revertSystemProxyIfApplied failed (continuing)"); + return false; + } +} + +/** + * Run the DNS/cert/system-proxy teardown steps of a repair, WITHOUT touching + * any of `manager.ts`'s in-memory session state (cached password, orphaned + * flag, PID file) — that bookkeeping stays in `manager.ts::repairMitm()`, + * which calls this as its first step. Split out purely to keep + * `src/mitm/manager.ts` under the repo's file-size cap; behavior is + * unchanged from the original inline implementation. (Gap 7.) + */ +export async function performRepairSteps(sudoPassword: string): Promise { + const plan = buildRepairPlan(); + const repaired: string[] = []; + + // 1. DNS — remove every host we may have spoofed (idempotent, reads /etc/hosts). + try { + await removeDNSEntry(sudoPassword); + if (plan.dnsHostsToRemove.length > 0) { + await removeDNSEntries(plan.dnsHostsToRemove, sudoPassword); + } + repaired.push("dns"); + } catch (err) { + log.error({ err }, "repairMitm: DNS cleanup failed (continuing)"); + } + + // 2. Certificate — uninstall the MITM root CA from the trust store. + if (plan.removeCert) { + try { + const certPath = path.join(resolveMitmDataDir(), "mitm", "server.crt"); + if (fs.existsSync(certPath)) { + await uninstallCert(sudoPassword, certPath); + repaired.push("cert"); + } + } catch (err) { + log.error({ err }, "repairMitm: cert removal failed (continuing)"); + } + } + + // 3. System proxy — best-effort revert if applied in this process. + if (plan.revertSystemProxy) { + if (await revertSystemProxyIfApplied()) repaired.push("system-proxy"); + } + + return repaired; +} From 3de7b97964220fca381a93069e1e17c8fbc1b18a Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Wed, 15 Jul 2026 09:43:29 -0300 Subject: [PATCH 7/7] fix(mitm): split stopMitm() DNS/kill steps to fix complexity ratchet regression stopMitm()'s new DNS-before-kill ordering (#1809) pushed its cyclomatic complexity to 18 (max 15), regressing the complexity ratchet from 2056 to 2057. Extract the DNS-removal step and the process-kill step (in-memory + PID-file fallback) into two private helpers, mirroring the existing performRepairSteps() extraction pattern in repair.ts. Behavior unchanged; complexity back at 2056 (cognitive-complexity drops to 889, one under baseline). --- src/mitm/manager.ts | 116 ++++++++++++++++++++++++++------------------ 1 file changed, 70 insertions(+), 46 deletions(-) diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index 99e5cef14da6..94fcf33b691f 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -638,6 +638,72 @@ async function startMitmInternal( }; } +/** + * DNS teardown step of stopMitm() (#1809) — split out purely to keep + * stopMitm()'s own cyclomatic complexity under the repo's ratchet; behavior + * is unchanged from the original inline implementation. + */ +async function removeStopDnsEntries( + deps: { + removeDNSEntry: (sudoPassword: string) => Promise; + removeDNSEntries: (hosts: string[], sudoPassword: string) => Promise; + collectManagedHosts: () => string[]; + }, + sudoPassword: string +): Promise { + log.info("Removing DNS entries..."); + await deps.removeDNSEntry(sudoPassword); + try { + const managed = deps.collectManagedHosts(); + if (managed.length > 0) { + await deps.removeDNSEntries(managed, sudoPassword); + } + } catch (err) { + log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); + } +} + +/** + * Kill the MITM server process during stop — either the in-memory + * `serverProcess` handle or, if that's gone, the PID recorded in `PID_FILE`. + * Split out of stopMitm() purely to keep that function's complexity under + * the repo's ratchet; behavior is unchanged from the original inline + * implementation. + */ +async function killMitmServerProcessOnStop(): Promise { + const proc = serverProcess; + if (proc && !proc.killed) { + log.info("Stopping MITM server..."); + proc.kill("SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 1000)); + if (!proc.killed) { + proc.kill("SIGKILL"); + } + serverProcess = null; + serverPid = null; + return; + } + + // Fallback: kill by PID file + try { + if (fs.existsSync(PID_FILE)) { + const savedPid = parseInt(fs.readFileSync(PID_FILE, "utf-8").trim(), 10); + if (savedPid && isProcessAlive(savedPid)) { + log.info({ pid: savedPid }, "Killing MITM server by PID..."); + process.kill(savedPid, "SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 1000)); + if (isProcessAlive(savedPid)) { + process.kill(savedPid, "SIGKILL"); + } + } + } + } catch { + // Ignore + } + serverProcess = null; + serverPid = null; +} + /** * Stop MITM proxy * @@ -667,54 +733,12 @@ export async function stopMitm( collectManagedHosts: _depsOverride?.collectManagedHosts ?? collectManagedHosts, }; - // 1. Remove DNS entries FIRST — Antigravity defaults PLUS every agent + - // custom host that startMitm() may have spoofed. removeDNSEntries is - // idempotent, so over-inclusion is safe; under-inclusion leaks - // /etc/hosts lines that hijack resolution machine-wide after stop - // (Gap 8). Doing this before the process kill closes the ECONNREFUSED - // window described above (#1809). - log.info("Removing DNS entries..."); - await deps.removeDNSEntry(sudoPassword); - try { - const managed = deps.collectManagedHosts(); - if (managed.length > 0) { - await deps.removeDNSEntries(managed, sudoPassword); - } - } catch (err) { - log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); - } + // 1. Remove DNS entries FIRST — see function doc + module doc above for why + // this must happen before the process kill (#1809, Gap 8). + await removeStopDnsEntries(deps, sudoPassword); // 2. Kill server process (in-memory or from PID file) - const proc = serverProcess; - if (proc && !proc.killed) { - log.info("Stopping MITM server..."); - proc.kill("SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 1000)); - if (!proc.killed) { - proc.kill("SIGKILL"); - } - serverProcess = null; - serverPid = null; - } else { - // Fallback: kill by PID file - try { - if (fs.existsSync(PID_FILE)) { - const savedPid = parseInt(fs.readFileSync(PID_FILE, "utf-8").trim(), 10); - if (savedPid && isProcessAlive(savedPid)) { - log.info({ pid: savedPid }, "Killing MITM server by PID..."); - process.kill(savedPid, "SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 1000)); - if (isProcessAlive(savedPid)) { - process.kill(savedPid, "SIGKILL"); - } - } - } - } catch { - // Ignore - } - serverProcess = null; - serverPid = null; - } + await killMitmServerProcessOnStop(); // 3. Clean up clearCachedPassword(); // Clear password from memory when proxy stops