fix: wait for the TUI to exit before removing its temp dir (#1801)

[cliffhall]

Closes #1801

Problem

smoke:tui intermittently failed with ENOTEMPTY after the TUI had already rendered successfully:

Error: ENOTEMPTY, Directory not empty: /var/folders/.../T/smoke-tui-xbpuq7
    at cleanup (scripts/smoke-tui.mjs:102:3)

done() called child.kill("SIGTERM") and then cleanup() synchronously. Nothing waited for the child, and the work dir doubles as the TUI's HOME — so the Ink process could still be writing into it while rmSync walked it. A file created after a directory has been read but before it is removed produces ENOTEMPTY on macOS.

Because smoke is part of the mandatory pre-push gate and smoke:tui self-skips under CI, this only bit local runs — exactly where the gate is supposed to be trusted.

Fix

Removal now waits for the child's exit event instead of racing it:

• done() registers a one-shot exit listener, then sends SIGTERM; cleanup + process.exit happen in finish() from that listener.
• Escalates to SIGKILL after SMOKE_TUI_EXIT_GRACE_MS (default 5s), and cleans up anyway with a warning after twice that, so a wedged child can't hang the smoke.
• A childExited flag, set by the pre-registered exit handler, short-circuits the wait on the crash-before-render path (where done() is called from that handler).
• cleanup() warns instead of throwing, so a leftover temp dir can never turn a passing smoke red. maxRetries/retryDelay were deliberately not used — they'd widen the race window rather than close it.

Verification

• npm run smoke:tui × 5 — all OK, no ENOTEMPTY, no cleanup warnings, temp-dir count in $TMPDIR unchanged across runs.
• Timeout path (SMOKE_TUI_TIMEOUT_MS=300, child still alive): fails as expected, cleans up.
• Crash-before-render path (child exiting immediately): fails as expected and returns instantly via the childExited short-circuit.
• npm run validate and npm run smoke both green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MvKF2mqi4hoQSUqQpZXNTq

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 7s —— View job

---

Review: fix: wait for the TUI to exit before removing its temp dir

• Read the diff and the full scripts/smoke-tui.mjs
• Analyze the exit/cleanup race and the new state machine
• Verify Node's spawn-failure event semantics empirically
• Check formatting / docs / test conventions
• Post review

Verdict: the fix is correct for the bug it targets — approve-with-one-nit. Waiting on the child's exit before rmSync is the right shape (the child can't write to HOME after it has exited, so the ENOTEMPTY window is genuinely closed), the SIGKILL/give-up escalation prevents a wedged child from hanging the gate, and the childExited short-circuit for the crash-before-render path is a real hazard the author spotted and handled. Declining maxRetries/retryDelay is the right call for the reason given.

One concrete finding, plus nits.

---

1. The error (spawn-failure) path is the missed sibling of childExited — it now hangs 10s and warns misleadingly

scripts/smoke-tui.mjs:186 calls done(1, …) from the error handler. But Node does not emit exit when a spawn fails — only error and close. Verified on this runner (Node v22.23.1):

$ node -e 'spawn("/definitely/not/a/binary")…'
events: error:ENOENT | close:-2:null      # no "exit"

So on that path childExited is still false, done() registers a child.once("exit") that will never fire, sends SIGTERM to a process that never existed, and only resolves via giveUp. I ran the PR's exact control flow against a failing spawn (grace shortened to 500ms for the trace):

+5ms    error ENOENT
+506ms  SIGKILL                    # kill on an unspawned child, no-op
+1006ms WARN did-not-exit          # "TUI did not exit within …ms of SIGTERM"
+1006ms finish(1)

At the real default that's 10 seconds and a warning claiming the TUI ignored a SIGTERM it was never sent. Before this PR the path exited immediately. This is exactly the bug class childExited fixes for the exit handler — it just wasn't extended to error.

Cheapest fix: wait on close instead of exit at scripts/smoke-tui.mjs:156. close fires in both cases (normal termination and spawn failure), and it additionally guarantees stdio is drained. Same trace with that one-word change:

+4ms error ENOENT
+6ms finish(1)                     # instant, no spurious warning

