From d2597277645709dce18cbf7eff0542f9ba2719e1 Mon Sep 17 00:00:00 2001 From: Saurabh Nandwana Date: Mon, 17 Aug 2026 13:16:51 +0530 Subject: [PATCH 1/2] fix(core): never filter a definitive leak out of the verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dogfooding this tool on a decoder-lifecycle fix, a run that ended with 47 live VideoDecoders and ten collected without close() printed "No leaked WebCodecs objects." The shim saw every one of them and warned on the console. checkLeaks() threw the evidence away: the default `types` is the frame-like types, and the accumulation loop visited only those, so both the live decoders and the GC'd-unclosed ones fell outside the report. Three changes: - collectedUnclosed is folded across every tracked type, whatever `types` says. The README calls it the most definitive leak there is; a filter aimed at live frames must not hide a decoder dropped on the floor. This can turn a previously green suite red, correctly. - The all-clear is qualified when an unenforced type holds live objects, so a report can no longer say "no leaks" beside VideoDecoder=47. - `types: 'all'` enforces all seven types without naming them. webcodecs_leak_sites now attributes every type unless one is named. It answers "which line is leaking", which is attribution, not a verdict — returning nothing for a codec leak was the same blind spot. test/assert.test.mjs pins all of it against synthetic censuses, including the GC'd-unclosed case no browser test can produce on demand. Each fix was reverted in turn and watched to fail. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 27 ++++++++ README.md | 7 +++ packages/core/README.md | 19 ++++++ packages/core/src/assert.ts | 80 +++++++++++++++++++----- packages/mcp/src/index.ts | 6 +- test/assert.test.mjs | 120 ++++++++++++++++++++++++++++++++++++ 6 files changed, 242 insertions(+), 17 deletions(-) create mode 100644 test/assert.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 666d4b6..7f93c76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,33 @@ Versioning follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- `checkLeaks()` filtered `collectedUnclosed` by `types`, so the leak this tool + calls the most definitive one there is could be dropped from the verdict. + Found while dogfooding: a run ending with 47 live `VideoDecoder`s and ten + collected without `close()` printed "No leaked WebCodecs objects." The shim + saw all of it and warned on the console; the assertion API filtered it out. + A GC'd-unclosed object of any tracked type now fails the check regardless of + `types`, which can turn a previously passing suite red — correctly. +- `checkLeaks()` and `summarize()` no longer print an unqualified all-clear + while an unenforced type holds live objects. The message now names them: + `No leaks in VideoFrame, AudioData, ImageBitmap — but VideoDecoder=47 still + live and not enforced.` +- `webcodecs_leak_sites` attributed only the default frame types, so an agent + asking which line is leaking got nothing back for a codec leak. Attribution + is not a verdict — it now covers every type unless one is named. + +### Added + +- `types: 'all'` on `checkLeaks()` / `expectNoLeaks()`, so enforcing the codecs + does not mean spelling out all seven type names. +- `LeakReport.unenforcedLive` and `LeakReport.enforced`: what was live but out + of scope, and what the filter resolved to. +- `test/assert.test.mjs`, which pins the verdict layer against synthetic + censuses — including a `VideoDecoder` collected without `close()`, which no + browser test can produce on demand. + ## [0.2.1] - 2026-08-14 ### Added diff --git a/README.md b/README.md index 4bd2a21..bf2157a 100644 --- a/README.md +++ b/README.md @@ -166,6 +166,13 @@ test('the editor releases every frame it decodes', async () => { `checkLeaks()` returns the same information without throwing. `minAgeMs` ignores objects that may still legitimately be in flight. +`types` decides what counts as live too long, and defaults to the frame-like types — a long-lived decoder is normal, a long-lived frame almost never is. Pass `types: 'all'` to hold the codecs to the same standard. Whatever you pass, an object the GC collected while it was still open fails the check, and a type left out of `types` is named in the message rather than quietly reported clean: + +``` +No leaks in VideoFrame, AudioData, ImageBitmap — but VideoDecoder=47 still +live and not enforced. Pass types: 'all' to check those too. +``` + ## The timeline, and why a snapshot lies A static count answers "how many are live". It cannot answer "was the pipeline busy when playback stalled" — and that difference matters. In the app this was built against, live decoder count did **not** predict failure: the highest count succeeded and lower counts stalled. A snapshot would have sent you after a resource-exhaustion bug that wasn't there. diff --git a/packages/core/README.md b/packages/core/README.md index 19959f3..fe7afb1 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -68,6 +68,25 @@ test('the editor releases every frame it decodes', async () => { `checkLeaks()` returns the same information without throwing. `minAgeMs` ignores objects that may still legitimately be in flight. +`types` decides what counts as live too long. It defaults to the frame-like +types, because a long-lived decoder is normal and a long-lived frame almost +never is. Pass `types: 'all'` to hold the codecs to the same standard: + +```js +expectNoLeaks([localCensus()], { types: 'all' }); +``` + +Two things `types` deliberately does not do. It never hides an object the GC +collected while it was still open — that is the definitive leak, and it fails +the check whatever its type. And it never lets the report claim a clean bill of +health for a type it did not look at: an unenforced type with live objects is +named in the message. + +``` +No leaks in VideoFrame, AudioData, ImageBitmap — but VideoDecoder=47 still +live and not enforced. Pass types: 'all' to check those too. +``` + ## What it counts, and why that is not obvious Tracked: `VideoDecoder`, `VideoEncoder`, `AudioDecoder`, `AudioEncoder`, diff --git a/packages/core/src/assert.ts b/packages/core/src/assert.ts index 7ab4265..2c93993 100644 --- a/packages/core/src/assert.ts +++ b/packages/core/src/assert.ts @@ -3,14 +3,22 @@ * rather than something a human has to notice in a panel. */ +import { TRACKED } from './types'; import type { ContextCensus, LeakSite, TrackedType } from './types'; export interface LeakReport { ok: boolean; - /** Live objects, summed across contexts, by type. */ + /** Live objects of the enforced types, summed across contexts. */ live: Partial>; - /** GC'd without close(), summed across contexts. Always a genuine leak. */ + /** Live objects of the types `types` left out. Reported, never failed on. */ + unenforcedLive: Partial>; + /** + * GC'd without close(), summed across contexts and across every tracked + * type. `types` cannot filter this one away — see `checkLeaks`. + */ collectedUnclosed: Partial>; + /** The types `types` resolved to. */ + enforced: TrackedType[]; /** Allocation sites holding live objects, worst first. */ sites: (LeakSite & { context: string })[]; message: string; @@ -18,10 +26,11 @@ export interface LeakReport { export interface LeakOptions { /** - * Types to enforce. Defaults to the frame-like types, because a long-lived - * decoder is normal and a long-lived frame almost never is. + * Types to enforce, or `'all'` for every tracked type. Defaults to the + * frame-like types, because a long-lived decoder is normal and a long-lived + * frame almost never is. */ - types?: TrackedType[]; + types?: TrackedType[] | 'all'; /** Tolerated live count per type. A steady-state pipeline holds a few. */ allow?: Partial>; /** Ignore live objects younger than this — they may be legitimately in flight. */ @@ -34,48 +43,86 @@ export function totalLive(censuses: ContextCensus[], type: TrackedType): number return censuses.reduce((sum, c) => sum + (c.live[type] ?? 0), 0); } -/** Build a report without throwing. `checkLeaks(...).ok` is the boolean form. */ +const counts = (m: Partial>) => + Object.entries(m) + .filter(([, n]) => n) + .map(([t, n]) => `${t}=${n}`) + .join(' '); + +/** + * Build a report without throwing. `checkLeaks(...).ok` is the boolean form. + * + * `types` narrows what counts as *live too long*. It deliberately does not + * narrow objects the GC collected while they were still open: that is the + * definitive leak, and a filter aimed at live frames must not hide a decoder + * that was dropped on the floor. + */ export function checkLeaks(censuses: ContextCensus[], options: LeakOptions = {}): LeakReport { - const types = options.types ?? DEFAULT_TYPES; + const enforced = options.types === 'all' ? [...TRACKED] : options.types ?? DEFAULT_TYPES; const allow = options.allow ?? {}; const minAgeMs = options.minAgeMs ?? 0; const live: Partial> = {}; + const unenforcedLive: Partial> = {}; const collectedUnclosed: Partial> = {}; const sites: (LeakSite & { context: string })[] = []; for (const c of censuses) { - for (const t of types) { - if (c.live[t]) live[t] = (live[t] ?? 0) + c.live[t]!; + for (const t of TRACKED) { + const n = c.live[t] ?? 0; + if (n) { + const bucket = enforced.includes(t) ? live : unenforcedLive; + bucket[t] = (bucket[t] ?? 0) + n; + } if (c.collectedUnclosed[t]) { collectedUnclosed[t] = (collectedUnclosed[t] ?? 0) + c.collectedUnclosed[t]!; } } for (const s of c.leakSites) { - if (types.includes(s.type) && s.oldestAgeMs >= minAgeMs) { + if (enforced.includes(s.type) && s.oldestAgeMs >= minAgeMs) { sites.push({ ...s, context: c.context }); } } } sites.sort((a, b) => b.count - a.count); - const over = types.filter((t) => (live[t] ?? 0) > (allow[t] ?? 0)); - const collected = types.filter((t) => (collectedUnclosed[t] ?? 0) > 0); + const over = enforced.filter((t) => (live[t] ?? 0) > (allow[t] ?? 0)); + const collected = TRACKED.filter((t) => (collectedUnclosed[t] ?? 0) > 0); const ok = over.length === 0 && collected.length === 0; - return { ok, live, collectedUnclosed, sites, message: describe(ok, over, collected, live, collectedUnclosed, sites, allow) }; + return { + ok, + live, + unenforcedLive, + collectedUnclosed, + enforced, + sites, + message: describe(ok, over, collected, enforced, live, unenforcedLive, collectedUnclosed, sites, allow), + }; } function describe( ok: boolean, over: TrackedType[], collected: TrackedType[], + enforced: TrackedType[], live: Partial>, + unenforcedLive: Partial>, collectedUnclosed: Partial>, sites: (LeakSite & { context: string })[], allow: Partial>, ): string { - if (ok) return 'No leaked WebCodecs objects.'; + const unenforced = counts(unenforcedLive); + + if (ok) { + if (!unenforced) return 'No leaked WebCodecs objects.'; + // An unqualified all-clear next to 47 live decoders is how this tool + // reported clean on the exact leak it was pointed at. + return ( + `No leaks in ${enforced.join(', ')} — but ${unenforced} still live and not enforced. ` + + `Pass types: 'all' to check those too.` + ); + } const lines: string[] = []; for (const t of collected) { @@ -84,6 +131,9 @@ function describe( for (const t of over) { lines.push(`${live[t]} ${t} still live (allowed ${allow[t] ?? 0}).`); } + if (unenforced) { + lines.push(`Not enforced, and still live: ${unenforced}. Pass types: 'all' to check those too.`); + } if (sites.length) { lines.push('', 'Held by:'); for (const s of sites.slice(0, 5)) { @@ -112,7 +162,7 @@ export function expectNoLeakedFrames(censuses: ContextCensus[], options: LeakOpt * full census is large and mostly stacks. */ export function summarize(censuses: ContextCensus[]): string { - const report = checkLeaks(censuses, { types: ['VideoFrame', 'AudioData', 'ImageBitmap'] }); + const report = checkLeaks(censuses); const lines = [`${censuses.length} context(s): ${censuses.map((c) => c.context).join(', ')}`]; for (const c of censuses) { diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 8c3fe60..cf5f4f2 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -17,7 +17,7 @@ import { } from '@modelcontextprotocol/sdk/types.js'; import { attach, launchChrome, type CensusSession } from '@motionvector/webcodecs-census-cdp'; -import { checkLeaks, summarize } from '@motionvector/webcodecs-census'; +import { checkLeaks, summarize, type TrackedType } from '@motionvector/webcodecs-census'; let session: CensusSession | null = null; let chrome: Awaited> | null = null; @@ -179,8 +179,10 @@ server.setRequestHandler(CallToolRequestSchema, async (request) => { case 'webcodecs_leak_sites': { const s = requireSession(); const censuses = await s.census(); + // Attribution, not a verdict: never hide a type the caller did not ask + // about, or "which line is leaking" answers nothing for a codec leak. const report = checkLeaks(censuses, { - types: args.type ? [args.type] : undefined, + types: args.type ? [args.type as TrackedType] : 'all', minAgeMs: args.minAgeMs, }); const sites = report.sites.slice(0, args.limit ?? 10); diff --git a/test/assert.test.mjs b/test/assert.test.mjs new file mode 100644 index 0000000..fb655c3 --- /dev/null +++ b/test/assert.test.mjs @@ -0,0 +1,120 @@ +/** + * The verdict layer, on synthetic censuses. No browser: the point is what + * `checkLeaks` does with numbers, and a GC'd-unclosed decoder is not something + * a test can make Chrome produce on demand. + * + * Found by dogfooding, 2026-08-17: a run that ended with 47 live VideoDecoders + * and ~10 collected without close() printed "No leaked WebCodecs objects." + * The shim saw all of it. The default `types` filter threw it away. + */ + +import { test, describe } from 'node:test'; +import assert from 'node:assert/strict'; + +import { checkLeaks, expectNoLeaks, summarize, TRACKED } from '../packages/core/dist/index.js'; + +/** A census with only the fields the verdict layer reads. */ +function census({ context = 'main', live = {}, collectedUnclosed = {}, leakSites = [] } = {}) { + return { + context, + uptimeMs: 30_000, + entered: {}, + left: {}, + live, + collectedUnclosed, + closedUnseen: 0, + leakSites, + oldestLive: [], + mediaElements: { total: 0, stalled: 0, byReadyState: {} }, + timeline: [], + problems: [], + }; +} + +const site = (type, count) => ({ + type, + origin: 'constructed', + stack: ` at leakOne (app.js:1:1)`, + count, + oldestAgeMs: 5_000, +}); + +describe('a leaking VideoDecoder under the default types', () => { + const leaked = [ + census({ + live: { VideoDecoder: 47 }, + collectedUnclosed: { VideoDecoder: 10 }, + leakSites: [site('VideoDecoder', 47)], + }), + ]; + + test('a decoder collected without close() fails whatever types says', () => { + const report = checkLeaks(leaked); + assert.equal(report.ok, false, `verdict was clean:\n${report.message}`); + assert.equal(report.collectedUnclosed.VideoDecoder, 10); + assert.match(report.message, /10 VideoDecoder garbage collected without close\(\)/); + }); + + test('the same holds for a type nobody asked about at all', () => { + const report = checkLeaks([census({ collectedUnclosed: { AudioEncoder: 3 } })], { + types: ['VideoFrame'], + }); + assert.equal(report.ok, false, `verdict was clean:\n${report.message}`); + assert.throws(() => expectNoLeaks([census({ collectedUnclosed: { AudioEncoder: 3 } })]), /AudioEncoder/); + }); + + test('live decoders are still not enforced by default — but never reported as clean', () => { + const live = [census({ live: { VideoDecoder: 47 }, leakSites: [site('VideoDecoder', 47)] })]; + const report = checkLeaks(live); + + assert.equal(report.ok, true, 'a live decoder is normal; the default must not fail on it'); + assert.equal(report.unenforcedLive.VideoDecoder, 47); + assert.doesNotMatch( + report.message, + /^No leaked WebCodecs objects\.$/, + 'an unqualified all-clear next to 47 live decoders is the bug', + ); + assert.match(report.message, /VideoDecoder=47/); + assert.match(summarize(live), /VideoDecoder=47/); + }); + + test("types: 'all' enforces every tracked type", () => { + const report = checkLeaks([census({ live: { VideoDecoder: 47 } })], { types: 'all' }); + assert.equal(report.ok, false); + assert.deepEqual(report.enforced, [...TRACKED]); + assert.equal(report.live.VideoDecoder, 47); + assert.match(report.message, /47 VideoDecoder still live/); + }); + + test("types: 'all' surfaces the codec's allocation site", () => { + const report = checkLeaks( + [census({ live: { VideoDecoder: 47 }, leakSites: [site('VideoDecoder', 47)] })], + { types: 'all' }, + ); + assert.equal(report.sites.length, 1); + assert.equal(report.sites[0].type, 'VideoDecoder'); + }); +}); + +describe('the clean case stays clean', () => { + test('nothing live, nothing collected', () => { + const report = checkLeaks([census()]); + assert.equal(report.ok, true); + assert.equal(report.message, 'No leaked WebCodecs objects.'); + }); + + test('frames within their allowance', () => { + const report = checkLeaks([census({ live: { VideoFrame: 2 } })], { allow: { VideoFrame: 2 } }); + assert.equal(report.ok, true); + assert.equal(report.message, 'No leaked WebCodecs objects.'); + }); + + test('leaked frames still fail, and still say where', () => { + const report = checkLeaks([ + census({ live: { VideoFrame: 5 }, leakSites: [site('VideoFrame', 5)] }), + ]); + assert.equal(report.ok, false); + assert.match(report.message, /5 VideoFrame still live \(allowed 0\)/); + assert.match(report.message, /Held by:/); + }); +}); From ad09139f75711469ac3dee5efa47282ee1cb87f3 Mon Sep 17 00:00:00 2001 From: Saurabh Nandwana Date: Mon, 17 Aug 2026 13:52:48 +0530 Subject: [PATCH 2/2] fix(release): stop shipping a version stamp two releases stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `VERSION` in the core and the version the MCP server announces were both hardcoded '0.1.0', and scripts/version.mjs rewrote neither. Every census payload from 0.2.0 and 0.2.1 therefore carried a version that had not been true since the first release — including the payloads an agent reads, and the handshake an MCP client sees. The bump script now carries both, and exits with an explanation if either literal stops matching its pattern, because a silent no-op is exactly how they went stale. test/version.test.mjs holds them to their package.json. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 +++++ packages/core/src/census.ts | 2 +- packages/mcp/src/index.ts | 2 +- scripts/version.mjs | 19 ++++++++++++++++++ test/version.test.mjs | 39 +++++++++++++++++++++++++++++++++++++ 5 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 test/version.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f93c76..4c2cbaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,11 @@ Versioning follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). while an unenforced type holds live objects. The message now names them: `No leaks in VideoFrame, AudioData, ImageBitmap — but VideoDecoder=47 still live and not enforced.` +- The version stamp. `VERSION` in the core and the version the MCP server + announces were both hardcoded `'0.1.0'`, and `scripts/version.mjs` rewrote + neither — so every census payload from 0.2.0 and 0.2.1 carried a version two + releases stale. The release script now carries both, and fails loudly rather + than silently if either stops matching its pattern. - `webcodecs_leak_sites` attributed only the default frame types, so an agent asking which line is leaking got nothing back for a codec leak. Attribution is not a verdict — it now covers every type unless one is named. diff --git a/packages/core/src/census.ts b/packages/core/src/census.ts index d171a19..1128509 100644 --- a/packages/core/src/census.ts +++ b/packages/core/src/census.ts @@ -659,4 +659,4 @@ function safely(what: string, fn: () => void): void { } } -export const VERSION = '0.1.0'; +export const VERSION = '0.2.1'; diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index cf5f4f2..535dcb3 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -117,7 +117,7 @@ const TOOLS = [ ]; const server = new Server( - { name: 'webcodecs-census', version: '0.1.0' }, + { name: 'webcodecs-census', version: '0.2.1' }, { capabilities: { tools: {} } }, ); diff --git a/scripts/version.mjs b/scripts/version.mjs index 1ae96a4..cabbb02 100644 --- a/scripts/version.mjs +++ b/scripts/version.mjs @@ -66,6 +66,25 @@ for (const dir of PACKAGES) { console.log(` ${pkg.name} -> ${next}`); } +// Two version strings live in source rather than package.json: the census +// stamps every payload with one, and the MCP server announces the other on the +// wire. Nothing rewrote them here, so both said 0.1.0 for two releases. +const STAMPS = [ + { path: 'packages/core/src/census.ts', re: /(export const VERSION = ')[^']+(')/ }, + { path: 'packages/mcp/src/index.ts', re: /(name: 'webcodecs-census', version: ')[^']+(')/ }, +]; + +for (const { path, re } of STAMPS) { + const src = readFileSync(path, 'utf8'); + // Loudly, not silently: a pattern that stops matching is how they went stale. + if (!re.test(src)) { + console.error(`\n ${path} no longer matches its version pattern.\n Fix scripts/version.mjs before releasing, or the stamp ships wrong.\n`); + process.exit(1); + } + writeFileSync(path, src.replace(re, `$1${next}$2`)); + console.log(` ${path} -> ${next}`); +} + // Promote the Unreleased section rather than inventing notes: the release body // is generated from this file, so an empty section is a release with no notes. const CHANGELOG = 'CHANGELOG.md'; diff --git a/test/version.test.mjs b/test/version.test.mjs new file mode 100644 index 0000000..9d718b8 --- /dev/null +++ b/test/version.test.mjs @@ -0,0 +1,39 @@ +/** + * Two version strings live in source rather than package.json, and both said + * 0.1.0 through the 0.2.0 and 0.2.1 releases: every census payload carried a + * wrong stamp, and the MCP server announced a wrong version on the wire. + * + * `scripts/version.mjs` rewrites them now. This is what catches it if that + * rewrite ever silently stops matching. + */ + +import { test, describe } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; + +import { VERSION } from '../packages/core/dist/index.js'; + +const read = (p) => readFileSync(new URL(p, import.meta.url), 'utf8'); +const version = (p) => JSON.parse(read(p)).version; + +describe('the version stamps track the packages', () => { + test('the census stamps payloads with the core version', () => { + assert.equal(VERSION, version('../packages/core/package.json')); + }); + + test('the MCP server announces its own version', () => { + const src = read('../packages/mcp/src/index.ts'); + const found = src.match(/name: 'webcodecs-census', version: '([^']+)'/)?.[1]; + assert.ok(found, 'the MCP server no longer declares a version the way version.mjs rewrites it'); + assert.equal(found, version('../packages/mcp/package.json')); + }); + + test('all three packages are on one version', () => { + const [core, cdp, mcp] = ['core', 'cdp', 'mcp'].map((p) => + version(`../packages/${p}/package.json`), + ); + // -cdp and -mcp depend on an exact core version; out of step is unpublishable. + assert.equal(cdp, core); + assert.equal(mcp, core); + }); +});