fix: protect Treehouse pool slot ownership - #135
Merged
Merged
Conversation
* fix(bin): verify pool-slot ownership before returning a worktree slot Workers were killed when cleanup returned a Treehouse pool slot that a different, live task had already taken. Teardown now proves the slot is genuinely this task's before releasing it: it refuses when another task record claims the same live worktree path, or when the endpoint's working directory contradicts the recorded slot, and that refusal holds under --force. Slot allocation, metadata publication, ownership verification, and slot return are serialized across linked firstmate homes, and forced secondmate cleanup verifies descendant slot ownership before returning any child worktree. Regression coverage drives the scripts with two task records naming one slot path and asserts the live worker survives and its slot is not reset. * no-mistakes(review): Protect slots across cloned Firstmate homes * no-mistakes(test): Gate teardown locking on genuine Treehouse slots * no-mistakes(test): Clarify pooled descendant slot gating * no-mistakes(test): Synchronize watcher re-arm test on process exit * no-mistakes(test): Wait for watcher cleanup before timeout escalation * no-mistakes(document): Document pool-slot ownership safeguards * no-mistakes(ci): Fixed all reported CI issues: normalized bare local Git origins to the same Treehouse project-lock identity as absolute clone origins; resolved ShellCheck SC1091 with explicit conditional sourcing; and taught concurrent Herdr teardown coverage to retry expected Treehouse lock contention. Added behavioral regression coverage for bare/absolute origin lock identity. Verified endpoint-safety tests, watcher tests, full CI lint, and the previously failing Herdr teardown assertion * fix(bin): resolve relative origins from repository root * no-mistakes(ci): Fixed teardown so an exact recorded endpoint may change cwd without falsely vetoing cleanup. Removed cwd-based ownership refusal while preserving cross-home record exclusivity and project locking. Updated behavioral coverage for both foreign slot ownership refusal and moved-cwd teardown success. Endpoint-safety, backend, watcher, checkpoint, and targeted lint checks pass. Real Herdr presentation E2E progressed successfully but exceeded the 600s local timeout (cherry picked from commit b028e8b)
… untouched (kunchenguid#4243) * fix(teardown): refuse to return a Treehouse pool slot reassigned to another task A pool slot is reused across tasks, so a finished task's worktree= line can name a slot a different, live task now holds. Teardown already refused when a second task record named the same live path, but that scan cannot prove the record it is tearing down is the current owner: the task that took the slot next may leave no record the scan can reach - its own worker may have exited and its record been cleaned up, or it may live in a home this machine does not register. Teardown then killed every process under the path, hard-reset it and returned it, and its unlanded-work refusal never fired because it was inspecting a directory that no longer belonged to the task being torn down (observed 2026-09-07). Treehouse's own state file cannot answer the ownership question. It records a slot's owner as a live process lease (owner_pid plus owner_started_at, with `treehouse status` reporting in-use from the processes actually running under the path), which names no task and is released by the very event that makes a record stale - the worker exiting. An unleased slot therefore reads identical whether it is still this task's or has since been handed on, and a slot whose new holder has also exited but left uncommitted work reads as free. So the identity source is Firstmate's own claim, not Treehouse's lease. fm-spawn writes that claim - the task id - into the slot at the moment it takes it, under the same project lock that allocates the slot, and fm-teardown drops it only after the slot is genuinely returned. It lives at <pool>/<slot>/.fm-slot-owner, a sibling of the repo checkout rather than a file inside it, so claiming a slot can never dirty the copy the landed-work checks inspect. A claim naming another task, or one that cannot be read, refuses; --force does not lift either refusal, because --force authorizes discarding this task's unlanded work, never another task's live work. A slot that cannot be claimed refuses the spawn instead. An absent claim proceeds on exactly the record-scan protection it had before: slots taken before claims existed, and slots already returned, carry none, and refusing those would strand every task in flight across this change on no evidence at all. The refusal is deliberately all-or-nothing rather than partially completing the task's own cleanup. state/<id>.meta is the only durable record naming the worktree and endpoint, so removing it would destroy the evidence needed to reconcile which record is wrong, and its removal is one step with the backlog transition. Nothing is stranded: clearing the stale worktree= line leaves a record with no slot to release, which then tears down normally, and the refusal names that remedy. Repairing the previous claimant's stale worktree= line at spawn time is left for separate work. It would have the new owner write another task's record - the same class of cross-task mutation this bug is - and would need that record's own meta lock; with the claim in place teardown refuses on evidence rather than depending on the stale pointer having been scrubbed. For the same reason the relaunch path writes no claim: it holds no allocation lock, and a record whose worktree= is already stale would stamp the wrong task's claim onto a live sibling's slot. The regression reproduces the reuse sequence with only one discoverable record, including a clean, fully landed ship copy torn down without --force - the shape of the real incident, which the previous code returned to the pool - and fails against the previous code; the existing two-record, cross-home, own-slot and no-claim cases still pass unchanged. This builds ON upstream b028e8b (kunchenguid#3837), which is already in this branch's base (origin/main 40c50ea) and owns the record-exclusivity scan. Nothing here replaces that scan; the claim is the positive proof it cannot supply. Claude-Session: https://claude.ai/code/session_01JTBmuqKugaPUj7k9TXQwFS * no-mistakes(review): teardown leaves reassigned slot; spawn abort drops claim * no-mistakes(review): narrow Treehouse lease evidence; gate abort claim release on lock * no-mistakes(review): pin spawn-side slot claim; narrow abort-release header * no-mistakes(document): docs: point slot-claim rationale at fm-wake-lib owner (cherry picked from commit 7d14fc1)
Adapts the lock anchoring to this fork's remote secondmate homes (the route=remote parent record bin/fm-secondmate-parent-lib.sh owns): a remote binding ends the local root walk at the home holding it, matching upstream 64d3905, so a remote-seeded home anchors and resolves its own project lock instead of failing to derive one and refusing every pool-slot teardown.
Bumps bin/fm-install-treehouse.sh from v2.1.1 to v2.3.0 with the release's published SHA-256 for all four archives (linux/darwin x amd64/arm64). v2.3.0's acquire fails closed on slots it cannot verify - skipping them and provisioning a fresh slot instead of attempting a reset - and its status reports a marker-absent slot as damaged rather than available, which removes the reset attempt that produced the misleading ancestry refusal in the aceh pool-orphan incident. Verified locally: a fresh install of the pinned archive reports v2.3.0; on a scratch pool containing a slot with a dangling .git marker and a slot with no marker at all, status reports the marker-absent slot damaged and get --lease skips both unverifiable slots without touching either, provisioning a new slot instead. Known-remaining gaps, documented rather than fixed per the captain's decision (scope: data/fm-aceh-pool-recovery-diagnosis/decision-pool-durable.md): a slot whose .git marker exists but resolves nowhere is still reported available (StatusDamaged covers only marker-absent slots), and the meta-absent lease-release path is still unimplemented.
…home Upstream b028e8b's collect_descendant_task_locks refuses any symbolic-link state path. This fork's secondmate contract (pinned by fm-secondmate-safety.test.sh) permits links that resolve inside the home - the target is removed with the home - while refusing links that escape it. Resolve the link target and compare it against the resolved home boundary: links escaping the home, or that cannot be resolved at all, still refuse; inside-home links proceed and the later blanket -L refusal is dropped.
7d14fc1 declares RELAUNCH_REPLACEMENT_* state for upstream's relaunch-replacement machinery, which this fork lacks. Shellcheck flagged the dead globals; removing them keeps the pick limited to the claim behavior being adapted.
The shared project lock now makes a concurrent same-project spawn refuse fast with "another Treehouse slot allocation or return is in progress" instead of racing allocation. Mirror upstream's task-set-lock tolerance: capture each spawn's status, accept that one refusal message, and retry the task once the winner publishes. Applied to the order pair, the post-create abort fixtures (which still must fail on their armed validation error), and the three focus waves. The cross-home wave keeps bare waits - those homes are not parent-linked, so each anchors its own lock and cannot contend.
…ed-secondmate child preflight (which consumed Orca mock responses twice) and allowed explicit STATE overrides to anchor the shared Treehouse project lock when the synthetic FM_HOME lacks state/. Verified fm-backend, fm-backend-orca, and fm-teardown-endpoint-safety tests pass. The Herdr presentation failure was an external timing/fixture timeout, not reproduced as a deterministic code defect
…POLLS The real-Herdr presentation suite failed twice in CI at the multi-home section's first spawn: treehouse get did not publish its acquired worktree within the fixed 60s. Diagnosis found no defect in the lock or pane path: the pane-side get is ~1s of work (~4s per spawn on CI), the pane never touches the project lock the waiting spawn holds, and treehouse's pool flock is the only unbounded wait in the path - a transient holder or a loaded runner stalls the pane's publication without bound. The ready-file wait is now read from FM_TREEHOUSE_READY_POLLS (default unchanged at 60s); the suite exports 240, ~18x over the worst observed CI get-bearing step, so a transient stall at the suite's peak-load point stays inside the budget.
dnth
changed the title
…n from either the Treehouse project lock or the task-publication lock, retrying after the winner completes. The full real-Herdr presentation E2E suite passes
…E2E multi-home spawn failure paths: parses inspect targets, captures pane output/process info/cwd, checks ready-file artifacts, and snapshots treehouse processes without changing test outcomes. `bash -n`, `git diff --check`, and the full presentation E2E suite pass locally
… with a scoped suppression preserving the required full `ps` snapshot. Targeted lint, syntax, and diff checks pass; the Herdr E2E suite passes locally. The CI Herdr failure appears environment-specific (Herdr 0.7.4 pane disappeared while an orphaned treehouse process remained), not reproduced locally
…he target pane disappears during ready-file polling, with an accurate pane-death error. Added a single retry for pane-disappeared multi-home spawns while retaining final diagnostics. `bin/fm-lint.sh`, shell syntax/diff checks, and the full Herdr presentation E2E suite pass locally
…eserve workspace-qualified pane IDs and capture qualified pane read, pane status fields, and process-info. Bash syntax, ShellCheck, diff checks, and the full Herdr presentation E2E test pass locally
…ulti-home E2E suite. It records workspace/pane lists, qualified pane status (agent_status, foreground_cwd, cwd), and recent pane output every ~15s, includes the snapshot tail in spawn failure diagnostics, and stops cleanly after the section. Bash syntax, ShellCheck, and diff checks pass; a local run began successfully but exceeded the 45s bounded smoke window
…`read-tree --reset` with ancestry validation and SAFE_FILE publication, propagated pane-side acquisition failures via `.failed` ready markers, and surfaced them promptly from `fm-spawn.sh`. `bash -n`, targeted ShellCheck, diff checks, and `fm-treehouse-orphan-recovery` plus `fm-spawn-pool-base-freshen` tests pass
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
3 participants
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
Ship the durable half of the Treehouse pool-orphan diagnosis (data/fm-aceh-pool-recovery-diagnosis/report.md): pool slots whose backing repositories disappeared or were reassigned must not be mistaken for usable slots, and firstmate teardown must not reset, kill, or return a slot another task owns.
Requirements: (1) bump the treehouse installer pin in bin/fm-install-treehouse.sh from v2.1.1 to a v2.3.x release with updated release hashes, with the installed treehouse failing closed on unverifiable slots instead of attempting resets; (2) adapt upstream firstmate b028e8b - slot-ownership verification before destructive teardown; (3) adapt upstream 7d14fc1 - a durable .fm-slot-owner claim written at spawn under the shared allocation lock; (4) do NOT implement the two open-ended gaps (marker-present-but-unresolvable slots still reported available; meta-absent lease-release path) - they are documented as known-remaining in the commit message per the captain's explicit scope decision.
Deliberate adaptation decisions a reviewer should not flag: upstream files absent from this fork (fm-backlog-transition-lib.sh, fm-remote-job-reap-orphans.sh, fm-home-summary-refresh.sh, submodule test helpers, relaunch-replacement machinery) were dropped rather than imported; fork contracts preserved - inside-home state symlinks remain allowed in collect_descendant_task_locks (only links escaping the home refuse), devin/hermes hook-token cleanup lists kept, validate_spawn_pool_lease and OMP marker clearing kept ahead of the claim write, and the claim is written only on the pane-driven get path because raw-launch and OMP paths already use Treehouse's durable --lease --lease-holder. The shared project lock intentionally makes a concurrent same-project spawn or teardown refuse fast with "another Treehouse slot allocation or return is in progress"; the herdr e2e test now retries the losing spawn once the winner publishes, mirroring upstream's task-set-lock tolerance. A remote secondmate parent binding terminates the local root-home walk (adapted from upstream 64d3905) so a remote-seeded home anchors its own project lock. Post-pipeline follow-up authorized by firstmate decision nm-ci-herdr-rerun: the ready-file publication wait reads FM_TREEHOUSE_READY_POLLS (production default unchanged at 60) and the herdr e2e suite exports 240, a bounded mitigation for a twice-observed CI-only 60s publication stall at the suite's peak-load point where the pane's get waits on treehouse's pool flock.
Acceptance: AC1 installer pin v2.3.x with correct hashes and fail-closed acquire/skip proven on a scratch pool; AC2 teardown refuses/leaves untouched a slot another task record names, with regression tests; AC3 a mismatched owner claim blocks return, with regression tests; AC4 full no-mistakes validation green on the final head. Never merge the PR - the captain holds merge authority.
Firstmate-Validation-Generation: 9266ce7b8687e04dbac7585b6756d3ec
What Changed
.fm-slot-ownerclaims; teardown now checks cross-home slot ownership and leaves reassigned or unreadable slots untouched while completing only the task’s own cleanup.Risk Assessment
Testing
Drove installer verification, fail-closed scratch-pool acquisition, contested and malformed ownership teardown guards, durable spawn claims, and orphan recovery end-to-end; all scenarios passed with reviewer-visible logs captured.
bash tests/fm-teardown-endpoint-safety.test.shbash tests/fm-teardown-endpoint-safety.test.shbash tests/fm-spawn-pool-base-freshen.test.shbash tests/fm-treehouse-orphan-recovery.test.shEvidence: Treehouse installer v2.3.0 verification
Evidence: Scratch pool fail-closed acquisition
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (4) ✅
bin/fm-teardown.sh:2479- Forced secondmate teardown now acquires each descendant home's.task-set.lockand assumes this serializes task publication, butfm-spawn.shnever acquires that lock before creating/publishing a task record (it only uses per-task and Treehouse project locks). A spawn can therefore publish a new child aftercollect_descendant_task_locksenumerates the home; the later descendant preflight/cleanup can process and return that newly published task's slot without lifecycle locking, leaving another worker's slot destructively reclaimed. Add the matching task-set lock acquisition to the spawn publication path with the documented lock ordering.bin/fm-watch-checkpoint.sh:66- The new timeout handler interpretsFM_SIGNAL_GRACE=0as unset because ofmy $grace = $ENV{FM_SIGNAL_GRACE} || 5, changing the established zero-grace behavior (used by existing watcher/test invocations) into a 5-second wait before SIGKILL. Preserve an explicitly supplied zero while defaulting only when the variable is absent or invalid.🔧 Fix applied.
2 issues (1 error, 1 warning) still open:
bin/fm-watch-checkpoint.sh:73- WhenFM_SIGNAL_GRACE=0, the timeout handler executesalarm 0, which cancels the SIGKILL alarm, then blocks forever inwaitpidif the child ignores SIGTERM. This regresses the documented zero-grace behavior and can leave checkpoint cleanup hung indefinitely; handle zero as an immediate kill (or otherwise preserve a non-cancelled kill path).15000000:1- The diff adds a tracked top-level file named15000000containingbad, which is unrelated to the Treehouse pool durability intent and has no required runtime or test role. Remove this unrequired component (the remedy is a scope cleanup requiring authorization under the simplification policy).🔧 Fix applied.
1 error still open:
bin/fm-wake-lib.sh:1054-fm_treehouse_slot_owner_stateaccepts duplicate claim fields and uses the lasttask=line (bin/fm-wake-lib.sh:1054-1066). A malformed or tampered marker such astask=stale-task\ntask=current-taskis therefore classified asmine, allowing teardown to kill/reset/return a slot actually reassigned to another task. Parse the claim as a strict format (exactly one validtask=and reject duplicate/unknown malformed lines) and leave itunsafeon ambiguity.🔧 Fix applied.
1 error still open:
bin/fm-spawn.sh:3932- Iffm_treehouse_slot_owner_claimfails after the pane-drivenfm-treehouse-get.shhas acquired a slot (for example, when.fm-slot-owneris an unclaimable directory),fm-spawn.shexits at this point without marking the acquisition for abort cleanup or returning the lease. The pane remains in the Treehouse shell and the slot can stay leased indefinitely, stranding pool capacity and causing later spawns to fail. Ensure the claim-failure path returns the just-acquired slot (or otherwise arms equivalent cleanup) before refusing the spawn.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-teardown-endpoint-safety.test.shbash tests/fm-teardown-endpoint-safety.test.shbash tests/fm-spawn-pool-base-freshen.test.shbash tests/fm-treehouse-orphan-recovery.test.shbash tests/fm-teardown-endpoint-safety.test.shbash tests/fm-spawn-pool-base-freshen.test.shbash tests/fm-treehouse-orphan-recovery.test.shbash bin/fm-install-treehouse.sh <temporary destination>Live v2.3.0 scratch-pool acquisition with tampered state path✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.