mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-04 06:12:10 +03:00
fix(sse): treat compression no-op as zero-savings, not inflation/silent-drop (#6883)
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 <chirag127@users.noreply.github.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <diegosouza.pw@gmail.com>
Co-authored-by: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com>
This commit is contained in:
@@ -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 };
|
||||
|
||||
@@ -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));
|
||||
|
||||
@@ -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)`);
|
||||
|
||||
117
tests/unit/compression-noop-guard.test.ts
Normal file
117
tests/unit/compression-noop-guard.test.ts
Normal file
@@ -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");
|
||||
});
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user