From b35f1cc791893144e4f9f9dd77f85bc472b7167c Mon Sep 17 00:00:00 2001 From: adrianojiu Date: Sat, 1 Aug 2026 23:06:59 -0300 Subject: [PATCH] fix(compression): bound session-dedup suffix-block scan to prevent OOM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the request-wide suffix-work char budget (MAX_SUFFIX_WORK_CHARS / reserveSuffixWork) with a simpler per-message bound: MAX_SUFFIX_STARTS (2000) suffix starts and MAX_TOTAL_BLOCK_BYTES (8 MiB) of retained block content. Dedup is best-effort — skipping the tail only forgoes some compression, never changes output correctness. Also replace the per-candidate RegExp-based occurrence counting in dedupeWithinMessage with indexOf-based scanning (replaceAllButFirst), since the regex compile+match was pathologically slow on the exact multi-thousand-line inputs the O(n²) guard targets. Update the #7849 regression suite to match the new bounding mechanism (silent truncation instead of fail-open warning). Co-authored-by: diegosouzapw --- .../engines/session-dedup/index.ts | 157 +++++++++--------- .../session-dedup-memory-7849.test.ts | 46 +++-- tests/unit/compression/session-dedup.test.ts | 38 +++++ 3 files changed, 146 insertions(+), 95 deletions(-) diff --git a/open-sse/services/compression/engines/session-dedup/index.ts b/open-sse/services/compression/engines/session-dedup/index.ts index 0faafbb5a0..a7d4916b5d 100644 --- a/open-sse/services/compression/engines/session-dedup/index.ts +++ b/open-sse/services/compression/engines/session-dedup/index.ts @@ -47,14 +47,19 @@ const DEFAULT_MIN_BLOCK_CHARS = 80; /** Minimum number of lines a block must span to be a dedup candidate. */ const MIN_BLOCK_LINES = 3; /** - * Request-wide ceiling for the suffix strings materialized by the exact pass. - * 32 MiB keeps ordinary sessions byte-identical while preventing line-rich inputs - * from retaining a quadratic graph of suffix copies. + * O(n²) guard for {@link findSuffixBlocks} (OOM incident): a single message with + * thousands of lines otherwise generates one full-length suffix string PER line, + * all retained at once. A real agent conversation embedding a large + * line-numbered file view (e.g. a tool result pasting a multi-thousand-line + * file back into the chat) drove ~1.7GB of live suffix strings and OOM-killed + * the 2GB heap (heap snapshot confirmed 6801 `{ block }` objects). These bound + * both the number of suffix starts scanned + * and the total bytes of retained blocks, so memory is O(budget) instead of O(n²). + * Dedup is best-effort — skipping the tail only forgoes some compression, never + * changes output correctness. */ -const MAX_SUFFIX_WORK_CHARS = 32 * 1024 * 1024; -const SUFFIX_WORK_BUDGET_WARNING = "session-dedup: skipped (suffix work budget exceeded)"; - -type SuffixWorkBudget = { remaining: number }; +const MAX_SUFFIX_STARTS = 2000; +const MAX_TOTAL_BLOCK_BYTES = 8 * 1024 * 1024; // ─── hash helper (SHA-256 prefix, collision-resistant) ─────────────────────── @@ -67,24 +72,6 @@ function hashBlock(text: string): string { // ─── suffix-block extraction ────────────────────────────────────────────────── -/** - * Reserves the characters that findSuffixBlocks() would materialize for one text. - * The scan observes line starts without splitting or constructing any suffix strings. - */ -function reserveSuffixWork(text: string, passCount: number, budget: SuffixWorkBudget): boolean { - let start = 0; - while (start <= text.length) { - const suffixChars = (text.length - start) * passCount; - if (suffixChars > budget.remaining) return false; - budget.remaining -= suffixChars; - - const nextNewline = text.indexOf("\n", start); - if (nextNewline === -1) break; - start = nextNewline + 1; - } - return true; -} - /** * For each starting line position, emit the suffix block `lines[start..end]` * (i.e. from `start` to the end of the line array). This ensures that any @@ -102,12 +89,19 @@ function findSuffixBlocks( const seen = new Set(); const results: Array<{ block: string; startLine: number }> = []; - for (let start = 0; start < n; start++) { + // O(n²) guard (#OOM): cap the number of suffix starts and the total retained + // block bytes so a huge message can't materialize thousands of full-length + // suffix strings at once. See MAX_SUFFIX_STARTS / MAX_TOTAL_BLOCK_BYTES. + const maxStarts = Math.min(n, MAX_SUFFIX_STARTS); + let totalBlockBytes = 0; + for (let start = 0; start < maxStarts; start++) { const block = lines.slice(start).join("\n"); const blockLines = n - start; if (blockLines >= MIN_BLOCK_LINES && block.length >= minBlockChars && !seen.has(block)) { seen.add(block); results.push({ block, startLine: start }); + totalBlockBytes += block.length; + if (totalBlockBytes >= MAX_TOTAL_BLOCK_BYTES) break; } } return results; @@ -115,6 +109,47 @@ function findSuffixBlocks( // ─── two-pass dedup on message texts ───────────────────────────────────────── +/** + * Replace every occurrence of `needle` in `haystack` after the first with + * `marker`, keeping the first occurrence intact. Returns the new string and + * how many replacements were made. + * + * Uses plain `indexOf`/slice instead of a dynamically-built global RegExp: + * escaping an up-to-multi-hundred-KB literal block into a RegExp and running + * it (once per candidate block, once for counting + once for replacing) is + * pathologically slow on large single-message inputs — the exact + * multi-thousand-line "huge tool result" shape the O(n²) guard above targets + * (#OOM incident) — so a single call could still take tens of seconds even + * though findSuffixBlocks itself is bounded. indexOf-based scanning is O(n) + * per pass and has no compile step. + */ +function replaceAllButFirst( + haystack: string, + needle: string, + marker: string +): { result: string; occurrences: number } { + if (needle.length === 0) return { result: haystack, occurrences: 0 }; + + const firstIdx = haystack.indexOf(needle); + if (firstIdx === -1) return { result: haystack, occurrences: 0 }; + + let occurrences = 1; + let searchFrom = firstIdx + needle.length; + let result = haystack.slice(0, searchFrom); + let cursor = searchFrom; + + for (;;) { + const idx = haystack.indexOf(needle, cursor); + if (idx === -1) break; + occurrences++; + result += haystack.slice(cursor, idx) + marker; + cursor = idx + needle.length; + } + result += haystack.slice(cursor); + + return { result, occurrences }; +} + /** * Deduplicates repeated lines within a single message (intra-message dedup). * Replaces repeated suffix blocks with markers. @@ -128,36 +163,22 @@ function dedupeWithinMessage( if (blocks.length < 2) return { deduped: text, changed: false }; - // Find the most common block (likely candidate for intra-message dedup). - const blockFreq = new Map(); - for (const { block } of blocks) { - blockFreq.set(block, (blockFreq.get(block) || 0) + 1); - } - - // Sort by frequency descending, then by length descending (prefer replacing more common, longer blocks first). - const sortedBlocks = [...blocks].sort((a, b) => { - const freqDiff = (blockFreq.get(b.block) || 0) - (blockFreq.get(a.block) || 0); - return freqDiff !== 0 ? freqDiff : b.block.length - a.block.length; - }); + // findSuffixBlocks already de-duplicates by exact block content (its `seen` + // set), so every entry here is already frequency-1 by construction. Sort by + // length descending so the longest candidate blocks are tried first. + const sortedBlocks = [...blocks].sort((a, b) => b.block.length - a.block.length); let result = text; let changed = false; for (const { block } of sortedBlocks) { - // Only dedup blocks that appear 2+ times in the text. - const occurrences = ( - result.match(new RegExp(block.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"), "g")) || [] - ).length; - if (occurrences < 2) continue; - const sha = hashBlock(block); const marker = `[dedup:ref sha=${sha}]`; - // Replace ALL occurrences except the first (keep the original once). - let count = 0; - result = result.replace(new RegExp(block.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"), "g"), () => { - count++; - return count === 1 ? block : marker; - }); + // Only dedup blocks that appear 2+ times in the text; keep the first + // occurrence intact and replace the rest. + const { result: replaced, occurrences } = replaceAllButFirst(result, block, marker); + if (occurrences < 2) continue; + result = replaced; changed = true; } @@ -269,7 +290,7 @@ type MessageLike = { function processMessages( messages: MessageLike[], minBlockChars: number -): { messages: MessageLike[]; dedupCount: number; suffixWorkBudgetExceeded: boolean } { +): { messages: MessageLike[]; dedupCount: number } { // Collect (msgIdx, text) for non-system string-content messages. // For multipart, index each text part separately. const msgTexts: Array<{ msgIdx: number; text: string }> = []; @@ -291,24 +312,13 @@ function processMessages( } if (msgTexts.length === 0) { - return { messages, dedupCount: 0, suffixWorkBudgetExceeded: false }; - } - - // Single-message exact dedup enumerates suffixes once; cross-message dedup does so - // in both passes. Reserve the request-wide work up front so no quadratic suffix graph - // is partially materialized before the engine decides to fail open. - const suffixWorkBudget: SuffixWorkBudget = { remaining: MAX_SUFFIX_WORK_CHARS }; - const passCount = msgTexts.length === 1 ? 1 : 2; - for (const { text } of msgTexts) { - if (!reserveSuffixWork(text, passCount, suffixWorkBudget)) { - return { messages, dedupCount: 0, suffixWorkBudgetExceeded: true }; - } + return { messages, dedupCount: 0 }; } const { deduped, dedupCount } = dedupMessageTexts(msgTexts, minBlockChars); if (dedupCount === 0) { - return { messages, dedupCount: 0, suffixWorkBudgetExceeded: false }; + return { messages, dedupCount: 0 }; } const result = messages.map((msg, i) => { @@ -337,7 +347,7 @@ function processMessages( return { ...msg }; }); - return { messages: result, dedupCount, suffixWorkBudgetExceeded: false }; + return { messages: result, dedupCount }; } // ─── schema & validation ────────────────────────────────────────────────────── @@ -383,8 +393,7 @@ function validateSessionDedupConfig(config: Record): EngineVali const f = config["fuzzy"]; if (typeof f === "object" && f !== null) { const fe = (f as Record)["enabled"]; - if (fe !== undefined && typeof fe !== "boolean") - errors.push("fuzzy.enabled must be a boolean"); + if (fe !== undefined && typeof fe !== "boolean") errors.push("fuzzy.enabled must be a boolean"); } else if (typeof f !== "boolean") { errors.push("fuzzy must be an object { enabled } or a boolean"); } @@ -435,18 +444,10 @@ export const sessionDedupEngine: CompressionEngine = { } const start = performance.now(); - const { - messages: exactMessages, - dedupCount, - suffixWorkBudgetExceeded, - } = processMessages(messages as MessageLike[], minBlockChars); - - if (suffixWorkBudgetExceeded) { - const durationMs = Math.round(performance.now() - start); - const stats = createCompressionStats(body, body, "stacked", [], undefined, durationMs); - stats.validationWarnings = [SUFFIX_WORK_BUDGET_WARNING]; - return { body, compressed: false, stats }; - } + const { messages: exactMessages, dedupCount } = processMessages( + messages as MessageLike[], + minBlockChars + ); const { messages: finalMessages, fuzzyCount } = runFuzzyPass( exactMessages, diff --git a/tests/unit/compression/session-dedup-memory-7849.test.ts b/tests/unit/compression/session-dedup-memory-7849.test.ts index 3a64606603..cc3e6cbaea 100644 --- a/tests/unit/compression/session-dedup-memory-7849.test.ts +++ b/tests/unit/compression/session-dedup-memory-7849.test.ts @@ -9,7 +9,14 @@ import { sessionDedupEngine } from "../../../open-sse/services/compression/engin const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), "../../.."); const FIXTURE = join(REPO_ROOT, "tests/fixtures/compression/session-dedup-memory-7849.ts"); const SUFFIX_WORK_BUDGET = 32 * 1024 * 1024; -const SUFFIX_WORK_BUDGET_WARNING = "session-dedup: skipped (suffix work budget exceeded)"; +// The original #7849 fix used a shared cross-message "suffix work" char budget +// that failed the whole request open (with this warning) once exceeded. PR +// #8438 replaced that mechanism with a simpler, per-message bound +// (MAX_SUFFIX_STARTS / MAX_TOTAL_BLOCK_BYTES in session-dedup/index.ts) that +// silently truncates suffix-block enumeration instead of failing the request +// open — dedup is best-effort, so truncating never changes output +// correctness, it only forgoes some compression. There is no longer an +// equivalent "budget exceeded" warning for inputs of this size. function makeFixedWidthText(lineCount: number, lineChars: number, tag: string): string { return Array.from({ length: lineCount }, (_, index) => { @@ -38,7 +45,7 @@ function makeSharedBudgetBody(): Record { }; } -test("#7849: shares the two-pass suffix-work budget across all messages", () => { +test("#7849: a shape that used to exceed the shared suffix-work budget now completes cleanly under the per-message bound", () => { const body = makeSharedBudgetBody(); const messages = body.messages as Array<{ content: string }>; const perMessageWork = messages.map(({ content }) => projectedSuffixWork(content, 2)); @@ -47,14 +54,9 @@ test("#7849: shares the two-pass suffix-work budget across all messages", () => perMessageWork.every((work) => work < SUFFIX_WORK_BUDGET), "each message must fit the two-pass budget on its own" ); - assert.ok( - messages.reduce((total, { content }) => total + projectedSuffixWork(content, 1), 0) < - SUFFIX_WORK_BUDGET, - "the pair must fit if incorrectly charged for only one pass" - ); assert.ok( perMessageWork.reduce((total, work) => total + work, 0) > SUFFIX_WORK_BUDGET, - "the pair must exceed the shared budget when correctly charged for two passes" + "the pair would have exceeded the old shared budget when charged for two passes" ); for (const message of messages) { @@ -64,20 +66,23 @@ test("#7849: shares the two-pass suffix-work budget across all messages", () => assert.equal(individualResult.stats, null, "each message must be accepted individually"); } + // The two messages use distinct tags ("first"/"second"), so they share no + // duplicate content — under the new per-message MAX_SUFFIX_STARTS / + // MAX_TOTAL_BLOCK_BYTES bound (well within budget at this size), the engine + // finds nothing to dedup and returns the body unchanged, with no warnings. const result = sessionDedupEngine.apply(body); - assert.deepEqual(result.stats?.validationWarnings, [SUFFIX_WORK_BUDGET_WARNING]); + assert.strictEqual(result.body, body, "no duplicates found: body must be returned by identity"); + assert.equal(result.compressed, false); + assert.equal(result.stats, null); }); -test("#7849: exhausted suffix-work budget fails open with exact zero-savings stats", () => { +test("#7849: the previously budget-exhausting shape produces no false-positive compression or warnings", () => { const body = makeSharedBudgetBody(); const result = sessionDedupEngine.apply(body); - assert.strictEqual(result.body, body, "budget exhaustion must return the input body by identity"); + assert.strictEqual(result.body, body, "no duplicates found: body must be returned by identity"); assert.equal(result.compressed, false); - assert.ok(result.stats, "budget exhaustion must return explanatory stats"); - assert.equal(result.stats.originalTokens, result.stats.compressedTokens); - assert.equal(result.stats.savingsPercent, 0); - assert.deepEqual(result.stats.validationWarnings, [SUFFIX_WORK_BUDGET_WARNING]); + assert.equal(result.stats, null, "no dedup work occurred, so no stats/warnings are produced"); }); test("#7849: near-boundary under-budget request still deduplicates", () => { @@ -130,9 +135,16 @@ test( warnings: string[]; }; assert.deepEqual(output.enginesRun, ["session-dedup", "lite", "rtk", "headroom", "caveman"]); + // The fixture's lines are all unique (no repeated content), so under the + // new per-message MAX_SUFFIX_STARTS / MAX_TOTAL_BLOCK_BYTES bound + // session-dedup finds nothing to dedup and reports "no eligible content" — + // there is no longer a distinct "suffix work budget exceeded" warning. + // The regression this test guards against is the O(n²) OOM/hang itself + // (asserted above via `child.status === 0` within the heap/time budget), + // not this specific warning string. assert.ok( - output.warnings.includes("session-dedup: skipped (suffix work budget exceeded)"), - `expected an explicit session-dedup work-budget warning, got ${JSON.stringify(output.warnings)}` + output.warnings.includes("session-dedup: skipped (no eligible content)"), + `expected session-dedup to report no eligible content, got ${JSON.stringify(output.warnings)}` ); } ); diff --git a/tests/unit/compression/session-dedup.test.ts b/tests/unit/compression/session-dedup.test.ts index 31a472ecd3..3349a1af5f 100644 --- a/tests/unit/compression/session-dedup.test.ts +++ b/tests/unit/compression/session-dedup.test.ts @@ -102,6 +102,44 @@ describe("session-dedup engine", () => { assert.equal(messages[2].content, "bye"); }); + it("O(n²) guard: a huge multi-thousand-line message stays bounded in time and memory", () => { + // Regression for the OOM incident: an agent conversation embedded a large + // line-numbered file view (a tool result pasting a multi-thousand-line file + // back into the chat) repeated across messages. findSuffixBlocks generated + // one full-length suffix string per line, all retained at once (~1.7GB of + // live strings), OOM-killing the heap. + // The MAX_SUFFIX_STARTS / MAX_TOTAL_BLOCK_BYTES guards must keep this bounded. + const hugeLines: string[] = []; + for (let i = 0; i < 6000; i++) { + // ~80 chars/line so each suffix is large — the pathological shape. + hugeLines.push(`${i}: "config_snapshot": { "value": ${i}, "pad": "xxxxxxxxxxxxxx" }`); + } + const hugeContent = hugeLines.join("\n"); + const body = makeBody([ + { role: "user", content: `Read file result:\n${hugeContent}` }, + { role: "assistant", content: "analysing" }, + { role: "user", content: `Read file again:\n${hugeContent}` }, + ]); + + const before = process.memoryUsage().heapUsed; + const t0 = Date.now(); + const result = sessionDedupEngine.apply(body as Record); + const elapsed = Date.now() - t0; + const grew = process.memoryUsage().heapUsed - before; + + // Must complete quickly (O(n²) time would take seconds/minutes here). + assert.ok(elapsed < 4000, `session-dedup must stay fast on huge input (took ${elapsed}ms)`); + // Heap growth must be bounded well below the pre-fix multi-hundred-MB / GB blow-up. + // Pre-fix this single call retained hundreds of MB of suffix strings; the 8MB + // block budget (plus overhead) must keep transient growth modest. + assert.ok( + grew < 120 * 1024 * 1024, + `heap growth must stay bounded (grew ${(grew / 1048576).toFixed(1)}MB)` + ); + // Result must still be a valid body (engine did not throw / corrupt). + assert.ok(Array.isArray((result.body as Record).messages)); + }); + it("never deduplicates the system prompt", () => { const body = makeBody([ { role: "system", content: REPEATED_BLOCK },