The pre-registered exit handler at :174 still fires first and still sets childExited, so the short-circuit keeps working unchanged. (The alternative — setting childExited = true inside the error handler — also works but is less honest: the child never existed.)

Impact is low in practice (it requires process.execPath to be unspawnable), so this is robustness, not a blocker. Fix this →

---

Nits (take or leave)

2. Crash diagnostics are still built at exit, before stderr is drained. scripts/smoke-tui.mjs:180-183 snapshots output.slice(0, 800) inside the exit handler, but the pipes may not have delivered the last of the crash's stderr yet — so the most diagnostically important path can print a truncated reason. Pre-existing, not introduced here, but the PR now waits anyway, so re-slicing output in finish() (or switching the wait to close per #1 and building the message there) would come nearly free. Worth a follow-up issue rather than expanding this PR.

3. finish() isn't idempotent. No settled-style guard; it's only safe because process.exit is synchronous and both call sites are mutually exclusive by construction (giveUp fires → exits; the listener clears giveUp). True today, quiet to break later. A one-line guard would make the invariant local instead of inferred.

4. Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000) (:112) — an empty or non-numeric override yields 0/NaN, i.e. an immediate SIGKILL and an immediate give-up. Matches the existing TIMEOUT_MS pattern at :36, so it's consistent; flagging only because a debugging knob is exactly what someone will export to an empty string.

---

Checks I ran

• prettier --check scripts/smoke-tui.mjs → clean (format:scripts covers scripts/**/*.mjs, so the gate applies).
• Node event-semantics experiments above, on Node v22.23.1.
• Tests: none required. test:scripts globs scripts/**/*.test.mjs and today covers only the pure parsers (scripts/lib/npm-scripts.mjs, verify-typecheck-coverage); the smoke:* drivers have never been unit-tested, so this is consistent with convention, and the PR's manual verification matrix (5× clean runs, timeout path, crash-before-render path) is the right substitute.
• New env knob undocumented: consistent — SMOKE_TUI_TIMEOUT_MS, SMOKE_WEB_PORT etc. aren't in any README either, so no doc gap is introduced.
• Confirmed the TUI does not auto-connect on boot (no connect() on mount in clients/tui/src/App.tsx), so no MCP stdio grandchild inherits HOME=work in this smoke — the "exit on the direct child doesn't imply descendants exited" caveat is moot today. It would stop being moot if the TUI ever gained boot-time auto-connect.

I did not run npm run smoke:tui — it self-skips under CI, which is exactly why the author's local 5× run is the meaningful evidence here.
· branch fix/1801-smoke-tui-enotempty

[cliffhall]

Good review — finding #1 is real, I reproduced it, and fixing it made #2 and #3 nearly free, so all three are in. #4 declined.

1. error path never sees exit — fixed, and confirmed independently

Reproduced on Node v24.2.0 before touching anything:

$ node -e 'spawn("/definitely/not/a/binary",[],{stdio:["ignore","pipe","pipe"]}) …'
events: error,close          # no "exit"

And the end-to-end timing, running the script itself against an unspawnable binary with the grace shortened to 500ms:

	result
before	1.04s real, plus TUI did not exit within 1000ms of SIGTERM
after	0.02s real, silent

At the default grace that's the 10s you predicted. Took your suggested fix — waiting on close, which covers both normal termination and spawn failure and drains stdio as a bonus.

One thing your fix note missed, which I hit while applying it: leaving the short-circuit keyed on childExited quietly defeats the drain. exit fires before close, so on the crash-before-render path done() is called from the exit handler with childExited === true, short-circuits straight to finish(), and never waits for the pipes — precisely the path where the quoted stderr matters most. So the gate moved too: childExited → childClosed, set by a close handler registered before done()'s one-shot. I also narrowed the signal to a child that's actually still running (exitCode === null && signalCode === null), since on both the crash and spawn-failure paths there is nothing left to signal.

2. Diagnostics built before the drain — fixed here, not deferred

You suggested a follow-up issue; it turned out to be ~3 lines on top of #1, so it's in this PR. message may now be a thunk, rendered inside finish() after the drain. Verified with a child that writes a detail line to stderr and exits 3:

