Repository navigation
fix: safely reap orphaned lab and test processes - #76
Merged
Merged
Conversation
A lock wait on a deleted directory spun forever (fm-wake-grant.sh release, lab watchers), and test stubs outlived dead tests and their fixtures. - fm_lock_acquire_wait returns 1 once the lock directory has been gone for a grace - bin/fm-test-reap-orphans.sh stops processes a fixture marker proves belong to an ended run - tests/lib.sh runs it at cleanup (owner) and at start (dead owners) - fm-test-run.sh runs it after each script
…and cleanup safety
… review-only test dispatches
…an ownership checks
…ure documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
I would like all bugs to be fixed so tomorrow can be focused entirely to Vernant and not problems preventning Vernant from getting built.
A recommended architecture should come with proof.
Context (the backlog record for this bug, as filed):
Reap orphaned lab and test processes (repo: firstmate) (kind: ship) (since 2026-10-08)
Reap orphaned scratch-copy watchers, poll-publish-holder.sh, fm-wake-grant.sh release orphans and leftover go test binaries; extend fm-test-run.sh reaping to lab copies. Only processes provably owned by an ended lab or test run.
Source: data/fm-sandbox-offload-analysis/report.md (2026-10-08). 21-minute sample at load 102 on 18 cores: 0.1% idle, 56% kernel time, ~61% of CPU in sub-second processes, ~2,850 new processes per second, firstmate supervision scripts start 61% of short-lived process sightings (sleeps excluded); 106 of 155 agent sessions idle the whole window holding ~33 GB. Target for the R4 set over one fleet day: 1-min load <= 36 for >= 90% of the day, process creation < 500/s, kernel time < 25%, no new timeout-class failures. Pattern: measure, fix the root, test, measure again (as fm-wake-drain-fast did).
What Changed
Risk Assessment
✅ Low: The ownership-gated cleanup and lock-failure propagation are consistent with the stated intent, and the complete source review found no substantiated material defects or unnecessary introduced components.
Testing
Focused regressions and live watcher, private-tmux, runner, identity-transition, and mutation checks passed after disposable-driver setup corrections; baseline execution reproduced the vanished-state hang, CLI/state evidence was retained, and fixtures were removed without running broad suites or fleet-day benchmarking.
Evidence: Real watcher, private runtime, and grant-release transcript
Source: Real watcher, private runtime, and grant-release transcript
Evidence: Runner reaps killed and timed-out scripts' detached processes
Source: Runner reaps killed and timed-out scripts' detached processes
Evidence: Actual ownership snapshots and surviving changed identities
Source: Actual ownership snapshots and surviving changed identities
Evidence: Observed lock-failure results for eleven operations
Source: Observed lock-failure results for eleven operations
Evidence: End-to-end ownership, Go cwd, and startup-recovery regressions
Source: End-to-end ownership, Go cwd, and startup-recovery regressions
Evidence: Supplementary deterministic ownership and mutation regressions
Source: Supplementary deterministic ownership and mutation regressions
Evidence: Baseline release remains running after state disappears
Source: Baseline release remains running after state disappears
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 6 issues found → auto-fixed (10) ✅
bin/fm-test-reap-orphans.sh:264- The ownership snapshot contains PID, parent and command, but no birth identity. If a selected short-lived descendant exits and its PID is reused before line 264, the replacement's identity is recorded and still_same subsequently authorizes signalling an unrelated process. Capture identity together with ownership evidence and revalidate that evidence before accepting the target. Related sites: bin/fm-test-reap-orphans.sh:163 captures the unbound snapshot; :244 propagates descendant ownership; :274 checks only the later identity; :294-295 signals CONT and TERM after one check; :308-309 authorizes KILL. The new tests also retain already-reaped PIDs and unconditionally signal them at tests/fm-test-reap-orphans.test.sh:30; record and check their identities before cleanup.bin/fm-test-reap-orphans.sh:127- A failed identity lookup is treated as proof that the owner ended, even after kill -0 established that its PID exists. For example, a live test has a detached polling stub, another test starts a sweep, and the owner's ps lookup fails under resource pressure: marker_owner_dead returns success and the live test's stub becomes eligible for termination. Unknown identity must leave the root untouched, not authorize reaping. Related sites: bin/fm-test-reap-orphans.sh:125 establishes liveness; :156 admits the root; :233 selects its detached processes; tests/lib.sh:432 and bin/fm-test-run.sh:2622 invoke this decision automatically.bin/fm-wake-lib.sh:1281- The newly possible acquisition failure is not propagated by existing callers. fm-wake-grant.sh uses set -u, not set -e, and continues after failure in activate, publish, release and deactivate. If a deleted state directory returns just after the grace expires and another process acquires its queue lock, the old waiter can inspect or change grant records without owning that lock. Update callers to stop before entering protected operations when acquisition fails. Related sites: bin/fm-wake-lib.sh:2121-2122 performs append after an unchecked acquire; :2181-2183 performs a supposedly locked read and can report success after failure; bin/fm-wake-grant.sh:44, :60, :89 and :98 all mark the lock held unconditionally.bin/fm-test-reap-orphans.sh:154- Required orphan classes remain outside the implementation. The criterion explicitly requires 'Reap orphaned scratch-copy watchers ... and leftover go test binaries; extend fm-test-run.sh reaping to lab copies.' The new scan accepts only fm-/.fm-test-fixture, whereas fm-lab-home.sh creates .fm-lab-home and fm-live-lab.sh creates fmlab. roots with .fm-live-lab records. An ended lab's detached watcher under a still-existing scratch home is therefore skipped. An already-reparented Go test binary whose argv names go-build/... rather than a marked fixture root is also skipped, even when its cwd belongs to the ended run. Related sites: bin/fm-test-reap-orphans.sh:148 requires the test marker in owner mode; :232 requires an argv root match; :244 can recover rootless descendants only while ancestry remains in the snapshot; bin/fm-test-run.sh:2618-2622 merely runs the same restricted reaper from the local copy. Complete attribution at the lab/process creation boundary without weakening ownership proof; any additional durable provenance machinery needed for that remedy requires authorization.bin/fm-test-reap-orphans.sh:65- Simplification: the new public --dry-run mode is not required by the orphan-reaping intent. Executable termination and non-termination assertions can supply the requested proof without introducing a separate preview interface. Recommend removing this optional mode and its parallel output/control paths at bin/fm-test-reap-orphans.sh:282-289, together with its dedicated cases at tests/fm-test-reap-orphans.test.sh:127-130 and :175-179.bin/fm-wake-lib.sh:1273- Simplification: FM_LOCK_PARENT_GONE_GRACE_SECONDS introduces a configurable behavior beyond the stated requirement to end waits on vanished state. No intent requirement needs operators to select this grace; a fixed five-second grace is narrower. Recommend removing the override and its validation branch at bin/fm-wake-lib.sh:1273-1274, and exercising the fixed grace in tests/fm-wake-queue.test.sh:2779 and :2807 rather than adding a configuration surface for those cases.🔧 Fix applied.
6 issues (4 errors, 2 warnings) still open:
bin/fm-wake-lib.sh:1281- The newly possible acquisition failure is not propagated by existing callers. fm-wake-grant.sh uses set -u, not set -e, and continues after failure in activate, publish, release and deactivate. If a deleted state directory returns just after the grace expires and another process acquires its queue lock, the old waiter can inspect or change grant records without owning that lock. Update callers to stop before entering protected operations when acquisition fails. Related sites: bin/fm-wake-lib.sh:2121-2122 performs append after an unchecked acquire; :2181-2183 performs a supposedly locked read and can report success after failure; bin/fm-wake-grant.sh:44, :60, :89 and :98 all mark the lock held unconditionally.bin/fm-test-reap-orphans.sh:154- Required orphan classes remain outside the implementation. The criterion explicitly requires 'Reap orphaned scratch-copy watchers ... and leftover go test binaries; extend fm-test-run.sh reaping to lab copies.' The new scan accepts only fm-/.fm-test-fixture, whereas fm-lab-home.sh creates .fm-lab-home and fm-live-lab.sh creates fmlab. roots with .fm-live-lab records. An ended lab's detached watcher under a still-existing scratch home is therefore skipped. An already-reparented Go test binary whose argv names go-build/... rather than a marked fixture root is also skipped, even when its cwd belongs to the ended run. Related sites: bin/fm-test-reap-orphans.sh:148 requires the test marker in owner mode; :232 requires an argv root match; :244 can recover rootless descendants only while ancestry remains in the snapshot; bin/fm-test-run.sh:2618-2622 merely runs the same restricted reaper from the local copy. Complete attribution at the lab/process creation boundary without weakening ownership proof; any additional durable provenance machinery needed for that remedy requires authorization.bin/fm-test-reap-orphans.sh:65- Simplification: the new public --dry-run mode is not required by the orphan-reaping intent. Executable termination and non-termination assertions can supply the requested proof without introducing a separate preview interface. Recommend removing this optional mode and its parallel output/control paths at bin/fm-test-reap-orphans.sh:282-289, together with its dedicated cases at tests/fm-test-reap-orphans.test.sh:127-130 and :175-179.bin/fm-wake-lib.sh:1273- Simplification: FM_LOCK_PARENT_GONE_GRACE_SECONDS introduces a configurable behavior beyond the stated requirement to end waits on vanished state. No intent requirement needs operators to select this grace; a fixed five-second grace is narrower. Recommend removing the override and its validation branch at bin/fm-wake-lib.sh:1273-1274, and exercising the fixed grace in tests/fm-wake-queue.test.sh:2779 and :2807 rather than adding a configuration surface for those cases.bin/fm-wake-lib.sh:1281- Round 1 fixed six acquisition callers but left reachable sibling critical sections unchecked. For example, an acknowledgement waits while STATE disappears, acquisition returns 1, and STATE returns with another queue writer holding the lock. fm-wake-drain.sh continues without ownership; an append between its queue snapshot and replacement at :962 is silently lost. Remaining sites: bin/fm-wake-drain.sh:836,916 (acknowledgement acquisitions); bin/fm-watch.sh:2099 (queued process-event presentation); bin/fm-mail.sh:378 (append and delivery journal; wake_for is called through || at :601); bin/fm-afk-return.sh:913 (catch-up publication, which itself recreates STATE at :210); bin/fm-lock.sh:164,209 (session-lock and identity publication); bin/fm-tasks-axi.sh:307,309 (task mutations); bin/fm-backlog-handoff.sh:852,859 (conditional pending-delivery recovery); bin/fm-control.sh:677 (busy-generation metadata cleanup); bin/fm-spawn.sh:1688,5860 (conditional presentation recovery and trace metadata publication); bin/fm-x-lib.sh:920,951,1009 (request-link publication, follow-up count, and link removal). These paths either lack errexit or execute inside condition-tested functions where it is disabled. Explicitly propagate acquisition failure at each critical-section entry before reading protected records, marking the lock held, or mutating state. The acknowledgement-atomicity claim at docs/watcher-continuity.md:319 must hold for these paths too.tests/fm-test-reap-orphans.test.sh:242- Round 1's F1 cleanup fix still accepts a replacement process's identity as ownership evidence. child_env normal completes only after its exit sweep has reaped the stub, but :242 then registers that already-reaped PID; if it has been reused, track at :40 records the unrelated replacement's identity and cleanup at :30-32 authorizes killing it. The killed-child registration at :249 has the same exposure when another automatic sweep reaps the dead owner's stub before registration. A remaining sibling is tests/fm-test-run.test.sh:2080-2087: after the runner has reaped the stub, the test polls the old PID and unconditionally kills any replacement found there. Remove registration of the already-cleaned normal stub, and have fixtures record identity while establishing process ownership for the other cases. Use that original identity for disappearance checks at tests/fm-test-reap-orphans.test.sh:45-50 and tests/fm-test-run.test.sh:2082-2087, and revalidate it before cleanup signals.🔧 Fix applied.
2 warnings still open:
tests/fm-wake-queue.test.sh:2789- The vanished-parent regression kills and waits for the holder before deleting state at lines 2789–2791. Between those operations, the waiter can legitimately reclaim the dead holder's lock and write waiter.rc=0, causing a false test failure. fm_lock_try_acquire immediately permits reclaiming a dead numeric holder; no freshness grace prevents this race. Round 2 retained this sequencing while changing the test to exercise the fixed grace. Remove or rename the parent while the holder is still alive, then terminate and reap the holder. The same test is reached through the ordinary suite call at tests/fm-wake-queue.test.sh:3781 and the targeted call at :3773.tests/fm-test-run.test.sh:2422- Simplification: Round 2 introduced permanent --orphan-review subset modes here and at tests/fm-wake-queue.test.sh:3772. The authorized fixes require executable regression tests, not additional suite-selection interfaces; both regressions already run through their ordinary suites. Revert these two dispatch branches to the minimal fix, retaining the regression tests and their normal suite calls.🔧 Fix applied.
3 warnings still open:
tests/fm-wake-queue.test.sh:2789- The vanished-parent regression kills and waits for the holder before deleting state at lines 2789–2791. Between those operations, the waiter can legitimately reclaim the dead holder's lock and write waiter.rc=0, causing a false test failure. fm_lock_try_acquire immediately permits reclaiming a dead numeric holder; no freshness grace prevents this race. Round 2 retained this sequencing while changing the test to exercise the fixed grace. Remove or rename the parent while the holder is still alive, then terminate and reap the holder. The same test is reached through the ordinary suite call at tests/fm-wake-queue.test.sh:3781 and the targeted call at :3773.tests/fm-test-run.test.sh:2422- Simplification: Round 2 introduced permanent --orphan-review subset modes here and at tests/fm-wake-queue.test.sh:3772. The authorized fixes require executable regression tests, not additional suite-selection interfaces; both regressions already run through their ordinary suites. Revert these two dispatch branches to the minimal fix, retaining the regression tests and their normal suite calls.tests/fm-wake-queue.test.sh:2788- Round 3 closes the dead-holder reclaim race but leaves two ways this regression can fail with correct production code. Recursive rm removes the lock before removing its parent; a waiter scheduled between those operations can legitimately acquire and record rc=0. Separately, the sleep/liveness check at :2785–2787 does not establish that the waiter finished sourcing fm-wake-lib.sh at :2780; a delayed waiter can run that library's mkdir -p initialization after deletion, recreate STATE, and acquire successfully. Synchronize on waiter readiness after library initialization, then atomically rename STATE away while the holder is alive before terminating and reaping it. The invariant applies to both entrypoints at tests/fm-wake-queue.test.sh:3773 (targeted dispatch) and :3779 (ordinary suite).🔧 Fix applied.
2 issues (1 error, 1 warning) still open:
bin/fm-test-reap-orphans.sh:220- Round 2 added lab provenance but left discovery limited to immediate children of the scan directory. A supported scratch-copy layout such as /tmp/fm-lab.XXXXXX/home carries its marker at home/.fm-lab-home, which this glob never reads. If its creator dies with a detached home/bin/fm-watch.sh running, the runner's sweep finds no proven root and exits successfully at :229 without stopping the watcher. Nested lab homes are already used at tests/fm-backend-herdr-recovery-lock-e2e.test.sh:56 and :634-635. The affected boundary is marker discovery here; bin/fm-lab-home.sh:69 writes provenance at the actual home, while bin/fm-test-run.sh:2622 and tests/lib.sh:432 both depend on this default discovery. Discover nested marked homes within the bounded temporary lab/test containers, retaining the existing owner, runtime, parent, and target-identity checks.tests/fm-orphan-safety.test.sh:240- Simplification: the new per-case suite-selection interface is another optional review-only dispatch surface, alongside the two removed in Round 4. No orphan-reaping requirement needs these selectors: the default all branch already executes every regression. Remove the identity selectors at :240, queue/grant selectors at :241, and request-link selectors at :242, retaining all regression functions and their ordinary suite execution. This reduces the earlier fix round to the required behavioral proof without changing product behavior.🔧 Fix applied.
2 warnings still open:
tests/fm-orphan-safety.test.sh:240- Simplification: the new per-case suite-selection interface is another optional review-only dispatch surface, alongside the two removed in Round 4. No orphan-reaping requirement needs these selectors: the default all branch already executes every regression. Remove the identity selectors at :240, queue/grant selectors at :241, and request-link selectors at :242, retaining all regression functions and their ordinary suite execution. This reduces the earlier fix round to the required behavioral proof without changing product behavior.tests/fm-test-reap-orphans.test.sh:49- Round 2's F8 fix introduced an identity lookup that selects the oldest registration of a PID. This suite repeatedly terminates fixtures and registers new processes without removing old registrations. If a later fixture receives a recycled PID, track appends its new identity, but alive selects the earlier, dead fixture's identity and reports the currently owned process as gone. Consequently, a correct reaper can fail a preservation assertion, or wait_gone can pass while the new fixture remains running. Correct the shared registration/lookup boundary to select the most recently registered ownership identity, retaining identity checks before cleanup signals. Related sites are tests/fm-test-reap-orphans.test.sh:41 and :42 (append-only registrations), :60 (disappearance lookup), and the implicit-identity consumers at :155, :160, :164, :167, :168, :169, :197, :204, :236, :237, :238, :262, :266, :278, :303, :306, :309, :314, :349, :353, :359, :364, :365, :366, :418, :421, :437, :438, :443, :444, and :446.🔧 Fix applied.
1 warning still open:
tests/fm-orphan-safety.test.sh:240- Simplification: the new per-case suite-selection interface is another optional review-only dispatch surface, alongside the two removed in Round 4. No orphan-reaping requirement needs these selectors: the default all branch already executes every regression. Remove the identity selectors at :240, queue/grant selectors at :241, and request-link selectors at :242, retaining all regression functions and their ordinary suite execution. This reduces the earlier fix round to the required behavioral proof without changing product behavior.🔧 Fix applied.
2 warnings still open:
tests/fm-orphan-safety.test.sh:240- Simplification: the new per-case suite-selection interface is another optional review-only dispatch surface, alongside the two removed in Round 4. No orphan-reaping requirement needs these selectors: the default all branch already executes every regression. Remove the identity selectors at :240, queue/grant selectors at :241, and request-link selectors at :242, retaining all regression functions and their ordinary suite execution. This reduces the earlier fix round to the required behavioral proof without changing product behavior.tests/fm-orphan-safety.test.sh:6- Simplification: prior fix-round commit 6804c9f introduced the optional FM_ORPHAN_SAFETY_BIN alternate-code-tree interface. The required ownership and lock-failure regressions do not need this permanent override; setting it makes their results describe another checkout rather than the change under review. It selects the reaper at tests/fm-orphan-safety.test.sh:74, queue/grant/acknowledgement implementations at :99–103, :143–150 and :161–176, and request-link implementations at :191–192, :212 and :226. Reduce that fix round to the required behavioral proof by replacing the override with BIN="$ROOT/bin", retaining every regression.🔧 Fix applied.
1 warning still open:
tests/fm-test-reap-orphans.test.sh:106- Round 2 introduced an initialization race when capturing fixture identities; Round 6's newest-registration fix leaves it reachable. The launcher backgrounds a command and immediately reads its identity, which includes argv (bin/fm-wake-lib.sh:89 and :104). If the child has forked but not yet exec'd, the recorded identity describes the inherited shell. After exec, alive reports that still-running fixture as gone: preservation assertions can fail with correct production code, and wait_gone can falsely prove termination. Publishing the PID file afterward does not establish child readiness. Capture the initialized executable's identity before publishing readiness, or use an exec-stable birth identity with the expected command, retaining identity checks before cleanup signals. The same invariant applies in tests/fm-test-reap-orphans.test.sh:40 (shared track, called at :117, :143, :190, :233, :258, :340, :345 and :428), :93/:245/:272/:287 (owner-marker captures), :123 (identity fixture), and :391 (child-script identity publication); affected shared consumers are :30 (cleanup), :53 (alive) and :60 (disappearance).🔧 Fix applied.
1 warning still open:
bin/fm-test-reap-orphans.sh:227- The F12 nested-home fix round (3fd8c97) left the parallel runner's test-fixture sibling undiscoverable. With --jobs, fixture markers live at /tmp/fm-test-run.XXXXXX/wN/tmp/fm-*.XXXXXX/.fm-test-fixture (bin/fm-test-run.sh:2798–2809; tests/lib.sh:392). If hard termination interrupts the owning run before its worker sweep, an orphaned poll-publish-holder.sh can remain indefinitely: its actual loop at tests/fm-pr-check-security.test.sh:3333 has no bound. A subsequent ordinary startup sweep at tests/lib.sh:432 scans /tmp, but :227 only discovers immediate test markers and :230 recursively discovers only lab-home markers. It therefore returns successfully without recognizing this ended test's root. Extend the existing bounded-container discovery to nested .fm-test-fixture markers as well, preserving all ownership and target-identity checks. Related changed-code boundaries are bin/fm-test-run.sh:2622 (runner sweep), tests/lib.sh:392 (marker publication), and tests/lib.sh:432 (later startup recovery).🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
TMPDIR=<worktree-local fixture> GOCACHE=<fixture>/go-cache bash tests/fm-test-reap-orphans.test.shTMPDIR=<worktree-local fixture> bash tests/fm-orphan-safety.test.sh— completed after increasing the initial 120-second execution bound.python3 .nm-live-validation/drive.py— real scratch-copy watcher, private tmux runtime, vanished-state grant release, and brief state restoration.python3 .nm-live-validation/runner-proof.py— real scratch-copy runner with SIGKILL and timeout fixtures; scan-only isolation wrapper forwarded to the unchanged reaper.python3 .nm-live-validation/identity-proof.py— real direct and descendant processes changed executable identity after the actual ownership snapshot.python3 .nm-live-validation/mutation-proof.py— actual-time acquisition failures and successful retries for grant, queue, acknowledgement, and request-link operations.git archive --format=tar fd9b8b020201a029880b3980b545f352ae357a39 binfollowed by the baselinefm-wake-grant.sh release— remained running after state was absent for ten seconds.Scoped final orphan sweeps and process inspection; removed disposable drivers, scratch copies, generated Go binaries/cache, state fixtures, and private socket directory.✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.