From d1b26bcb626929a35885cde676bc8d25c048d314 Mon Sep 17 00:00:00 2001 From: IAMBOBJIM <220105265+IAMBOBJIM@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:58:14 -0400 Subject: [PATCH] fix(sse): aggregate findInsensitive collision warning into one line per build (#12972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit modelMetadataRegistry's findInsensitive() warned once per colliding key while building its lowercase index. On a real catalog that is hundreds of lines per rebuild: a production log carried 27,296 of these in a single file — 40% of all lines, in ~500/sec bursts — driving 52 MB log rotations and ~466 MB of logs on disk. The warning itself is worth keeping: a case-insensitive collision is a genuine upstream data-quality signal (models.dev returning both "OpenAI" and "openai" as distinct provider keys), and first-match-wins silently discards the later value. Only the volume was wrong. Collisions are now collected during the index build and reported as a single line carrying the total count plus the first 5 keys, so the diagnostic survives at 1/N the volume. No behavior change: the index, the first-match-wins resolution, and the WeakMap identity cache are untouched. Validated by TDD (Hard Rule #18): tests/unit/model-metadata-registry-collision-log.test.ts fails on the old implementation (3 collisions -> 3 warnings, 50 -> 50) and passes after (always 1). Also covers the no-collision case emitting nothing, and asserts the aggregated line still names colliding keys. Note for reviewers: the test fixture deliberately spells the provider key "OpenAI" rather than "openai". findInsensitive short-circuits on `if (key in obj) return obj[key]` before the index is ever built, so a fixture containing the literal lookup key produces zero warnings and proves nothing. Gates: eslint clean on both changed files. typecheck:core reports 9 pre-existing errors in open-sse/services/compression/omniglyph* — unrelated to this change (those files are byte-identical to origin/release/v3.8.50) and caused by a local stale node_modules carrying omniglyph 1.3.1 against the required ^1.4.0. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --- src/lib/modelMetadataRegistry.ts | 31 +++-- ...el-metadata-registry-collision-log.test.ts | 113 ++++++++++++++++++ 2 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 tests/unit/model-metadata-registry-collision-log.test.ts diff --git a/src/lib/modelMetadataRegistry.ts b/src/lib/modelMetadataRegistry.ts index 64887ae10c..bc0dc39ee6 100644 --- a/src/lib/modelMetadataRegistry.ts +++ b/src/lib/modelMetadataRegistry.ts @@ -303,6 +303,9 @@ export function getCanonicalModelMetadata(input: { // a rebuild instead of rebuilt per lookup. const lowercaseIndexCache = new WeakMap>(); +/** Colliding keys named in the aggregated collision warning before it truncates. */ +const COLLISION_SAMPLE_SIZE = 5; + /** Test hook (#13601): exercised directly by the collision-naming unit test. */ export function findInsensitive( obj: Record | null | undefined, @@ -313,24 +316,36 @@ export function findInsensitive( let index = lowercaseIndexCache.get(obj); if (!index) { index = new Map(); + const collisions: string[] = []; const firstKeyByLower = new Map(); for (const [k, v] of Object.entries(obj)) { const lowerKey = k.toLowerCase(); - // Warn once at index-build time (not per-lookup) if two keys collide - // case-insensitively — a real data-quality signal from an upstream sync (e.g. - // models.dev returning both "OpenAI" and "openai" as distinct provider keys). - // Names both keys so the operator can tell which entries clash; resolution - // stays deterministic first-seen-wins (#13601). + // Collisions are a real data-quality signal from an upstream sync (e.g. + // models.dev returning both "OpenAI" and "openai" as distinct provider + // keys), so they are surfaced rather than swallowed — first-seen-wins, + // matching the pre-#8697 scan's behavior. Each collision is recorded with + // BOTH spellings (#13601) so the operator can tell which entries clash; + // they are reported together once the index finishes building. const firstKey = firstKeyByLower.get(lowerKey); if (firstKey !== undefined) { - console.warn( - `[modelMetadataRegistry] findInsensitive: case-insensitive key collision on "${lowerKey}" ("${firstKey}" vs "${k}") — keeping first-seen value, later one discarded` - ); + collisions.push(`"${lowerKey}" ("${firstKey}" vs "${k}")`); continue; } firstKeyByLower.set(lowerKey, k); index.set(lowerKey, v); } + // Aggregate into ONE line per index build. Warning per colliding key made + // the signal unreadable and expensive: a real catalog collides on hundreds + // of keys, and a production log carried 27,296 of these lines (40% of the + // file, ~500/sec bursts) driving 52 MB rotations. The count plus a bounded + // sample keeps the diagnostic without the flood. + if (collisions.length > 0) { + const sample = collisions.slice(0, COLLISION_SAMPLE_SIZE).join(", "); + const more = collisions.length > COLLISION_SAMPLE_SIZE ? ", …" : ""; + console.warn( + `[modelMetadataRegistry] findInsensitive: ${collisions.length} case-insensitive key collision(s) — keeping first-seen value, later ones discarded. Keys: ${sample}${more}` + ); + } lowercaseIndexCache.set(obj, index); } return index.get(key.toLowerCase()) as T | undefined; diff --git a/tests/unit/model-metadata-registry-collision-log.test.ts b/tests/unit/model-metadata-registry-collision-log.test.ts new file mode 100644 index 0000000000..58e9f9b9b5 --- /dev/null +++ b/tests/unit/model-metadata-registry-collision-log.test.ts @@ -0,0 +1,113 @@ +/** + * `findInsensitive()` in modelMetadataRegistry builds a lowercase-key index and + * warns when two keys collide case-insensitively (e.g. models.dev returning both + * "OpenAI" and "openai"). The warning is a genuine upstream data-quality signal + * and must be kept. + * + * The problem is volume, not the signal: it logged once PER COLLIDING KEY per + * index build. On a real catalog that is hundreds of lines per rebuild — a + * production log captured 27,296 of these in a single file, 40% of all lines, + * in bursts of ~500/sec, driving 52 MB log rotations. + * + * This test pins the aggregate shape: one warning per index build, carrying the + * collision count, no matter how many keys collide. It fails against the + * per-key implementation (3 warnings for 3 collisions). + */ + +import assert from "node:assert/strict"; +import test from "node:test"; + +import { enrichCatalogModelEntry } from "@/lib/modelMetadataRegistry"; + +type Warn = (...args: unknown[]) => void; + +/** Runs `fn` with console.warn captured; returns the warning lines. */ +function captureWarnings(fn: () => void): string[] { + const lines: string[] = []; + const original: Warn = console.warn; + console.warn = (...args: unknown[]) => { + lines.push(args.map(String).join(" ")); + }; + try { + fn(); + } finally { + console.warn = original; + } + return lines.filter((l) => l.includes("findInsensitive")); +} + +/** + * Fresh object per call: the lowercase index is cached in a WeakMap keyed by + * object identity, so reusing one would skip the rebuild (and the warning). + * + * The provider key is deliberately spelled "OpenAI", not "openai". findInsensitive + * short-circuits on `if (key in obj) return obj[key]` — an exact hit returns before + * the index is ever built, so a fixture containing the literal lookup key produces + * zero warnings and proves nothing. + */ +function pricingWithCollisions(count: number): Record { + const pricing: Record = { + OpenAI: { "gpt-4o": { input: 1, output: 2 } }, + }; + for (let i = 0; i < count; i++) { + // Same key differing only in case -> collides with a previously inserted one. + pricing[`Dup${i}Provider`] = { m: { input: 1, output: 2 } }; + pricing[`dup${i}provider`] = { m: { input: 9, output: 9 } }; + } + return pricing; +} + +test("findInsensitive emits ONE aggregated warning per index build, not one per collision", () => { + const warnings = captureWarnings(() => { + enrichCatalogModelEntry( + { id: "gpt-4o", owned_by: "openai" }, + { provider: "openai", model: "gpt-4o" }, + { modelsDevPricing: pricingWithCollisions(3) } as never + ); + }); + + assert.equal( + warnings.length, + 1, + `expected a single aggregated warning, got ${warnings.length}:\n${warnings.join("\n")}` + ); + assert.match(warnings[0], /3 case-insensitive key collision/); +}); + +test("the aggregated warning still names colliding keys so the signal survives", () => { + const warnings = captureWarnings(() => { + enrichCatalogModelEntry( + { id: "gpt-4o", owned_by: "openai" }, + { provider: "openai", model: "gpt-4o" }, + { modelsDevPricing: pricingWithCollisions(2) } as never + ); + }); + + assert.equal(warnings.length, 1); + assert.match(warnings[0], /dup0provider/); +}); + +test("warning count stays at one as collisions scale", () => { + const warnings = captureWarnings(() => { + enrichCatalogModelEntry( + { id: "gpt-4o", owned_by: "openai" }, + { provider: "openai", model: "gpt-4o" }, + { modelsDevPricing: pricingWithCollisions(50) } as never + ); + }); + + assert.equal(warnings.length, 1, "50 collisions must still produce exactly one line"); + assert.match(warnings[0], /50 case-insensitive key collision/); +}); + +test("no collisions means no warning at all", () => { + const warnings = captureWarnings(() => { + enrichCatalogModelEntry( + { id: "gpt-4o", owned_by: "openai" }, + { provider: "openai", model: "gpt-4o" }, + { modelsDevPricing: { OpenAI: { "gpt-4o": { input: 1, output: 2 } } } } as never + ); + }); + + assert.deepEqual(warnings, []); +});