feat(stability): add memory-leak skill - #81
Conversation
Two-phase retention review for JavaScript/TypeScript. Phase 1 is a static read of a diff: enumerate the retention primitives the change introduces — listeners, timers, pending-request registries, subscriptions, module singletons, growing collections — and pair every acquire with its release site. A primitive with a teardown is safe; one without is the finding. Phase 2 escalates to DevTools/CDP heap snapshots only for a primitive the read cannot pair. Leading with the read rather than the instrument settles most leak claims without ever taking a snapshot.
memory-leak-hunt skill
The skill covers runtime retention behaviour, not code authoring, and `coding` reads as language- and style-level guidance. Registers `/domains/stability/` in CODEOWNERS alongside the other platform-owned domains.
memory-leak-hunt skillmemory-leak-hunt skill
Context budgetWhat this PR costs an agent, measured from an install rather than read from the diff. Three tiers, and only the first is unavoidable.
Frontmatter is the only tier paid unconditionally — every agent loads it on every run once the skill is installed, used or not, because it is what the agent reads to decide relevance. The 28 skills across the eleven open skill PRs sit at a median of ~1,716 tokens selected and ~1,860 with references followed. All are within the 1,536-character description budget. Selected is paid only when the agent picks the skill. + refs & knowledge is the ceiling if every bundled reference is then read; it is a worst case, not an expectation. Method
These figures are pinned to the commit above and drift on every push; #96 tracks automating them. |
`hunt` disambiguated nothing. No sibling skill targets memory leaks, and none would: this one already covers both halves — the static retention read from the diff and the heap investigation when the read cannot settle it — so there is no detection/diagnosis split for the suffix to mark. Installed as `mms-memory-leak`.
memory-leak-hunt skillmemory-leak skill
Missed when the rename swept the other branches: the description, the section heading, and two prose references all still named `pr-validate`. The evidence category is now linked to the catalog rather than named bare.
The listener pass matched only `.on('event', handler)` and
`.addListener('event', handler)` — a method named exactly `on` or `addListener`
with a quoted event name. Any form carrying the event in the method name was
invisible.
Running it on extension#42823 returned "no retention path INTRODUCED" for a file
containing `background.onNotification(routeMessengerEventNotification)` with zero
`removeOnNotification` call sites anywhere in `ui/`. A clean verdict over a real
unpaired listener is the worst output this script can produce, because it reports
what the pattern can see as though it were what is there.
Adds `onXxx(handler)`, `subscribe(handler)`, `addEventListener`, and
`addXxxListener` forms, each paired against its corresponding release
(`removeOnXxx`/`offXxx`, `unsubscribe`, `removeEventListener`, `removeXxx`).
The same file now reports the primitive as NEW and OPEN.
Verified no regression: `client.on('connected', connected)` and its siblings in
qr-sync-controller.ts are still detected by the quoted-event pass.
Also drops a hardcoded `PR #40684` from the header, which printed on every run
whatever was scanned, and a re-run hint naming a script that does not exist.
Adds
memory-leakto thestabilitydomain — a two-phase retention review for JavaScript/TypeScript.A memory leak is a retention path: something acquires a reference (a listener, a timer, a map entry, a subscription) and never releases it at the boundary where it should. The skill's core move is to pair every acquire with its release.
Phase 1 — static, from the diff (the lead)
Enumerate the retention primitives the change introduces and, for each, name the holder → held set → outlived boundary triple, then pair the acquire against its release:
.on/addEventListeneroff/removeEventListener— same handler referencesetInterval/ recurringsetTimeoutclearInterval/clearTimeoutmap.set(id, {resolve})map.delete(id)on every completion path.subscribe()/messenger.subscribethisscopepush/set/addFindings are scoped to what the diff adds versus what pre-exists — a pre-existing unpaired primitive is not this PR's finding.
Phase 2 — runtime, only on escalation
DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test — reached for only where the static read cannot pair a primitive.
Why this ordering. A heap snapshot is expensive and slow to interpret; the decisive cheap step is the read a reviewer already performs. Most leak claims are settled without ever taking a snapshot, and the ones that aren't have a specific unpaired primitive to point the profiler at.
Files
domains/stability/skills/memory-leak/—skill.md,references/heap-investigation.md(snapshot workflow and retainer-graph reading),scripts/retention-scan.py(acquire/release pairing over a diff),scripts/heap-over-cycles.example.ts.Shipped experimental; no
repos/overlay, so it applies to extension, mobile, and core alike. Commits GPG-signed.stabilityis a new domain — a leak's failure mode is the tab dying or the service worker being killed, which is availability rather than throughput, so it sits better here than undercoding(the catch-all) orperformance. The PR registers it in.github/CODEOWNERS, defaulted to the platform teams.Note for reviewers
The
descriptionis 1,027 characters, within the 1,536 budget in #47. It is written for skill-discovery matching — the trigger cues are what route a request here rather than to a sibling skill, so length there buys selection accuracy.Showcase — what it produces on two real PRs
Both are in
metamask-extension, both merged, and they are deliberately opposite cases: one where the honest answer is no leak, one where there is.#40684 — extract patch-store substream — proving absence. Phase 1 enumerates the retention primitives the diff introduces and pairs every acquire with its release. When all pair, the finding is that the change adds no unreleased retention — stated as a bounded claim about the diff, not as "no leak anywhere". No heap snapshot needed, so the cost is a careful read.
#44352 — Firefox detached-window leak — a real one. Phase 1 finds nothing to pair, because the leak is a native object's lifecycle rather than a listener or timer the diff adds: reading
win.documentPictureInPicture.requestWindowlazily instantiates a per-window object whose preserved-wrapper cycle Firefox's collector cannot break. The evidence is Phase 2:The pair is the point: the same discipline that proves absence in #40684 localises a real leak in #44352, and neither answer is reached by looking harder at a heap dump.
Validation runs
Trial runs of this PR's skills against merged
metamask-extensionPRs nobody flagged. Every claim was re-verified against the real diff before posting. Clean results are included on purpose — a skill that only ever reports problems cannot be calibrated.memory-leakmemory-leakEach comment carries a trial-run disclaimer and links back here for feedback.