From a96fab2fb769ac275c0f6a7eff1ef7c5815a7f03 Mon Sep 17 00:00:00 2001 From: AG Werschky Date: Mon, 3 Aug 2026 13:07:16 -0600 Subject: [PATCH 1/9] fix: durably lease ship and scout worktrees at spawn Plain treehouse get left pool slots available, so a later spawn could reuse a parked task's worktree and hard-reset unlanded work (#1441). Ship and scout spawns now acquire with treehouse get --lease --lease-holder , cd the pane into that path, and keep isolation assertions. Recovery reuses a recorded worktree without a second lease. Successful teardown still returns (and thus releases); refused teardown never reaches return, so the lease stays held. Pre-lease unleased worktrees remain usable without adopting a lease. --- bin/backends/cmux.sh | 4 +- bin/backends/herdr.sh | 10 +- bin/backends/tmux.sh | 12 +- bin/backends/zellij.sh | 16 +- bin/fm-spawn.sh | 114 ++++-- bin/fm-teardown.sh | 9 +- docs/architecture.md | 3 + tests/fm-spawn-dispatch-profile.test.sh | 16 +- tests/fm-spawn-worktree-lease.test.sh | 454 ++++++++++++++++++++++++ tests/fm-spawn-worktree-settle.test.sh | 40 ++- 10 files changed, 614 insertions(+), 64 deletions(-) create mode 100755 tests/fm-spawn-worktree-lease.test.sh diff --git a/bin/backends/cmux.sh b/bin/backends/cmux.sh index 12dc7629eb..66136a5b81 100644 --- a/bin/backends/cmux.sh +++ b/bin/backends/cmux.sh @@ -504,8 +504,8 @@ fm_backend_cmux_send_key() { # [expected-label] # fm_backend_cmux_send_text_line: send one line of TEXT then submit. cmux has # no single-call atomic "run and submit" primitive (like herdr's `pane run`), # so this composes send (literal) + send-key enter, exactly like zellij's -# equivalent - used for the fixed spawn-time commands (treehouse get, the -# GOTMPDIR export). +# equivalent - used for the fixed spawn-time commands (cd into the leased +# worktree, the GOTMPDIR export). fm_backend_cmux_send_text_line() { # [expected-label] fm_backend_cmux_send_literal "$1" "$2" "${3:-}" || return 1 fm_backend_cmux_send_key "$1" Enter "${3:-}" diff --git a/bin/backends/herdr.sh b/bin/backends/herdr.sh index 77c48a84a4..a94c703e5c 100644 --- a/bin/backends/herdr.sh +++ b/bin/backends/herdr.sh @@ -2275,16 +2275,16 @@ fm_backend_herdr_target_ready() { # # fm_backend_herdr_current_path: the live FOREGROUND process's cwd, or empty on # any error. Mirrors tmux's pane_current_path poll used for worktree-path -# discovery after `treehouse get`. +# discovery after the spawn-time cd into the leased worktree. # # Verified pitfall: `pane get`'s `.result.pane.cwd` is the pane's cwd AT # CREATION TIME - the top-level shell's cwd - and does NOT update when that -# shell `cd`s or enters a subshell (as `treehouse get` does). Reading it here +# shell `cd`s (as the spawn-time enter into a leased worktree does). Reading it here # would make fm-spawn.sh's worktree-discovery poll never see the pane "leave" # the project directory, since `cwd` stays frozen at the original path forever. # `.result.pane.foreground_cwd` tracks the ACTUALLY RUNNING foreground -# process's cwd instead, which is what changes when `treehouse get` enters its -# worktree subshell - confirmed live against a real treehouse acquisition. +# process's cwd instead, which is what changes when the pane cds into the +# leased worktree - confirmed live against a real treehouse acquisition. fm_backend_herdr_current_path() { # fm_backend_herdr_target_ready "$1" || return 0 fm_backend_herdr_cli "$FM_BACKEND_HERDR_SESSION" pane get "$FM_BACKEND_HERDR_PANE" 2>/dev/null \ @@ -2293,7 +2293,7 @@ fm_backend_herdr_current_path() { # # fm_backend_herdr_send_text_line: send one line of TEXT then submit, # ATOMICALLY - mirrors tmux's `send-keys -t T text Enter`. Used for the fixed -# spawn-time commands (treehouse get, the GOTMPDIR export). `pane run` types +# spawn-time commands (cd into the leased worktree, the GOTMPDIR export). `pane run` types # the command and submits it in one call (verified). fm_backend_herdr_send_text_line() { # fm_backend_herdr_target_ready "$1" || return 1 diff --git a/bin/backends/tmux.sh b/bin/backends/tmux.sh index f8da21bf0d..b7da51a44f 100644 --- a/bin/backends/tmux.sh +++ b/bin/backends/tmux.sh @@ -8,10 +8,10 @@ # default (tmux, `backend=` absent) path stays byte-identical. Sourced only # through bin/fm-backend.sh's fm_backend_source, never directly. # -# Worktree acquisition (running `treehouse get` inside the pane, and polling -# its cwd) is unchanged by this extraction: P1 scopes only the session -# provider, not the worktree provider, so fm-spawn.sh still drives that part -# inline with these same send/current-path primitives. +# Worktree acquisition (durable treehouse lease under the task id, then cd +# into the leased path and poll the pane cwd) is owned by fm-spawn.sh: P1 +# scopes only the session provider, not the worktree provider, so spawn still +# drives that part inline with these same send/current-path primitives. # # The verified composer/busy-detection and verify-and-retry-submit primitives # already live in bin/fm-tmux-lib.sh, shared with the away-mode daemon @@ -103,8 +103,8 @@ fm_backend_tmux_current_path() { # # fm_backend_tmux_send_text_line: send one line of TEXT then Enter, with no # composer verification - used for the fixed spawn-time commands -# (`treehouse get`, the GOTMPDIR export) that already ran this exact sequence -# inline in fm-spawn.sh. Mirrors `tmux send-keys -t "$T" "" Enter`. +# (cd into the leased worktree, the GOTMPDIR export) that already ran this +# exact sequence inline in fm-spawn.sh. Mirrors `tmux send-keys -t "$T" "" Enter`. fm_backend_tmux_send_text_line() { # tmux send-keys -t "$1" "$2" Enter } diff --git a/bin/backends/zellij.sh b/bin/backends/zellij.sh index 20d53a3c2d..370a06202a 100644 --- a/bin/backends/zellij.sh +++ b/bin/backends/zellij.sh @@ -383,15 +383,15 @@ fm_backend_zellij_target_ready() { # [expected-label] # fm_backend_zellij_current_path: the live pane's cwd, or empty on any error. # Mirrors tmux's pane_current_path poll used for worktree-path discovery after -# `treehouse get`. +# the spawn-time cd into a leased worktree. # # Verified pitfall (docs/zellij-backend.md "Worktree-path discovery: pane_cwd # does not track a subshell"): `list-panes --json`'s `pane_cwd` DOES reflect a -# `cd` run directly in the pane's own top-level shell, but stays FROZEN at -# whatever directory the pane's shell was in when it launched `treehouse get` -# as a foreground command - it never follows that command's own internal `cd` -# into the acquired worktree, even after the subshell is fully interactive and -# a `pwd` typed into it prints the correct live path on screen. Zellij's CLI +# `cd` run directly in the pane's own top-level shell (what fm-spawn uses after +# a durable treehouse lease), but stays FROZEN at whatever directory the pane's +# shell was in when it launched a nested foreground subshell - it never follows +# that command's own internal `cd`, even after the subshell is fully interactive +# and a `pwd` typed into it prints the correct live path on screen. Zellij's CLI # exposes no per-pane pid and no live-process cwd field to read instead # (unlike herdr's `foreground_cwd`), so passive JSON polling cannot solve # this. Active probe instead: print the pane's `$PWD` with a unique marker @@ -462,8 +462,8 @@ fm_backend_zellij_send_key() { # [expected-label] # fm_backend_zellij_send_text_line: send one line of TEXT then submit, # ATOMICALLY - mirrors tmux's `send-keys -t T text Enter` / herdr's `pane -# run`. Used for the fixed spawn-time commands (treehouse get, the GOTMPDIR -# export). Zellij has no single-call atomic "run and submit" action, so this +# run`. Used for the fixed spawn-time commands (cd into the leased worktree, +# the GOTMPDIR export). Zellij has no single-call atomic "run and submit" action, so this # composes paste (literal) + send-keys Enter, exactly like send_literal + # send_key are composed elsewhere - the two-step form is the ONLY form for # this adapter, unlike tmux/herdr which have a genuinely atomic primitive. diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index b6ffdfd347..7668b00478 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -19,10 +19,16 @@ # then tmux. # Spawn-capable backends are the reference tmux adapter and experimental # herdr, zellij, orca, and cmux. Orca owns both the task worktree and -# terminal, so ship/scout Orca spawns do not run treehouse get; cmux is a -# session provider only, exactly like herdr/zellij, so it does. An -# auto-detected herdr or cmux spawn prints a loud stderr notice; -# auto-detected tmux stays silent; zellij and orca are never auto-detected. +# terminal, so ship/scout Orca spawns do not use treehouse; cmux is a +# session provider only, exactly like herdr/zellij, so it does. Ship and +# scout treehouse worktrees are acquired with a durable lease held under +# the task id (treehouse get --lease --lease-holder ), then the pane +# cds into that path - never a plain interactive get, which leaves the +# pool slot available for another spawn to steal. Recovery that still has +# a recorded worktree reuses that path and keeps its lease rather than +# double-leasing. An auto-detected herdr or cmux spawn prints a loud +# stderr notice; auto-detected tmux stays silent; zellij and orca are +# never auto-detected. # codex-app is not a known backend yet; docs/codex-app-backend.md owns that # blocked backend contract. Default tmux spawns do not write backend= to meta; # absent backend= means tmux. cmux does not support --secondmate spawns yet. @@ -261,6 +267,10 @@ fi ORCA_ABORT_CLEANUP=0 ORCA_WORKTREE_ID= ORCA_TERMINAL= +# Set when this spawn freshly leased a treehouse ship/scout worktree; cleared +# after successful meta publication. Abort cleanup returns only a lease this +# spawn acquired, never a recovered/reused worktree whose lease must stay held. +TREEHOUSE_LEASE_ACQUIRED=0 HERDR_PROJECTION_ABORT_CLEANUP=0 HERDR_PROJECTION_ABORT_SESSION= HERDR_PROJECTION_ABORT_TASK_PANE= @@ -337,6 +347,15 @@ spawn_abort_cleanup() { fi fi fi + # Release only a treehouse lease this spawn just acquired. A recovery relaunch + # that reuses a recorded worktree never sets TREEHOUSE_LEASE_ACQUIRED, so an + # abort there cannot free a still-owned slot. + if [ "${TREEHOUSE_LEASE_ACQUIRED:-0}" = 1 ] && [ -n "${WT:-}" ]; then + TREEHOUSE_LEASE_ACQUIRED=0 + if [ -n "${PROJ_ABS:-}" ] && [ -d "$WT" ] && command -v treehouse >/dev/null 2>&1; then + ( cd "$PROJ_ABS" && treehouse return --force "$WT" ) >/dev/null 2>&1 || true + fi + fi if [ "$SPAWN_TASK_LOCK_HELD" = 1 ]; then SPAWN_TASK_LOCK_HELD=0 fm_lock_release "$SPAWN_TASK_LOCK" || true @@ -1293,39 +1312,69 @@ kimi_spawn_fail() { # } if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then - spawn_send_text_line "$WT_TARGET" 'treehouse get' + # Ship/scout worktree acquisition: durable lease under the task id, matching + # secondmate homes (bin/fm-home-seed.sh). A plain interactive `treehouse get` + # leaves the pool slot available, so a later get can hand the same slot to a + # new spawn and hard-reset unlanded work (kunchenguid/firstmate#1441). + # + # Recovery / relaunch: if this task already records a still-present isolated + # worktree, reuse that path and keep whatever lease (or pre-lease in-use + # occupancy) it has. Never call get --lease again for the same task identity. + # Pre-existing unleased worktrees from before this change remain usable: spawn + # and teardown do not require a lease marker, and teardown's treehouse return + # still frees the slot when cleanup is allowed. + existing_wt= + if [ -f "$STATE/$ID.meta" ]; then + existing_wt=$(grep '^worktree=' "$STATE/$ID.meta" 2>/dev/null | cut -d= -f2- || true) + fi + if [ -n "$existing_wt" ] && [ -d "$existing_wt" ]; then + WT=$existing_wt + validate_spawn_worktree "recorded worktree" "$T" + else + WT=$(cd "$PROJ_ABS" && treehouse get --lease --lease-holder "$ID") || { + echo "error: treehouse get --lease failed to lease a worktree for $ID under $PROJ_ABS" >&2 + exit 1 + } + [ -n "$WT" ] || { + echo "error: treehouse get --lease did not report a worktree path for $ID" >&2 + exit 1 + } + TREEHOUSE_LEASE_ACQUIRED=1 + # Fail closed on a leased-but-wrong path before the pane is moved into it. + validate_spawn_worktree "treehouse get --lease" "$T" + fi + + # Enter the leased (or recovered) path in the pane. A top-level cd is visible + # to every backend's current-path probe (unlike the old interactive get + # subshell, which some backends cannot observe). + spawn_send_text_line "$WT_TARGET" "cd $(shell_quote "$WT")" - # Wait for the treehouse subshell: the pane's cwd moves from the project to the worktree. - # Target the stable window id, not the name: if the name is ever lost (e.g. an - # automatic-rename slips through), display-message -t falls back to the - # active client's window, which would misread firstmate's OWN pane path as the - # worktree and tangle a hook into the primary checkout. The window id never lies. - # Compare against PROJ_ABS_REAL (physical), not PROJ_ABS: a symlinked project - # prefix would otherwise make the pane's OS-level cwd read differ from - # PROJ_ABS on the very first poll, before the pane has actually moved. + # Wait until the pane cwd matches the acquired path. Target the stable window + # id, not the name: if the name is ever lost (e.g. an automatic-rename slips + # through), display-message -t falls back to the active client's + # window, which would misread firstmate's OWN pane path as the worktree and + # tangle a hook into the primary checkout. The window id never lies. + # Compare against the physical form of the leased path: a symlinked project + # prefix would otherwise make the pane's OS-level cwd read differ from the + # logical WT on the very first poll. # - # A single read that already differs from PROJ_ABS_REAL is not proof the pane - # settled there: on some tmux/WSL setups a brand-new window's pane_current_path - # transiently reports an unrelated stale path (seen live as another real git - # checkout entirely) before the shell catches up with treehouse get's cd. That - # stale path still passes the PROJ_ABS_REAL comparison and validate_spawn_worktree - # below (it resolves to a real, distinct worktree top-level too), so accepting it - # on one read alone silently records the wrong worktree= in state/.meta. Require - # two consecutive reads to agree on the same non-project path before accepting it; - # a mismatch just becomes the new candidate rather than resetting the wait, so a - # pane that is already settled by the first real read only costs the one existing - # inter-poll sleep as confirmation, not a whole extra cycle on top. + # A single matching read is not proof the pane settled: on some tmux/WSL + # setups a brand-new window's pane_current_path transiently reports an + # unrelated path before the shell catches up with cd. Require two consecutive + # reads to agree on the expected leased path before accepting it. + expected_wt_real=$(real_path_or_raw "$WT") candidate="" + settled=0 for _ in $(seq 1 60); do p=$(spawn_current_path "$WT_TARGET" || true) if [ -n "$p" ]; then p_real=$(real_path_or_raw "$p") - if [ "$p_real" != "$PROJ_ABS_REAL" ]; then + if [ "$p_real" = "$expected_wt_real" ]; then if [ -n "$candidate" ] && [ "$p_real" = "$candidate" ]; then - WT="$p" + settled=1 break fi - candidate="$p_real" + candidate=$p_real else candidate="" fi @@ -1334,12 +1383,14 @@ if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then fi sleep 1 done - if [ -z "$WT" ]; then - echo "error: treehouse get did not enter a worktree within 60s; inspect window $T" >&2 + if [ "$settled" -ne 1 ]; then + echo "error: pane did not enter leased worktree $WT within 60s; inspect window $T" >&2 exit 1 fi - validate_spawn_worktree "treehouse get" "$T" + # Re-check isolation after the pane settled: a leased-but-wrong worktree must + # still fail loudly rather than launch in the primary checkout. + validate_spawn_worktree "treehouse worktree enter" "$T" fi # Per-task temp root: /tmp/fm-/ with Go's build temp nested at gotmp/. Go won't @@ -1644,6 +1695,9 @@ META_WINDOW=$T fi } > "$STATE/$ID.meta" [ "$BACKEND" = orca ] && ORCA_ABORT_CLEANUP=0 +# Meta now owns the worktree path; abort must not release a successfully +# published ship/scout lease (teardown is the only release path). +TREEHOUSE_LEASE_ACQUIRED=0 sq_brief=$(shell_quote "$BRIEF") sq_turnend=$(shell_quote "$TURNEND") diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index ed1da4fa95..18bfa7e852 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -50,8 +50,13 @@ # is the approved discard path that prevalidates child removal targets, discards # child work, kills child runtime endpoints, and removes the retired home. Removing a # leased home releases its durable treehouse lease so the pool slot is freed, -# never left leased forever. If the treehouse return fails, teardown leaves the -# leased home and state in place instead of hiding a still-held lease. +# never left leased forever. Ship and scout treehouse worktrees are leased the +# same way under the task id at spawn (bin/fm-spawn.sh); a successful +# treehouse return here releases that lease so the slot is reusable. A +# refused teardown (dirty or unlanded work, missing scout report, etc.) must +# never reach return, so the lease stays held and the pool cannot hand the +# slot to another spawn. If the treehouse return fails, teardown leaves the +# leased home or worktree and state in place instead of hiding a still-held lease. # Usage: fm-teardown.sh [--force] # --force skips ordinary-task dirty and landed-work checks, skips scout report # checks, and discards secondmate child work for kind=secondmate. Only use it diff --git a/docs/architecture.md b/docs/architecture.md index 0a06f274a2..02a0d768ec 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -133,6 +133,9 @@ Codex App support is recorded in `docs/codex-app-backend.md`; it is not selectab ## Worktrees, not branches in your checkout Crewmates never intentionally touch your project clone; [treehouse](https://github.com/kunchenguid/treehouse) pools clean worktrees for tmux, herdr, zellij, and cmux tasks, while Orca creates its own worktrees for `backend=orca`. +Ship and scout treehouse worktrees are acquired with a durable lease held under the task id (`treehouse get --lease --lease-holder `), so a parked or idle task's slot is never handed to a later `treehouse get` and never pruned until successful teardown returns it. +Recovery that still has a recorded worktree reuses that path and keeps the existing lease rather than acquiring a second one. +Tasks spawned before durable ship leases remain usable without a lease marker: spawn reuses a still-present recorded path, and teardown's `treehouse return` still frees the slot when cleanup is allowed. For ship and scout work, `fm-spawn.sh` refuses to launch unless the resolved task path is a real git worktree root that is distinct from the project primary checkout. The firstmate repo has one extra exposure because it can dispatch crewmates to work on itself. diff --git a/tests/fm-spawn-dispatch-profile.test.sh b/tests/fm-spawn-dispatch-profile.test.sh index b19d25379f..fa4d3c4bfe 100755 --- a/tests/fm-spawn-dispatch-profile.test.sh +++ b/tests/fm-spawn-dispatch-profile.test.sh @@ -42,7 +42,21 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse pi-signed + # Durable lease: print the test worktree path so ship/scout spawns proceed. + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +set -u +case "${1:-}" in + get) + printf '%s\n' "${FM_FAKE_PANE_PATH:?FM_FAKE_PANE_PATH unset}" + exit 0 + ;; + return) exit 0 ;; +esac +exit 0 +SH + chmod +x "$fakebin/treehouse" + fm_fake_exit0 "$fakebin" pi-signed printf '%s\n' "$fakebin" } diff --git a/tests/fm-spawn-worktree-lease.test.sh b/tests/fm-spawn-worktree-lease.test.sh new file mode 100755 index 0000000000..e105526636 --- /dev/null +++ b/tests/fm-spawn-worktree-lease.test.sh @@ -0,0 +1,454 @@ +#!/usr/bin/env bash +# Durable ship/scout worktree leases (bin/fm-spawn.sh + bin/fm-teardown.sh). +# +# A plain interactive `treehouse get` leaves a pool slot available, so a later +# get can hand the same slot to a new spawn and hard-reset unlanded work +# (kunchenguid/firstmate#1441). Ship and scout spawns must lease under the task +# id; successful teardown releases; refused teardown must never release; +# recovery into a recorded worktree must not double-lease; pre-lease worktrees +# must keep working. +set -u + +# shellcheck source=tests/lib.sh disable=SC1091 +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" + +SPAWN="$ROOT/bin/fm-spawn.sh" +TEARDOWN="$ROOT/bin/fm-teardown.sh" +TMP_ROOT=$(fm_test_tmproot fm-spawn-worktree-lease) +fm_git_identity fmtest fmtest@example.invalid + +# Fake treehouse that models a small pool with durable leases. +# Env: +# FM_FAKE_TREEHOUSE_POOL_DIR - dir with slots/ and leases/ +# FM_FAKE_TREEHOUSE_LOG - append-only command log +# FM_FAKE_TREEHOUSE_GET_PATH - optional fixed path for a single-slot pool +# FM_FAKE_TREEHOUSE_RETURN_FAIL=1 - make return fail +install_lease_treehouse() { + local fakebin=$1 + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +set -u +log=${FM_FAKE_TREEHOUSE_LOG:-/dev/null} +printf 'treehouse %s\n' "$*" >> "$log" +pool=${FM_FAKE_TREEHOUSE_POOL_DIR:?FM_FAKE_TREEHOUSE_POOL_DIR unset} +mkdir -p "$pool/slots" "$pool/leases" + +is_leased() { + local path=$1 f holder + for f in "$pool"/leases/*; do + [ -e "$f" ] || continue + if [ "$(cat "$f")" = "$path" ]; then + return 0 + fi + done + return 1 +} + +case "${1:-}" in + get) + shift + lease=0 + holder= + while [ $# -gt 0 ]; do + case "$1" in + --lease) lease=1 ;; + --lease-holder) shift; holder=${1:-} ;; + --lease-holder=*) holder=${1#--lease-holder=} ;; + --json) ;; + esac + shift || true + done + if [ "$lease" -ne 1 ]; then + # Plain get only hands out unleased available slots (the defect surface). + if [ -n "${FM_FAKE_TREEHOUSE_GET_PATH:-}" ]; then + path=$FM_FAKE_TREEHOUSE_GET_PATH + if is_leased "$path"; then + echo "error: no available worktree (all leased)" >&2 + exit 1 + fi + printf '%s\n' "$path" + exit 0 + fi + for slot in "$pool"/slots/*; do + [ -e "$slot" ] || continue + path=$(cat "$slot") + is_leased "$path" && continue + printf '%s\n' "$path" + exit 0 + done + echo "error: no available worktree" >&2 + exit 1 + fi + [ -n "$holder" ] || holder=${TREEHOUSE_LEASE_HOLDER:-unknown} + # Prefer re-binding the same holder to its existing lease (not used by spawn + # recovery, which skips get entirely; present for pool realism). + if [ -f "$pool/leases/$holder" ]; then + printf '%s\n' "$(cat "$pool/leases/$holder")" + exit 0 + fi + if [ -n "${FM_FAKE_TREEHOUSE_GET_PATH:-}" ]; then + path=$FM_FAKE_TREEHOUSE_GET_PATH + if is_leased "$path"; then + echo "error: no available worktree (all leased)" >&2 + exit 1 + fi + printf '%s\n' "$path" > "$pool/leases/$holder" + printf 'leased worktree for %s\n' "$holder" >&2 + printf '%s\n' "$path" + exit 0 + fi + for slot in "$pool"/slots/*; do + [ -e "$slot" ] || continue + path=$(cat "$slot") + is_leased "$path" && continue + printf '%s\n' "$path" > "$pool/leases/$holder" + printf 'leased worktree for %s\n' "$holder" >&2 + printf '%s\n' "$path" + exit 0 + done + echo "error: no available worktree" >&2 + exit 1 + ;; + return) + shift + target= + while [ $# -gt 0 ]; do + case "$1" in + --force) ;; + *) target=$1 ;; + esac + shift || true + done + [ -z "${FM_FAKE_TREEHOUSE_RETURN_FAIL:-}" ] || exit 17 + if [ -n "$target" ]; then + for f in "$pool"/leases/*; do + [ -e "$f" ] || continue + if [ "$(cat "$f")" = "$target" ]; then + rm -f "$f" + fi + done + fi + exit 0 + ;; + status) + exit 0 + ;; +esac +exit 0 +SH + chmod +x "$fakebin/treehouse" +} + +install_spawn_tmux() { + local fakebin=$1 + cat > "$fakebin/tmux" <<'SH' +#!/usr/bin/env bash +set -u +log=${FM_FAKE_TMUX_LOG:-/dev/null} +printf 'tmux %s\n' "$*" >> "$log" +case "$*" in + *"#{pane_current_path}"*) + printf '%s\n' "${FM_FAKE_PANE_PATH:-}" + exit 0 + ;; +esac +case "${1:-}" in + display-message) printf 'firstmate\n'; exit 0 ;; + list-windows) exit 0 ;; + has-session|new-session|new-window|kill-window) exit 0 ;; + send-keys) + # After spawn sends `cd `, tests can flip the pane path by matching + # the cd target against FM_FAKE_PANE_PATH (already set to the leased path). + if [ -n "${FM_FAKE_TMUX_SEND_LOG:-}" ]; then + printf '%s\n' "$*" >> "$FM_FAKE_TMUX_SEND_LOG" + fi + exit 0 + ;; +esac +exit 0 +SH + chmod +x "$fakebin/tmux" +} + +# make_lease_case [slot2_name] +# Builds home, project, one (or two) real worktrees as pool slots, and fakes. +make_lease_case() { + local name=$1 id=$2 case_dir home proj wt fakebin pool + case_dir="$TMP_ROOT/$name" + home="$case_dir/home" + proj="$case_dir/project" + wt="$case_dir/wt" + fakebin=$(fm_fakebin "$case_dir/fake") + pool="$case_dir/pool" + mkdir -p "$home/data/$id" "$home/projects" "$home/state" "$home/config" "$pool/slots" "$pool/leases" + printf 'codex\n' > "$home/config/crew-harness" + fm_git_worktree "$proj" "$wt" "wt-$name" + printf 'brief for %s\n' "$id" > "$home/data/$id/brief.md" + touch "$home/state/.last-watcher-beat" + printf '%s\n' "$wt" > "$pool/slots/1" + if [ -n "${3:-}" ]; then + local wt2="$case_dir/wt2" + git -C "$proj" worktree add --quiet -b "wt2-$name" "$wt2" + printf '%s\n' "$wt2" > "$pool/slots/2" + fi + install_lease_treehouse "$fakebin" + install_spawn_tmux "$fakebin" + # Default gh/tasks stubs so teardown can complete when work is landed. + cat > "$fakebin/gh-axi" <<'SH' +#!/usr/bin/env bash +case "${1:-} ${2:-}" in + "pr list") printf '%s\n' "count: 0 (showing first 0)" "pull_requests[]: []" ; exit 0 ;; + "pr view") echo "error: pull request not found" >&2 ; exit 1 ;; +esac +exit 0 +SH + cat > "$fakebin/gh" <<'SH' +#!/usr/bin/env bash +exit 0 +SH + cat > "$fakebin/tasks-axi" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" = --version ] && { printf '0.1.1\n'; exit 0; } +exit 0 +SH + chmod +x "$fakebin/gh-axi" "$fakebin/gh" "$fakebin/tasks-axi" + printf '%s\n' "$case_dir|$home|$proj|$wt|$fakebin|$pool|$id" +} + +read_lease_case() { + IFS='|' read -r CASE_DIR HOME_DIR PROJ_DIR WT_DIR FAKEBIN_DIR POOL_DIR TASK_ID <&1 +} + +run_teardown() { + local id=$1 + shift + FM_ROOT_OVERRIDE='' FM_HOME="$HOME_DIR" \ + FM_STATE_OVERRIDE="$HOME_DIR/state" FM_DATA_OVERRIDE="$HOME_DIR/data" \ + FM_PROJECTS_OVERRIDE="$HOME_DIR/projects" FM_CONFIG_OVERRIDE="$HOME_DIR/config" \ + FM_FAKE_TREEHOUSE_POOL_DIR="$POOL_DIR" \ + FM_FAKE_TREEHOUSE_LOG="$CASE_DIR/treehouse.log" \ + PATH="$FAKEBIN_DIR:$PATH" \ + "$TEARDOWN" "$id" "$@" 2>&1 +} + +lease_holder_file() { + local holder=$1 + printf '%s\n' "$POOL_DIR/leases/$holder" +} + +# --------------------------------------------------------------------------- +# 1. Spawn leases; a subsequent plain get must not hand out the same path. +# --------------------------------------------------------------------------- +test_spawn_lease_blocks_second_get() { + local rec out status lease_file second + rec=$(make_lease_case lease-blocks-get ship-lease-a1) + read_lease_case "$rec" + + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "spawn should lease a worktree"$'\n'"$out" + assert_contains "$out" "spawned $TASK_ID" "spawn did not report success" + assert_grep "worktree=$WT_DIR" "$HOME_DIR/state/$TASK_ID.meta" \ + "meta did not record the leased worktree" + lease_file=$(lease_holder_file "$TASK_ID") + [ -f "$lease_file" ] || fail "spawn did not record a durable lease under the task id" + [ "$(cat "$lease_file")" = "$WT_DIR" ] || fail "lease path mismatch: $(cat "$lease_file")" + grep -F "treehouse get --lease --lease-holder $TASK_ID" "$CASE_DIR/treehouse.log" >/dev/null \ + || fail "spawn did not call treehouse get --lease --lease-holder under the task id" + grep -E 'send-keys .*[[:space:]]cd[[:space:]]' "$CASE_DIR/tmux-send.log" >/dev/null \ + || fail "spawn did not cd the pane into the leased worktree: $(cat "$CASE_DIR/tmux-send.log" 2>/dev/null)" + + # Prove the negative: while the task is live, a subsequent plain get must not + # return the leased path (single-slot pool). + second=$( + FM_FAKE_TREEHOUSE_POOL_DIR="$POOL_DIR" \ + FM_FAKE_TREEHOUSE_LOG="$CASE_DIR/treehouse.log" \ + FM_FAKE_TREEHOUSE_GET_PATH="$WT_DIR" \ + PATH="$FAKEBIN_DIR:$PATH" \ + treehouse get 2>/dev/null || true + ) + [ -z "$second" ] || fail "plain treehouse get handed out still-leased path: $second" + pass "spawned ship slot is leased and not handed out by a subsequent treehouse get" +} + +# --------------------------------------------------------------------------- +# 2. Successful teardown releases the lease. +# --------------------------------------------------------------------------- +test_successful_teardown_releases_lease() { + local rec out status lease_file + rec=$(make_lease_case teardown-releases ship-lease-b2) + read_lease_case "$rec" + + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "spawn for release case failed"$'\n'"$out" + lease_file=$(lease_holder_file "$TASK_ID") + [ -f "$lease_file" ] || fail "lease missing after spawn" + + # Land the work on origin so teardown is allowed without --force. + git -C "$WT_DIR" -c user.email=t@t -c user.name=t commit -q --allow-empty -m "landed" + git -C "$PROJ_DIR" remote add origin "$CASE_DIR/origin.git" 2>/dev/null || true + git init -q --bare "$CASE_DIR/origin.git" + git -C "$WT_DIR" push -q "$CASE_DIR/origin.git" "HEAD:refs/heads/$(git -C "$WT_DIR" rev-parse --abbrev-ref HEAD)" + git -C "$PROJ_DIR" fetch -q origin 2>/dev/null || true + # Ensure the worktree sees a remote-tracking ref for landed check. + git -C "$PROJ_DIR" remote remove origin 2>/dev/null || true + git -C "$PROJ_DIR" remote add origin "$CASE_DIR/origin.git" + git -C "$PROJ_DIR" fetch -q origin + # Point the task branch remote-tracking ref that work_is_landed looks for. + branch=$(git -C "$WT_DIR" rev-parse --abbrev-ref HEAD) + git -C "$PROJ_DIR" update-ref "refs/remotes/origin/$branch" "$(git -C "$WT_DIR" rev-parse HEAD)" + + : > "$CASE_DIR/treehouse.log" + out=$(run_teardown "$TASK_ID") + status=$? + expect_code 0 "$status" "teardown should succeed for landed work"$'\n'"$out" + [ ! -e "$lease_file" ] || fail "successful teardown left the lease held" + grep -F "treehouse return --force" "$CASE_DIR/treehouse.log" >/dev/null \ + || fail "successful teardown did not call treehouse return" + pass "successful teardown releases the lease and frees the slot" +} + +# --------------------------------------------------------------------------- +# 3. REFUSED teardown leaves the lease held (exercise the refusal). +# --------------------------------------------------------------------------- +test_refused_teardown_keeps_lease() { + local rec out status lease_file + rec=$(make_lease_case teardown-refuses ship-lease-c3) + read_lease_case "$rec" + + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "spawn for refuse case failed"$'\n'"$out" + lease_file=$(lease_holder_file "$TASK_ID") + [ -f "$lease_file" ] || fail "lease missing after spawn" + + # Unlanded real content change (not --allow-empty: empty commits match the + # "content already in default branch" landed fallback and would tear down). + printf 'unlanded payload\n' > "$WT_DIR/unlanded.txt" + git -C "$WT_DIR" add unlanded.txt + git -C "$WT_DIR" -c user.email=t@t -c user.name=t commit -q -m "unlanded only" + : > "$CASE_DIR/treehouse.log" + set +e + out=$(run_teardown "$TASK_ID" 2>&1) + status=$? + set -e + [ "$status" -ne 0 ] || fail "teardown should refuse unlanded work, got success: $out" + assert_contains "$out" "REFUSED" "teardown refusal did not report REFUSED" + [ -f "$lease_file" ] || fail "refused teardown released the lease" + [ "$(cat "$lease_file")" = "$WT_DIR" ] || fail "refused teardown altered the lease path" + if grep -F "treehouse return" "$CASE_DIR/treehouse.log" >/dev/null 2>&1; then + fail "refused teardown called treehouse return (would release the lease)" + fi + pass "refused teardown leaves the lease held (refusal exercised)" +} + +# --------------------------------------------------------------------------- +# 4. Recovery into an existing recorded worktree does not create a second lease. +# --------------------------------------------------------------------------- +test_recovery_reuses_worktree_without_second_lease() { + local rec out status lease_file get_count + rec=$(make_lease_case recovery-reuse ship-lease-d4) + read_lease_case "$rec" + + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "initial spawn for recovery case failed"$'\n'"$out" + lease_file=$(lease_holder_file "$TASK_ID") + [ -f "$lease_file" ] || fail "lease missing after initial spawn" + get_count=$(grep -c 'treehouse get --lease' "$CASE_DIR/treehouse.log" || true) + [ "$get_count" -eq 1 ] || fail "expected exactly one lease acquire on first spawn, got $get_count" + + # Simulate recovery: endpoint gone, recorded worktree + lease still present. + # Re-spawn under the same task id; must reuse worktree= and not call get again. + : > "$CASE_DIR/treehouse.log" + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "recovery spawn should reuse the recorded worktree"$'\n'"$out" + assert_grep "worktree=$WT_DIR" "$HOME_DIR/state/$TASK_ID.meta" \ + "recovery spawn lost the recorded worktree" + if grep -F 'treehouse get --lease' "$CASE_DIR/treehouse.log" >/dev/null 2>&1; then + fail "recovery spawn acquired a second lease: $(cat "$CASE_DIR/treehouse.log")" + fi + [ -f "$lease_file" ] || fail "recovery spawn dropped the original lease" + [ "$(cat "$lease_file")" = "$WT_DIR" ] || fail "recovery spawn changed the lease path" + # Only one lease file for this holder (no double-lease artifact). + [ "$(find "$POOL_DIR/leases" -type f | wc -l | tr -d ' ')" = 1 ] \ + || fail "recovery left more than one lease file" + pass "recovery into an existing worktree does not create a second lease" +} + +# --------------------------------------------------------------------------- +# 5. Pre-existing unleased worktrees do not break spawn or teardown. +# --------------------------------------------------------------------------- +test_legacy_unleased_worktree_spawn_and_teardown() { + local rec out status + rec=$(make_lease_case legacy-unleased ship-lease-e5) + read_lease_case "$rec" + + # Pre-lease era: meta already points at a real worktree, no lease file. + fm_write_meta "$HOME_DIR/state/$TASK_ID.meta" \ + "window=firstmate:fm-$TASK_ID" \ + "endpoint_task_id=$TASK_ID" \ + "worktree=$WT_DIR" \ + "project=$PROJ_DIR" \ + "harness=codex" \ + "kind=ship" \ + "mode=no-mistakes" \ + "yolo=off" + [ ! -e "$(lease_holder_file "$TASK_ID")" ] || fail "fixture incorrectly created a lease" + + : > "$CASE_DIR/treehouse.log" + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "spawn must accept a pre-existing unleased recorded worktree"$'\n'"$out" + if grep -F 'treehouse get --lease' "$CASE_DIR/treehouse.log" >/dev/null 2>&1; then + fail "legacy recovery incorrectly leased an already-owned unleased worktree" + fi + assert_grep "worktree=$WT_DIR" "$HOME_DIR/state/$TASK_ID.meta" \ + "legacy recovery lost the recorded worktree" + + # Land and teardown: return must succeed without requiring a prior lease. + git -C "$WT_DIR" -c user.email=t@t -c user.name=t commit -q --allow-empty -m "landed legacy" + git init -q --bare "$CASE_DIR/origin.git" + git -C "$PROJ_DIR" remote remove origin 2>/dev/null || true + git -C "$PROJ_DIR" remote add origin "$CASE_DIR/origin.git" + branch=$(git -C "$WT_DIR" rev-parse --abbrev-ref HEAD) + git -C "$WT_DIR" push -q origin "HEAD:refs/heads/$branch" + git -C "$PROJ_DIR" fetch -q origin + git -C "$PROJ_DIR" update-ref "refs/remotes/origin/$branch" "$(git -C "$WT_DIR" rev-parse HEAD)" + + : > "$CASE_DIR/treehouse.log" + out=$(run_teardown "$TASK_ID") + status=$? + expect_code 0 "$status" "teardown of legacy unleased worktree should succeed"$'\n'"$out" + grep -F "treehouse return --force" "$CASE_DIR/treehouse.log" >/dev/null \ + || fail "legacy teardown did not return the worktree" + pass "pre-existing unleased worktrees do not break spawn or teardown" +} + +test_spawn_lease_blocks_second_get +test_successful_teardown_releases_lease +test_refused_teardown_keeps_lease +test_recovery_reuses_worktree_without_second_lease +test_legacy_unleased_worktree_spawn_and_teardown + +echo "# all fm-spawn-worktree-lease tests passed" diff --git a/tests/fm-spawn-worktree-settle.test.sh b/tests/fm-spawn-worktree-settle.test.sh index a2d8ffbd57..1c2a57512b 100755 --- a/tests/fm-spawn-worktree-settle.test.sh +++ b/tests/fm-spawn-worktree-settle.test.sh @@ -1,17 +1,14 @@ #!/usr/bin/env bash -# Regression test for the fm-spawn.sh treehouse-get worktree-detection settle -# loop (bin/fm-spawn.sh, the `for _ in $(seq 1 60)` loop after `treehouse get`). +# Regression test for the fm-spawn.sh worktree-enter settle loop +# (bin/fm-spawn.sh, the dual-read loop after durable lease + cd into the path). # # On some tmux/WSL setups a brand-new window's pane_current_path transiently # reports a stale, unrelated-but-real path on the very first poll, before the -# pane actually settles into the worktree treehouse get moved it to. That stale -# path still passes the loop's "differs from the project" check and -# validate_spawn_worktree's "is a real, distinct worktree" check (it IS a real -# git checkout, just the wrong one), so a naive single-read loop silently -# records the wrong worktree= in state/.meta. This test simulates that +# pane actually settles into the leased worktree the spawn cds into. A naive +# single-read loop could accept that stale path. This test simulates that # transient-then-settled pane_current_path sequence with a fake tmux and -# asserts the recorded worktree resolves to the real, settled worktree, never -# the stale first read. +# asserts the recorded worktree resolves to the real, leased worktree, never +# the stale first read, and that two consecutive matching reads are required. set -u # shellcheck source=tests/lib.sh @@ -54,7 +51,30 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse + # Durable lease acquire: print the settled worktree path (FM_FAKE_PANE_PATH) + # so spawn records that path, then waits for the pane to match it. + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +set -u +case "${1:-}" in + get) + shift + while [ $# -gt 0 ]; do + case "$1" in + --lease|--json) ;; + --lease-holder) shift ;; + --lease-holder=*) ;; + esac + shift || true + done + printf '%s\n' "${FM_FAKE_PANE_PATH:?FM_FAKE_PANE_PATH unset}" + exit 0 + ;; + return) exit 0 ;; +esac +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } From 9ded669c9aa892a5dd7527558adea56255f9a686 Mon Sep 17 00:00:00 2001 From: AG Werschky Date: Mon, 3 Aug 2026 13:32:28 -0600 Subject: [PATCH 2/9] no-mistakes(review): Captain, harden durable worktree lease recovery --- AGENTS.md | 2 +- bin/fm-spawn.sh | 55 ++++++++++- bin/fm-teardown.sh | 136 +++++++++++++++++++++----- docs/architecture.md | 3 +- tests/fm-backend.test.sh | 14 ++- tests/fm-busy-adapter-wiring.test.sh | 8 +- tests/fm-gate-refuse.test.sh | 7 +- tests/fm-grok-harness.test.sh | 8 +- tests/fm-kimi-harness.test.sh | 8 +- tests/fm-spawn-worktree-lease.test.sh | 123 +++++++++++++++++++++++ tests/fm-tangle-guard.test.sh | 24 +++-- 11 files changed, 343 insertions(+), 45 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c1e5272828..6734f79009 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -92,7 +92,7 @@ state/ volatile runtime signals; gitignored .turn-ended touched by turn-end hooks .grok-turnend-token firstmate-owned grok hook registry token for the task; removed by teardown .kimi-turnend-token firstmate-owned Kimi hook registry token for the task; removed by teardown - .meta written by fm-spawn: window=, endpoint_task_id=, worktree=, project=, harness=, model=, effort=, kind=, mode=, yolo=, tasktmp=; kind=secondmate also records home= and projects=; a non-default runtime backend records further backend-specific fields (docs/configuration.md "Runtime backend"; bin/fm-backend.sh, section 8); fm-pr-check, including through fm-pr-merge, records one canonical pr= and the forge's pr_head= when available (GitHub pull requests and GitLab merge requests; docs/gitlab-merge-watch.md); fm-x-link appends x_request=, x_request_ts=, x_followups=, and optional x_platform=/x_reply_max_chars= for an X-mode-originated task (section 14) + .meta written by fm-spawn: window=, endpoint_task_id=, worktree=, project=, harness=, model=, effort=, kind=, mode=, yolo=, tasktmp=; leased ship/scout worktrees also record treehouse_lease_holder= and treehouse_lease_state=; kind=secondmate also records home= and projects=; a non-default runtime backend records further backend-specific fields (docs/configuration.md "Runtime backend"; bin/fm-backend.sh, section 8); fm-pr-check, including through fm-pr-merge, records one canonical pr= and the forge's pr_head= when available (GitHub pull requests and GitLab merge requests; docs/gitlab-merge-watch.md); fm-x-link appends x_request=, x_request_ts=, x_followups=, and optional x_platform=/x_reply_max_chars= for an X-mode-originated task (section 14) .herdr-presentation quarantinable attempt and restart-binding journal for Herdr's optional visual projection; never task or endpoint authority; see docs/herdr-backend.md "Optional presentation spaces" .check.sh authenticated slow poll; the watcher dispatches validated PR data and the byte-identified X shim through trusted repository scripts, runs registered custom checks from hash-validated private snapshots, and rejects every other state check without execution .check-trust private content binding created by fm-check-register.sh for an intentional custom check diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index 7668b00478..441e2acbba 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -25,8 +25,9 @@ # the task id (treehouse get --lease --lease-holder ), then the pane # cds into that path - never a plain interactive get, which leaves the # pool slot available for another spawn to steal. Recovery that still has -# a recorded worktree reuses that path and keeps its lease rather than -# double-leasing. An auto-detected herdr or cmux spawn prints a loud +# a recorded worktree reuses that path only while treehouse still reports +# its task-held lease; marker-free pre-lease worktrees retain their legacy +# reuse behavior. An auto-detected herdr or cmux spawn prints a loud # stderr notice; auto-detected tmux stays silent; zellij and orca are # never auto-detected. # codex-app is not a known backend yet; docs/codex-app-backend.md owns that @@ -271,6 +272,8 @@ ORCA_TERMINAL= # after successful meta publication. Abort cleanup returns only a lease this # spawn acquired, never a recovered/reused worktree whose lease must stay held. TREEHOUSE_LEASE_ACQUIRED=0 +TREEHOUSE_LEASE_HOLDER= +TREEHOUSE_LEASE_STATE= HERDR_PROJECTION_ABORT_CLEANUP=0 HERDR_PROJECTION_ABORT_SESSION= HERDR_PROJECTION_ABORT_TASK_PANE= @@ -353,7 +356,7 @@ spawn_abort_cleanup() { if [ "${TREEHOUSE_LEASE_ACQUIRED:-0}" = 1 ] && [ -n "${WT:-}" ]; then TREEHOUSE_LEASE_ACQUIRED=0 if [ -n "${PROJ_ABS:-}" ] && [ -d "$WT" ] && command -v treehouse >/dev/null 2>&1; then - ( cd "$PROJ_ABS" && treehouse return --force "$WT" ) >/dev/null 2>&1 || true + ( cd "$PROJ_ABS" && treehouse return --force --if-lease-holder "$ID" "$WT" ) >/dev/null 2>&1 || true fi fi if [ "$SPAWN_TASK_LOCK_HELD" = 1 ]; then @@ -914,6 +917,19 @@ validate_spawn_worktree() { # fi } +recorded_treehouse_lease_is_owned() { # + local worktree=$1 holder=$2 status + case "$worktree" in + *'"'*|*'\'*|*$'\n'*|*'},{'*) return 1 ;; + esac + status=$(CDPATH='' cd -- "$PROJ_ABS" && treehouse status --json) || return 1 + printf '%s\n' "$status" \ + | awk '{ gsub(/\},\{/, "}\n{"); print }' \ + | grep -F "\"path\":\"$worktree\"" \ + | grep -F '"status":"leased"' \ + | grep -F "\"lease_holder\":\"$holder\"" >/dev/null +} + herdr_projection_meta_field_exact() { # local meta=$1 key=$2 count [ -f "$meta" ] && [ ! -L "$meta" ] || return 1 @@ -1318,16 +1334,41 @@ if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then # new spawn and hard-reset unlanded work (kunchenguid/firstmate#1441). # # Recovery / relaunch: if this task already records a still-present isolated - # worktree, reuse that path and keep whatever lease (or pre-lease in-use - # occupancy) it has. Never call get --lease again for the same task identity. + # worktree, reuse that path only when its recorded task lease is still held, + # or when marker-free metadata identifies a pre-lease worktree. Never call + # get --lease again for the same task identity. # Pre-existing unleased worktrees from before this change remain usable: spawn # and teardown do not require a lease marker, and teardown's treehouse return # still frees the slot when cleanup is allowed. existing_wt= + existing_lease_holder= + existing_lease_state= if [ -f "$STATE/$ID.meta" ]; then existing_wt=$(grep '^worktree=' "$STATE/$ID.meta" 2>/dev/null | cut -d= -f2- || true) + existing_lease_holder=$(fm_meta_get "$STATE/$ID.meta" treehouse_lease_holder) + existing_lease_state=$(fm_meta_get "$STATE/$ID.meta" treehouse_lease_state) fi if [ -n "$existing_wt" ] && [ -d "$existing_wt" ]; then + case "$existing_lease_state" in + '') ;; + held) + if [ "$existing_lease_holder" != "$ID" ] \ + || ! recorded_treehouse_lease_is_owned "$existing_wt" "$ID"; then + echo "error: recorded worktree $existing_wt is not leased under task $ID; refusing stale recovery" >&2 + exit 1 + fi + TREEHOUSE_LEASE_HOLDER=$ID + TREEHOUSE_LEASE_STATE=held + ;; + returning|returned) + echo "error: recorded worktree $existing_wt has lease state $existing_lease_state; refusing recovery after return" >&2 + exit 1 + ;; + *) + echo "error: recorded worktree $existing_wt has unknown lease state $existing_lease_state; refusing recovery" >&2 + exit 1 + ;; + esac WT=$existing_wt validate_spawn_worktree "recorded worktree" "$T" else @@ -1340,6 +1381,8 @@ if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then exit 1 } TREEHOUSE_LEASE_ACQUIRED=1 + TREEHOUSE_LEASE_HOLDER=$ID + TREEHOUSE_LEASE_STATE=held # Fail closed on a leased-but-wrong path before the pane is moved into it. validate_spawn_worktree "treehouse get --lease" "$T" fi @@ -1665,6 +1708,8 @@ META_WINDOW=$T echo "tasktmp=$TASK_TMP" echo "model=${MODEL:-default}" echo "effort=${EFFORT:-default}" + [ -z "$TREEHOUSE_LEASE_HOLDER" ] || echo "treehouse_lease_holder=$TREEHOUSE_LEASE_HOLDER" + [ -z "$TREEHOUSE_LEASE_STATE" ] || echo "treehouse_lease_state=$TREEHOUSE_LEASE_STATE" [ -z "${BUSY_GEN:-}" ] || echo "busy_gen=$BUSY_GEN" # backend= is written only for a non-default (non-tmux) backend, so the # default path's meta stays byte-identical (absent backend= means tmux; diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index 18bfa7e852..be5ed28245 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -124,9 +124,25 @@ FORCE=${2:-} # Fail closed before any fleet mutation: a no-mistakes gate agent must never tear # down a worktree (see bin/fm-gate-refuse-lib.sh). fm_refuse_if_gate_agent +# shellcheck source=bin/fm-wake-lib.sh +. "$SCRIPT_DIR/fm-wake-lib.sh" FM_LOCK_LOG_PREFIX=teardown META="$STATE/$ID.meta" +TEARDOWN_TASK_LOCK="$STATE/.spawn-$ID.lock" +TEARDOWN_TASK_LOCK_HELD=0 +teardown_release_task_lock() { + if [ "$TEARDOWN_TASK_LOCK_HELD" = 1 ]; then + TEARDOWN_TASK_LOCK_HELD=0 + fm_lock_release "$TEARDOWN_TASK_LOCK" || true + fi +} +if ! fm_lock_try_acquire "$TEARDOWN_TASK_LOCK"; then + echo "error: another spawn or teardown is already handling task $ID" >&2 + exit 1 +fi +TEARDOWN_TASK_LOCK_HELD=1 +trap teardown_release_task_lock EXIT [ -f "$META" ] || { echo "error: no meta for task $ID at $META" >&2; exit 1; } # This is the first cleanup authorization check. It is metadata-only and must # complete before fm-guard, a backend command, file removal, branch deletion, @@ -150,6 +166,49 @@ if [ -z "$BUSY_GEN" ]; then fi ORCA_WORKTREE_ID=$(fm_meta_get "$META" orca_worktree_id) ORCA_PATH_MATCH_VERIFIED=0 +TREEHOUSE_LEASE_HOLDER=$(fm_meta_get "$META" treehouse_lease_holder) +TREEHOUSE_LEASE_STATE=$(fm_meta_get "$META" treehouse_lease_state) + +case "$TREEHOUSE_LEASE_STATE" in + '') + if [ -n "$TREEHOUSE_LEASE_HOLDER" ]; then + echo "REFUSED: task $ID has a treehouse lease holder without a lease state; preserving metadata." >&2 + exit 1 + fi + ;; + held|returned) + if [ "$TREEHOUSE_LEASE_HOLDER" != "$ID" ]; then + echo "REFUSED: task $ID has mismatched treehouse lease holder ${TREEHOUSE_LEASE_HOLDER:-}; preserving metadata." >&2 + exit 1 + fi + ;; + returning) + echo "REFUSED: task $ID has an indeterminate treehouse return; preserving metadata for manual lease inspection." >&2 + exit 1 + ;; + *) + echo "REFUSED: task $ID has unknown treehouse lease state $TREEHOUSE_LEASE_STATE; preserving metadata." >&2 + exit 1 + ;; +esac + +treehouse_lease_state_set() { # + local state=$1 tmp + tmp=$(mktemp "$STATE/.${ID}.meta.treehouse-lease.XXXXXX") || return 1 + if ! { grep -v '^treehouse_lease_state=' "$META" || true; } > "$tmp"; then + rm -f "$tmp" + return 1 + fi + printf 'treehouse_lease_state=%s\n' "$state" >> "$tmp" || { + rm -f "$tmp" + return 1 + } + mv -f "$tmp" "$META" || { + rm -f "$tmp" + return 1 + } + TREEHOUSE_LEASE_STATE=$state +} KIND=$(grep '^kind=' "$META" | cut -d= -f2- || true) [ -n "$KIND" ] || KIND=ship @@ -673,12 +732,16 @@ cleanup_stale_lock_for_safety_check() { # Return a worktree/home via `treehouse return --force`, tolerating a transient or # stale git index.lock left by a killed crew process. See the script header. teardown_treehouse_return() { - local dir=$1 cd_dir=$2 label=$3 post_cleanup_check=${4:-} + local dir=$1 cd_dir=$2 label=$3 post_cleanup_check=${4:-} lease_holder=${5:-} local out lock attempt=0 max_retries lock_desc + local -a return_args + return_args=(return --force) + [ -z "$lease_holder" ] || return_args+=(--if-lease-holder "$lease_holder") + return_args+=("$dir") # Capture stdout+stderr so non-lock failures stay visible and lock failures can # be matched by signature even when the lock file is already gone mid-check. - if out=$( ( cd "$cd_dir" && treehouse return --force "$dir" ) 2>&1 ); then + if out=$( ( cd "$cd_dir" && treehouse "${return_args[@]}" ) 2>&1 ); then [ -n "$out" ] && printf '%s\n' "$out" return 0 fi @@ -703,7 +766,7 @@ teardown_treehouse_return() { echo "teardown: $label return failed with transient git lock ($lock_desc); waiting ${TREEHOUSE_RETURN_LOCK_RETRY_WAIT_SECS}s and retrying ($attempt/${max_retries})" >&2 sleep "$TREEHOUSE_RETURN_LOCK_RETRY_WAIT_SECS" - if out=$( ( cd "$cd_dir" && treehouse return --force "$dir" ) 2>&1 ); then + if out=$( ( cd "$cd_dir" && treehouse "${return_args[@]}" ) 2>&1 ); then [ -n "$out" ] && printf '%s\n' "$out" echo "teardown: $label return succeeded on retry; lock cleared on its own" >&2 return 0 @@ -730,7 +793,7 @@ teardown_treehouse_return() { return 1 fi fi - if out=$( ( cd "$cd_dir" && treehouse return --force "$dir" ) 2>&1 ); then + if out=$( ( cd "$cd_dir" && treehouse "${return_args[@]}" ) 2>&1 ); then [ -n "$out" ] && printf '%s\n' "$out" echo "teardown: $label return succeeded after stale-lock cleanup" >&2 return 0 @@ -1094,6 +1157,11 @@ FMEOF TEARDOWN_HERDR_LOCK_RECORDS= } +teardown_release_lifecycle_locks() { + teardown_release_herdr_locks + teardown_release_task_lock +} + teardown_herdr_session_lock_held() { # local session=$1 lock_session lock_path [ -n "$TEARDOWN_HERDR_LOCK_RECORDS" ] || return 1 @@ -1183,7 +1251,7 @@ $session $lock_path" else TEARDOWN_HERDR_LOCK_RECORDS="$session $lock_path" fi - trap teardown_release_herdr_locks EXIT + trap teardown_release_lifecycle_locks EXIT return 0 fi sleep 0.1 @@ -1392,7 +1460,8 @@ if [ "$BACKEND" = orca ] && [ "$KIND" != scout ] && [ "$KIND" != secondmate ] && ORCA_PATH_MATCH_VERIFIED=1 fi -if [ -d "$WT" ] && [ "$FORCE" != "--force" ]; then +if [ -d "$WT" ] && [ "$FORCE" != "--force" ] \ + && [ "$TREEHOUSE_LEASE_STATE" != returned ]; then if validate_worktree_teardown_safety; then : else @@ -1442,27 +1511,44 @@ if [ "$BACKEND" = orca ] && [ "$KIND" != secondmate ]; then [ -z "$T_ORCA" ] || fm_backend_kill "$BACKEND" "$T" "$(meta_value "$META" zellij_tab_id)" "fm-$ID" 2>/dev/null || true fm_backend_remove_worktree "$BACKEND" "$ORCA_WORKTREE_ID" elif [ -d "$WT" ] && [ "$KIND" != secondmate ]; then - branch=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) - if [ "$branch" != "HEAD" ]; then - if git -C "$WT" checkout --detach -q 2>/dev/null; then - git -C "$WT" branch -D "$branch" >/dev/null 2>&1 || true + if [ "$TREEHOUSE_LEASE_STATE" != returned ]; then + branch=$(git -C "$WT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo HEAD) + if [ "$branch" != "HEAD" ]; then + if git -C "$WT" checkout --detach -q 2>/dev/null; then + git -C "$WT" branch -D "$branch" >/dev/null 2>&1 || true + fi + fi + # Remove our hook file so a reused pool worktree cannot fire signals for a dead task. + rm -f "$WT/.claude/settings.local.json" "$WT/.opencode/plugins/fm-turn-end.js" \ + "$WT/.fm-grok-turnend" "$WT/.fm-kimi-turnend" + # Kills remaining processes in the worktree (including the agent), resets, returns + # to pool. treehouse resolves the pool from the working directory, so run it from + # the project. teardown_treehouse_return tolerates transient and stale git locks + # left by a killed crew process; see the script header for retry and stale-lock proof. + post_lock_cleanup_check= + if [ "$FORCE" != "--force" ] && [ "$KIND" != scout ] && [ "$KIND" != secondmate ]; then + post_lock_cleanup_check=validate_worktree_teardown_safety + fi + if [ "$TREEHOUSE_LEASE_STATE" = held ]; then + treehouse_lease_state_set returning || { + echo "error: could not record treehouse return ownership for $ID; teardown aborted" >&2 + exit 1 + } + fi + teardown_treehouse_return "$WT" "$PROJ" "worktree" "$post_lock_cleanup_check" "$TREEHOUSE_LEASE_HOLDER" || { + if [ -n "$TREEHOUSE_LEASE_HOLDER" ]; then + treehouse_lease_state_set held || true + fi + echo "error: treehouse return failed for worktree $WT; teardown aborted" >&2 + exit 1 + } + if [ -n "$TREEHOUSE_LEASE_HOLDER" ]; then + treehouse_lease_state_set returned || { + echo "error: worktree returned but returned lease state could not be recorded for $ID; preserving metadata" >&2 + exit 1 + } fi fi - # Remove our hook file so a reused pool worktree cannot fire signals for a dead task. - rm -f "$WT/.claude/settings.local.json" "$WT/.opencode/plugins/fm-turn-end.js" \ - "$WT/.fm-grok-turnend" "$WT/.fm-kimi-turnend" - # Kills remaining processes in the worktree (including the agent), resets, returns - # to pool. treehouse resolves the pool from the working directory, so run it from - # the project. teardown_treehouse_return tolerates transient and stale git locks - # left by a killed crew process; see the script header for retry and stale-lock proof. - post_lock_cleanup_check= - if [ "$FORCE" != "--force" ] && [ "$KIND" != scout ] && [ "$KIND" != secondmate ]; then - post_lock_cleanup_check=validate_worktree_teardown_safety - fi - teardown_treehouse_return "$WT" "$PROJ" "worktree" "$post_lock_cleanup_check" || { - echo "error: treehouse return failed for worktree $WT; teardown aborted" >&2 - exit 1 - } fi HERDR_PRESENTATION_JOURNAL="$STATE/$ID.herdr-presentation" diff --git a/docs/architecture.md b/docs/architecture.md index 02a0d768ec..b9c578aef7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,7 +134,8 @@ Codex App support is recorded in `docs/codex-app-backend.md`; it is not selectab Crewmates never intentionally touch your project clone; [treehouse](https://github.com/kunchenguid/treehouse) pools clean worktrees for tmux, herdr, zellij, and cmux tasks, while Orca creates its own worktrees for `backend=orca`. Ship and scout treehouse worktrees are acquired with a durable lease held under the task id (`treehouse get --lease --lease-holder `), so a parked or idle task's slot is never handed to a later `treehouse get` and never pruned until successful teardown returns it. -Recovery that still has a recorded worktree reuses that path and keeps the existing lease rather than acquiring a second one. +Recovery reuses a leased recorded path only when treehouse still reports that exact path leased under the task id, so returned or reassigned slots fail closed instead of being re-entered. +Teardown serializes with recovery on the task lifecycle lock and records the lease as returned before any post-return endpoint cleanup can retain task metadata. Tasks spawned before durable ship leases remain usable without a lease marker: spawn reuses a still-present recorded path, and teardown's `treehouse return` still frees the slot when cleanup is allowed. For ship and scout work, `fm-spawn.sh` refuses to launch unless the resolved task path is a real git worktree root that is distinct from the project primary checkout. diff --git a/tests/fm-backend.test.sh b/tests/fm-backend.test.sh index 3052ebc422..e07bae6549 100755 --- a/tests/fm-backend.test.sh +++ b/tests/fm-backend.test.sh @@ -806,7 +806,12 @@ esac exit 0 SH chmod +x "$fb/tmux" - fm_fake_exit0 "$fb" treehouse + cat > "$fb/treehouse" < "$fb/treehouse" < "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" != get ] || printf '%s\n' "${FM_FAKE_PANE_PATH:-}" +exit 0 +SH + chmod +x "$fakebin/treehouse" + fm_fake_exit0 "$fakebin" pi opencode claude codex printf '%s\n' "$fakebin" } diff --git a/tests/fm-gate-refuse.test.sh b/tests/fm-gate-refuse.test.sh index aff57ab18e..570c64d27e 100755 --- a/tests/fm-gate-refuse.test.sh +++ b/tests/fm-gate-refuse.test.sh @@ -152,7 +152,12 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" != get ] || printf '%s\n' "${FM_FAKE_PANE_PATH:-}" +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } diff --git a/tests/fm-grok-harness.test.sh b/tests/fm-grok-harness.test.sh index edb5001b9a..3c00c63417 100755 --- a/tests/fm-grok-harness.test.sh +++ b/tests/fm-grok-harness.test.sh @@ -26,7 +26,13 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse gh-axi gh + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" != get ] || printf '%s\n' "${FM_FAKE_PANE_PATH:-}" +exit 0 +SH + chmod +x "$fakebin/treehouse" + fm_fake_exit0 "$fakebin" gh-axi gh printf '%s\n' "$fakebin" } diff --git a/tests/fm-kimi-harness.test.sh b/tests/fm-kimi-harness.test.sh index b9f8480345..4b28455078 100755 --- a/tests/fm-kimi-harness.test.sh +++ b/tests/fm-kimi-harness.test.sh @@ -125,7 +125,13 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse gh-axi gh + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" != get ] || printf '%s\n' "${FM_FAKE_PANE_PATH:-}" +exit 0 +SH + chmod +x "$fakebin/treehouse" + fm_fake_exit0 "$fakebin" gh-axi gh fm_fake_exit0 "$fakebin" kimi ln -s "$JQ_BIN" "$fakebin/jq" printf '%s\n' "$fakebin" diff --git a/tests/fm-spawn-worktree-lease.test.sh b/tests/fm-spawn-worktree-lease.test.sh index e105526636..9d4be4db9e 100755 --- a/tests/fm-spawn-worktree-lease.test.sh +++ b/tests/fm-spawn-worktree-lease.test.sh @@ -112,18 +112,29 @@ case "${1:-}" in return) shift target= + required_holder= while [ $# -gt 0 ]; do case "$1" in --force) ;; + --if-lease-holder) shift; required_holder=${1:-} ;; *) target=$1 ;; esac shift || true done [ -z "${FM_FAKE_TREEHOUSE_RETURN_FAIL:-}" ] || exit 17 + if [ -n "${FM_FAKE_TREEHOUSE_RETURN_ENTERED:-}" ]; then + : > "$FM_FAKE_TREEHOUSE_RETURN_ENTERED" + while [ ! -e "${FM_FAKE_TREEHOUSE_RETURN_RELEASE:?}" ]; do + sleep 0.01 + done + fi if [ -n "$target" ]; then for f in "$pool"/leases/*; do [ -e "$f" ] || continue if [ "$(cat "$f")" = "$target" ]; then + if [ -n "$required_holder" ] && [ "$(basename "$f")" != "$required_holder" ]; then + exit 18 + fi rm -f "$f" fi done @@ -131,6 +142,27 @@ case "${1:-}" in exit 0 ;; status) + first=1 + printf '[' + for slot in "$pool"/slots/*; do + [ -e "$slot" ] || continue + path=$(cat "$slot") + holder= + for f in "$pool"/leases/*; do + [ -e "$f" ] || continue + if [ "$(cat "$f")" = "$path" ]; then + holder=$(basename "$f") + break + fi + done + state=available + [ -z "$holder" ] || state=leased + [ "$first" -eq 1 ] || printf ',' + first=0 + printf '{"name":"%s","path":"%s","status":"%s","lease_id":"test","lease_holder":"%s","leased_at":null,"processes":[]}' \ + "$(basename "$slot")" "$path" "$state" "$holder" + done + printf ']\n' exit 0 ;; esac @@ -268,6 +300,10 @@ test_spawn_lease_blocks_second_get() { assert_contains "$out" "spawned $TASK_ID" "spawn did not report success" assert_grep "worktree=$WT_DIR" "$HOME_DIR/state/$TASK_ID.meta" \ "meta did not record the leased worktree" + assert_grep "treehouse_lease_holder=$TASK_ID" "$HOME_DIR/state/$TASK_ID.meta" \ + "meta did not record the treehouse lease holder" + assert_grep "treehouse_lease_state=held" "$HOME_DIR/state/$TASK_ID.meta" \ + "meta did not record the held treehouse lease state" lease_file=$(lease_holder_file "$TASK_ID") [ -f "$lease_file" ] || fail "spawn did not record a durable lease under the task id" [ "$(cat "$lease_file")" = "$WT_DIR" ] || fail "lease path mismatch: $(cat "$lease_file")" @@ -388,6 +424,8 @@ test_recovery_reuses_worktree_without_second_lease() { if grep -F 'treehouse get --lease' "$CASE_DIR/treehouse.log" >/dev/null 2>&1; then fail "recovery spawn acquired a second lease: $(cat "$CASE_DIR/treehouse.log")" fi + grep -F 'treehouse status --json' "$CASE_DIR/treehouse.log" >/dev/null \ + || fail "recovery spawn did not verify the recorded task-held lease" [ -f "$lease_file" ] || fail "recovery spawn dropped the original lease" [ "$(cat "$lease_file")" = "$WT_DIR" ] || fail "recovery spawn changed the lease path" # Only one lease file for this holder (no double-lease artifact). @@ -396,6 +434,89 @@ test_recovery_reuses_worktree_without_second_lease() { pass "recovery into an existing worktree does not create a second lease" } +test_recovery_refuses_returned_stale_meta() { + local rec out status meta_tmp + rec=$(make_lease_case recovery-stale-return ship-lease-f6) + read_lease_case "$rec" + + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "initial spawn for stale-return case failed"$'\n'"$out" + FM_FAKE_TREEHOUSE_POOL_DIR="$POOL_DIR" \ + FM_FAKE_TREEHOUSE_LOG="$CASE_DIR/treehouse.log" \ + PATH="$FAKEBIN_DIR:$PATH" \ + treehouse return --force --if-lease-holder "$TASK_ID" "$WT_DIR" + [ ! -e "$(lease_holder_file "$TASK_ID")" ] || fail "stale-return fixture left the lease held" + + : > "$CASE_DIR/treehouse.log" + set +e + out=$(run_spawn "$TASK_ID") + status=$? + set -e + [ "$status" -ne 0 ] || fail "recovery reused a returned worktree from stale metadata" + assert_contains "$out" "not leased under task $TASK_ID" \ + "stale returned-worktree recovery did not fail on ownership proof" + if grep -F 'treehouse get --lease' "$CASE_DIR/treehouse.log" >/dev/null 2>&1; then + fail "stale recovery acquired a replacement lease instead of failing closed" + fi + + meta_tmp="$CASE_DIR/returned.meta" + grep -v '^treehouse_lease_state=' "$HOME_DIR/state/$TASK_ID.meta" > "$meta_tmp" + printf 'treehouse_lease_state=returned\n' >> "$meta_tmp" + mv "$meta_tmp" "$HOME_DIR/state/$TASK_ID.meta" + set +e + out=$(run_spawn "$TASK_ID") + status=$? + set -e + [ "$status" -ne 0 ] || fail "recovery reused a worktree explicitly recorded as returned" + assert_contains "$out" "lease state returned" \ + "returned lease state did not block recovery" + pass "recovery refuses a returned worktree retained in stale metadata" +} + +test_teardown_serializes_against_recovery() { + local rec out status entered release teardown_out teardown_pid spawn_out spawn_status i + rec=$(make_lease_case teardown-race ship-lease-g7) + read_lease_case "$rec" + + out=$(run_spawn "$TASK_ID") + status=$? + expect_code 0 "$status" "initial spawn for teardown race failed"$'\n'"$out" + entered="$CASE_DIR/return-entered" + release="$CASE_DIR/return-release" + teardown_out="$CASE_DIR/teardown.out" + FM_FAKE_TREEHOUSE_RETURN_ENTERED="$entered" \ + FM_FAKE_TREEHOUSE_RETURN_RELEASE="$release" \ + run_teardown "$TASK_ID" --force >"$teardown_out" 2>&1 & + teardown_pid=$! + i=0 + while [ ! -e "$entered" ] && [ "$i" -lt 200 ]; do + sleep 0.01 + i=$((i + 1)) + done + [ -e "$entered" ] || { + : > "$release" + wait "$teardown_pid" || true + fail "teardown did not reach the blocked return: $(cat "$teardown_out")" + } + assert_grep 'treehouse_lease_state=returning' "$HOME_DIR/state/$TASK_ID.meta" \ + "teardown did not persist return-in-progress ownership" + + set +e + spawn_out=$(run_spawn "$TASK_ID") + spawn_status=$? + set -e + [ "$spawn_status" -ne 0 ] || fail "recovery entered while teardown held the task lifecycle lock" + assert_contains "$spawn_out" "another spawn is already creating task $TASK_ID" \ + "recovery did not observe teardown's shared task lock" + + : > "$release" + wait "$teardown_pid" + status=$? + expect_code 0 "$status" "blocked teardown did not finish after release"$'\n'"$(cat "$teardown_out")" + pass "teardown and recovery serialize on the task lifecycle lock" +} + # --------------------------------------------------------------------------- # 5. Pre-existing unleased worktrees do not break spawn or teardown. # --------------------------------------------------------------------------- @@ -449,6 +570,8 @@ test_spawn_lease_blocks_second_get test_successful_teardown_releases_lease test_refused_teardown_keeps_lease test_recovery_reuses_worktree_without_second_lease +test_recovery_refuses_returned_stale_meta +test_teardown_serializes_against_recovery test_legacy_unleased_worktree_spawn_and_teardown echo "# all fm-spawn-worktree-lease tests passed" diff --git a/tests/fm-tangle-guard.test.sh b/tests/fm-tangle-guard.test.sh index 4df2706121..65b60f3780 100755 --- a/tests/fm-tangle-guard.test.sh +++ b/tests/fm-tangle-guard.test.sh @@ -149,7 +149,7 @@ test_brief_assertion_precedes_branch() { # --- GUARD 1b: fm-spawn isolation abort ------------------------------------- -# A fake tmux that reports FM_FAKE_PANE_PATH as the post-`treehouse get` pane cwd +# A fake tmux that reports FM_FAKE_PANE_PATH as the post-acquisition pane cwd # (so the spawn's worktree-resolution loop resolves to a path we control), names # the session on '#S', and swallows window ops. Echoes the fakebin dir. make_spawn_fakebin() { @@ -169,7 +169,12 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" != get ] || printf '%s\n' "${FM_FAKE_PANE_PATH:-}" +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } @@ -225,7 +230,7 @@ test_spawn_isolation_abort() { # collides under base-index 1; # - the window id is captured (-P -F #{window_id}) and automatic-rename/allow-rename # are disabled so the fm- name survives treehouse cd'ing into the worktree; -# - the treehouse-get send-keys and the worktree wait loop target that stable +# - the worktree cd send-keys and the worktree wait loop target that stable # window id, never the (possibly-renamed) name - a lost name would let # display-message fall back to the active client's window and misread firstmate's # OWN pane as the worktree, tangling a hook into the primary checkout. @@ -248,7 +253,12 @@ esac exit 0 SH chmod +x "$fakebin/tmux" - fm_fake_exit0 "$fakebin" treehouse + cat > "$fakebin/treehouse" <<'SH' +#!/usr/bin/env bash +[ "${1:-}" != get ] || printf '%s\n' "${FM_FAKE_PANE_PATH:-}" +exit 0 +SH + chmod +x "$fakebin/treehouse" printf '%s\n' "$fakebin" } @@ -292,9 +302,9 @@ test_spawn_tmux_window_construction() { assert_grep "set-window-option -t @spawnwid allow-rename off" "$rec" \ "must disable allow-rename on the spawned window" - # Bug 2 fix (b): treehouse-get and the worktree wait loop target the stable id. - assert_grep "send-keys -t @spawnwid treehouse get Enter" "$rec" \ - "treehouse get must be sent to the stable window id" + # Bug 2 fix (b): worktree entry and the worktree wait loop target the stable id. + assert_grep "send-keys -t @spawnwid cd '$wt' Enter" "$rec" \ + "worktree cd must be sent to the stable window id" assert_grep "display-message -p -t @spawnwid #{pane_current_path}" "$rec" \ "the worktree wait loop must query the stable window id, not the name" From b2f4f5d9193f3a30aabf913156fa60c53abff893 Mon Sep 17 00:00:00 2001 From: AG Werschky Date: Mon, 3 Aug 2026 13:58:23 -0600 Subject: [PATCH 3/9] no-mistakes(review): Captain, harden nested worktree lease lifecycle --- .github/workflows/ci.yml | 8 +- bin/fm-bootstrap.sh | 24 +++-- bin/fm-install-treehouse.sh | 18 ++-- bin/fm-spawn.sh | 80 +++++++++----- bin/fm-teardown.sh | 143 ++++++++++++++++++++++--- tests/fm-bootstrap.test.sh | 66 ++++++++++-- tests/fm-secondmate-harness.test.sh | 9 +- tests/fm-secondmate-liveness.test.sh | 9 +- tests/fm-secondmate-safety.test.sh | 103 ++++++++++++++++++ tests/fm-secondmate-sync.test.sh | 9 +- tests/fm-session-start.test.sh | 10 +- tests/fm-spawn-worktree-lease.test.sh | 71 ++++++++++++ tests/fm-startup-memory-budget.test.sh | 10 +- tests/fm-x-mode.test.sh | 10 +- tests/secondmate-helpers.sh | 2 + 15 files changed, 490 insertions(+), 82 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 177ab3ba2c..8f3eb05e3f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -188,7 +188,13 @@ jobs: command -v herdr >/dev/null || { echo "::error::herdr not on PATH after install"; exit 1; } command -v treehouse >/dev/null || { echo "::error::treehouse not on PATH after install"; exit 1; } herdr --version - treehouse --version + [ "$(treehouse --version | tr -d '[:space:]')" = "v2.1.0" ] || { + echo "::error::expected exact Treehouse pin v2.1.0" + exit 1 + } + treehouse get --help | grep -F -- '--lease-holder' + treehouse return --help | grep -F -- '--if-lease-holder' + treehouse status --help | grep -F -- '--json' status=$(herdr status --json) printf '%s\n' "$status" version=$(printf '%s' "$status" | jq -r '.client.version // empty') diff --git a/bin/fm-bootstrap.sh b/bin/fm-bootstrap.sh index 0ac06b162f..00a1b6f42f 100755 --- a/bin/fm-bootstrap.sh +++ b/bin/fm-bootstrap.sh @@ -45,8 +45,8 @@ # A TANGLE line means the firstmate primary checkout (FM_ROOT) is stranded # on a feature branch instead of its default branch - a crewmate's work # landed in the primary instead of its own worktree; restore it per the line. -# treehouse is also MISSING when its installed version lacks -# "treehouse get --lease" support. +# treehouse is also MISSING below 2.1.0 or when its installed build +# lacks the get, status, and guarded-return lease lifecycle flags. # no-mistakes is also MISSING when its installed version is older than # 1.31.2. # tasks-axi and quota-axi are required bootstrap tools (same class as @@ -536,9 +536,17 @@ if ! BACKEND_TOOLS=$(fm_backend_required_tools "$BACKEND"); then fi TOOLS="$BACKEND_TOOLS $COMMON_TOOLS" NO_MISTAKES_MIN=1.31.2 - -treehouse_supports_lease() { - treehouse get --help 2>&1 | grep -Eq '(^|[^[:alnum:]_-])--lease([^[:alnum:]_-]|$)' +TREEHOUSE_MIN=2.1.0 + +treehouse_supports_lease_lifecycle() { + local get_help return_help status_help + get_help=$(treehouse get --help 2>&1) || return 1 + return_help=$(treehouse return --help 2>&1) || return 1 + status_help=$(treehouse status --help 2>&1) || return 1 + printf '%s\n' "$get_help" | grep -Eq '(^|[^[:alnum:]_-])--lease([^[:alnum:]_-]|$)' \ + && printf '%s\n' "$get_help" | grep -Eq '(^|[^[:alnum:]_-])--lease-holder([^[:alnum:]_-]|$)' \ + && printf '%s\n' "$return_help" | grep -Eq '(^|[^[:alnum:]_-])--if-lease-holder([^[:alnum:]_-]|$)' \ + && printf '%s\n' "$status_help" | grep -Eq '(^|[^[:alnum:]_-])--json([^[:alnum:]_-]|$)' } # Shared semantic-version floor for the tool gates below. A version string that @@ -865,11 +873,13 @@ done for t in $COMMON_TOOLS; do command -v "$t" >/dev/null || missing_tool_diagnostic "$t" done -# The treehouse lease-support upgrade check is only relevant when the resolved +# The treehouse lease-lifecycle upgrade check is only relevant when the resolved # backend actually requires treehouse (every backend except orca, which owns its # own worktrees); an orca home must not be told to upgrade a provider it never uses. if fm_backend_list_contains "$TOOLS" treehouse \ - && command -v treehouse >/dev/null 2>&1 && ! treehouse_supports_lease; then + && command -v treehouse >/dev/null 2>&1 \ + && { ! tool_version_at_least treehouse "$TREEHOUSE_MIN" \ + || ! treehouse_supports_lease_lifecycle; }; then echo "MISSING: treehouse (install: $(install_cmd treehouse))" fi if command -v no-mistakes >/dev/null 2>&1 && ! tool_version_at_least no-mistakes "$NO_MISTAKES_MIN"; then diff --git a/bin/fm-install-treehouse.sh b/bin/fm-install-treehouse.sh index 9d181439c8..c165cb11ff 100755 --- a/bin/fm-install-treehouse.sh +++ b/bin/fm-install-treehouse.sh @@ -9,12 +9,12 @@ # Usage: # fm-install-treehouse.sh # -# Pins Treehouse v2.0.1, the version exercised by the local real-Herdr suite. +# Pins Treehouse v2.1.0, the lease-lifecycle floor exercised by real-Herdr CI. set -eu -FM_TREEHOUSE_CI_VERSION=2.0.1 +FM_TREEHOUSE_CI_VERSION=2.1.0 FM_TREEHOUSE_CI_TAG="v${FM_TREEHOUSE_CI_VERSION}" -# Bounded download ceiling (bytes). Official 2.0.1 archives are under 8 MiB. +# Bounded download ceiling (bytes). Official 2.1.0 archives are under 8 MiB. FM_TREEHOUSE_CI_MAX_BYTES=15000000 FM_TREEHOUSE_CI_REPO=kunchenguid/treehouse @@ -30,19 +30,19 @@ arch=$(uname -m) case "${os}-${arch}" in Linux-x86_64) ARCHIVE=treehouse-v${FM_TREEHOUSE_CI_VERSION}-linux-amd64.tar.gz - SHA256=1d5a32751ab921670103fd201ddb2b91b47338cb13976f45642b827cf8976af2 + SHA256=ff030255663bb5d384309cdf1b3a0bd62006e0ac978340d292039697cb70c225 ;; Linux-aarch64|Linux-arm64) ARCHIVE=treehouse-v${FM_TREEHOUSE_CI_VERSION}-linux-arm64.tar.gz - SHA256=eaccc9c5b98125df8bd77425598eeecee66cb0371db4eb1cf75f0d813c18fab9 + SHA256=d4e37ad11d50fb6381c060d34e7372e944e01af78b0c6707c6d0ff966c7c2c44 ;; Darwin-arm64) ARCHIVE=treehouse-v${FM_TREEHOUSE_CI_VERSION}-darwin-arm64.tar.gz - SHA256=7ee5078f3d1f33c01196548797fce65408e459d53530b77d4ba56e074fa1c1a2 + SHA256=a2e9bcba88d643a828062f3425dcf601886372fc2aca87b542b36fc8bbf2c992 ;; Darwin-x86_64) ARCHIVE=treehouse-v${FM_TREEHOUSE_CI_VERSION}-darwin-amd64.tar.gz - SHA256=1cf44580a5837f995e1d3bb74f4fbd3112b642acd20406087d9735a8106112fd + SHA256=8eb2eb3b63f409a316bd9c19af5d3035e5b72f6feffc98f0fac658e1ed33a397 ;; *) die "unsupported platform ${os}-${arch}; official Treehouse assets are linux/darwin amd64 and arm64" @@ -68,7 +68,7 @@ fi [ "$ACTUAL_SHA256" = "$SHA256" ] || die "checksum mismatch for $ARCHIVE (expected $SHA256, got $ACTUAL_SHA256)" tar -xzf "$TMP/$ARCHIVE" -C "$TMP" -# Archive layout: a single `treehouse` binary at the archive root (verified for v2.0.1). +# Archive layout: a single `treehouse` binary at the archive root (verified for v2.1.0). if [ -f "$TMP/treehouse" ]; then BIN="$TMP/treehouse" elif [ -f "$TMP/treehouse-v${FM_TREEHOUSE_CI_VERSION}/treehouse" ]; then @@ -82,7 +82,7 @@ mkdir -p "$DESTINATION" install -m 0755 "$BIN" "$DESTINATION/treehouse" installed_version=$("$DESTINATION/treehouse" --version 2>/dev/null | tr -d '[:space:]') -# treehouse prints "v2.0.1" (leading v) on --version. +# treehouse prints "v2.1.0" (leading v) on --version. case "$installed_version" in "v${FM_TREEHOUSE_CI_VERSION}"|"${FM_TREEHOUSE_CI_VERSION}") ;; *) diff --git a/bin/fm-spawn.sh b/bin/fm-spawn.sh index 441e2acbba..fa5319336d 100755 --- a/bin/fm-spawn.sh +++ b/bin/fm-spawn.sh @@ -274,6 +274,7 @@ ORCA_TERMINAL= TREEHOUSE_LEASE_ACQUIRED=0 TREEHOUSE_LEASE_HOLDER= TREEHOUSE_LEASE_STATE= +META_TMP= HERDR_PROJECTION_ABORT_CLEANUP=0 HERDR_PROJECTION_ABORT_SESSION= HERDR_PROJECTION_ABORT_TASK_PANE= @@ -303,7 +304,7 @@ parse_orca_worktree_result() { } spawn_abort_cleanup() { - local status=$? + local status=$? published_wt published_holder published_state if [ "$HERDR_PROJECTION_ABORT_CLEANUP" = 1 ] \ && [ "$HERDR_PRESENTATION_ORDER_LOCK_HELD" != 1 ]; then if ! spawn_herdr_presentation_order_lock_acquire "${HERDR_PROJECTION_ABORT_SESSION:-}"; then @@ -354,11 +355,24 @@ spawn_abort_cleanup() { # that reuses a recorded worktree never sets TREEHOUSE_LEASE_ACQUIRED, so an # abort there cannot free a still-owned slot. if [ "${TREEHOUSE_LEASE_ACQUIRED:-0}" = 1 ] && [ -n "${WT:-}" ]; then - TREEHOUSE_LEASE_ACQUIRED=0 - if [ -n "${PROJ_ABS:-}" ] && [ -d "$WT" ] && command -v treehouse >/dev/null 2>&1; then - ( cd "$PROJ_ABS" && treehouse return --force --if-lease-holder "$ID" "$WT" ) >/dev/null 2>&1 || true + published_wt=$(fm_meta_get "$STATE/$ID.meta" worktree) + published_holder=$(fm_meta_get "$STATE/$ID.meta" treehouse_lease_holder) + published_state=$(fm_meta_get "$STATE/$ID.meta" treehouse_lease_state) + if [ "$published_wt" = "$WT" ] \ + && [ "$published_holder" = "$ID" ] \ + && [ "$published_state" = held ]; then + TREEHOUSE_LEASE_ACQUIRED=0 + else + TREEHOUSE_LEASE_ACQUIRED=0 + if [ -n "${PROJ_ABS:-}" ] && [ -d "$WT" ] && command -v treehouse >/dev/null 2>&1; then + ( cd "$PROJ_ABS" && treehouse return --force --if-lease-holder "$ID" "$WT" ) >/dev/null 2>&1 || true + fi fi fi + if [ -n "${META_TMP:-}" ]; then + rm -f "$META_TMP" 2>/dev/null || true + META_TMP= + fi if [ "$SPAWN_TASK_LOCK_HELD" = 1 ]; then SPAWN_TASK_LOCK_HELD=0 fm_lock_release "$SPAWN_TASK_LOCK" || true @@ -1348,27 +1362,33 @@ if [ "$KIND" != secondmate ] && [ "$BACKEND" != orca ]; then existing_lease_holder=$(fm_meta_get "$STATE/$ID.meta" treehouse_lease_holder) existing_lease_state=$(fm_meta_get "$STATE/$ID.meta" treehouse_lease_state) fi - if [ -n "$existing_wt" ] && [ -d "$existing_wt" ]; then - case "$existing_lease_state" in - '') ;; - held) - if [ "$existing_lease_holder" != "$ID" ] \ - || ! recorded_treehouse_lease_is_owned "$existing_wt" "$ID"; then - echo "error: recorded worktree $existing_wt is not leased under task $ID; refusing stale recovery" >&2 - exit 1 - fi - TREEHOUSE_LEASE_HOLDER=$ID - TREEHOUSE_LEASE_STATE=held - ;; - returning|returned) - echo "error: recorded worktree $existing_wt has lease state $existing_lease_state; refusing recovery after return" >&2 + case "$existing_lease_state" in + '') + if [ -n "$existing_lease_holder" ]; then + echo "error: recorded worktree ${existing_wt:-} has a treehouse lease holder without a lease state; refusing recovery" >&2 exit 1 - ;; - *) - echo "error: recorded worktree $existing_wt has unknown lease state $existing_lease_state; refusing recovery" >&2 + fi + ;; + held) + if [ "$existing_lease_holder" != "$ID" ] \ + || [ -z "$existing_wt" ] || [ ! -d "$existing_wt" ] \ + || ! recorded_treehouse_lease_is_owned "$existing_wt" "$ID"; then + echo "error: recorded worktree ${existing_wt:-} is not leased under task $ID; refusing stale recovery" >&2 exit 1 - ;; - esac + fi + TREEHOUSE_LEASE_HOLDER=$ID + TREEHOUSE_LEASE_STATE=held + ;; + returning|returned) + echo "error: recorded worktree ${existing_wt:-} has lease state $existing_lease_state; refusing recovery after return" >&2 + exit 1 + ;; + *) + echo "error: recorded worktree ${existing_wt:-} has unknown lease state $existing_lease_state; refusing recovery" >&2 + exit 1 + ;; + esac + if [ -n "$existing_wt" ] && [ -d "$existing_wt" ]; then WT=$existing_wt validate_spawn_worktree "recorded worktree" "$T" else @@ -1696,6 +1716,10 @@ fi META_WINDOW=$T [ "$BACKEND" = orca ] && META_WINDOW=$W +META_TMP=$(mktemp "$STATE/.${ID}.meta.XXXXXX") || { + echo "error: could not create temporary metadata for task $ID" >&2 + exit 1 +} { echo "window=$META_WINDOW" echo "endpoint_task_id=$ID" @@ -1738,7 +1762,15 @@ META_WINDOW=$T echo "home=$PROJ_ABS" echo "projects=$SECONDMATE_PROJECTS" fi -} > "$STATE/$ID.meta" +} > "$META_TMP" || { + echo "error: could not write metadata for task $ID" >&2 + exit 1 +} +mv -f "$META_TMP" "$STATE/$ID.meta" || { + echo "error: could not publish metadata for task $ID" >&2 + exit 1 +} +META_TMP= [ "$BACKEND" = orca ] && ORCA_ABORT_CLEANUP=0 # Meta now owns the worktree path; abort must not release a successfully # published ship/scout lease (teardown is the only release path). diff --git a/bin/fm-teardown.sh b/bin/fm-teardown.sh index be5ed28245..63fc718a3a 100755 --- a/bin/fm-teardown.sh +++ b/bin/fm-teardown.sh @@ -131,7 +131,18 @@ FM_LOCK_LOG_PREFIX=teardown META="$STATE/$ID.meta" TEARDOWN_TASK_LOCK="$STATE/.spawn-$ID.lock" TEARDOWN_TASK_LOCK_HELD=0 +TEARDOWN_CHILD_TASK_LOCKS= teardown_release_task_lock() { + local child_lock + if [ -n "$TEARDOWN_CHILD_TASK_LOCKS" ]; then + while IFS= read -r child_lock; do + [ -n "$child_lock" ] || continue + fm_lock_release "$child_lock" || true + done <&2 + exit 1 +fi -treehouse_lease_state_set() { # - local state=$1 tmp - tmp=$(mktemp "$STATE/.${ID}.meta.treehouse-lease.XXXXXX") || return 1 - if ! { grep -v '^treehouse_lease_state=' "$META" || true; } > "$tmp"; then +treehouse_lease_state_set_for_meta() { # + local meta=$1 task_id=$2 state=$3 state_dir tmp + state_dir=$(dirname "$meta") + tmp=$(mktemp "$state_dir/.${task_id}.meta.treehouse-lease.XXXXXX") || return 1 + if ! { grep -v '^treehouse_lease_state=' "$meta" || true; } > "$tmp"; then rm -f "$tmp" return 1 fi @@ -203,10 +219,15 @@ treehouse_lease_state_set() { # rm -f "$tmp" return 1 } - mv -f "$tmp" "$META" || { + mv -f "$tmp" "$meta" || { rm -f "$tmp" return 1 } +} + +treehouse_lease_state_set() { # + local state=$1 + treehouse_lease_state_set_for_meta "$META" "$ID" "$state" || return 1 TREEHOUSE_LEASE_STATE=$state } @@ -1112,13 +1133,87 @@ remove_firstmate_home() { safe_rm_rf "$abs_home_path" "$label" } +treehouse_lease_metadata_validate() { #