From 78d8a531a04e60f54b49b4a63d7374359523b81d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 27 Jul 2026 19:36:05 -0400 Subject: [PATCH 1/4] fix: wait for the TUI to exit before removing its temp dir (#1801) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `done()` sent SIGTERM and then immediately `rmSync`'d the work dir, which doubles as the TUI's HOME. Nothing waited for the child, so the Ink process could still be writing into the dir while rmSync walked it — a file landing after a directory was read but before it was removed surfaces as ENOTEMPTY on macOS, failing `smoke:tui` after a successful render. Removal now happens only once the child's `exit` event fires: SIGTERM, then SIGKILL after SMOKE_TUI_EXIT_GRACE_MS (default 5s), then clean up regardless after twice that. The crash-before-render path short-circuits via a `childExited` flag set by the pre-registered exit handler. cleanup() also warns instead of throwing, so a leftover temp dir can never fail a passing smoke. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq --- scripts/smoke-tui.mjs | 52 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index 0f241d243..1360609a0 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -105,16 +105,26 @@ const child = spawn( let output = ""; let settled = false; +let childExited = false; + +// How long to let the TUI wind down after SIGTERM before escalating to SIGKILL, +// and then before giving up on the exit event entirely. +const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000); function cleanup() { - rmSync(work, { recursive: true, force: true }); + try { + rmSync(work, { recursive: true, force: true }); + } catch (err) { + // Never turn a passing smoke into a failure over a leftover temp dir; the + // OS reclaims tmpdir anyway. This should not fire now that removal waits + // for the child to exit (see finish()), so say so loudly if it does. + console.warn( + `smoke:tui — could not remove temp dir ${work}: ${err.message}`, + ); + } } -function done(code, message) { - if (settled) return; - settled = true; - clearTimeout(timer); - if (!child.killed) child.kill("SIGTERM"); +function finish(code, message) { cleanup(); if (code === 0) { console.log(`smoke:tui OK — ${message}`); @@ -124,6 +134,33 @@ function done(code, message) { process.exit(code); } +function done(code, message) { + if (settled) return; + settled = true; + clearTimeout(timer); + if (childExited) { + finish(code, message); + return; + } + // Wait for the child to actually exit before removing the work dir (#1801). + // The dir doubles as the TUI's HOME, so the Ink process is still writing into + // it when we signal; an rmSync racing those writes fails with ENOTEMPTY when + // a file lands after a directory has been read but before it is removed. + const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS); + const giveUp = setTimeout(() => { + console.warn( + `smoke:tui — TUI did not exit within ${EXIT_GRACE_MS * 2}ms of SIGTERM; cleaning up anyway`, + ); + finish(code, message); + }, EXIT_GRACE_MS * 2); + child.once("exit", () => { + clearTimeout(forceKill); + clearTimeout(giveUp); + finish(code, message); + }); + if (!child.killed) child.kill("SIGTERM"); +} + function onData(chunk) { output += chunk.toString(); if (output.includes(RENDER_MARKER)) { @@ -135,6 +172,9 @@ child.stdout.on("data", onData); child.stderr.on("data", onData); child.on("exit", (code) => { + // Registered before done()'s own one-shot listener, so this always runs first + // — done() can trust `childExited` when it is called from inside this handler. + childExited = true; if (settled) return; // Exiting before the render marker appeared is a failure (crash on boot). done( From 20456f841b3d1d4ba983293fc41f863c48c962f3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 27 Jul 2026 19:52:53 -0400 Subject: [PATCH 2/4] fix: wait on close, not exit, so spawn failure resolves instantly (review #1814) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed spawn emits `error` + `close` and never `exit`, so the previous `child.once("exit")` wait never fired on that path: done() sent SIGTERM to a process that never existed and resolved only via the give-up timer — 10s at the default grace, with a warning blaming a SIGTERM the child never received. Waiting on `close` covers both normal termination and spawn failure, and additionally guarantees the stdio pipes are drained. The short-circuit gate moves with it, `childExited` → `childClosed`: keying it on `exit` would have skipped the drain on the crash-before-render path, which is exactly where the quoted output matters. Messages that quote that output are now thunks rendered in finish(), after the drain, instead of being sliced at call time. SIGTERM is now sent only to a child that is still running, since on both the crash and spawn-failure paths there is nothing left to signal. finish() also gets a `finished` guard so its single-entry invariant is local rather than inferred from process.exit() being synchronous. Measured, spawn failure (grace shortened to 500ms): 1.04s + spurious warning before, 0.02s and silent after. Crash-before-render now prints the child's stderr detail line that the pre-drain slice could truncate. Happy path 3x, timeout path, and full `npm run smoke` + `npm run validate` all green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq --- scripts/smoke-tui.mjs | 47 ++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index 1360609a0..b3b158342 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -105,10 +105,10 @@ const child = spawn( let output = ""; let settled = false; -let childExited = false; +let childClosed = false; // How long to let the TUI wind down after SIGTERM before escalating to SIGKILL, -// and then before giving up on the exit event entirely. +// and then before giving up on the close event entirely. const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000); function cleanup() { @@ -124,12 +124,20 @@ function cleanup() { } } +// `message` may be a string or a thunk. Callers that quote the child's output +// pass a thunk so it is rendered here — after the wait below, by which point +// `close` guarantees stdout/stderr have been fully drained. Building the string +// at call time instead can truncate the crash reason that matters most. +let finished = false; function finish(code, message) { + if (finished) return; + finished = true; cleanup(); + const text = typeof message === "function" ? message() : message; if (code === 0) { - console.log(`smoke:tui OK — ${message}`); + console.log(`smoke:tui OK — ${text}`); } else { - console.error(`smoke:tui FAILED — ${message}`); + console.error(`smoke:tui FAILED — ${text}`); } process.exit(code); } @@ -138,7 +146,7 @@ function done(code, message) { if (settled) return; settled = true; clearTimeout(timer); - if (childExited) { + if (childClosed) { finish(code, message); return; } @@ -146,6 +154,11 @@ function done(code, message) { // The dir doubles as the TUI's HOME, so the Ink process is still writing into // it when we signal; an rmSync racing those writes fails with ENOTEMPTY when // a file lands after a directory has been read but before it is removed. + // + // Wait on `close`, not `exit`: a *spawn failure* emits `error` + `close` and + // never `exit`, so an `exit` wait would hang here until the give-up timer and + // then blame a SIGTERM the child never received. `close` also guarantees the + // stdio pipes are drained, which is what makes the thunked messages complete. const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS); const giveUp = setTimeout(() => { console.warn( @@ -153,12 +166,17 @@ function done(code, message) { ); finish(code, message); }, EXIT_GRACE_MS * 2); - child.once("exit", () => { + child.once("close", () => { clearTimeout(forceKill); clearTimeout(giveUp); finish(code, message); }); - if (!child.killed) child.kill("SIGTERM"); + // Only signal a child that is still running: on the crash-before-render and + // spawn-failure paths there is nothing left to signal, and we are here purely + // to wait out the remaining `close`. + if (child.exitCode === null && child.signalCode === null && !child.killed) { + child.kill("SIGTERM"); + } } function onData(chunk) { @@ -171,15 +189,19 @@ function onData(chunk) { child.stdout.on("data", onData); child.stderr.on("data", onData); +// Registered before done()'s own one-shot listener, so this always runs first — +// done() can trust `childClosed` even when called from inside a close handler. +child.on("close", () => { + childClosed = true; +}); + child.on("exit", (code) => { - // Registered before done()'s own one-shot listener, so this always runs first - // — done() can trust `childExited` when it is called from inside this handler. - childExited = true; if (settled) return; // Exiting before the render marker appeared is a failure (crash on boot). done( 1, - `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${output.slice(0, 800)}`, + () => + `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${output.slice(0, 800)}`, ); }); @@ -190,6 +212,7 @@ child.on("error", (err) => { const timer = setTimeout(() => { done( 1, - `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${output.slice(0, 800)}`, + () => + `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${output.slice(0, 800)}`, ); }, TIMEOUT_MS); From 82e58020c72abb38ce810068b0bad6c03001fbb4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 27 Jul 2026 20:01:19 -0400 Subject: [PATCH 3/4] fix: bound the drain, and make the give-up warning true (review #1814 r2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ons from the second review pass: The give-up warning still said the TUI "did not exit within Xms of SIGTERM", but after switching the wait to `close` neither clause is reliably true — the timer's most plausible trigger is now a child that has already exited, and on the crash and spawn-failure paths no SIGTERM is sent at all. That is the same defect this PR fixed one round ago (a warning naming a signal never sent), so the deadline is now re-armed with a message describing what is actually being awaited. `close` is bounded by whoever holds the stdio pipes, which can outlive the direct child: a descendant that inherited them keeps it pending. That is the one real cost of `exit` → `close`, and it is moot only because the TUI spawns nothing at boot — a reason that lives entirely outside this file. Once `exit` has fired the deadline drops to DRAIN_MS, keeping the drain guarantee without inheriting an unrelated process's lifetime. Measured against a grandchild holding the pipes for 3s: 0.54s and an accurate warning, instead of 3s. The quoted output is sliced from the tail rather than the head. The thunk added last round exists to capture bytes that arrive late, which a head slice then discards as soon as output exceeds 800 chars — and on the timeout path the last frame is what diagnoses a stuck render, not the first. Verified: a 2KB stderr crash now reports its final line. The sibling scripts keep `slice(0, 800)`; changing them is a deliberate sweep, not a drive-by here. Also corrected the cleanup() comment — the wait lives in done(), and it is for `close`, not `exit`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq --- scripts/smoke-tui.mjs | 49 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index b3b158342..a2c32c80c 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -105,11 +105,19 @@ const child = spawn( let output = ""; let settled = false; +let childExited = false; let childClosed = false; // How long to let the TUI wind down after SIGTERM before escalating to SIGKILL, // and then before giving up on the close event entirely. const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000); +// Once the child itself is gone, only the pipe drain remains, so the wait drops +// to this. `close` is bounded by whoever holds the stdio pipes, which can +// outlive the direct child: any descendant that inherited them keeps it pending. +// The TUI spawns nothing at boot today (it does not auto-connect), so this +// never fires — but that reason lives outside this file, so cap it here rather +// than inherit an unrelated process's lifetime if that ever changes. +const DRAIN_MS = 500; function cleanup() { try { @@ -117,7 +125,7 @@ function cleanup() { } catch (err) { // Never turn a passing smoke into a failure over a leftover temp dir; the // OS reclaims tmpdir anyway. This should not fire now that removal waits - // for the child to exit (see finish()), so say so loudly if it does. + // for the child's `close` (see done()), so say so loudly if it does. console.warn( `smoke:tui — could not remove temp dir ${work}: ${err.message}`, ); @@ -160,15 +168,37 @@ function done(code, message) { // then blame a SIGTERM the child never received. `close` also guarantees the // stdio pipes are drained, which is what makes the thunked messages complete. const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS); - const giveUp = setTimeout(() => { - console.warn( - `smoke:tui — TUI did not exit within ${EXIT_GRACE_MS * 2}ms of SIGTERM; cleaning up anyway`, + + // Deadline for the wait. Re-armed shorter once the child is gone, since from + // that point we are only draining pipes. Each message describes what was + // actually being waited on — the timer can fire for a child that already + // exited and was never signalled, so it must not claim otherwise. + let deadline; + const armDeadline = (ms, warning) => { + clearTimeout(deadline); + deadline = setTimeout(() => { + console.warn(`smoke:tui — ${warning}`); + finish(code, message); + }, ms); + }; + armDeadline( + EXIT_GRACE_MS * 2, + `TUI did not close its output streams within ${EXIT_GRACE_MS * 2}ms; cleaning up anyway`, + ); + + const drainOnly = () => { + clearTimeout(forceKill); + armDeadline( + DRAIN_MS, + `TUI exited but held its output streams open for ${DRAIN_MS}ms (a descendant may have inherited them); cleaning up anyway`, ); - finish(code, message); - }, EXIT_GRACE_MS * 2); + }; + if (childExited) drainOnly(); + else child.once("exit", drainOnly); + child.once("close", () => { clearTimeout(forceKill); - clearTimeout(giveUp); + clearTimeout(deadline); finish(code, message); }); // Only signal a child that is still running: on the crash-before-render and @@ -196,12 +226,13 @@ child.on("close", () => { }); child.on("exit", (code) => { + childExited = true; if (settled) return; // Exiting before the render marker appeared is a failure (crash on boot). done( 1, () => - `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${output.slice(0, 800)}`, + `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${output.slice(-800)}`, ); }); @@ -213,6 +244,6 @@ const timer = setTimeout(() => { done( 1, () => - `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${output.slice(0, 800)}`, + `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${output.slice(-800)}`, ); }, TIMEOUT_MS); From 096646d209b5a215dcf27541bcc5a81953326ae2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 27 Jul 2026 20:09:09 -0400 Subject: [PATCH 4/4] docs: correct the cleanup() and DRAIN_MS comments; tail-slice cleanly (review #1814 r3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cleanup() comment claimed its warning "should not fire now that removal waits for close" — true only of the normal path. Both give-up branches call finish() without close, and those are precisely the states where something may still hold the dir, so the warning is expected there. Same defect class as the two warnings already fixed in this PR (a message asserting what the code no longer guarantees), relocated into a comment. The DRAIN_MS comment explained why the cap exists but not what it costs: on that path the work dir is removed while a descendant holding HOME=work may still be writing to it — #1801's race, re-entered deliberately. That is only acceptable because cleanup() warns instead of throwing, so record the two as load-bearing for each other where the constant is defined. Tail-slicing an Ink stream can begin mid-CSI-sequence, leading a diagnostic with an escape-code fragment that mangles the line after it. outputTail() drops through the first newline when the output was actually truncated, and returns short output untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq --- scripts/smoke-tui.mjs | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index a2c32c80c..e7d4012e7 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -117,6 +117,12 @@ const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000); // The TUI spawns nothing at boot today (it does not auto-connect), so this // never fires — but that reason lives outside this file, so cap it here rather // than inherit an unrelated process's lifetime if that ever changes. +// +// The cost of capping: on that path we remove the work dir while a descendant +// holding HOME=work may still be writing to it — the #1801 race, re-entered +// deliberately. That is only acceptable because cleanup() warns instead of +// throwing, so the worst case is a warning plus a leaked temp dir in tmpdir, +// never a red smoke. The two are load-bearing for each other. const DRAIN_MS = 500; function cleanup() { @@ -124,14 +130,25 @@ function cleanup() { rmSync(work, { recursive: true, force: true }); } catch (err) { // Never turn a passing smoke into a failure over a leftover temp dir; the - // OS reclaims tmpdir anyway. This should not fire now that removal waits - // for the child's `close` (see done()), so say so loudly if it does. + // OS reclaims tmpdir anyway. On the normal path this should not fire, since + // removal waits for the child's `close` (see done()) — but the two give-up + // branches there call finish() without it, and a process may still hold the + // dir, so this is expected rather than anomalous on those. console.warn( `smoke:tui — could not remove temp dir ${work}: ${err.message}`, ); } } +// Tail of the child's output, for quoting in a diagnostic. Slicing an Ink +// stream can land mid-CSI-sequence, so drop through the first newline rather +// than lead with an escape-code fragment that mangles the line after it. +function outputTail(limit = 800) { + const tail = output.slice(-limit); + const nl = tail.indexOf("\n"); + return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail; +} + // `message` may be a string or a thunk. Callers that quote the child's output // pass a thunk so it is rendered here — after the wait below, by which point // `close` guarantees stdout/stderr have been fully drained. Building the string @@ -232,7 +249,7 @@ child.on("exit", (code) => { done( 1, () => - `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${output.slice(-800)}`, + `TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${outputTail()}`, ); }); @@ -244,6 +261,6 @@ const timer = setTimeout(() => { done( 1, () => - `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${output.slice(-800)}`, + `TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${outputTail()}`, ); }, TIMEOUT_MS);