From 5cebefe64a761f2740636f23122d04b7cd7f1cef Mon Sep 17 00:00:00 2001 From: Chirag Singhal <76880977+chirag127@users.noreply.github.com> Date: Sun, 12 Jul 2026 10:34:25 +0530 Subject: [PATCH] fix(sse): treat compression no-op as zero-savings, not inflation/silent-drop (#6883) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A structural engine (ccr / session-dedup) that finds nothing to compress returns the body unchanged. That no-op was mishandled three ways — the code-level root cause of the #6465–#6493 "0% savings, no reason" symptom class: A. Inflation guard mislabelled a no-op as inflation. guardPipelineInflation used `compressedTokens >= originalTokens`, so an unchanged body (compressedTokens === originalTokens) tripped the guard, setting fallbackApplied=true and emitting a misleading "did not shrink; reverted to original" warning. Changed to strict `>` — only a strictly larger output is inflation; equality is a no-op. Genuine inflation still reverts. B. Disabled-engine skip was silent and asymmetric with the breaker skip. Both stacked loops (sync + async) skipped a registry-disabled engine with a bare `continue`, recording no validationWarning — while the sibling breaker-open branch does. Both loops now add `${engine}: skipped (engine disabled in registry)`, mirroring the breaker branch. C. No-op engine lost its identity in engineBreakdown. mergeStackStep early-returned on null stats, pushing no breakdown entry, so ensureEngineBreakdown synthesized a generic "stacked" 0% node. It now records a zero-savings entry keyed on the engine that actually ran, preserving identity. Tests: tests/unit/compression-noop-guard.test.ts covers A (equal-token no-op not inflated; strictly-larger still reverts), B (disabled skip surfaces a "disabled" warning), and C (no-op engine keeps its own id in the breakdown). Updated the existing inflation-guard test whose net-zero case encoded the old buggy behaviour, and switched its wire test to object-form pipeline steps so the intended engine runs. Co-authored-by: Chirag Singhal Co-authored-by: Diego Rodrigues de Sa e Souza Co-authored-by: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> --- .../services/compression/pipelineGuards.ts | 14 ++- .../services/compression/stackedStepCore.ts | 13 ++ .../services/compression/strategySelector.ts | 10 +- tests/unit/compression-noop-guard.test.ts | 117 ++++++++++++++++++ ...mpression-pipeline-inflation-guard.test.ts | 12 +- 5 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 tests/unit/compression-noop-guard.test.ts diff --git a/open-sse/services/compression/pipelineGuards.ts b/open-sse/services/compression/pipelineGuards.ts index 2977ff4aec..a94ecf84ec 100644 --- a/open-sse/services/compression/pipelineGuards.ts +++ b/open-sse/services/compression/pipelineGuards.ts @@ -27,9 +27,15 @@ export interface PipelineInflationResult { } /** - * Honest aggregate inflation guard. If the fully-stacked body did not actually shrink — its token - * count is `>=` the original — the compressed body is discarded and the verbatim original is - * returned. + * Honest aggregate inflation guard. Only genuine INFLATION — the fully-stacked body is strictly + * LARGER than the original (`compressedTokens > originalTokens`) — discards the compressed body and + * returns the verbatim original. + * + * A net-zero result (`compressedTokens === originalTokens`) is a NO-OP, not inflation: a structural + * engine (e.g. `ccr`, `session-dedup`) that found no candidate returns the body unchanged, so its + * token count equals the original. That is zero savings, not a revert — flagging it as inflation + * would emit a misleading "did not shrink; reverted to original" warning for an engine that never + * touched the payload. Equality therefore must NOT trip the guard. * * Safe by construction: the only alternative it ever returns is `originalBody`, the unmodified * request, which is always a valid payload. A (rare) false trigger therefore can never corrupt a @@ -40,7 +46,7 @@ export interface PipelineInflationResult { */ export function guardPipelineInflation(input: PipelineInflationInput): PipelineInflationResult { const { originalTokens, compressedTokens } = input; - if (originalTokens > 0 && compressedTokens >= originalTokens) { + if (originalTokens > 0 && compressedTokens > originalTokens) { return { body: input.originalBody, inflated: true }; } return { body: input.compressedBody, inflated: false }; diff --git a/open-sse/services/compression/stackedStepCore.ts b/open-sse/services/compression/stackedStepCore.ts index 5c9cdb0db9..ad15d5100d 100644 --- a/open-sse/services/compression/stackedStepCore.ts +++ b/open-sse/services/compression/stackedStepCore.ts @@ -80,7 +80,20 @@ export function mergeStackStep( result: CompressionResult ): void { if (!result.stats) { + // No-op engine (e.g. ccr / session-dedup found no candidate): stats is null so there is no + // telemetry to fold, but the engine still RAN — record a zero-savings breakdown entry so its + // identity survives. Without this the breakdown stays empty and ensureEngineBreakdown + // synthesizes a generic "stacked" 0% node, hiding which engine an operator actually asked for. + // Also surface a validation warning so operators can tell "engine ran but had nothing to do" + // apart from "engine never ran" (#6479, #6491). recordNullStatsStep(acc, engineId); + acc.breakdown.push({ + engine: engineId, + originalTokens: 0, + compressedTokens: 0, + savingsPercent: 0, + techniquesUsed: [], + }); return; } result.stats.techniquesUsed.forEach((technique) => acc.techniques.add(technique)); diff --git a/open-sse/services/compression/strategySelector.ts b/open-sse/services/compression/strategySelector.ts index 1beb167e7f..e95581b814 100644 --- a/open-sse/services/compression/strategySelector.ts +++ b/open-sse/services/compression/strategySelector.ts @@ -855,7 +855,10 @@ function runStackedCompression( } // Respect the registry enabled flag: a step naming a disabled engine is skipped, so an // operator can turn an engine off (setEngineEnabled) without editing every pipeline. - if (getEngineEntry(step.engine)?.enabled === false) continue; + if (getEngineEntry(step.engine)?.enabled === false) { + acc.validationWarnings.add(`${step.engine}: skipped (engine disabled in registry)`); + continue; + } // T02: when the per-engine breaker is OPEN, skip this step (verbatim body kept — fail-open). if (breakerOn && !canRunEngine(step.engine, breaker)) { acc.validationWarnings.add(`${step.engine}: skipped (pipeline circuit-breaker open)`); @@ -957,7 +960,10 @@ async function runStackedCompressionAsync( continue; } // Respect the registry enabled flag (same as the sync loop) — keep both in lockstep. - if (getEngineEntry(step.engine)?.enabled === false) continue; + if (getEngineEntry(step.engine)?.enabled === false) { + acc.validationWarnings.add(`${step.engine}: skipped (engine disabled in registry)`); + continue; + } // T02: skip an engine whose breaker is OPEN (verbatim body kept — fail-open). Lockstep w/ sync. if (breakerOn && !canRunEngine(step.engine, breaker)) { acc.validationWarnings.add(`${step.engine}: skipped (pipeline circuit-breaker open)`); diff --git a/tests/unit/compression-noop-guard.test.ts b/tests/unit/compression-noop-guard.test.ts new file mode 100644 index 0000000000..b6dd237a86 --- /dev/null +++ b/tests/unit/compression-noop-guard.test.ts @@ -0,0 +1,117 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { guardPipelineInflation } from "../../open-sse/services/compression/pipelineGuards.ts"; +import { applyStackedCompression } from "../../open-sse/services/compression/strategySelector.ts"; +import { + registerCompressionEngine, + setEngineEnabled, +} from "../../open-sse/services/compression/engines/registry.ts"; +import type { CompressionEngine } from "../../open-sse/services/compression/engines/types.ts"; +import type { CompressionPipelineStep } from "../../open-sse/services/compression/types.ts"; + +// Regression suite for the three compression NO-OP defects: a structural engine (ccr / +// session-dedup) that finds nothing to compress must be treated as ZERO SAVINGS, never as an +// inflation revert (A), never a silent skip (B), and never lose its identity in the breakdown (C). + +// A test-only engine that mimics a structural no-op: body unchanged, compressed:false, stats:null. +function makeNoopEngine(id: string): CompressionEngine { + return { + id, + name: "Test Noop", + description: "test-only engine that finds nothing to compress", + icon: "bug_report", + targets: ["messages"], + stackable: true, + stackPriority: 0, + metadata: { + id, + name: "Test Noop", + description: "test-only", + inputScope: "messages", + targetLatencyMs: 0, + supportsPreview: false, + stable: true, + }, + apply(body) { + return { body, compressed: false, stats: null }; + }, + compress(body) { + return this.apply(body); + }, + getConfigSchema() { + return []; + }, + validateConfig() { + return { valid: true, errors: [] }; + }, + }; +} + +// --- Defect A: a net-zero (equal-token) no-op is NOT inflation --- + +test("guardPipelineInflation: equal tokens (no-op) is NOT flagged as inflation", () => { + const original = { a: 1 }; + const compressed = { a: 1 }; + const r = guardPipelineInflation({ + originalBody: original, + compressedBody: compressed, + originalTokens: 100, + compressedTokens: 100, + }); + assert.equal(r.inflated, false); + assert.equal(r.body, compressed); +}); + +test("guardPipelineInflation: strictly larger output still reverts as inflation", () => { + const original = { a: 1 }; + const compressed = { a: 1, pad: "xxxx" }; + const r = guardPipelineInflation({ + originalBody: original, + compressedBody: compressed, + originalTokens: 100, + compressedTokens: 101, + }); + assert.equal(r.inflated, true); + assert.equal(r.body, original); +}); + +// --- Defect B: a disabled engine skip records a validationWarning (symmetric with breaker skip) --- + +const DISABLED_ID = "test-noop-disabled-guard"; + +test("applyStackedCompression: a disabled engine skip surfaces a 'disabled' validationWarning", () => { + registerCompressionEngine(makeNoopEngine(DISABLED_ID)); + setEngineEnabled(DISABLED_ID, false); + + const body = { messages: [{ role: "user", content: "hello world" }] }; + const result = applyStackedCompression(body, [{ engine: DISABLED_ID } as CompressionPipelineStep]); + + const warnings = result.stats?.validationWarnings ?? []; + assert.ok( + warnings.some((w) => w.includes("disabled")), + `expected a 'disabled' validationWarning, got: ${JSON.stringify(warnings)}` + ); + assert.ok( + warnings.some((w) => w.includes(DISABLED_ID)), + "the warning should name the disabled engine" + ); +}); + +// --- Defect C: a no-op engine keeps its identity in engineBreakdown (not a generic "stacked") --- + +const NOOP_ID = "test-noop-breakdown-guard"; + +test("applyStackedCompression: a no-op engine records its own identity in engineBreakdown", () => { + registerCompressionEngine(makeNoopEngine(NOOP_ID)); + setEngineEnabled(NOOP_ID, true); + + const body = { messages: [{ role: "user", content: "hello world this is a message" }] }; + const result = applyStackedCompression(body, [{ engine: NOOP_ID } as CompressionPipelineStep]); + + const breakdown = result.stats?.engineBreakdown ?? []; + const entry = breakdown.find((e) => e.engine === NOOP_ID); + assert.ok(entry, `expected a breakdown entry keyed on "${NOOP_ID}", got: ${JSON.stringify(breakdown)}`); + assert.equal(entry?.savingsPercent, 0); + // The requested engine's identity must be preserved — never collapsed into a generic "stacked". + assert.notEqual(entry?.engine, "stacked"); +}); diff --git a/tests/unit/compression-pipeline-inflation-guard.test.ts b/tests/unit/compression-pipeline-inflation-guard.test.ts index 359f5fef3c..2bc93b0682 100644 --- a/tests/unit/compression-pipeline-inflation-guard.test.ts +++ b/tests/unit/compression-pipeline-inflation-guard.test.ts @@ -7,6 +7,7 @@ import { setEngineEnabled, } from "../../open-sse/services/compression/engines/registry.ts"; import type { CompressionEngine } from "../../open-sse/services/compression/engines/types.ts"; +import type { CompressionPipelineStep } from "../../open-sse/services/compression/types.ts"; // --- Pure guard logic --- @@ -23,7 +24,10 @@ test("guardPipelineInflation reverts to the original when the stacked output did assert.equal(r.body, original); }); -test("guardPipelineInflation reverts on a net-zero (equal-token) result", () => { +test("guardPipelineInflation treats a net-zero (equal-token) no-op as NOT inflated", () => { + // A structural engine (ccr / session-dedup) that finds nothing returns the body unchanged, so + // compressedTokens === originalTokens. That is a no-op (zero savings), not inflation — the guard + // must keep the compressed body and never emit the "did not shrink; reverted" warning. const original = { a: 1 }; const compressed = { b: 2 }; const r = guardPipelineInflation({ @@ -32,8 +36,8 @@ test("guardPipelineInflation reverts on a net-zero (equal-token) result", () => originalTokens: 50, compressedTokens: 50, }); - assert.equal(r.inflated, true); - assert.equal(r.body, original); + assert.equal(r.inflated, false); + assert.equal(r.body, compressed); }); test("guardPipelineInflation keeps the compressed body when it actually shrank", () => { @@ -117,7 +121,7 @@ test("applyStackedCompression reverts to the original body when the pipeline inf // set of built-in bare-string aliases ("standard"/"rtk"/"lite"/"aggressive"/"ultra") and // silently falls back to `{ engine: "caveman" }` for any other string — a bare custom-engine // id here would silently run caveman instead of the registered `inflatingEngine`. - const result = applyStackedCompression(body, [{ engine: INFLATE_ID }]); + const result = applyStackedCompression(body, [{ engine: INFLATE_ID } as CompressionPipelineStep]); // The inflating engine produced a bigger body, so the aggregate guard discarded it. assert.equal(result.compressed, false);