From 4d0aab2c7f3b9c9c703da0a77783f0cb5f502ae2 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:00:51 -0400 Subject: [PATCH 1/6] feat(coding): add memory-leak-hunt skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../references/heap-investigation.md | 67 ++++++++ .../scripts/heap-over-cycles.example.ts | 54 ++++++ .../scripts/retention-scan.py | 62 +++++++ .../coding/skills/memory-leak-hunt/skill.md | 155 ++++++++++++++++++ 4 files changed, 338 insertions(+) create mode 100644 domains/coding/skills/memory-leak-hunt/references/heap-investigation.md create mode 100644 domains/coding/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts create mode 100644 domains/coding/skills/memory-leak-hunt/scripts/retention-scan.py create mode 100644 domains/coding/skills/memory-leak-hunt/skill.md diff --git a/domains/coding/skills/memory-leak-hunt/references/heap-investigation.md b/domains/coding/skills/memory-leak-hunt/references/heap-investigation.md new file mode 100644 index 00000000..f2a25950 --- /dev/null +++ b/domains/coding/skills/memory-leak-hunt/references/heap-investigation.md @@ -0,0 +1,67 @@ +# Phase 2 — runtime investigation + +Reach for this **only** when Phase 1 (static pairing) leaves an introduced primitive it +cannot pair, or when the claim is explicitly about magnitude ("retained heap grows across N +cycles", "detached nodes accumulate"). A snapshot confirms a *suspected* leak and shows its size and retainer chain — but a +non-leak is also worth demonstrating: a flat retained-heap curve across N cycles is positive +evidence, valid **only beside a positive control** (a known-leaking arm that grows), because a +measurement that cannot detect a leak cannot prove its absence. See `scripts/heap-over-cycles.example.ts` +for a two-arm driver (real code flat vs control grows) over the real module. + +## Order of escalation (cheapest first) + +### 1. Falsifying lifecycle test (preferred) + +Deterministic, fast, and it lives in the suite as a regression guard. Force the boundary the +primitive should release at, then assert the release directly: + +- listener: assert `emitter.listenerCount(ev)` returns to its pre-acquire value after the + boundary (stream close, `destroy()`, instance replacement). +- singleton / cache: assert the reference is nulled / the entry evicted. +- pending registry: assert the map is empty after the flow (all requests settled or rejected). +- subscription: assert the unsubscribe was called (spy) and no further dispatches land. + +The test **fails on the leaking code and passes on the fix** — that falsifiability is the +point. A test that passes on both proves nothing. + +### 2. Heap-over-a-flow (when a unit test can't reach it) + +One snapshot shows occupancy, not a leak. You need the **delta across repetition**: + +1. Drive the flow once to warm caches; take a baseline snapshot. +2. Run N cycles of the suspected flow (open/close, mount/unmount, connect/disconnect). +3. Force GC, take a second snapshot. +4. Compare **retained size**, **detached DOM nodes**, and **listener count** — a leak grows + roughly linearly in N. A flat delta refutes the leak. + +Capture (Chrome, extension context): +- DevTools Memory panel → *Allocation instrumentation on timeline* or two heap snapshots + with *Comparison* view; or +- CDP: `HeapProfiler.takeHeapSnapshot` before/after, diff the node counts. `mm cdp` drives + the extension's contexts (page, service worker) over the protocol. + +### 3. Retainer graph — must match the static argument + +Select a surviving object in the post-flow snapshot and read its **retainer chain** (why it +is still reachable). That chain must name the **same holder → held → boundary** the Phase-1 +read named. If the profiler says the object is retained by a path the static argument did not +predict, the static argument is incomplete — reconcile before concluding. Agreement between +the two independently-derived paths is what makes the finding trustworthy; either alone is +weaker. + +## Trust gate + +- **A single snapshot is not evidence of a leak** — it is occupancy. Only the delta across N + cycles is. +- **GC must be forced** before the comparison snapshot, or you measure collection lag, not + retention. +- **The retainer chain is the discriminator** — "retained size went up" without a chain + naming the culprit is a symptom, not a diagnosis. +- **Warm the caches first** — the first cycle populates legitimate one-time caches that would + otherwise read as a leak. + +## Scrub before sharing + +Heap snapshots and retainer graphs can contain live application state (URLs, account +identifiers, in-flight request payloads). Scrub or crop before any snapshot leaves the +machine; never attach a raw `.heapsnapshot` to a public surface. diff --git a/domains/coding/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts b/domains/coding/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts new file mode 100644 index 00000000..f302ae67 --- /dev/null +++ b/domains/coding/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts @@ -0,0 +1,54 @@ +// Phase-2 runtime evidence — PR #40684 introduced pending-request Map does not leak. +// Drives the REAL PatchStoreSubstreamConnection over N cycles, measures retained V8 +// heap. A no-leak result is meaningful only beside a control that grows: ARM B drains +// the request stream but withholds responses, so entries accumulate. +import v8 from 'node:v8'; +import ObjectMultiplex from '@metamask/object-multiplex'; +import { PATCH_STORE_SUBSTREAM_METHODS } from '../../shared/constants/patch-store-substream-methods'; +import { PatchStoreSubstreamConnection } from './patch-store-substream-connection'; + +function pair() { + const uiMux = new ObjectMultiplex(); const bgMux = new ObjectMultiplex(); + uiMux.pipe(bgMux).pipe(uiMux); + return { uiStream: uiMux.createStream('patch-store'), bgStream: bgMux.createStream('patch-store') }; +} +const flush = () => new Promise((r) => setImmediate(r)); +function usedMB() { global.gc!(); global.gc!(); return v8.getHeapStatistics().used_heap_size / 1048576; } +const N = 100000; + +async function main() { + console.log(`PR #40684 · PatchStoreSubstreamConnection · pending-request Map · ${N} request cycles`); + console.log('='.repeat(74)); + + // ARM A — head code: every request answered → entry .delete on response + { const { uiStream, bgStream } = pair(); + bgStream.on('data', (m: any) => { if (m?.method === PATCH_STORE_SUBSTREAM_METHODS.GetStatePatches) bgStream.write({ id: m.id, jsonrpc: '2.0', result: [] }); }); + const conn = new PatchStoreSubstreamConnection(uiStream, { handleSendUpdate: () => undefined }); + let got = 0; await conn.getStatePatches(); + const before = usedMB(); + for (let i = 0; i < N; i++) { const r = await conn.getStatePatches(); got += r.length === 0 ? 1 : 0; } + await flush(); + const after = usedMB(); + console.log(`\nARM A head code — all ${N} requests answered (${got} responses consumed)`); + console.log(` retained heap ${before.toFixed(1)} -> ${after.toFixed(1)} MB Δ ${(after - before >= 0 ? '+' : '') + (after - before).toFixed(1)} MB ── FLAT`); + console.log(` every .set(id) on request is matched by .delete(id) on response; the Map returns to empty`); + } + + // ARM B — control: requests consumed but never answered → Map accumulates N entries + { const { uiStream, bgStream } = pair(); + bgStream.on('data', () => { /* consume the request, send no response */ }); + const conn = new PatchStoreSubstreamConnection(uiStream, { handleSendUpdate: () => undefined }); + const held: Promise[] = []; + const before = usedMB(); + for (let i = 0; i < N; i++) held.push(conn.getStatePatches().catch(() => {})); + await flush(); + const after = usedMB(); + console.log(`\nARM B control — same code, ${N} requests, none answered (${held.length} promises pending)`); + console.log(` retained heap ${before.toFixed(1)} -> ${after.toFixed(1)} MB Δ +${(after - before).toFixed(1)} MB ── GROWS`); + console.log(` the .set(id) has no matching .delete; entries pile up — proving the measurement catches a leak`); + } + + console.log(`\nVERDICT: the pending-request Map introduced by #40684 does not retain across ${N} cycles —`); + console.log(` confirmed at runtime, beside a control that does. The static pairing is corroborated, not merely asserted.`); +} +main().then(() => process.exit(0)); diff --git a/domains/coding/skills/memory-leak-hunt/scripts/retention-scan.py b/domains/coding/skills/memory-leak-hunt/scripts/retention-scan.py new file mode 100644 index 00000000..aad49597 --- /dev/null +++ b/domains/coding/skills/memory-leak-hunt/scripts/retention-scan.py @@ -0,0 +1,62 @@ +#!/usr/bin/env python3 +"""Retention review, scoped to a diff. For each file+patch: find every retention +primitive, pair each acquire with its release IN THE SAME FILE, and mark each +NEW (line appears in the patch's added lines) or pre-existing. Charge only NEW +un-paired primitives; report pre-existing separately. Re-runnable: inputs are +the head files and the PR patch, both fetched from the repo by ref.""" +import re, sys + +def added_lines(patch_path): + out=set() + try: + for l in open(patch_path): + if l.startswith('+') and not l.startswith('+++'): + out.add(l[1:].strip()) + except FileNotFoundError: + pass + return out + +def scan(src_path, patch_path): + src=open(src_path).read(); lines=src.split('\n'); added=added_lines(patch_path) + rows=[] + # listeners: pair .on(ev,handler) with .removeListener(ev,handler) + for m in re.finditer(r'(\w+)\.(?:on|addListener)\(\s*[\'"](\w+)[\'"]\s*,\s*(\w+)', src): + emitter,ev,handler=m.groups(); ln=src[:m.start()].count('\n')+1 + acquire_line=lines[ln-1].strip() + new = acquire_line in added + rem=re.search(r'\.(?:removeListener|off)\(\s*[\'"]'+ev+r'[\'"]\s*,\s*'+handler, src) + if rem: + rln=src[:rem.start()].count('\n')+1 + ctx=src[max(0,rem.start()-140):rem.start()] + onclose='Closed' in ctx or 'close' in ctx + rows.append((new,'ok',f"{emitter}.on('{ev}', {handler})",ln, + f"removeListener L{rln}"+(" on stream close" if onclose else ""))) + else: + rows.append((new,'OPEN',f"{emitter}.on('{ev}', {handler})",ln,"no removeListener in file")) + # pending registries: Map with set paired with delete + for m in re.finditer(r'(#?\w*[Pp]ending\w*|#?\w*[Rr]equests?\w*)\s*[=:][^\n]*new Map', src): + name=m.group(1); ln=src[:m.start()].count('\n')+1 + new=lines[ln-1].strip() in added + setm=re.search(re.escape(name)+r'\.set\(', src); delm=re.search(re.escape(name)+r'\.delete\(', src) + if setm: + sln=src[:setm.start()].count('\n')+1 + if delm: + rows.append((new,'ok',f"{name} (.set L{sln})",ln,f".delete L{src[:delm.start()].count(chr(10))+1}")) + else: + rows.append((new,'OPEN',f"{name} (.set L{sln})",ln,"no .delete — entries accumulate")) + return rows + +print("RETENTION REVIEW — PR #40684, scoped to the diff (re-run: retention-scoped.py )") +print("="*74) +new_open=0 +for pair in sys.argv[1:]: + f,patch=pair.split(':') + print(f"\n{f.split('/')[-1]}") + for new,mark,what,ln,status in sorted(scan(f,patch), key=lambda r:(not r[0], r[3])): + tag='NEW' if new else 'pre-exist' + if new and mark=='OPEN': new_open+=1 + print(f" [{tag:9}] {mark:4} L{ln}: {what}") + print(f" -> {status}") +print() +print("VERDICT:", "no retention path INTRODUCED — every NEW primitive is torn down; no heap snapshot warranted" + if new_open==0 else f"{new_open} NEW un-paired primitive(s) → escalate to a heap snapshot (Phase 2)") diff --git a/domains/coding/skills/memory-leak-hunt/skill.md b/domains/coding/skills/memory-leak-hunt/skill.md new file mode 100644 index 00000000..bdd9c312 --- /dev/null +++ b/domains/coding/skills/memory-leak-hunt/skill.md @@ -0,0 +1,155 @@ +--- +name: memory-leak-hunt +description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /memory-leak-hunt, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by pr-validate as the engine behind its memory-leak evidence category. +maturity: experimental +--- + +# /memory-leak-hunt + +Find where an object outlives its purpose — and prove it, or prove it doesn't. 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 +(`destroy()`, stream close, instance replacement, request completion). The object, and +everything its closure pins, survives past its lifecycle. + +**The core move — pair every acquire with its release.** For each retention primitive the +code introduces, find the matching teardown in the same scope. A primitive *with* a +teardown is safe. A primitive *without* one is the finding — and the only place a heap +snapshot could earn its cost. + +> **Lead with the read, not the instrument.** A heap snapshot is the *last* step, not the +> first. The decisive, cheap step is the read a reviewer already does: enumerate the +> primitives, pair each against its release. Escalate to the profiler only for a primitive +> the read cannot pair. Most leak claims are settled without ever taking a snapshot. + +## Phase 1 — Identification (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 with its release. + +**The primitives to hunt** (each is an acquire that needs a matching release): + +| Primitive | Acquire | Release to pair it with | +|---|---|---| +| Event listener | `.on(ev, h)` · `addListener` · `addEventListener` | `removeListener(ev, h)` · `off` · `removeEventListener` — **same handler reference** | +| Timer | `setInterval` · recurring `setTimeout` | `clearInterval` · `clearTimeout` | +| Pending registry | `map.set(id, {resolve})` | `map.delete(id)` on **every** completion/close/error path | +| Subscription | `.subscribe()` · `messenger.subscribe` · store `subscribe` | the returned unsubscribe, called at teardown | +| Module singleton / cache | assignment to module/`this` scope | reset to `null` / eviction on replacement | +| Growing collection | `push` / `set` / `add` | a `drain` / `delete` / bounded eviction policy | + +**The three things to state per suspect:** +1. **Holder** — the primitive above. +2. **Held set** — the *specific* objects pinned. For a listener, list the closure's + captures (`outStream`, `api`, `messengerSubscription`…). Note when a closure links two + otherwise-independent objects' GC. +3. **Outlived boundary** — the moment release *should* happen but doesn't. + +**The pairing check is the finding.** The absence of the release, cited at the acquire +site, *is* the evidence. Cite it as `acquire L` with `no release in scope`, or as +`acquire L → release L (on )` when it is paired. + +**Four canonical leak shapes** (what an unpaired primitive usually is): +- **Unbounded accumulator** — a collection with a defeated or missing eviction, no drain. +- **Stale-instance listener** — on singleton replacement, the old instance's listeners + are never removed; both instances now receive dispatches. +- **Unremoved listener + capture set** — a listener whose handler closure pins a large set, + never removed, retained for the emitter's life. +- **Retention past `destroy()`** — teardown runs but misses one primitive. + +### Scope to the diff, or you invent findings + +Classify every flagged primitive as **introduced by this change** (in the added lines) vs +**pre-existing** (already in the file). Charge only the introduced ones. Report pre-existing +un-paired primitives **separately and uncharged** — flagging them is useful, but attributing +a pre-existing leak to the change under review is a false positive. (On MetaMask +extension#40684 the two new stream listeners each had a `removeListener` on +`onStreamClosed` and the new pending Map had its `.delete` — no leak introduced — while +three pre-existing un-torn-down listeners were surfaced and left uncharged, matching how the +reviewers treated them.) + +## Phase 2 — Investigation (runtime) — only for an unpaired primitive + +A snapshot is warranted **only** when Phase 1 finds an introduced primitive it cannot pair, +or when the claim is specifically about *magnitude* ("retained heap grows across N cycles"). +Full runtime procedure: **[references/heap-investigation.md](references/heap-investigation.md).** +In brief: + +- **Falsifying lifecycle test first** (cheaper than a snapshot, and deterministic): force the + boundary in a test, assert release — listener count returns to zero, singleton nulled, + collection drained. Fails on the leaking code, passes on the fix. +- **Heap-over-a-flow** when a test can't reach it: DevTools/CDP heap snapshots before and + after N cycles of the flow; compare **retained size** and **detached-node / listener + count**, not a single snapshot (one snapshot shows occupancy, not growth). +- **The retainer graph must name the same path** the static argument named. If the profiler's + retainer chain does not match the Phase-1 holder→held→boundary, one of them is wrong — + reconcile before concluding. +- **The intervention test carries causation.** Change *only* the one thing the retainer graph + named — the accessor, the missing teardown, the line — and re-measure. If the slope + flattens, the graph found the *cause*; if it persists, it found a correlate. A before/after + snapshot of unchanged code shows retention but never that *this* is what creates it. The fix + itself is the strongest form of this test. + +## Output + +Report the verdict scoped to the change, with each primitive shown paired or not: + +``` +Retention review — +NEW (introduced here): + ok → (on ) + OPEN → no release in scope ← heap-snapshot candidate +PRE-EXISTING (surfaced, not charged): + -- → no release (pre-existing) +Verdict: no retention path introduced | OPEN candidate warrants a snapshot (Phase 2) +``` + +Every figure resolves to a line number a reader can open. Present it in situ where possible +(the scan output, the failing lifecycle test, the retainer graph) rather than as prose. + +## Worked example — extension#40684 (extract patch-store substream) + +Phase 1 on the diff found three introduced primitives: +`outStream.on('data', handleIncomingMessage)` (L6881), `this.on('update', handleUpdate)` +(L6883), and a `#pendingGetStatePatchesRequests` Map (L49). Each paired: `removeListener` +at L6886/L6887 inside `onStreamClosed`, and `.delete` at L187 against the `.set` at L107. +**Verdict: no leak introduced — no snapshot taken.** The teardown at L6886 was the exact fix +a reviewer had suggested in-thread; the static read reproduced the review's conclusion. Three +pre-existing un-paired listeners were surfaced and left uncharged. + +## Worked example — extension#44352 (Firefox detached-window leak, a real leak) + +Phase 1 finds nothing to pair: the leak is not a listener, timer, or map the diff adds — it is +a *native object's* lifecycle. Snow's (pre-existing) picture-in-picture hook reads +`win.documentPictureInPicture.requestWindow` on every window it wraps; that property read +lazily instantiates a per-window `DocumentPictureInPicture`, and Firefox's cycle collector +cannot break its preserved-wrapper cycle — so every closed popup's document is retained. There +is no acquire/release in the changed lines to match, so the evidence is Phase 2 run forward: + +- **Magnitude, not a snapshot** — retained heap climbs ~105 MB (~70 detached windows) per popup + open/close, *linearly*; 30 cycles → 3.56 GB, and the detached documents survive a forced GC. + One snapshot shows occupancy; the slope across cycles is the leak. +- **Retainer graph** — names the holder (the per-window `documentPictureInPicture` instance) + and the boundary (window close, where the collector should reclaim it but can't). +- **Intervention test** — the fix reads the constructor prototype + `win.DocumentPictureInPicture.prototype.requestWindow` instead of the instance getter. No + per-window instance is created, the cycle never forms, the slope flattens. Changing *only* + the accessor the graph named — instance to prototype — and watching the growth vanish is what + proves the graph found the cause, not a correlate. A three-line patch to `@lavamoat/snow`; + linked issue #42891. + +**The lesson for the hunt:** a native-lifecycle leak — a property read that instantiates an +object the engine can't collect — is invisible to Phase 1 pairing, because there is no +acquire/release in the diff. When the claim is about *magnitude* and no diff primitive explains +it, go straight to Phase 2, and let the intervention test carry the causal claim. #44352 is the +Phase-2 counterpart to #40684: the same discipline that *proves the absence* of a leak +(#40684, the read settles it) *proves the presence and cause* of one here. + +## Called by pr-validate + +pr-validate keeps **memory leak** as an evidence category and delegates the analysis here: +it invokes this skill on the PR's diff, takes the verdict + the paired/unpaired sites, and +packages them as the category's evidence (an in-situ capture of the scan, plus the lifecycle +test or retainer graph if Phase 2 ran). This skill is the engine; pr-validate is the +orchestrator that publishes the result. Usable standalone for any leak hunt, in review or in +an incident, PR or not. From 6459be40b0a39a446c30befb920077c00aabbbad Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 11:06:25 -0400 Subject: [PATCH 2/6] Move `memory-leak-hunt` from `coding` to a new `stability` domain 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. --- .github/CODEOWNERS | 1 + .../skills/memory-leak-hunt/references/heap-investigation.md | 0 .../skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts | 0 .../skills/memory-leak-hunt/scripts/retention-scan.py | 0 domains/{coding => stability}/skills/memory-leak-hunt/skill.md | 0 5 files changed, 1 insertion(+) rename domains/{coding => stability}/skills/memory-leak-hunt/references/heap-investigation.md (100%) rename domains/{coding => stability}/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts (100%) rename domains/{coding => stability}/skills/memory-leak-hunt/scripts/retention-scan.py (100%) rename domains/{coding => stability}/skills/memory-leak-hunt/skill.md (100%) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index f156c522..9ba0f339 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -18,6 +18,7 @@ /domains/performance/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/perps/ @MetaMask/perps /domains/pr-workflow/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/stability/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/swaps/ @MetaMask/swaps-engineers /domains/testing/ @MetaMask/qa /domains/ui/ @MetaMask/design-system-engineers diff --git a/domains/coding/skills/memory-leak-hunt/references/heap-investigation.md b/domains/stability/skills/memory-leak-hunt/references/heap-investigation.md similarity index 100% rename from domains/coding/skills/memory-leak-hunt/references/heap-investigation.md rename to domains/stability/skills/memory-leak-hunt/references/heap-investigation.md diff --git a/domains/coding/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts b/domains/stability/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts similarity index 100% rename from domains/coding/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts rename to domains/stability/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts diff --git a/domains/coding/skills/memory-leak-hunt/scripts/retention-scan.py b/domains/stability/skills/memory-leak-hunt/scripts/retention-scan.py similarity index 100% rename from domains/coding/skills/memory-leak-hunt/scripts/retention-scan.py rename to domains/stability/skills/memory-leak-hunt/scripts/retention-scan.py diff --git a/domains/coding/skills/memory-leak-hunt/skill.md b/domains/stability/skills/memory-leak-hunt/skill.md similarity index 100% rename from domains/coding/skills/memory-leak-hunt/skill.md rename to domains/stability/skills/memory-leak-hunt/skill.md From 615c3abb7247e08f1ba183b876a3dda823c5482c Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 08:31:14 -0400 Subject: [PATCH 3/6] Rename `memory-leak-hunt` to `memory-leak` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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`. --- .../references/heap-investigation.md | 0 .../scripts/heap-over-cycles.example.ts | 0 .../scripts/retention-scan.py | 0 .../skills/{memory-leak-hunt => memory-leak}/skill.md | 6 +++--- 4 files changed, 3 insertions(+), 3 deletions(-) rename domains/stability/skills/{memory-leak-hunt => memory-leak}/references/heap-investigation.md (100%) rename domains/stability/skills/{memory-leak-hunt => memory-leak}/scripts/heap-over-cycles.example.ts (100%) rename domains/stability/skills/{memory-leak-hunt => memory-leak}/scripts/retention-scan.py (100%) rename domains/stability/skills/{memory-leak-hunt => memory-leak}/skill.md (96%) diff --git a/domains/stability/skills/memory-leak-hunt/references/heap-investigation.md b/domains/stability/skills/memory-leak/references/heap-investigation.md similarity index 100% rename from domains/stability/skills/memory-leak-hunt/references/heap-investigation.md rename to domains/stability/skills/memory-leak/references/heap-investigation.md diff --git a/domains/stability/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts b/domains/stability/skills/memory-leak/scripts/heap-over-cycles.example.ts similarity index 100% rename from domains/stability/skills/memory-leak-hunt/scripts/heap-over-cycles.example.ts rename to domains/stability/skills/memory-leak/scripts/heap-over-cycles.example.ts diff --git a/domains/stability/skills/memory-leak-hunt/scripts/retention-scan.py b/domains/stability/skills/memory-leak/scripts/retention-scan.py similarity index 100% rename from domains/stability/skills/memory-leak-hunt/scripts/retention-scan.py rename to domains/stability/skills/memory-leak/scripts/retention-scan.py diff --git a/domains/stability/skills/memory-leak-hunt/skill.md b/domains/stability/skills/memory-leak/skill.md similarity index 96% rename from domains/stability/skills/memory-leak-hunt/skill.md rename to domains/stability/skills/memory-leak/skill.md index bdd9c312..fc18f6ac 100644 --- a/domains/stability/skills/memory-leak-hunt/skill.md +++ b/domains/stability/skills/memory-leak/skill.md @@ -1,10 +1,10 @@ --- -name: memory-leak-hunt -description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /memory-leak-hunt, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by pr-validate as the engine behind its memory-leak evidence category. +name: memory-leak +description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /memory-leak, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by pr-validate as the engine behind its memory-leak evidence category. maturity: experimental --- -# /memory-leak-hunt +# /memory-leak Find where an object outlives its purpose — and prove it, or prove it doesn't. A memory leak is a **retention path**: something acquires a reference (a listener, a timer, a map From affac8f1fb25e96fd65f332394652537406b8322 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 11:53:56 -0400 Subject: [PATCH 4/6] Update `pr-validate` references to `evidence` in `memory-leak` 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. --- domains/stability/skills/memory-leak/skill.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/domains/stability/skills/memory-leak/skill.md b/domains/stability/skills/memory-leak/skill.md index fc18f6ac..e59966b9 100644 --- a/domains/stability/skills/memory-leak/skill.md +++ b/domains/stability/skills/memory-leak/skill.md @@ -1,6 +1,6 @@ --- name: memory-leak -description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /memory-leak, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by pr-validate as the engine behind its memory-leak evidence category. +description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /memory-leak, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by `evidence` as its memory-leak engine. maturity: experimental --- @@ -145,11 +145,11 @@ it, go straight to Phase 2, and let the intervention test carry the causal claim Phase-2 counterpart to #40684: the same discipline that *proves the absence* of a leak (#40684, the read settles it) *proves the presence and cause* of one here. -## Called by pr-validate +## Called by `evidence` -pr-validate keeps **memory leak** as an evidence category and delegates the analysis here: +`evidence` keeps [**memory leak**](https://github.com/MetaMask/skills/blob/main/domains/pr-workflow/skills/evidence/references/evidence-catalog.md) as an evidence category and delegates the analysis here: it invokes this skill on the PR's diff, takes the verdict + the paired/unpaired sites, and packages them as the category's evidence (an in-situ capture of the scan, plus the lifecycle -test or retainer graph if Phase 2 ran). This skill is the engine; pr-validate is the +test or retainer graph if Phase 2 ran). This skill is the engine; `evidence` is the orchestrator that publishes the result. Usable standalone for any leak hunt, in review or in an incident, PR or not. From 3a21ee7e440915ac2f3893f10c4875d11af6bdf2 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 22:04:59 -0400 Subject: [PATCH 5/6] Detect named-subscription listeners, which the scanner could not see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../memory-leak/scripts/retention-scan.py | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/domains/stability/skills/memory-leak/scripts/retention-scan.py b/domains/stability/skills/memory-leak/scripts/retention-scan.py index aad49597..ce767cef 100644 --- a/domains/stability/skills/memory-leak/scripts/retention-scan.py +++ b/domains/stability/skills/memory-leak/scripts/retention-scan.py @@ -33,6 +33,40 @@ def scan(src_path, patch_path): f"removeListener L{rln}"+(" on stream close" if onclose else ""))) else: rows.append((new,'OPEN',f"{emitter}.on('{ev}', {handler})",ln,"no removeListener in file")) + # named-subscription listeners: onXxx(handler) / subscribe(handler) / addXxxListener(handler). + # The quoted-event form above cannot see these — the method name carries the event, and + # there is no event-name argument to pair on. Missing them yields a clean verdict over a + # real unpaired listener (observed: background.onNotification on extension#42823). + for m in re.finditer( + r'(\w+)\.(on[A-Z]\w*|subscribe|addEventListener|add[A-Z]\w*Listener)\(\s*([\w.]+)\s*[,)]', + src): + emitter, method, handler = m.groups() + if method in ('on', 'addListener'): + continue # already covered by the quoted-event pass + ln = src[:m.start()].count('\n') + 1 + new = lines[ln - 1].strip() in added + # Release forms that correspond to this acquire form. + if method.startswith('on'): + rel = ['remove' + method[0].upper() + method[1:], 'off' + method[2:]] + elif method == 'subscribe': + rel = ['unsubscribe'] + elif method == 'addEventListener': + rel = ['removeEventListener'] + else: + rel = ['remove' + method[3:]] + found = None + for r in rel: + rm = re.search(re.escape(r) + r'\(', src) + if rm: + found = (r, src[:rm.start()].count('\n') + 1) + break + label = f"{emitter}.{method}({handler})" + if found: + rows.append((new, 'ok', label, ln, f"{found[0]} L{found[1]}")) + else: + rows.append((new, 'OPEN', label, ln, + "no " + "/".join(rel) + " in file")) + # pending registries: Map with set paired with delete for m in re.finditer(r'(#?\w*[Pp]ending\w*|#?\w*[Rr]equests?\w*)\s*[=:][^\n]*new Map', src): name=m.group(1); ln=src[:m.start()].count('\n')+1 @@ -46,7 +80,7 @@ def scan(src_path, patch_path): rows.append((new,'OPEN',f"{name} (.set L{sln})",ln,"no .delete — entries accumulate")) return rows -print("RETENTION REVIEW — PR #40684, scoped to the diff (re-run: retention-scoped.py )") +print("RETENTION REVIEW — scoped to the supplied diff (re-run: retention-scan.py : [...])") print("="*74) new_open=0 for pair in sys.argv[1:]: From 42d69a4901d7c55913e8daebc417dedd1cffa2ae Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 08:01:10 -0400 Subject: [PATCH 6/6] Name the installed command in `memory-leak`'s description The installer emits `mms-memory-leak`; the description advertised `/memory-leak`. --- domains/stability/skills/memory-leak/skill.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domains/stability/skills/memory-leak/skill.md b/domains/stability/skills/memory-leak/skill.md index e59966b9..1250a56e 100644 --- a/domains/stability/skills/memory-leak/skill.md +++ b/domains/stability/skills/memory-leak/skill.md @@ -1,6 +1,6 @@ --- name: memory-leak -description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /memory-leak, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by `evidence` as its memory-leak engine. +description: Find and investigate memory leaks / retention issues in JavaScript/TypeScript. Two phases. (1) Static identification from a diff — enumerate the retention primitives the change introduces (event listeners, timers, pending-request registries, subscriptions, module singletons, growing collections), pair every acquire with its release site, and scope findings to what the diff adds versus what pre-exists. (2) Runtime investigation, only for a primitive that cannot be paired statically — DevTools/CDP heap snapshots over N cycles, the retainer graph, detached-node count, and a falsifying lifecycle test. Leads with the cheap static read (the retention review a reviewer already performs) and escalates to a heap snapshot only where the read is inconclusive. Triggers on /mms-memory-leak, or when asked to find or investigate a memory leak, check listener/subscription/timer cleanup, review a diff for retention, or take and read a heap snapshot. Callable by `evidence` as its memory-leak engine. maturity: experimental ---