fix(temp): managed temp-directory lifecycle with signal-safe reaping - #30
Conversation
…sted] Node does not unwind the stack on SIGTERM/SIGINT/SIGKILL at default disposition, so the finally in runCodexOnce never ran on a killed process and stranded a full runner-repo clone (105-155MB each). Add a termination coordinator plus a flock-backed ownership registry: directories are created under an excluded staging name, locked, stamped with PID / /proc start time / boot ID, then atomically renamed into the managed namespace. A startup sweeper takes a nonblocking lock to distinguish live from orphaned and quarantines before deleting. On signal, disposal is deliberately skipped and the ownership lock is held until process death, so a live runner tree is never deleted out from under a detached process group; the next startup reaps it. Resolve the temp root from NEEDLEFISH_TMPDIR in code rather than relying on ambient TMPDIR, which is why CI runners lacking it wrote into a 4GB tmpfs. Linux-gated; macOS/Windows fall back to the previous behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The coordinator, runner-group registry and temp-directory registry were gated behind isLinux(), so on macOS and Windows a SIGINT/SIGTERM left runner process groups and temp trees behind. Only the flock/proc-based startup sweep genuinely needs Linux; it stays gated. Portable shutdown blocks new scheduling, signals registered groups, waits the grace interval, force-kills, then waits for each child's real close event before deleting only the directories whose runners were confirmed closed. A live-but-unconfirmed runner preserves its tree rather than having the filesystem pulled out from under it. Windows taskkill now sends a graceful close before escalating to /F. Tests drive the portable path with process.platform overridden to darwin and PATH sabotaged, so a regression back to the Linux branch fails rather than silently passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An independent review of the temp-lifecycle change found nine defects that the (green) test suite did not catch. Each fix below ships with a regression test that fails without it. Two could destroy data or unrelated processes: - Shutdown could SIGKILL a recycled process group. The unregister closure refused to remove entries once termination began, so runners that closed during the grace window left stale PIDs behind, and all three kill paths signalled those PIDs as process groups. Signalling now goes through the live ChildProcess handle and bails once the child is reaped; unregistering always removes; the last runner closing wakes the wait instead of sleeping out the timer. - The startup sweep recursively deleted any needlefish-shaped directory older than seven days with no ownership marker required. Since NEEDLEFISH_TMPDIR is user-settable, pointing it at a populated directory destroyed data we never created. Markerless legacy reaping is now opt-in via NEEDLEFISH_REAP_LEGACY_TMPDIRS; quarantine removal requires valid ownership metadata, a dead owner, an unlocked owner lock and an age grace. The rest: - A dead lock holder permanently poisoned every later allocation; it is now evicted and reacquired, with caches scoped per resolved temp root. - A holder that died after writing its readiness file left a stale marker the poll could observe before the exit callback, letting two sweepers run unlocked. Readiness now verifies the target is actually locked and rechecks holder state on both sides of the probe. - Malformed owner metadata threw out of the startup sweep, which every command awaits, so one truncated JSON file disabled the whole CLI. It now reads as "unowned", consistent with every other validation failure in that function. - Termination cancellation surfaced as DEEP PASS FAILED and fed the residual-risk path as a model failure. RunnerTerminatingError is now typed and recognised by isRunnerSafetyError. - The credential-leak regression test redirected TMPDIR while production resolves NEEDLEFISH_TMPDIR first, so on every runner that exports it the test inspected an empty directory and would have shipped a real leak green. The tests now control the variable production reads and assert the inspected root actually holds the allocation. - Owner lock files accumulated forever; dead, unlocked, unreferenced ones are now collected, and one unattributable directory no longer aborts the whole pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM ✅ — Introduces managed per-invocation temp directories, orphan reaping, and signal-coordinated runner shutdown.
Coverage: 7/7 changed files deep-reviewed across 1 hotspot
Findings
No actionable findings. Prefer this over padding weak ones.
Checked (1)
- [Runner temp lifecycle, cleanup, and termination coordination] No actionable correctness, lifecycle, or safety defect found in this surface.
3 calls · map 47s → deep:Runner temp lifecycle, cleanup, and termination coordination 2m 26s → critic 8s · total 3m 22s
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 542b6852eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function killRunnerProcessTree(pid: number | undefined, signal: NodeJS.Signals): void { | ||
| function signalRunnerProcessTree(child: ChildProcessWithoutNullStreams, signal: NodeJS.Signals): void { | ||
| const pid = child.pid; |
There was a problem hiding this comment.
When a runner spawns a same-process-group helper that inherits stdout/stderr and then the runner process exits, Node can have child.exitCode set while close is still pending because the helper keeps the pipes open. This new guard makes timeout or termination skip process.kill(-pid, ...), even though that process group may still contain the helper, so the review can return after give-up with a live runner descendant holding the sandbox/temp tree. The previous process-group kill could still clean up this case; only skip once the group is actually gone.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
|
|
||
| await initializeTempLifecycle(); |
There was a problem hiding this comment.
When NEEDLEFISH_TMPDIR points inside the target repo, including a relative value like tmp, this call creates lifecycle artifacts such as .needlefish-sweep.lock before local mode checks git status. A clean branch review is then classified as dirty by diffBundle and can switch to uncommitted mode, where the empty lifecycle file is skipped and the real branch diff is not reviewed. Initialize after the local bundle is built or ensure lifecycle artifacts cannot be created under the target repo.
Useful? React with 👍 / 👎.
| if (activeRunnerProcessGroups.size === 0) { | ||
| terminateImmediately(signal); |
There was a problem hiding this comment.
On non-Linux platforms there is no flock/proc reaper, but a SIGINT/SIGTERM that arrives after createManagedTempDirectory() and before the runner is registered still takes this immediate-exit path because activeRunnerProcessGroups is empty. That skips disposeManagedTempDirectory() and leaves the active temp tree behind; if the signal lands after prepareEphemeralHome(), copied runner credentials remain on disk. The immediate path should remove safe registered temp directories on portable platforms before exiting.
Useful? React with 👍 / 👎.
1 participant
Why
Needlefish leaked its invocation temp directories on this machine until
/tmp(a 4G tmpfs, so RAM) hit 86%. Root cause, proven and reproduced end to end:runCodexcleaned up in afinally, and Node does not runfinallyon SIGTERM/SIGINT at default disposition. Every signalled invocation stranded its whole sandbox.The leak surfaced here because needlefish protects its own runner (
.envTMPDIR+ areview.ymlpreflight) but is invoked by other repos' runners, which had neither.What changed
createManagedTempDirectory/disposeManagedTempDirectoryreplace the ad-hocmkdtempSync+finallyincodex.ts.finallythat never runs.flocked, stamped with pid +/proc/<pid>/statstart time + boot ID, then atomically renamed into the managed namespace. A sweeper that can take the lock knows the owner is gone. PID reuse cannot fool it.taskkillsends a graceful close before escalating to/F.Review history
This branch was reviewed adversarially rather than diff-read. That mattered:
SIGKILLa recycled, unrelated process group, and the sweep could recursively delete any needlefish-shaped directory with no ownership marker required.The single most important fix is the last one on the list below: the credential-leak security test could pass vacuously. It redirected
TMPDIR, but production resolvesNEEDLEFISH_TMPDIRfirst. On every runner that exports that variable, the test inspected an empty directory. A real credential leak would have shipped green.Verification
pnpm test516/516,pnpm check,pnpm lintclean.Every fix was mutation-tested independently of the implementer — each defect was reintroduced and the suite confirmed to go red:
NEEDLEFISH_TMPDIR(test/prod divergence)Deploy note
Merging is not sufficient — the runners currently run release
53e4ed7b. This needs a redeploy to take effect. Machine-side config (per-runnerTMPDIR/NEEDLEFISH_TMPDIRon disk-backed storage, plus a workflow preflight in the calling repo) is already in place and is what is holding/tmpat 14%.🤖 Generated with Claude Code