From 8401275a90f23c677b6609bb3eba00d9f6012a7e Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:04:37 +0530 Subject: [PATCH 1/8] Background tasks: an exit is reported once the group's fate is known, never "processes left" by guess backgroundSession.test.ts:92 (macOS CI on #417) refused to remove a task that had just exited: "a running task, or one whose processes still run, is never removed". Root cause: when the command's leader exited, the supervisor finished the task with `leftovers = group.alive`, but `alive` is the guard's own liveness (the guard is a member of the group and confirms it is empty with `ps` before it exits). Every task that exited cleanly was therefore reported, for one guard probe (tens of ms, longer under load), as "exited, processes left", with Delete refused; users saw the red "processes of this task still run" row flicker. The test removed the task inside that window on a loaded runner (reproduced 3 of 60 under 10x parallel load). The guard now says once when members remain (a `left` marker beside its `exit` file), and the supervisor finishes a cleanly exited leader only on the guard's verdict: the guard exits (nothing left) or marks what is left. If the guard cannot tell within 3 s (its ps keeps failing), something is reported as left, as before. A stop requested meanwhile keeps the stopping semantics. New test: a task that leaves nothing behind is never reported with leftovers, even for a moment, and is removable at once (fails on the old supervisor every run). Refs #410 --- src/backgroundTasks/supervisor.ts | 25 ++++++++++++++++++++----- src/backgroundTasks/taskGroup.ts | 5 +++++ src/backgroundTasks/taskGroupGuard.ts | 6 ++++++ tests/backgroundSupervisor.test.ts | 12 ++++++++++++ 4 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/backgroundTasks/supervisor.ts b/src/backgroundTasks/supervisor.ts index 4c3e5cb2..583b7b33 100644 --- a/src/backgroundTasks/supervisor.ts +++ b/src/backgroundTasks/supervisor.ts @@ -79,12 +79,16 @@ interface LiveTask { /** The live guard owns signalling from inside its group; no numeric group id is retained. */ group?: TaskGroup; leaderExited?: boolean; + /** Since when the leader's exit waits for the guard's verdict on the rest of the group (performance.now()). */ + verdictSince?: number; pending?: ReturnType; /** Observes the command leader separately from the guard's eventual exit. */ watch?: ReturnType; } const ORDER: readonly StopLevel[] = ['interrupt', 'terminate', 'kill']; +/** How long a finished leader waits for the guard's verdict when it cannot get one (its `ps` keeps failing). */ +const VERDICT_MS = 3000; export class BackgroundTaskSupervisor { private readonly tasks = new Map(); @@ -124,11 +128,20 @@ export class BackgroundTaskSupervisor { this.tasks.set(id, task); pty.onData(data => { task.output.append(data); this.changed(task); }); const observeLeader = () => { - if (task.leaderExited) return; - const exit = task.group?.leaderExit(); - if (exit) { + if (!task.leaderExited) { + const exit = task.group?.leaderExit(); + if (!exit) return; + task.leaderExited = true; + this.exited(task, exit.exitCode, exit.signal); + } + // The command has exited. The guard decides how it ended: it exits when nothing else of the group runs (onExit + // below), or says that something does. Only if it cannot tell in time is something reported as left. + if (task.verdictSince === undefined) { clearInterval(task.watch); task.watch = undefined; return; } + if (task.group?.leftBehind() || performance.now() - task.verdictSince > VERDICT_MS) { + task.verdictSince = undefined; clearInterval(task.watch); task.watch = undefined; - task.leaderExited = true; this.exited(task, exit.exitCode, exit.signal); + if (task.record.stopRequested) { task.record.leftovers = true; this.changed(task, true); } + else this.finish(task); } }; task.watch = setInterval(observeLeader, 25); @@ -140,7 +153,7 @@ export class BackgroundTaskSupervisor { clearInterval(task.watch); task.watch = undefined; if (!task.leaderExited) this.exited(task, exitCode, signal); - else if (task.record.state === 'stopping') this.finish(task); + else if (task.record.state === 'stopping' || task.verdictSince !== undefined) { task.verdictSince = undefined; this.finish(task); } else { task.record.leftovers = false; this.changed(task, true); } }); this.changed(task, true); @@ -236,6 +249,8 @@ export class BackgroundTaskSupervisor { this.changed(task, true); return; } + // Whether anything else of the group still runs is not known yet: wait for the guard's verdict, never guess. + if (task.group?.alive === true) { task.verdictSince = performance.now(); return; } this.finish(task); } diff --git a/src/backgroundTasks/taskGroup.ts b/src/backgroundTasks/taskGroup.ts index 9c011597..537d3aaa 100644 --- a/src/backgroundTasks/taskGroup.ts +++ b/src/backgroundTasks/taskGroup.ts @@ -53,6 +53,11 @@ export class TaskGroup { return undefined; } + /** The guard found members of the group still running after the command's leader exited. */ + leftBehind(): boolean { + try { return readFileSync(join(this.directory, 'left'), 'utf8') === '1'; } catch { return false; } + } + close(): void { this.alive = false; rmSync(this.directory, {recursive: true, force: true}); diff --git a/src/backgroundTasks/taskGroupGuard.ts b/src/backgroundTasks/taskGroupGuard.ts index 771c0ad6..ec413203 100644 --- a/src/backgroundTasks/taskGroupGuard.ts +++ b/src/backgroundTasks/taskGroupGuard.ts @@ -35,6 +35,7 @@ const saveExit = (exitCode: number, signal?: number) => { child.on('error', () => saveExit(127)); child.on('exit', (code, signal) => saveExit(code ?? 0, signal ? constants.signals[signal] : undefined)); let lastProbe = -Infinity; +let reported = false; const poll = setInterval(() => { // An orphaned guard owns its group, never the former parent's possibly recycled PID. if (process.ppid !== parent) { rmSync(directory, {recursive: true, force: true}); process.kill(0, 'SIGKILL'); } @@ -49,4 +50,9 @@ const poll = setInterval(() => { return Number(pgid) === process.pid && Number(pid) !== process.pid && Number(ppid) !== process.pid && !state?.startsWith('Z'); }); if (!members) { clearInterval(poll); process.exit(0); } + // Say once that members remain, so the supervisor reports them now instead of waiting for this guard to exit. + if (!reported) { + reported = true; + try { writeFileSync(`${directory}/left-next`, '1', {mode: 0o600}); renameSync(`${directory}/left-next`, `${directory}/left`); } catch { /* The supervisor's bound applies. */ } + } }, 25); diff --git a/tests/backgroundSupervisor.test.ts b/tests/backgroundSupervisor.test.ts index dbcd13c1..a61c30c9 100644 --- a/tests/backgroundSupervisor.test.ts +++ b/tests/backgroundSupervisor.test.ts @@ -109,6 +109,18 @@ test('a pipeline stops as a whole; a detached helper it started (like the adb se }); }); +test('a task that leaves nothing behind is never reported as having processes left, even for a moment; it is removable once exited', async () => { + await withSupervisor(async (supervisor, cwd, changes) => { + for (let run = 0; run < 5; run += 1) { + const {task} = supervisor.start({command: `printf 'done ${run}\\n'`, cwd, shell: SHELL, env: env()}) as {ok: true; task: TaskRecord}; + await waitFor(() => supervisor.get(task.id)!.state === 'exited', 'exited'); + assert.deepEqual(changes.filter(change => change.id === task.id && change.leftovers).map(change => change.state), [], + 'the guard still being alive is not a process left behind'); + assert.equal(supervisor.remove(task.id), true, 'exited with nothing left: removable at once'); + } + }); +}); + test('processes left in the group after the leader exits are reported, and killed only on request', async () => { await withSupervisor(async (supervisor, cwd) => { const {task} = supervisor.start({command: afterSleeperReady(cwd, '60000', 'sleep 1; exit 0'), cwd, shell: SHELL, env: env()}) as {ok: true; task: TaskRecord}; From 9d046cce8a78cb83edc56c33220702c162fe5556 Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:04:37 +0530 Subject: [PATCH 2/8] Configured completion: its private directory is gone when a startup timeout returns configuredCompletion.test.ts:89 (Ubuntu CI on #417) found the helper's private root still present right after a startup timeout. At that moment two parties remove it (NMSh's reset and the helper's own EXIT trap `rm -rf`) while a helper shell may still write its pid file into it. rmSync then threw ENOTEMPTY (macOS, 39 of 300 rounds in a reproduction) or, on Linux with Node 26, returned normally with the directory still there (24 of 300), and a throw from reset escaped a timer. removeDirectoryNow renames the directory into a fresh sibling first (atomic, same filesystem), so no path to it remains for a late write or a concurrent remover, then removes the renamed copy in a few bounded passes (GNU rm works through directory descriptors and can follow the rename) and never throws. The new test races a writer-and-remover against it 120 times; plain rmSync fails it on both platforms. Refs #410 --- src/shell/ConfiguredCompletion.ts | 4 +++- src/util/removeDirectory.ts | 27 +++++++++++++++++++++++ tests/removeDirectory.test.ts | 36 +++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 src/util/removeDirectory.ts create mode 100644 tests/removeDirectory.test.ts diff --git a/src/shell/ConfiguredCompletion.ts b/src/shell/ConfiguredCompletion.ts index 582f5ea7..ecedeef1 100644 --- a/src/shell/ConfiguredCompletion.ts +++ b/src/shell/ConfiguredCompletion.ts @@ -4,6 +4,7 @@ import {mkdtempSync, writeFileSync, readFileSync, rmSync, existsSync, statSync} import {tmpdir} from 'node:os'; import {dirname, join} from 'node:path'; import {fileURLToPath} from 'node:url'; +import {removeDirectoryNow} from '../util/removeDirectory.js'; import {commandIdentity, completionLabel, type CompletionCandidate, type CompletionContext, type CompletionSource} from './completion.js'; const MAX_BYTES = 1024 * 1024; @@ -124,7 +125,8 @@ export class ConfiguredCompletionSource implements CompletionSource { } this.wait?.resolve(false); this.wait = undefined; - if (this.root) rmSync(this.root, {recursive: true, force: true}); + // The helper's own cleanup may be removing it, and a shell may still write its pid file, at this moment. + if (this.root) removeDirectoryNow(this.root); this.root = undefined; this.output = ''; } diff --git a/src/util/removeDirectory.ts b/src/util/removeDirectory.ts new file mode 100644 index 00000000..a5cf4175 --- /dev/null +++ b/src/util/removeDirectory.ts @@ -0,0 +1,27 @@ +import {existsSync, mkdtempSync, renameSync, rmSync} from 'node:fs'; +import {basename, dirname, join} from 'node:path'; + +/** + * Remove a private directory that helper processes may still be writing into or removing themselves. `rmSync` alone is + * not enough then: a file created during its walk made it throw ENOTEMPTY, and on Linux a concurrent `rm -rf` made it + * return with the directory still there. Renaming the directory away first (atomic, same filesystem) leaves no path to + * it for anyone to write into, so the path is gone when this returns; the renamed copy is then removed. Never throws. + */ +export function removeDirectoryNow(path: string): void { + let grave: string | undefined; + try { + grave = mkdtempSync(join(dirname(path), `${basename(path)}-gone-`)); + renameSync(path, join(grave, 'directory')); + } catch { /* Already gone, or not movable: removed where it is below. */ } + for (const target of grave ? [grave, path] : [path]) removeAll(target); +} + +function removeAll(target: string): void { + // A remover that already holds the directory open (`rm -rf` works through directory descriptors, so it follows the + // rename) can make one rmSync stop early without an error; another pass finishes what is left. + for (let pass = 0; pass < 5; pass += 1) { + try { rmSync(target, {recursive: true, force: true, maxRetries: 3, retryDelay: 5}); } catch { /* Another pass. */ } + if (!existsSync(target)) return; + } + setTimeout(() => { try { rmSync(target, {recursive: true, force: true}); } catch { /* Best effort. */ } }, 100).unref(); +} diff --git a/tests/removeDirectory.test.ts b/tests/removeDirectory.test.ts new file mode 100644 index 00000000..a3882779 --- /dev/null +++ b/tests/removeDirectory.test.ts @@ -0,0 +1,36 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import {spawn} from 'node:child_process'; +import {once} from 'node:events'; +import {existsSync, mkdtempSync, readdirSync, realpathSync, rmSync, writeFileSync} from 'node:fs'; +import {tmpdir} from 'node:os'; +import {join} from 'node:path'; +import {removeDirectoryNow} from '../src/util/removeDirectory.js'; + +// The configured-completion helper's private directory (#417 CI): its shells may write a pid file and its own cleanup +// may `rm -rf` the directory while NMSh removes it. Plain rmSync then threw ENOTEMPTY (macOS) or returned with the +// directory still present (Linux) in roughly one round in ten. +test('a directory another process is writing into and removing is gone when removeDirectoryNow returns', async () => { + const base = realpathSync(mkdtempSync(join(tmpdir(), 'nmsh-rmnow-'))); + try { + for (let round = 0; round < 120; round += 1) { + const root = mkdtempSync(join(base, 'r-')); + for (let index = 0; index < 40; index += 1) writeFileSync(join(root, `f${index}`), 'x'); + const helper = spawn('/bin/sh', ['-c', `i=0; while [ $i -lt 10 ]; do : > '${root}/w'$i 2>/dev/null; i=$((i+1)); done; rm -rf '${root}'`]); + await new Promise(resolve => setTimeout(resolve, round % 5)); + removeDirectoryNow(root); + assert.equal(existsSync(root), false, `round ${round}: the directory is gone`); + if (helper.exitCode === null && helper.signalCode === null) await once(helper, 'close'); + } + assert.deepEqual(readdirSync(base), [], 'no renamed copy is left behind'); + } finally { rmSync(base, {recursive: true, force: true}); } +}); + +test('removeDirectoryNow: a missing directory is fine; nothing outside it is touched', () => { + const base = realpathSync(mkdtempSync(join(tmpdir(), 'nmsh-rmnow-'))); + try { + writeFileSync(join(base, 'keep'), 'x'); + removeDirectoryNow(join(base, 'missing')); + assert.deepEqual(readdirSync(base), ['keep']); + } finally { rmSync(base, {recursive: true, force: true}); } +}); From cd5fafccee03d5cc4fc56d0f2703f0c7a26e248a Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:04:37 +0530 Subject: [PATCH 3/8] Test runner: a test that stalls with a process running fails visibly instead of hanging the run #415's Ubuntu shard sat silent for six minutes until the job limit, with a node test worker and a `fish` still running. The reporter flushes a file's results when the file completes; output stopped after shellAdapters, so the next file, shellContextEnvironment, never completed. A test that passes its timeout is cancelled, but nothing ends what it started: a live shell PTY (there, `start()` and its prompt waits sit outside the try/finally that kills the shell) keeps the file's process alive, its failure is never printed, and the run waits for the CI limit. 121 test files start live sessions this way, so the fix is in the runner. Each test file now preloads test-file-teardown.mjs: once the file's tests are done, an unref'd timer gives the file's own cleanup 15 s; if the process is still alive then, every process it left running (its own process groups, never the runner's) is ended and named on stderr, so the stall surfaces as the test failure it is. The new runner test stalls a file with a piped child: it now ends in about a second with a failure and no child left; without the preload it waits for the child. Refs #410 --- scripts/test-file-teardown.mjs | 49 ++++++++++++++++++++++++++++++++++ scripts/test.mjs | 4 ++- tests/tmpdirHygiene.test.ts | 25 +++++++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 scripts/test-file-teardown.mjs diff --git a/scripts/test-file-teardown.mjs b/scripts/test-file-teardown.mjs new file mode 100644 index 00000000..422ab8a2 --- /dev/null +++ b/scripts/test-file-teardown.mjs @@ -0,0 +1,49 @@ +import {after} from 'node:test'; +import {spawnSync} from 'node:child_process'; + +/** + * Preloaded into every test file's process by scripts/test.mjs. A test that stalls past its timeout is cancelled, but + * whatever it started keeps running: a live shell PTY or a piped child then keeps the file's process alive, the file + * never finishes, its failure is never printed, and a CI job waits for its own time limit (#415's Ubuntu shard). + * Once the file's tests are done, a timer that cannot itself keep the process alive gives the file's own cleanup time + * (NMSH_TEST_TEARDOWN_MS, default 15 s); if the process is still alive then, every process the file left running is + * ended and named, so the stall is reported instead of hanging the run. + */ +if (process.env.NODE_TEST_CONTEXT) { + after(() => { + const delay = Number(process.env.NMSH_TEST_TEARDOWN_MS) || 15_000; + setTimeout(endLeftovers, delay).unref(); + }); +} + +export function endLeftovers() { + const rows = processTable(); + const own = rows.find(row => row.pid === process.pid)?.group; + const left = descendants(rows, process.pid).filter(row => !/(?:^|\/)(?:esbuild|ps)$/u.test(row.command)); + for (const row of left) { + // A leader of its own group (a PTY shell, a detached helper) takes its whole group with it; never our own group. + const target = row.group === row.pid && row.group !== own ? -row.group : row.pid; + try { process.kill(target, 'SIGKILL'); } catch { /* Already gone. */ } + } + if (left.length) process.stderr.write(`test teardown: ended ${left.length} process${left.length === 1 ? '' : 'es'} this file left running: ${left.map(row => `${row.pid} ${row.command}`).join(', ')}\n`); + return left; +} + +function processTable() { + const result = spawnSync('ps', ['-A', '-o', 'pid=,ppid=,pgid=,comm='], {encoding: 'utf8', timeout: 5000, maxBuffer: 8 * 1024 * 1024}); + if (result.status !== 0 || !result.stdout) return []; + return result.stdout.trim().split('\n').map(line => { + const [pid, parent, group, ...command] = line.trim().split(/\s+/u); + return {pid: Number(pid), parent: Number(parent), group: Number(group), command: command.join(' ')}; + }); +} + +function descendants(rows, root) { + const found = []; + const queue = [root]; + while (queue.length) { + const parent = queue.shift(); + for (const row of rows) if (row.parent === parent && row.pid !== root) { found.push(row); queue.push(row.pid); } + } + return found; +} diff --git a/scripts/test.mjs b/scripts/test.mjs index 36f564a2..902711d0 100644 --- a/scripts/test.mjs +++ b/scripts/test.mjs @@ -27,7 +27,9 @@ export async function runTestFiles(files, args = [], {cwd = process.cwd(), stdio // PTY workers also own shell/helper processes. Bound fan-out on larger // development hosts; explicit Node --test-concurrency options still win. const concurrency = testConcurrency(process.env.NMSH_TEST_CONCURRENCY, availableParallelism()); - const child = spawn(process.execPath, ['--import=tsx', '--test', `--test-concurrency=${concurrency}`, ...args, ...files], { + // Each file's process ends what it left running once its tests are done (a stalled test cannot hang the run). + const teardown = new URL('./test-file-teardown.mjs', import.meta.url).href; + const child = spawn(process.execPath, ['--import=tsx', `--import=${teardown}`, '--test', `--test-concurrency=${concurrency}`, ...args, ...files], { cwd, stdio, env, }); let interrupted = false; diff --git a/tests/tmpdirHygiene.test.ts b/tests/tmpdirHygiene.test.ts index 4a4c808e..58ff75b3 100644 --- a/tests/tmpdirHygiene.test.ts +++ b/tests/tmpdirHygiene.test.ts @@ -62,6 +62,31 @@ test('suite runner uses short roots, removes only its own root, and reports norm } finally { rmSync(fixture, {recursive: true, force: true}); } }); +test('a test file that stalls with a child still running finishes, reports the stall, and leaves nothing running (#415 CI)', async () => { + const fixture = mkdtempSync(join(tmpdir(), 'runner-')); + try { + const pidFile = join(fixture, 'child.pid'); + const stalled = join(fixture, 'stalled.test.mjs'); + // A piped child keeps the file's process alive after its test is cancelled: without teardown the run never ends. + writeFileSync(stalled, `import test from 'node:test'; import {spawn} from 'node:child_process'; import {writeFileSync} from 'node:fs'; +test('stalls past its timeout', {timeout: 300}, async () => { + const child = spawn('sleep', ['120'], {stdio: ['ignore', 'pipe', 'pipe']}); + writeFileSync(${JSON.stringify(pidFile)}, String(child.pid)); + await new Promise(() => {}); +});\n`); + const started = Date.now(); + const previous = process.env.NMSH_TEST_TEARDOWN_MS; + process.env.NMSH_TEST_TEARDOWN_MS = '500'; + let result; + try { result = await runTestFiles([stalled], [], {stdio: 'ignore'}); } + finally { if (previous === undefined) delete process.env.NMSH_TEST_TEARDOWN_MS; else process.env.NMSH_TEST_TEARDOWN_MS = previous; } + assert.equal(result.code, 1, 'the cancelled test is reported as a failure'); + assert.ok(Date.now() - started < 20_000, 'the run ends instead of waiting for the child'); + const pid = Number(fs.readFileSync(pidFile, 'utf8')); + assert.throws(() => process.kill(pid, 0), 'the child it left running was ended'); + } finally { rmSync(fixture, {recursive: true, force: true}); } +}); + test('sandbox disposal waits for an untracked late writer whose cwd still owns the sandbox', async () => { const sandbox = new LiveSandbox(); const child = spawn(process.execPath, ['-e', "setTimeout(() => require('node:fs').writeFileSync('late-journal.txt', 'done'), 150)"], From 25759cea376845d51afaddf7b3692f171031515a Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:05:36 +0530 Subject: [PATCH 4/8] Native capture test: bound the early-reap window, not the owner's cold start "native cancellation reaps a PTY before it can process the initialization command" failed once in a local verify:release (passes alone, and 40 of 40 under 10x parallel isolated runs): its 3 s wait for the inner shell also covered the owner process's cold `node --import=tsx` start compiling CompletionService's imports, which a full parallel suite can stretch past 3 s. The owner now marks when it is loaded; that gets a generous bound, and the 3 s bound starts there. Not reproduced in isolation; the full release run is the check. Refs #410 --- tests/nativeCaptureLifecycle.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/nativeCaptureLifecycle.test.ts b/tests/nativeCaptureLifecycle.test.ts index c6ff5207..475d29b7 100644 --- a/tests/nativeCaptureLifecycle.test.ts +++ b/tests/nativeCaptureLifecycle.test.ts @@ -55,7 +55,8 @@ test('native cancellation reaps a PTY before it can process the initialization c // The inner shell never reads the queued command that formerly recorded its PID. const module = new URL('../src/shell/CompletionService.ts', import.meta.url).href; const program = `import {NativeCompletionSource} from ${JSON.stringify(module)}; - import {existsSync} from 'node:fs'; + import {existsSync, writeFileSync} from 'node:fs'; + writeFileSync(${JSON.stringify(join(root, 'owner-ready'))}, ''); const controller = new AbortController(); const request = new NativeCompletionSource().query({buffer:'g',cwd:${JSON.stringify(root)}},controller.signal); while (!existsSync(${JSON.stringify(join(root, 'early-pid'))})) await new Promise(r=>setTimeout(r,5)); @@ -69,6 +70,9 @@ test('native cancellation reaps a PTY before it can process the initialization c }); let inner = 0; try { + // A cold `node --import=tsx` compiling CompletionService's imports is not what this test bounds; under a full parallel + // run it alone took more than three seconds. The tight bounds below start once the owner is running. + await until(() => existsSync(join(root, 'owner-ready')) || owner.exitCode !== null, 30_000, 'the owner process to load'); await until(() => { inner = publishedPid(join(root, 'early-pid')) ?? 0; return inner > 1; }, 3000, 'inner before initialization'); await until(() => owner.exitCode !== null || owner.signalCode !== null, 2000, 'cancelled owner'); assert.equal(owner.exitCode, 0); From 8f57e6aca4cadb4ea51d09c2f0b03799892bfa16 Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 18:22:47 +0530 Subject: [PATCH 5/8] removeDirectoryNow: remove the renamed copy entry by entry while another remover still works in it CI on #422 (Ubuntu) left a renamed copy behind after the five rmSync passes: GNU `rm -rf` works through directory descriptors, so the helper's own cleanup keeps deleting inside the renamed directory, and Node's rmSync stops early (no error) each time an entry vanishes under it. The path itself was gone every round. The copy is now removed pass by pass: list it, remove each entry tolerating ones that vanished, then remove the directory, retrying while it is not empty. Nothing can add entries to it (its path is gone), so every pass shrinks it; a deferred removal remains only as a last resort. Passes 3/3 on macOS and 5/5 on Linux. Refs #410 --- src/util/removeDirectory.ts | 41 +++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/util/removeDirectory.ts b/src/util/removeDirectory.ts index a5cf4175..f49454fb 100644 --- a/src/util/removeDirectory.ts +++ b/src/util/removeDirectory.ts @@ -1,4 +1,4 @@ -import {existsSync, mkdtempSync, renameSync, rmSync} from 'node:fs'; +import {mkdtempSync, readdirSync, renameSync, rmdirSync, rmSync} from 'node:fs'; import {basename, dirname, join} from 'node:path'; /** @@ -13,15 +13,38 @@ export function removeDirectoryNow(path: string): void { grave = mkdtempSync(join(dirname(path), `${basename(path)}-gone-`)); renameSync(path, join(grave, 'directory')); } catch { /* Already gone, or not movable: removed where it is below. */ } - for (const target of grave ? [grave, path] : [path]) removeAll(target); + for (const target of grave ? [grave, path] : [path]) { + if (!removeTree(target)) { + setTimeout(() => { try { rmSync(target, {recursive: true, force: true}); } catch { /* Best effort. */ } }, 100).unref(); + } + } } -function removeAll(target: string): void { - // A remover that already holds the directory open (`rm -rf` works through directory descriptors, so it follows the - // rename) can make one rmSync stop early without an error; another pass finishes what is left. - for (let pass = 0; pass < 5; pass += 1) { - try { rmSync(target, {recursive: true, force: true, maxRetries: 3, retryDelay: 5}); } catch { /* Another pass. */ } - if (!existsSync(target)) return; +/** + * Remove a directory tree while another remover may be deleting inside it. A remover that already holds the directory + * open (`rm -rf` works through directory descriptors, so it follows a rename) makes entries vanish mid-walk, and one + * rmSync then stops early without an error. Each pass removes what is listed, tolerating what vanished, and retries + * the directory itself while it is not empty; nothing can add entries (its path is gone), so every pass shrinks it. + */ +function removeTree(target: string): boolean { + for (let pass = 0; pass < 50; pass += 1) { + let names: string[]; + try { names = readdirSync(target); } + catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT') return true; + if (code !== 'ENOTDIR') return false; + try { rmSync(target, {force: true}); return true; } catch { return false; } + } + for (const name of names) { + try { rmSync(join(target, name), {recursive: true, force: true}); } catch { /* Vanished or retried next pass. */ } + } + try { rmdirSync(target); return true; } + catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT') return true; + if (code !== 'ENOTEMPTY' && code !== 'EEXIST' && code !== 'EBUSY') return false; + } } - setTimeout(() => { try { rmSync(target, {recursive: true, force: true}); } catch { /* Best effort. */ } }, 100).unref(); + return false; } From c3500577810c91fce60ba54578417e200bb8a72e Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 18:54:09 +0530 Subject: [PATCH 6/8] Codex adapter test: wait for what the fake server has recorded, not the moment the turn settles Failed once on dev (dispatched CI, Ubuntu): `client.length` 5, expected 6. The fake server records a client message when it reads it. When its request 101 reached the client after the person had answered 100, the client's refusal of 101 was written after that answer; the server completed the turn first and the test read the log at `settled`, one message short. Not a product defect: both responses are sent. The fixture now sends 101 a moment later, so this order happens every run (the old test then fails 3 of 3), and the helper waits (bounded) until the messages the test asserts are on record. 5 of 5 runs pass. Refs #410 --- tests/codexAdapter.test.ts | 14 +++++++++++--- tests/fixtures/fake-codex-app-server.mjs | 4 +++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/codexAdapter.test.ts b/tests/codexAdapter.test.ts index ef005ce0..89639216 100644 --- a/tests/codexAdapter.test.ts +++ b/tests/codexAdapter.test.ts @@ -33,7 +33,13 @@ test('messages: items, approvals and turn results normalize from structured fiel } }); -async function session(scenario: string, act: (session: CodexSession, events: AgentEvent[], wait: (kind: AgentEvent['kind'], count?: number) => Promise) => Promise) { +/** + * Runs one scenario and returns what the client sent, as the fake server recorded it. The server records a message when + * it reads it, which can be after the turn settles (a refusal written after the person's answer), so `recorded` says + * what the test needs on record; it is waited for (bounded) rather than read at whatever moment the turn settled. + */ +async function session(scenario: string, act: (session: CodexSession, events: AgentEvent[], wait: (kind: AgentEvent['kind'], count?: number) => Promise) => Promise, + recorded: (client: Array>) => boolean = () => true) { const directory = await mkdtemp(join(tmpdir(), 'nmsh-codex-')); const log = join(directory, 'client.log'); const events: AgentEvent[] = []; @@ -50,7 +56,9 @@ async function session(scenario: string, act: (session: CodexSession, events: Ag try { assert.deepEqual(codex.start(), {ok: true}); await act(codex, events, wait); - const client = (await readFile(log, 'utf8').catch(() => '')).trim().split('\n').filter(Boolean).map(line => JSON.parse(line) as Record); + const read = async () => (await readFile(log, 'utf8').catch(() => '')).trim().split('\n').filter(Boolean).map(line => JSON.parse(line) as Record); + let client = await read(); + for (const deadline = Date.now() + 5000; !recorded(client) && Date.now() < deadline; client = await read()) await new Promise(resolve => setTimeout(resolve, 20)); return client; } finally { codex.close(); @@ -78,7 +86,7 @@ test('a session: handshake, thread, a message, one approval answered by the pers ]); assert.deepEqual(events.filter(event => event.kind === 'assistant'), [{kind: 'assistant', text: 'Cleaned.'}]); assert.deepEqual(events.at(-1), {kind: 'settled', ok: true}); - }); + }, client => client.some(message => message.id === 100) && client.some(message => message.id === 101)); // The two responses race (the refusal is immediate, the approval waits for the person), so only the first four are ordered. assert.deepEqual(client.slice(0, 4).map(message => message.method), ['initialize', 'initialized', 'thread/start', 'turn/start']); assert.equal(client.length, 6); diff --git a/tests/fixtures/fake-codex-app-server.mjs b/tests/fixtures/fake-codex-app-server.mjs index baa1dc6b..bb012c0a 100644 --- a/tests/fixtures/fake-codex-app-server.mjs +++ b/tests/fixtures/fake-codex-app-server.mjs @@ -37,7 +37,9 @@ createInterface({input: process.stdin}).on('line', line => { if (scenario === 'fail') return send({method: 'turn/completed', params: {threadId: 'thr_1', turn: {id: 'turn_1', status: 'failed', error: {message: 'usage limit reached\nsecond line'}, items: []}}}); send({method: 'item/started', params: {threadId: 'thr_1', turnId: 'turn_1', startedAtMs: 1, item: {type: 'commandExecution', id: 'item_1', command: 'rm -rf build', commandActions: [], cwd: '/tmp', status: 'inProgress'}}}); send({id: 100, method: 'item/commandExecution/requestApproval', params: {threadId: 'thr_1', turnId: 'turn_1', itemId: 'item_1', startedAtMs: 1, command: 'rm -rf build', cwd: '/tmp', reason: 'cleans the build'}}); - send({id: 101, method: 'item/tool/requestUserInput', params: {threadId: 'thr_1', turnId: 'turn_1', itemId: 'item_9', questions: []}}); + // A moment later, as a separate read: the person may already have answered 100, so the client's refusal of 101 can + // reach this server after that answer (and after the turn completes). Tests must wait for what they assert. + setTimeout(() => send({id: 101, method: 'item/tool/requestUserInput', params: {threadId: 'thr_1', turnId: 'turn_1', itemId: 'item_9', questions: []}}), 30); return; } if (id === 100) { From 50cdf02675cd02954652ac856016aa7eb17d1a32 Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 18:57:23 +0530 Subject: [PATCH 7/8] Native capture tests: the owner's cold start is waited for before the capture windows begin Two more tests in this file failed in a local verify:release run with the load average at 35-67 (the machine shared with other workloads): their 3 s windows began at spawn, and the owner process had not finished its cold `node --import=tsx` start ("loadedAfterMs": null). Like 25759cea, they now wait (bounded, 30 s) for the owner to load, and their tight windows start there. 9/9 pass, three runs. Refs #410 --- tests/nativeCaptureLifecycle.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/nativeCaptureLifecycle.test.ts b/tests/nativeCaptureLifecycle.test.ts index 475d29b7..1d186cb6 100644 --- a/tests/nativeCaptureLifecycle.test.ts +++ b/tests/nativeCaptureLifecycle.test.ts @@ -246,6 +246,9 @@ for (const configured of [false, true]) test(`${configured ? 'configured' : 'nat owner.stderr!.on('data', data => { ownerError = (ownerError + String(data)).slice(-4096); }); let inner = 0; try { + // The owner's cold `node --import=tsx` start is not what this test bounds (under a full parallel run it alone can + // take longer than the capture's own window); the bounds below start once it has loaded. + await until(() => loadedAt !== undefined || owner.exitCode !== null, 30_000, 'the owner process to load'); const identity = await publishedCapture(root, prefix); const transport = identity.transport; inner = identity.pid; @@ -300,12 +303,15 @@ exec /bin/zsh "$@" chmodSync(join(bin, 'zsh'), 0o700); const module = new URL('../src/shell/CompletionService.ts', import.meta.url).href; const program = `import {NativeCompletionSource} from ${JSON.stringify(module)}; +import {writeFileSync} from 'node:fs'; +writeFileSync(${JSON.stringify(join(root, 'owner-ready'))}, ''); await new NativeCompletionSource().query({buffer:'g',cwd:${JSON.stringify(root)}},new AbortController().signal);`; const owner = spawn(process.execPath, ['--import=tsx', '--input-type=module', '-e', program], { env: {...process.env, HOME: root, FPATH: root, TMPDIR: root, PATH: `${bin}:${process.env.PATH}`}, stdio: 'ignore', detached: true, }); let inner = 0; try { + await until(() => existsSync(join(root, 'owner-ready')) || owner.exitCode !== null, 30_000, 'the owner process to load'); await until(() => existsSync(marker), 3000, 'controlled PID rewrite'); inner = Number(readFileSync(join(root, 'inner-pid'), 'utf8')); assert.ok(Number.isSafeInteger(inner) && inner > 1); From 3056d8ec6e7953f72321550efd0e8639ead19cbd Mon Sep 17 00:00:00 2001 From: raiseCatError <315733358+raiseCatError@users.noreply.github.com> Date: Sun, 18 Oct 2026 19:19:57 +0530 Subject: [PATCH 8/8] Two timing-bound tests stop measuring cold starts they do not test From the last local verify:release (3,138 of 3,142 passing, 2 skipped): two tests in code v0.21 does not change failed under load and pass alone (5/5). - directoryNavigation: the fake zoxide was a Node script whose cold start can exceed the zoxide query's 1.5 s limit, so the service (rightly) fell back to Native. It is now a shell script, quick to start like the real native binary. - configuredCompletion 'bounded to 4096 records': it tests the record cap, not startup; its helper now gets the full startup cap (5 s) so a loaded machine's compinit does not turn into an empty result. Refs #410 --- tests/configuredCompletion.test.ts | 4 +++- tests/directoryNavigation.test.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/configuredCompletion.test.ts b/tests/configuredCompletion.test.ts index 73cd07c0..639b1460 100644 --- a/tests/configuredCompletion.test.ts +++ b/tests/configuredCompletion.test.ts @@ -10,7 +10,9 @@ import {CompletionService} from '../src/shell/CompletionService.js'; test('real configured candidate capture is bounded to 4096 records', async () => { const home = mkdtempSync(join(tmpdir(), 'nmsh-large-completion-')); writeFileSync(join(home, '.zshrc'), 'autoload -Uz compinit\ncompinit -D\n_large() { compadd -J large -- value{1..5000}; }\ncompdef _large large\n'); - const source = new ConfiguredCompletionSource({env: {...process.env, HOME: home}, startupMs: 4000, queryMs: 2000}); + // This bounds the records, not startup: the helper's own startup cap (5 s) and a long query window keep a loaded + // machine's compinit from turning into an empty result. + const source = new ConfiguredCompletionSource({env: {...process.env, HOME: home}, startupMs: 5000, queryMs: 2000}); try { const values = await source.query({buffer: 'large v', cwd: home}, new AbortController().signal); assert.equal(values.length, 4096); diff --git a/tests/directoryNavigation.test.ts b/tests/directoryNavigation.test.ts index d34635d2..d9d1b33f 100644 --- a/tests/directoryNavigation.test.ts +++ b/tests/directoryNavigation.test.ts @@ -46,7 +46,9 @@ test('zoxide query uses a temporary copy, caches results and falls back on missi try { await writeFile(join(directory, 'db.zo'), 'original', {mode: 0o600}); const binary = join(directory, 'zoxide'); - await writeFile(binary, `#!${process.execPath}\nconst fs=require('node:fs');if(process.env._ZO_DATA_DIR===process.env.ORIGINAL_DB)process.exit(2);fs.writeFileSync(process.env._ZO_DATA_DIR+'/db.zo','sorted');fs.appendFileSync(process.env.CALLS,JSON.stringify(process.argv.slice(2))+'\\n');process.stdout.write('20.5 /work/one\\n 4.0 /work/two spaces\\n');\n`, {mode: 0o700}); + // A shell script, like the real (native) zoxide quick to start: a Node stand-in's cold start could exceed the query's + // 1.5 s limit on a loaded machine, and the service then (rightly) fell back to Native. + await writeFile(binary, `#!/bin/sh\n[ "$_ZO_DATA_DIR" = "$ORIGINAL_DB" ] && exit 2\nprintf sorted > "$_ZO_DATA_DIR/db.zo"\nprintf '["%s","%s","%s","%s"]\\n' "$1" "$2" "$3" "$4" >> "$CALLS"\nprintf '20.5 /work/one\\n 4.0 /work/two spaces\\n'\n`, {mode: 0o700}); const calls = join(directory, 'calls'); const service = new DirectoryService({...process.env, PATH: directory, _ZO_DATA_DIR: directory, ORIGINAL_DB: directory, CALLS: calls}); assert.equal((await service.query([], '', 'zoxide'))[0]?.path, '/work/one');