From 6f3a6ee363414c29dac6b4f330341edf4971b448 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 02:58:48 -0800 Subject: [PATCH 1/2] fix(cli): stop a test's real MCP warm, and let the temp guard see every leftover The CLI temp-cleanup guard has dequeued #3207 twice and #3210 once with ENOTEMPTY on its own final rm. The cause is one test in the TASK-256 witness block: "a seat that does not run the shipped Commonly MCP is not witnessed" declares a hand-run MCP copy, so the spawn resolves no version, and a spawn with no version always wants a warm. The fresh registry answer the block writes doesn't stop that. The warm is a detached npm install of @commonlyai/mcp from the real registry. It outlived the suite, refilled the test's home after afterEach removed it (17-29 MB), and raced the guard's rm. The guard never saw the leak itself, because it counted a fixed list of prefixes and `cli-claude-mcp-home-` wasn't on it (nor any `cli-claude-*`). - The witness block now holds a warm lock in its home, the pattern another test already uses. Probe (kickWarm instrumented to log real spawns): one real warm on main, none with this change. - The guard counts everything left in the probe root except Jest's and Node's own caches, after a 1.5 s settle so a detached child shows itself. Its final rm retries, and a cleanup that still fails after both leak checks passed is a ::warning, not a red queue run. Reverse check: the old test under the new guard fails pass 1 naming cli-claude-mcp-home-mIwHbl (17 MB) instead of ENOTEMPTY; the fixed test passes both passes with 0 dirs and the probe root removed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- .../adapters.claude.environment.test.mjs | 10 ++++-- cli/scripts/verify-temp-cleanup.mjs | 31 +++++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/cli/__tests__/adapters.claude.environment.test.mjs b/cli/__tests__/adapters.claude.environment.test.mjs index 4e5ac54435..4dd809aea8 100644 --- a/cli/__tests__/adapters.claude.environment.test.mjs +++ b/cli/__tests__/adapters.claude.environment.test.mjs @@ -1806,10 +1806,14 @@ describe('claude adapter: the public seat Commonly MCP witness (TASK-256)', () = })); fs.writeFileSync(path.join(pkgDir, 'src', 'index.js'), ''); fs.writeFileSync(path.join(home, 'current'), '0.3.13\n'); - // A fresh registry answer naming the installed build, so no turn here starts - // a background warm: a detached `npm install` outlives the suite and writes - // into TMPDIR after it ends (the CLI temp-cleanup guard caught it). fs.writeFileSync(path.join(home, '.registry.json'), JSON.stringify({ version: '0.3.13', checkedAt: Date.now() })); + // A held warm lock, so no turn here starts a background warm. A fresh + // registry answer isn't enough: a seat running a hand-run copy resolves no + // version from this home, and a spawn with no version always wants a warm. + // That warm is a detached `npm install` from the real registry. It outlived + // the suite, refilled this home after the teardown below removed it, and + // raced the CLI temp-cleanup guard's own rm (ENOTEMPTY, two dequeued PRs). + fs.writeFileSync(path.join(home, '.warm.lock'), 'warming\n'); prior = process.env.COMMONLY_MCP_HOME; process.env.COMMONLY_MCP_HOME = home; Object.defineProperty(process, 'platform', { value: 'darwin' }); diff --git a/cli/scripts/verify-temp-cleanup.mjs b/cli/scripts/verify-temp-cleanup.mjs index f34521788a..0192470594 100644 --- a/cli/scripts/verify-temp-cleanup.mjs +++ b/cli/scripts/verify-temp-cleanup.mjs @@ -2,10 +2,16 @@ /** * Run the CLI Jest files that create temp homes twice in an isolated TMPDIR and - * fail if any test-created temp directory survives either run. The full CLI + * fail if anything the tests created survives either run. The full CLI * suite runs in CI separately; this is a cheap regression guard for the * teardown contract. The default list is the files that leaked ~40 GB * (TASK-436); pass file paths to check others. + * + * Everything left in the probe root counts except the tools' own caches. It + * used to count a fixed list of prefixes, and a home under a new prefix + * (`cli-claude-mcp-home-`) leaked 29 MB unseen: a detached MCP warm that a test + * started kept writing into it after Jest exited, and the only symptom was this + * script's final rm failing with ENOTEMPTY on whichever PR was in the queue. */ import { spawn } from 'node:child_process'; import { lstat, mkdtemp, readdir, rm } from 'node:fs/promises'; @@ -20,13 +26,11 @@ const targets = process.argv.length > 2 '__tests__/adapters.codex.test.mjs', '__tests__/adapters.pi.test.mjs', ]; -const prefixes = [ - 'opencode-adapter-test-', - 'cli-mcp-home-', - 'cli-mcp-pin-', - 'cli-mcp-home-old-', - 'commonly-mcp-home-jest-', -]; +// Caches Jest and Node write to TMPDIR for themselves, not for a test. +const toolCaches = [/^jest_/, /^node-compile-cache$/]; +// A detached child a test started gets this long to show itself in the probe +// root before the root is read. +const SETTLE_MS = 1500; const bytesUnder = async (path) => { const info = await lstat(path); @@ -38,7 +42,7 @@ const bytesUnder = async (path) => { }; const snapshot = async (root) => { - const entries = (await readdir(root)).filter((name) => prefixes.some((prefix) => name.startsWith(prefix))); + const entries = (await readdir(root)).filter((name) => !toolCaches.some((cache) => cache.test(name))); let bytes = 0; for (const entry of entries) bytes += await bytesUnder(join(root, entry)); return { count: entries.length, bytes, entries }; @@ -71,6 +75,7 @@ try { for (let pass = 1; pass <= 2; pass += 1) { const before = await snapshot(probeRoot); await run(probeRoot); + await new Promise((resolve) => { setTimeout(resolve, SETTLE_MS); }); const after = await snapshot(probeRoot); console.log(`CLI temp cleanup pass ${pass}: before=${before.count} dirs/${before.bytes} bytes, after=${after.count} dirs/${after.bytes} bytes`); if (after.count !== 0 || after.bytes !== 0) { @@ -78,5 +83,11 @@ try { } } } finally { - await rm(probeRoot, { recursive: true, force: true }); + // Both leak checks have run by now, so a cleanup that still can't finish is + // reported, not failed: it is this script's own scratch root. + try { + await rm(probeRoot, { recursive: true, force: true, maxRetries: 5, retryDelay: 200 }); + } catch (error) { + console.log(`::warning::CLI temp cleanup could not remove ${probeRoot} (${error.code || error.message}); a process the tests started may still be writing there`); + } } From f0480457ed933f804a03c87778d708c4917238b6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 1 Nov 2026 03:10:27 -0800 Subject: [PATCH 2/2] fix(cli): a probe root the temp guard can't remove still fails the run CodeRabbit (Major), and the residual both the review and SRE named: the warning-only catch let a run go green while something a test started was still writing into TMPDIR after the last snapshot. That's the leak the guard exists to catch, and the cause is now fixed at the test, so a recurrence should be loud. The final rm keeps its retries; if it still fails, the run sets exitCode 1 and names what was left in the root (with a hint for ENOTEMPTY). exitCode, not a throw, so a leak check's own error stays the reported one. Exercised with a scratch test leaving a read-only dir: exit 1, the leak check's "survived pass 1: locked" error, and the rm line naming `locked`. The four guarded files still pass both passes with 0 dirs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015AKgpQ2pr6xRMhN5mcfoHF --- cli/scripts/verify-temp-cleanup.mjs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cli/scripts/verify-temp-cleanup.mjs b/cli/scripts/verify-temp-cleanup.mjs index 0192470594..5440973883 100644 --- a/cli/scripts/verify-temp-cleanup.mjs +++ b/cli/scripts/verify-temp-cleanup.mjs @@ -12,6 +12,7 @@ * (`cli-claude-mcp-home-`) leaked 29 MB unseen: a detached MCP warm that a test * started kept writing into it after Jest exited, and the only symptom was this * script's final rm failing with ENOTEMPTY on whichever PR was in the queue. + * That rm still fails the run, now naming what was still being written. */ import { spawn } from 'node:child_process'; import { lstat, mkdtemp, readdir, rm } from 'node:fs/promises'; @@ -83,11 +84,18 @@ try { } } } finally { - // Both leak checks have run by now, so a cleanup that still can't finish is - // reported, not failed: it is this script's own scratch root. try { await rm(probeRoot, { recursive: true, force: true, maxRetries: 5, retryDelay: 200 }); } catch (error) { - console.log(`::warning::CLI temp cleanup could not remove ${probeRoot} (${error.code || error.message}); a process the tests started may still be writing there`); + // A root that won't go is a leak the snapshots missed. ENOTEMPTY after the + // retries means something kept writing after the last snapshot, the + // detached-child case above. exitCode, not a throw, so a leak check's own + // error stays the one reported when both happen. + const left = await readdir(probeRoot).catch(() => []); + const hint = error.code === 'ENOTEMPTY' + ? ' Something kept writing there after the last snapshot, usually a process a test started that outlived Jest.' + : ''; + console.error(`CLI temp cleanup could not remove ${probeRoot} (${error.code || error.message}); left in it: ${left.join(', ') || 'nothing readable'}.${hint}`); + process.exitCode = 1; } }