Repository navigation
fix(bin): tear down each task's own temp root and agent scratch - #3457
jason-curtis wants to merge 21 commits into
Conversation
Confidence Score: 4/5The PR is not yet safe to merge because a failed fresh-spawn backlog commit can remove the task record while leaving its temporary root undiscoverable. Fresh metadata publication disables root cleanup before the final backlog transition, but failure of that transition rolls the metadata back and exits, leaving no durable owner that teardown can use to reclaim the root. Files Needing Attention: bin/fm-spawn.sh Reviews (9): Last reviewed commit: "no-mistakes(document): attribute the rel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Speaking as Kun's firstmate: first-time fork CI approved after a full-thread + full-diff review (no Inspected HEAD Attestation: MISSING. Full PR body (38316 chars, GraphQL + REST) has zero HTML comments and zero Contract-class: new-default. Main already specified a per-task temp root and teardown of it, but the specified spawn env was GOTMPDIR-only: VISION.md per-rule
Deletion-safety (security-sensitive
Pipeline review claims verified on HEAD (not just claimed):
Not merging. Not Firstmate-flagging (new-default, and not otherwise ready: attestation missing, CI/NM not green yet). After a MATCH attestation lands, this still will not auto-merge; it would wait on green CI+NM and a Firstmate flag only when completely ready. |
cf38ab5 to
a93be1f
Compare
jason-curtis
changed the title
d4f757b to
fbf0cfe
Compare
jason-curtis
changed the title
|
Speaking as Kun's firstmate: tip moved since the 2026-09-01T18:32:35Z waiting-author stamp. Re-reviewed full thread + full diff on HEAD Attestation: MATCH. Body Contract-class: new-default. Main's specified default path is VISION.md per-rule
Deletion-safety (security-sensitive
Overlap: open #3483 ( Not merging (new-default). This is otherwise ready: attestation MATCH, CI+NM green, workflow-zero, deletion-safety sound, author blockers cleared. Waiting on the captain, not the author. Firstmate-flagged for the always-on home-scoped default path (and the related #3483 overlap). |
|
Speaking as Kun's firstmate: tip is no longer otherwise-ready. This is waiting on the author, not the captain, until conflicts are cleared. HEAD still Contract-class: new-default unchanged (always-on home-scoped Please rebase/resolve onto current main and keep the no-mistakes attestation bound to the new tip. Once CLEAN + attestation MATCH + CI/NM green again, Firstmate will restamp for the captain default-behavior decision (not auto-merge). Do not treat this comment as a captain grant. |
fbf0cfe to
42681fe
Compare
jason-curtis
changed the title
|
Speaking as Kun's firstmate: tip moved since the 2026-09-02T11:15:00Z waiting-author stamp, then main moved under it again. This is waiting on the author, not the captain, until conflicts are cleared. HEAD Attestation: MATCH on this tip ( CI: fork CI re-approved this pass ( Contract-class: new-default unchanged. Main still specifies Overlap: open #3483 ( Please rebase/resolve onto current main |
Firstmate already gave every task a temp root at /tmp/fm-<id>/ and removed it at cleanup, but only Go's build temp was pointed there. The agent's own harness scratch kept landing under /tmp/claude-<uid>/<worktree-slot>/<session>/, outside anything teardown owned, so every task leaked its scratch until reboot - and on a RAM-backed /tmp that is leaked memory, which is why it went unnoticed until a command failed with "Disk quota exceeded" beside 234 GB of free disk. fm-spawn now pins the launched agent's TMPDIR to that task's own temp root, so its scratch is confined to the one directory teardown already removes. Removal is task-scoped on purpose. The worktree pool reuses slot numbers, so a single slot's scratch path accumulates sessions from many tasks, live ones included; deleting by slot, by age, or by pattern across sessions would destroy a running task's work. bin/fm-task-tmp-lib.sh owns the path shape for both scripts, and teardown removes only the path this task's own record names, only when it is exactly that derivation, never through a symlink. A mismatch or a failed removal is reported and cleanup continues, because losing worktree cleanup over a leftover temp directory would be worse than the leak. A raw launch command is the unverified-adapter escape hatch and is still passed through exactly as given. Scout reports keep no exception: 2 of 26 reports on disk cite a scratch path, both labeling it disposable and reproducible, and both cited directories are already gone. The scout brief now states the predictable rule instead - evidence a report depends on is copied next to the report, which survives cleanup. Verified against real Claude Code 2.1.251 that a pinned TMPDIR moves the whole scratch tree (docs/verification/runtime-backends.md), and covered by portable regressions including a live sibling task in a reused slot that must survive.
The scratch-teardown commit's message said "both cited directories are already gone, since /tmp is a RAM disk that does not survive reboot." The counts hold; that explanation does not, and the corrected account is the stronger one. Rechecked on this host: 26 scout reports under data/*/report.md, 7 cite any /tmp path, and 2 cite an actual agent scratch directory. Both of those citations are already dead as evidence - one path no longer exists, and the other survives only as an empty directory whose contents the 15 Aug manual cleanup removed. Crucially, `uptime -s` reports 2026-06-01, so there has been no reboot since the incident: these citations died during ordinary operation, not at a restart. That is what carries the argument. Had they died at a reboot, the case for copying evidence beside the report would rest on a machine restarting; because they died in normal operation, a scout report that points at scratch is unreliable even on a host that never reboots. Tmpfs volatility is an additional reason such a citation cannot be relied on, not the observed cause here. To recheck cheaply: grep data/*/report.md for /tmp paths, then ls the two cited scratch paths and run `uptime -s`. The answer to whether a torn-down scout's scratch should be preserved when its report cites it is still no, and the shape of the answer is that the NEED for a preservation rule is removed rather than a rule invented: bin/fm-brief.sh's scout scaffold states the predictable rule that evidence a report depends on is copied into data/<id>/ beside the report, which survives cleanup, and anything left in scratch is disposable. Legacy undiscriminated <base>/fm-<id> temp roots are left alone on purpose. Ownership cannot be established across firstmate homes for that shape: two homes with colliding task ids both recorded exactly it, and nothing available at teardown distinguishes them, so deleting one risks a live sibling home's directory. The forfeited material is small and bounded - those old roots hold Go build temp only, because agent scratch was never pinned there before this change - and the historic /tmp/claude-<uid>/... scratch is not reachable by any task-scoped rule. Two test failures a reviewer will see on a host like this one are pre-existing and independent of this change. These causes supersede the earlier check that ran `bash tests/fm-teardown.test.sh` on a clean detached worktree at origin/main f66be0f: that check established only THAT the case fails without this change, while these establish WHY. 1. tests/fm-teardown.test.sh "leaked-process-reap" fails because lsof is NOT INSTALLED on this host, so bin/fm-teardown.sh's reaper falls back to its tmux process-group path, which the fixture's fake tmux does not satisfy. That code path is untouched by this change. Command: `bash tests/fm-teardown.test.sh`. With a /proc-based `lsof -a -d cwd -Fpn` stand-in on PATH the entire suite passes, including test_leaked_tasktmp_process_is_reaped - the case this change modified, which never ran otherwise because the suite stops at the first failure. 2. tests/fm-test-run.test.sh "portable shard union, disjointness, and coverage guard" fails under LANG=en_US.UTF-8: run_coverage_guard() (untouched by this change) writes its lane lists with `LC_ALL=C sort` but compares them with plain `comm`, which validates sortedness under the ambient locale. `LC_ALL=C bash bin/fm-test-run.sh --check-coverage` prints "FM_TEST_COVERAGE ok" and the suite passes under LC_ALL=C. No code change: the claim was wrong, not the behavior.
…ion in verification
The per-task temp root's name is deterministic and its default parent /tmp is world-writable, and the launch now pins the agent's TMPDIR to that root, so a symlink planted at the name would have redirected the whole scratch tree to a directory this user neither controls nor tears down. Teardown's symlink refusal runs too late to prevent those writes, so fm_task_tmp_create creates the root with a plain mkdir - which fails on an existing path rather than following it - accepts an already-present root only when it is a real directory owned by this user, and gives it mode 0700. A fresh spawn also created the root before anything recorded it, so an abort between creation and the meta publish left a root no teardown could find - the same leak this root exists to close. The abort trap now removes it, through the same validation teardown uses, and only for a fresh spawn: a relaunch's root is already named by the record that outlives the abort.
Upstream deliberately pinned GOTMPDIR only, with a comment saying TMPDIR is too broad, so pinning TMPDIR on every non-raw launch shipped a new default that assumes consent. It now ships as a flag to enable instead. config/task-tmpdir-pin is a gitignored presence flag in the same shape as config/trace-context and config/herdr-presentation-spaces, and it is inherited by secondmate homes so a secondmate's own crewmates follow the primary's choice. With the flag absent the launch adds no TMPDIR prefix and GOTMPDIR behaves exactly as it does on main. Everything else is unchanged: the per-task temp root is still created with the validated private creation, still recorded as tasktmp=, still holds Go's build temp at gotmp/, and is still removed by teardown through the exact-match ownership check, including forced-secondmate children and the aborted-spawn cleanup. The docs say plainly what the default does not solve: with the flag off, harness scratch under /tmp/claude-<uid>/ is not redirected into the task root and is not removed by teardown, so that part of the original leak remains and only Go's build temp is bounded. The spawn suite pins both sides of the flag so neither can go vacuous, and the suites that assert the enabled pin now enable it explicitly.
…rification record
A relaunch of a task recorded before tasktmp= existed creates the temp root itself, and nothing names that path until the replacement record is published. The abort trap was armed only for non-relaunch spawns, so an abort in that window left an undiscoverable root in the RAM-backed temp filesystem, which is the leak this whole path exists to close. Arm the trap on the outcome that actually matters - whether this spawn created the root - and disarm it once the relaunch has published a record naming the path, mirroring the fresh-spawn publication. A root that was already there is still left alone, since it can be a live task's own.
The abort cleanup released the per-task spawn lock and only then removed the root it had created. In that window another spawn of the same id could take the lock and adopt the directory, because fm_task_tmp_create accepts an existing directory this user owns so a relaunch can reuse its own root. The aborting process then deleted it underneath the new spawn, leaving a live agent whose TMPDIR and GOTMPDIR no longer exist - the same "never delete a live task's scratch" rule this whole path exists to keep. Move the removal above the lock release so adoption and removal are mutually exclusive. It stays below the Orca recovery record, which names the root, and a root already removed is a silent no-op at teardown. This is an ordering fix, not a new guard: nothing else changes, and the existing ownership, symlink, non-directory and created-fresh checks remain the only conditions under which anything is removed.
The record described both relaunch cases as if each started from a task recorded before tasktmp= existed. Only the pre-publication abort case uses that legacy fixture; the post-publication case starts from a record that already names the root and asserts the root survives. Split the sentence so each half names what its own test does.
fac5ab7 to
4e57c71
Compare
| # The published record now carries tasktmp=, so teardown can find the root; a | ||
| # later abort must leave it for teardown rather than remove it here. | ||
| TASK_TMP_ABORT_CLEANUP=0 |
There was a problem hiding this comment.
When a fresh spawn publishes its metadata but both attempts to move its backlog item to In flight fail, this code has already disabled TASK_TMP_ABORT_CLEANUP, while spawn_fresh_commit_rollback removes the record before exit. The abort trap then leaves the created task root without any durable metadata through which teardown can discover it, causing its scratch directory to leak indefinitely.
jason-curtis
changed the title
|
A data point for this PR: we hit the same fixed |
|
Speaking as Kun's firstmate: closing this as stale. It has been waiting on a contributor update for 14+ days with no author push or comment. Reopen if you want to pick it back up. Why close: last author activity was jason-curtis force-push 2026-09-02T15:56:44Z (~20 days). The 2026-09-23T14:59:30Z thread bump is from Thoughts-One (not the author) and does not reset the clock. Still CONFLICTING/DIRTY vs main — waiting-author, not CLEAN, not a captain hold. contract-class: new-default (own tip-vs-main from prior full review; reconfirmed by prior stamps and body). Tip adds per-task temp-root library + teardown of task temp roots / agent scratch on a path the unconfigured product did not previously clean that way; TMPDIR pin is claimed opt-in but the new teardown/temp-root surface is a default cleanup path change. Prior stamps already classed new-default; not re-litigated for stale close. VISION (brief, for the close): One captain/interface — aligns (less leftover scratch noise). Authority — aligns (no new consent surface). Scripts/mechanics — aligns (deterministic teardown). Restart — aligns (durable cleanup). Delegation — aligns. Fleet/vendor — aligns. Scope — aligns (command-layer temp hygiene). Stale waiting-author overrides merge consideration. |
Intent
Rebind the no-mistakes pipeline attestation to the current tip of an unchanged change set.
Underlying work (unchanged, already fully validated on the immediately preceding head): bin/fm-teardown.sh removes its own task's scratch directory so agent scratch stops leaking until reboot. The safe unit is the TASK, never the worktree slot, because the treehouse pool reuses slot numbers; teardown removes exactly the path its own task's record names and only when it equals the path bin/fm-task-tmp-lib.sh derives for that id, with no deletion by slot, age, or cross-session pattern. The TMPDIR pin is OPT-IN and OFF BY DEFAULT behind the gitignored config/task-tmpdir-pin presence flag; that opt-in shape is an upstream captain product decision and MUST NOT be changed here. Also included and unchanged: home-scoped roots via fm_backend_hometag, the deliberate refusal to remove legacy undiscriminated roots, forced-secondmate child cleanup deriving the child home tag, validated private creation (plain mkdir that fails rather than following a symlink, reuse only of a directory this user owns, mode 0700), an abort trap armed only for roots the spawn actually created, and removal of an aborted spawn's root BEFORE the per-task lock is released so a waiting same-id spawn cannot adopt a doomed directory.
WHY THIS RUN SKIPS TEST, DOCUMENT AND LINT: upstream main advanced from 8988af2 to 77ee3c8 and the PR went conflicting again. The branch was rebased onto 77ee3c8 with a single conflict in docs/verification/runtime-backends.md, where main added a new dated Pi verification section and this branch appends a 'Per-task temp root and harness scratch' section; both were kept whole and neither was altered. Verified after the rebase: the branch's own bin/ and tests/ content is byte-identical to the previously validated head, the only differences being main's own new Pi test files; 21 commits, 0 behind main, no Co-Authored-By trailer on any commit, exactly one scout-scratch evidence claim commit, and bin/fm-lint.sh exits 0 locally with ShellCheck 0.11.0 and actionlint 1.7.12 already verified on this exact tree. The content therefore carries the full validation it already passed, and the only thing that changed is the commit SHAs the rebase produced. The sole purpose here is to bind the attestation to the new tip so upstream's body-compliance check sees a matching head_sha.
Make NO code, test or documentation change, and no design change to the opt-in pin shape. If review raises a finding that requires a real fix, say so rather than editing silently. Push to the fork https://github.com/jason-curtis/firstmate and update PR #3457 against origin. Do not merge; upstream owns that.
What Changed
bin/fm-task-tmp-lib.shas the single owner of the per-task temp root: a home-scoped<base>/fm-<tag>-<id>path derived withfm_backend_hometag, guarded creation (plainmkdir -m 700that fails rather than following a symlink, reusing an existing root only when this user owns it), and removal that acts only on the exact path the task's owntasktmp=record names — never by worktree slot, age, or sibling pattern, and never on the older undiscriminated<base>/fm-<id>shape, which is refused and reported instead.FM_TASK_TMP_BASEoverrides the/tmpparent for tests.bin/fm-spawn.shnow derives and validates the root through that library, arms an abort cleanup only for a root the spawn actually created, and removes it before releasing the per-task lock so a waiting same-id spawn cannot adopt a doomed directory; the launchTMPDIRpin to that root is opt-in behind the gitignoredconfig/task-tmpdir-pinpresence flag (off by default, never applied to a raw launch command, inherited by secondmate homes).bin/fm-teardown.shremoves its own task's root through the same ownership check, feeds the validated path to its process reaper, and applies the same removal to each child of a forced secondmate retirement using that child's own home tag; refusals and failures warn on stderr without failing teardown.FM_TASK_TMP_BASEoverride indocs/configuration.md,AGENTS.md,docs/scripts.md, the secondmate-provisioning skill, and a new "Per-task temp root and harness scratch" verification record; extendedbin/fm-test-run.shchange-path routing for the new library, the home-tag library, and teardown, and added coverage acrosstests/fm-gotmp.test.sh,fm-spawn-dispatch-profile,fm-kimi-harness,fm-control-relaunch,fm-teardown, andfm-backend-orca.Risk Assessment
✅ Low: The destructive path is tightly bounded by an exact-match ownership guard with a single owning library, every intent-required behavior is present and covered by concrete regressions (removal, ownership refusals, symlink/non-directory creation refusals, both sides of the opt-in pin, and both aborted-spawn arming outcomes), and the only two findings are non-reachable-in-practice hygiene items in an unchanged-content rebase.
Testing
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-spawn.sh:2517- The abort-cleanup arming reads FM_TASK_TMP_CREATED only after fm_task_tmp_create returns success, but the library sets that flag to 1 as soon asmkdir -m 700 -- "$root"succeeds and can still return 1 afterwards ifmkdir -p -- "$root/gotmp"fails (bin/fm-task-tmp-lib.sh:159). Concrete sequence: spawn creates /tmp/fm-<tag>-<id> successfully, the nested gotmp/ mkdir fails (ENOSPC, quota, or a transient filesystem error), fm_task_tmp_create returns 1, line 2517 exits 1 before line 2525 runs, so TASK_TMP_ABORT_CLEANUP stays 0 and the trap leaves behind a root this spawn created and nothing will ever name. The leaked directory is empty, so impact is a stray 0-byte directory rather than retained scratch, and the failure mode is narrow. The mechanical remedy is to arm from FM_TASK_TMP_CREATED regardless of the return status (set the flag, then check the status), which preserves the "only roots this spawn created" contract exactly. Noted rather than requested: this run's intent forbids code changes, and the exposure does not justify one.tests/fm-gotmp.test.sh:409- test_teardown_survives_an_unremovable_tasktmp brackets the teardown invocation withset +e(line 405) andset +eagain (line 409), where the closing line reads as an intendedset -e. The suite only ever setsset -u(line 29) and never enables errexit, so both lines are no-ops and no assertion is weakened - restoringset -ethere would actually be wrong, since the rest of the suite runs without it. The defect is purely that the pair reads as a guarded region it is not, inviting a future edit to "restore" errexit into a suite that has never used it. The honest remedy is to drop both lines. Noted only; this run's intent forbids test changes.⏭️ **Test** - skipped
Step was skipped.
⏭️ **Document** - skipped
Step was skipped.
⏭️ **Lint** - skipped
Step was skipped.
✅ **Push** - passed
✅ No issues found.