smoke:tui FAILED — TUI exited (code 3) before rendering "MCP Servers"
fatal: boom detail line

That trailing line is the part a pre-drain slice can lose.

3. finish() not idempotent — fixed

Added the finished guard. Agreed with your framing: it was true-by-construction but the invariant was inferred from process.exit() being synchronous, which is exactly the kind of thing a later edit breaks silently.

4. Number(env ?? 5000) on an empty override — declining

You flagged it as consistent-with-existing, and that's the reason I'm leaving it. TIMEOUT_MS two dozen lines up has identical semantics; hardening only the newer knob makes the file less predictable, and hardening both means changing TIMEOUT_MS's behavior in a PR about an rmSync race. If it's worth doing it's worth doing to both knobs, in all four smoke:* scripts, deliberately.

Verification after the change

Happy path ×3, timeout path, crash-before-render, spawn-failure, plus full npm run smoke (all five green, TUI included) and npm run validate. prettier --check clean via format:check:scripts. Temp-dir count in $TMPDIR unchanged across runs.

Also: thanks for checking whether an MCP stdio grandchild inherits HOME=work — that's the caveat that would actually reopen this bug, and you're right that it's moot only because the TUI doesn't auto-connect on boot today.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 39s —— View job

---

Review: fix: wait for the TUI to exit before removing its temp dir

• Read the diff and the full scripts/smoke-tui.mjs
• Re-check the round-2 changes (close wait, childClosed, thunk message, finished guard)
• Verify Node event semantics for every path the state machine handles
• Check formatting / conventions / sibling scripts
• Post review

Verdict: LGTM. The round-2 changes are correct, and I re-verified each path empirically rather than taking the previous round's reasoning on faith. Three small notes below, none blocking.

---

What I verified (Node v22.23.1)

Spawn failure resolves instantly, and the new exitCode/signalCode narrowing is what makes it safe. child.exitCode is already -2 by the time the error handler runs, so the guard at scripts/smoke-tui.mjs:177 correctly declines to signal a process that never existed:

+0ms error ENOENT | exitCode: -2 signalCode: null killed: false
   guard would send SIGTERM? false
+2ms close [-2, null]

Crash-before-render likewise skips the signal and waits only for the drain — exitCode is set before exit is emitted:

in exit handler: exitCode=3 signalCode=null killed=false -> guard sends SIGTERM? false
close fired

The childExited → childClosed correction is the right call, and the reasoning in the PR comment holds: exit precedes close, so keying the short-circuit on exit would have jumped straight to finish() on the crash path — the one path where the drained stderr is the entire diagnostic. Catching that while applying the fix was the substantive part of this round.

Scope also checks out: no sibling smoke has this race. smoke-cli.mjs reaps via spawnSync or awaits its one async child before the finally { rmSync }, and prod-web-server.mjs doesn't hand its child a temp HOME. This really is TUI-specific.

---

1. The giveUp warning can now be wrong on both counts (nit)

scripts/smoke-tui.mjs:164-166 still says "TUI did not exit within Xms of SIGTERM", but after this PR the wait is on close, not exit, and SIGTERM is conditional. The most plausible way to actually reach this timer now is a child that has exited whose pipes are still held open — in which case the message asserts two things that are both false. That's the same shape as the bug fixed this round (a warning blaming a signal never sent), just narrower. Something like "TUI did not close its output streams within Xms; cleaning up anyway" is accurate for every path that can reach it. Fix this →

2. close inherits a grandchild's lifetime in a way exit doesn't — worth a comment (latent)

This is the one real cost of the exit → close switch, and it isn't noted in the file. A grandchild that inherited the pipes keeps close pending long after the direct child is gone:

+36ms   data: parent about to exit
+90ms   exit 0
+3061ms close 0        # grandchild held the inherited stderr for 3s

Moot today — the previous round confirmed the TUI doesn't connect on boot, so no MCP stdio grandchild exists. But if the TUI ever gains boot-time auto-connect, and the SDK's stdio transport inherits stderr (it typically does), every smoke:tui run silently goes from instant to EXIT_GRACE_MS * 2 plus the warning from #1. It can't hang — which is exactly what the escalation ladder buys you, and a good argument for having added it. Two options, either fine:

• Cheapest: one comment near scripts/smoke-tui.mjs:169 recording that close is bounded by descendants holding the pipes, and that it's acceptable only because the TUI spawns nothing at boot. The reason this is safe currently lives entirely outside this file, which is what makes it fragile.
• Slightly better: once exit has fired, shorten the remaining wait to a short drain window (a few hundred ms) instead of the full EXIT_GRACE_MS * 2. You keep the drain guarantee that motivated Request logging #2 without inheriting an unrelated process's lifetime.

3. output.slice(0, 800) takes the head, which caps what the new thunk buys you (nit)

The thunk at scripts/smoke-tui.mjs:204/:216 exists to capture output that arrives late — i.e. the tail. But the slice takes the first 800 chars, so as soon as total output exceeds that (plausible once Ink has painted a frame or two of ANSI redraws), the freshly-drained bytes are dropped anyway. On the timeout path it's more clearly backwards: you get the first frame, when what diagnoses a stuck render is the last one. slice(-800) would preserve both.

That said — slice(0, 800) is the house pattern in smoke-launcher.mjs:69 and pack-and-verify.mjs:275 too, so this lands in the same bucket as the declined #4: worth doing across all three deliberately, or not here. Your call; I'd lean follow-up issue.

Micro: the cleanup() comment at :118-120 says removal now "waits for the child to exit (see finish())" — the wait lives in done(), and it's for close.

---

Checks run

• prettier --check scripts/smoke-tui.mjs → clean (covered by format:check:scripts).
• Three Node experiments above (spawn failure, crash-before-render, grandchild-held pipes) on v22.23.1.
• Tests: none expected. test:scripts globs scripts/**/*.test.mjs and covers only the pure parsers; the smoke:* drivers have never been unit-tested, so the manual matrix in the PR comments is the right substitute here.
• Did not run smoke:tui — it self-skips under CI, which is precisely why the author's local runs are the meaningful evidence.
  · branch fix/1801-smoke-tui-enotempty

[cliffhall]

All three are in, including the two you'd have deferred — this is the last round, so anything worth doing gets done here rather than becoming a stale follow-up.

1. giveUp warning inaccurate — fixed

You're right, and the framing is the convincing part: it's the same defect this PR fixed one round ago, just narrower. A warning that names a signal never sent is exactly what sent me chasing the wrong thing on the spawn-failure path. The deadline is now re-armed with a message describing what is actually being awaited, so neither clause can be false on any path that reaches it.

2. close inherits a descendant's lifetime — took the "slightly better" option

Went with the bounded-drain rather than the comment, because the comment alone leaves the failure mode live and only documents it. Once exit has fired the deadline drops to DRAIN_MS (500ms), keeping the drain guarantee that motivated the thunk without inheriting an unrelated process's lifetime. Kept a comment too, since the reason this is currently safe genuinely does live outside this file.

Reproduced your scenario — a grandchild holding the inherited pipes for 3s:

smoke:tui — TUI exited but held its output streams open for 500ms (a descendant may have inherited them); cleaning up anyway
smoke:tui OK — rendered "MCP Servers" panel from --catalog within timeout
real 0.54

0.54s instead of 3s, with a warning that's true. And it names the actual cause, so whoever adds boot-time auto-connect gets told what happened instead of being told the TUI ignored a SIGTERM.

3. slice(0, 800) takes the head — fixed here, and I'd push back on the follow-up framing

You called this consistent-with-siblings and leaned follow-up, but it's not the same as the Number(env) one I declined: this PR introduced the inconsistency. The thunk exists specifically to capture late-arriving bytes, and a head slice then throws them away the moment output exceeds 800 chars — so as shipped last round, item #2 bought nothing on any real run. Leaving that in place to preserve symmetry with two scripts that never had a thunk would be preserving the wrong invariant.

Verified with a 2KB stderr crash:

smoke:tui FAILED — TUI exited (code 3) before rendering "MCP Servers"
…LAST-LINE-MATTERS

Siblings keep slice(0, 800) — sweeping all three is a deliberate change, not a drive-by in a PR about an rmSync race.

Micro: fixed, cleanup() now says the wait lives in done() and is for close.

Verification

Happy path ×3, spawn failure (0.02s), crash-before-render, grandchild-held pipes, timeout path, full npm run smoke (all five green) and npm run validate. prettier/eslint clean.

Thanks for re-deriving the event semantics empirically both rounds instead of trusting the previous round's write-up — the exitCode === -2 observation is what confirmed the new signal guard is load-bearing rather than decorative. Wrapping review here at three rounds.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 44s —— View job

---

Review: fix: wait for the TUI to exit before removing its temp dir

• Read the full scripts/smoke-tui.mjs and the round-3 diff
• Re-verify the state machine across all paths (happy, timeout, crash, spawn-failure, wedged, grandchild-held pipes)
• Check formatting / conventions / sibling-script scope
• Post review

Verdict: LGTM — ship it. I rebuilt the round-3 state machine as a standalone harness and drove it against six real children rather than reasoning about it, and every path terminates promptly with an accurate message. The two changes this round are both improvements over what I'd suggested: the re-armed deadline is strictly better than the comment I proposed for #2, and you were right that the head-slice was an inconsistency this PR introduced, not a pre-existing house pattern to preserve.

Only nits below, one of which is the comment-twin of the thing we've now fixed twice.

---

Verified: all six paths (Node v22.23.1, EXIT_GRACE_MS=1000 for legibility)

path	trace	outcome
happy	+27ms SIGTERM → +31ms drainOnly → +31ms close	OK, 31ms
crash-before-render	+26ms drainOnly → +26ms no signal (exitCode=3) → close	FAILED, 26ms, tail quoted
spawn failure	+4ms no signal (exitCode=-2) → +6ms close	FAILED, 6ms, no warning
render timeout	+304ms SIGTERM → drainOnly → close	FAILED, 309ms
wedged (ignores SIGTERM)	SIGTERM → +1025ms SIGKILL → drainOnly → close	OK, 1029ms
grandchild holds pipes 3s	SIGTERM → drainOnly → +534ms WARN … held its output streams open	OK, 534ms

The last row is the one worth calling out: the re-armed deadline turns a 3s hang into 534ms and the warning it prints names the actual cause. Worst case for a wedged child is now EXIT_GRACE_MS + DRAIN_MS (5.5s), not the EXIT_GRACE_MS * 2 the original description promised — the ladder is tighter than advertised.

I also re-derived the load-bearing bit independently: child.exitCode is -2 and pid is undefined inside the error handler, so the exitCode === null && signalCode === null && !killed guard declines to signal — which is why the spawn-failure row is 6ms and silent.

Scope re-check: smoke-cli.mjs hands its children the same temp HOME (:173, :469) and does an unguarded rmSync in a finally (:499), but every child there goes through runCli → spawnSync, which reaps before returning, so there's no async child to race. Genuinely TUI-only.

---

1. cleanup()'s comment is now the last thing in the file that can be false (nit)

scripts/smoke-tui.mjs:126-128:

This should not fire now that removal waits for the child's close (see done()), so say so loudly if it does.

But two branches call finish() without close having fired — the EXIT_GRACE_MS * 2 give-up and the DRAIN_MS drain give-up. Those are exactly the states where something may still hold the dir, so on those paths the warning is expected, not anomalous. A future debugger who hits it will read that comment and go looking for a regression in the wait.

This is the same defect class as the SIGTERM warning fixed in r2 and the give-up warning fixed in r3 — a message asserting something the code no longer guarantees — just relocated into a comment. One clause fixes it: "…should not fire on the normal path, where removal waits for close; the two give-up branches below can legitimately hit it." Fix this →

2. The DRAIN_MS give-up bounds the wait by re-opening the original window — worth half a sentence (nit)

The DRAIN_MS comment at :114-119 explains why the cap exists but not what it costs: on that path we rmSync at exit + 500ms while a descendant that inherited HOME=work may still be writing into it. That is the #1801 race, re-entered deliberately.

I tried to reproduce the failure and couldn't on Linux — a grandchild writing to HOME every 60ms while rmSync(recursive, force) walks the tree: removal succeeded and the dir stayed gone. ENOTEMPTY is the macOS-observed shape, so I can't rule it out there. Either way the blast radius is one warning plus a leaked temp dir, never a red smoke, because cleanup() warns instead of throwing — the two changes are load-bearing for each other in a way the comments don't currently connect. Right tradeoff; just say so where the constant is defined.

3. output.slice(-800) can begin mid-escape-sequence (micro)

Tail-slicing an Ink stream can start inside a CSI sequence, so the quoted diagnostic may lead with a fragment of an escape code and mangle the line after it. Cosmetic, and much less costly than the head-slice it replaced. If you ever touch it: slice, then drop through the first \n.

4. DRAIN_MS is the one timing constant with no env override

TIMEOUT_MS and EXIT_GRACE_MS both take one; DRAIN_MS is hard-coded. Flagging for symmetry only — it's consistent with declining the env-hardening nit last round (fewer knobs, not more), and 500ms needs no tuning on a path that shouldn't be reachable today. No action.

---

Checks run

• prettier --check scripts/smoke-tui.mjs → clean. Note scripts/ is prettier-only per AGENTS.md ("the root has no eslint config for .mjs"), so the "eslint clean" in your verification is vacuous for this file — nothing to fix, just so the gate isn't over-credited.
• Six-path harness above, plus the error-handler exitCode probe and the grandchild-vs-rmSync experiment, on Node v22.23.1.
• Tests: none expected — test:scripts globs scripts/**/*.test.mjs and covers only the pure parsers; the smoke:* drivers have never been unit-tested. The manual matrix across three rounds is the right substitute.
• Docs: SMOKE_TUI_EXIT_GRACE_MS stays undocumented, consistent with SMOKE_TUI_TIMEOUT_MS / SMOKE_WEB_PORT. No gap introduced.
• Did not run smoke:tui — it self-skips under CI, which is why your local runs are the meaningful evidence.

Nothing here blocks the merge; all four are comment/cosmetic. Good place to stop.
· branch fix/1801-smoke-tui-enotempty

[cliffhall]

Taking 1, 2, and 3 — all comment/cosmetic, so they go in and we stop here at three rounds.

1. cleanup() comment — fixed, and you're right that it's the third instance of the same defect. Both give-up branches call finish() without close, and those are exactly the states where the warning is expected. A comment telling a future debugger to hunt for a regression in the wait, on the one path where the wait was deliberately abandoned, is worse than no comment. Now says it should not fire on the normal close-waited path and that the give-up branches can legitimately hit it.

2. DRAIN_MS cost undocumented — fixed. The connection you drew is the one worth writing down: capping the drain re-enters #1801's race on purpose, and that's only acceptable because cleanup() warns instead of throwing. Two changes from different rounds that turn out to be load-bearing for each other, with nothing in the file saying so. Noted at the constant.

Your reproduction attempt is useful too — grandchild writing every 60ms while rmSync walks, no failure on Linux. Matches the issue: ENOTEMPTY is the macOS-observed shape.

3. Mid-escape-sequence tail — fixed. Extracted outputTail(), which drops through the first newline when the output was actually truncated and returns short output untouched. Verified with a 2KB ANSI-wrapped stderr crash: the fragment line is gone, LAST-LINE-MATTERS survives.

4. DRAIN_MS has no env override — agreed, no action, for your reason: it's the same fewer-knobs logic as declining the env-hardening nit, on a path that shouldn't be reachable today.

On the eslint correction — you're right and I'll take it. scripts/ is prettier-only; npx eslint scripts/smoke-tui.mjs exiting 0 tells you nothing, so citing it as evidence over-credited the gate. prettier --check via format:check:scripts is the real coverage for this file. Worth flagging.

The six-path harness is the most useful thing in this review — particularly worst-case EXIT_GRACE_MS + DRAIN_MS being tighter than the ladder I described. Final state re-verified: happy ×2, crash with ANSI, short-output crash, full npm run smoke (5/5) and npm run validate green.