diff --git a/bin/backends/herdr.sh b/bin/backends/herdr.sh index 260d2b666..14e06b50d 100644 --- a/bin/backends/herdr.sh +++ b/bin/backends/herdr.sh @@ -303,20 +303,90 @@ fm_backend_herdr_workspace_prune_seeded_default_tab() { # + local session=$1 label key lock i holder_pid misses=0 + label=$(fm_backend_herdr_workspace_label) + key=$(printf '%s' "$FM_HOME" | cksum | cut -d' ' -f1) + lock="${TMPDIR:-/tmp}/fm-herdr-ws.$key.$session.$label.lock" + for i in $(seq 1 120); do + if mkdir "$lock" 2>/dev/null; then + printf '%s' "$$" > "$lock/pid" 2>/dev/null || true + FM_BACKEND_HERDR_WS_LOCK=$lock + return 0 + fi + if [ ! -d "$lock" ]; then + misses=$((misses + 1)) + [ "$misses" -lt 3 ] || return 1 + continue + fi + misses=0 + holder_pid=$(cat "$lock/pid" 2>/dev/null || true) + if [ -n "$holder_pid" ] && ! kill -0 "$holder_pid" 2>/dev/null; then + if mv "$lock" "$lock.breaking.$$" 2>/dev/null; then + rm -rf "$lock.breaking.$$" 2>/dev/null || true + continue + fi + fi + sleep 0.1 + done + return 1 +} + +fm_backend_herdr_workspace_lock_release() { + [ -n "${FM_BACKEND_HERDR_WS_LOCK:-}" ] || return 0 + rm -rf "$FM_BACKEND_HERDR_WS_LOCK" 2>/dev/null || true + FM_BACKEND_HERDR_WS_LOCK="" +} + fm_backend_herdr_workspace_ensure() { # local session=$1 cwd=$2 wsid out label FM_BACKEND_HERDR_WS_ID="" FM_BACKEND_HERDR_WS_SEEDED_TAB_ID="" + FM_BACKEND_HERDR_WS_LOCK="" + # Serialize the find-or-create window below against concurrent spawns of + # this same home (see fm_backend_herdr_workspace_lock_acquire above). + # Acquire failure warns and proceeds unlocked - never blocks the spawn. + fm_backend_herdr_workspace_lock_acquire "$session" \ + || echo "warning: herdr workspace-ensure lock not acquired; proceeding unlocked (concurrent first spawns may mint duplicate workspaces)" >&2 wsid=$(fm_backend_herdr_workspace_find "$session") if [ -n "$wsid" ]; then + fm_backend_herdr_workspace_lock_release FM_BACKEND_HERDR_WS_ID=$wsid printf '%s' "$wsid" return 0 fi label=$(fm_backend_herdr_workspace_label) - out=$(fm_backend_herdr_cli "$session" workspace create --cwd "$cwd" --label "$label" --no-focus 2>/dev/null) || return 1 + out=$(fm_backend_herdr_cli "$session" workspace create --cwd "$cwd" --label "$label" --no-focus 2>/dev/null) \ + || { fm_backend_herdr_workspace_lock_release; return 1; } wsid=$(printf '%s' "$out" | jq -r '.result.workspace.workspace_id // empty' 2>/dev/null) - [ -n "$wsid" ] || return 1 + [ -n "$wsid" ] || { fm_backend_herdr_workspace_lock_release; return 1; } FM_BACKEND_HERDR_WS_ID=$wsid # Herdr seeds a new workspace with one auto-created default tab firstmate # never uses. It is NOT pruned here: at this instant it is the workspace's @@ -326,6 +396,7 @@ fm_backend_herdr_workspace_ensure() { # # once the first real task tab exists alongside it, and only ever targets # this exact captured tab_id. FM_BACKEND_HERDR_WS_SEEDED_TAB_ID=$(printf '%s' "$out" | jq -r '.result.tab.tab_id // empty' 2>/dev/null) + fm_backend_herdr_workspace_lock_release printf '%s' "$wsid" } diff --git a/docs/herdr-backend.md b/docs/herdr-backend.md index 0bed40dd1..975624f02 100644 --- a/docs/herdr-backend.md +++ b/docs/herdr-backend.md @@ -123,7 +123,7 @@ Workspace-per-task - tried against the real binary in P2 and again considered he ## Workspace lifecycle: one persistent per-home workspace, reused -Each home's own workspace (`firstmate` for the primary, `2ndmate-` for a secondmate - see "Label derivation" above) is created once per session and reused by every subsequent spawn from that home: `fm_backend_herdr_workspace_ensure` calls `fm_backend_herdr_workspace_find` first and creates a workspace only when none labelled for that home exists yet. +Each home's own workspace (`firstmate` for the primary, `2ndmate-` for a secondmate - see "Label derivation" above) is created once per session and reused by every subsequent spawn from that home: `fm_backend_herdr_workspace_ensure` calls `fm_backend_herdr_workspace_find` first and creates a workspace only when none labelled for that home exists yet, with the find-or-create window serialized against concurrent spawns of the same home (see "Incident (2026-07-19)" below). Teardown (`fm_backend_herdr_kill`) closes only the task's pane/tab, never the workspace. Reserved-keyword guard: never name a `jq --arg`/`--argjson` after a `jq` keyword (`label`, `and`, `or`, `not`, `if`, `then`, `else`, `end`, `reduce`, `foreach`, `import`, `def`, `as`, `__loc__`). @@ -158,6 +158,33 @@ Because closing a workspace's last tab deletes it, a home's workspace does not o A workspace whose label this adapter did not derive (see "Label derivation" above) is never adopted, reused, or torn down by firstmate - `fm_backend_herdr_workspace_find` and `fm_backend_herdr_list_live` only ever match a home's own derived label. +## Incident (2026-07-19): parallel first spawns minted duplicate per-home workspaces + +Observed on real herdr 0.7.4 (protocol 16), macOS aarch64, firstmate `b2bf95f`, primary home, session `default`. +A batch request had the primary spawn three crewmates in parallel as each task's FIRST spawn of the session: `crew-readme-a1` (grok ship), `crew-herdr-scout-b2` (grok scout), and `crew-todo-c3` (claude ship), every task meta recording `backend=herdr`. +The spaces sidebar immediately showed three workspaces all labeled `firstmate`, one task tab in each, instead of one `firstmate` workspace with three tabs. +After the fastest task finished and tore down, `herdr workspace list` still showed the two surviving duplicates (the third had already vanished through the verified closing-the-last-tab-deletes-the-workspace behavior): + +```sh +herdr workspace list | jq -c '.result.workspaces[] | {workspace_id, label}' +``` + +```text +{"workspace_id":"w2","label":"captain"} +{"workspace_id":"w3","label":"firstmate"} +{"workspace_id":"w4","label":"firstmate"} +``` + +The root cause is the adapter's own find-before-create window, not herdr: `fm_backend_herdr_workspace_ensure` ran `workspace_find` and then `workspace create` with no serialization between concurrent spawn processes. +Three parallel first spawns each found no workspace yet, so each minted its own; herdr enforces no label uniqueness ("Label collisions" above), so nothing downstream failed and the fleet silently fragmented across duplicate spaces. +Sequential spawns were never affected because the second spawn's `workspace_find` adopts the first spawn's workspace. + +**Fix:** `fm_backend_herdr_workspace_lock_acquire` / `_release` in `bin/backends/herdr.sh` serialize the find-or-create window with an atomic `mkdir` lock (portable; macOS ships no flock) under `${TMPDIR:-/tmp}`, keyed by a cksum of `$FM_HOME` plus the session and the home's own label - the same axes that scope the workspace itself, so different homes and sessions never contend. +The holder records its pid; a waiter breaks the lock only when that pid is provably dead (`kill -0` fails), and the break is atomic - the waiter renames the lock dir aside before removing it, so of several concurrent waiters exactly one wins the break and the rest keep waiting. A lock dir whose pid file has not appeared yet is simply waited out within the bounded acquire budget (12s). +The lock is fail-open by design: on acquire timeout - or promptly, after three consecutive failed creation attempts, when the lock dir cannot be created at all (e.g. an unwritable `TMPDIR`) - the spawn proceeds unlocked after one stderr warning, so the worst case is exactly the pre-fix race (a duplicate workspace), never a blocked spawn - mirroring the "quota trouble never blocks dispatch" posture. +Unit coverage in `tests/fm-backend-herdr.test.sh`: `test_workspace_ensure_concurrent_first_spawns_mint_one_workspace` (three genuinely parallel `container_ensure` calls against the stateful fake mint exactly one workspace and share it), `test_workspace_ensure_waits_for_live_lock_holder_then_adopts` (a deterministic hold-then-release proof that a waiter adopts the winner's workspace with zero creates), and `test_workspace_ensure_breaks_provably_dead_lock_holder` (a crashed prior holder's lock is broken promptly and released after ensure). +Duplicate workspaces minted BEFORE this fix self-heal through normal teardown: each duplicate disappears when its last task tab closes, and the next spawn adopts whichever single labeled workspace remains. + ## Target string and meta fields A herdr task's `window=` meta field holds `:`, for example `default:w1:p2`. diff --git a/tests/fm-backend-herdr.test.sh b/tests/fm-backend-herdr.test.sh index 5186cca2c..4f3e3be86 100755 --- a/tests/fm-backend-herdr.test.sh +++ b/tests/fm-backend-herdr.test.sh @@ -1587,6 +1587,99 @@ EOF pass "herdr repeated spawn/teardown: one persistent firstmate workspace reused, zero orphans, default tab pruned, create ran once" } +# --- workspace-ensure serialization (2026-07-19 duplicate-workspace fix) ---- +# +# Root cause and fix are documented at fm_backend_herdr_workspace_lock_acquire +# in bin/backends/herdr.sh and docs/herdr-backend.md's "Incident (2026-07-19)" +# section: N first spawns launched in PARALLEL each ran workspace_find while +# no workspace existed yet, and each then minted its own workspace with the +# identical home label (herdr enforces no label uniqueness, so nothing +# downstream failed - the fleet just fragmented across duplicate spaces). + +test_workspace_ensure_concurrent_first_spawns_mint_one_workspace() { + # Three container_ensure calls launched in PARALLEL against an empty state, + # reproducing the exact 2026-07-19 batch-spawn shape. The mkdir lock (scoped + # to this test via TMPDIR) must serialize the find-or-create window so + # exactly ONE workspace create runs and all three callers resolve the SAME + # container. + local dir log state fb i p raw1 raw2 raw3 wscount created + dir="$TMP_ROOT/concurrent-ensure"; mkdir -p "$dir/locks"; log="$dir/log"; state="$dir/state.json"; : > "$log" + fb=$(make_herdr_statefake "$dir") + for i in 1 2 3; do + ( PATH="$fb:$PATH" FM_HERDR_LOG="$log" FM_FAKE_HERDR_STATE="$state" HERDR_SESSION=fmtest \ + FM_HOME="$ROOT" TMPDIR="$dir/locks" \ + bash -c '. "$0/bin/backends/herdr.sh"; fm_backend_herdr_container_ensure /proj' "$ROOT" > "$dir/out.$i" 2>/dev/null ) & + done + for p in $(jobs -p); do + wait "$p" || fail "a concurrent container_ensure failed" + done + wscount=$(jq -r '[.workspaces[]|select(.label=="firstmate")]|length' "$state") + [ "$wscount" = 1 ] || fail "expected exactly 1 firstmate workspace after 3 concurrent first spawns, got $wscount: $(jq -c '.workspaces' "$state")" + created=$(grep -c $'\x1f''workspace'$'\x1f''create' "$log") + [ "$created" = 1 ] || fail "workspace create should run exactly once across 3 concurrent ensures, ran $created times" + raw1=$(cut -d$'\t' -f1 "$dir/out.1"); raw2=$(cut -d$'\t' -f1 "$dir/out.2"); raw3=$(cut -d$'\t' -f1 "$dir/out.3") + [ -n "$raw1" ] || fail "concurrent ensure 1 echoed no container" + { [ "$raw1" = "$raw2" ] && [ "$raw2" = "$raw3" ]; } \ + || fail "all three concurrent ensures must resolve the same container, got '$raw1' '$raw2' '$raw3'" + pass "fm_backend_herdr_workspace_ensure: 3 concurrent first spawns mint exactly one workspace and share it" +} + +test_workspace_ensure_waits_for_live_lock_holder_then_adopts() { + # Deterministic serialization proof, no timing luck: this test process + # itself holds the lock (a live pid), a background ensure must WAIT (no + # workspace create while held), and after the "winner" workspace appears + # and the lock is released, the waiter must ADOPT it (empty seeded tab id, + # still zero creates). + local dir log state fb key lock bgpid out created + dir="$TMP_ROOT/lock-wait"; mkdir -p "$dir/locks"; log="$dir/log"; state="$dir/state.json"; : > "$log" + fb=$(make_herdr_statefake "$dir") + key=$(printf '%s' "$ROOT" | cksum | cut -d' ' -f1) + lock="$dir/locks/fm-herdr-ws.$key.fmtest.firstmate.lock" + mkdir "$lock" && printf '%s' "$$" > "$lock/pid" + ( PATH="$fb:$PATH" FM_HERDR_LOG="$log" FM_FAKE_HERDR_STATE="$state" HERDR_SESSION=fmtest \ + FM_HOME="$ROOT" TMPDIR="$dir/locks" \ + bash -c '. "$0/bin/backends/herdr.sh"; fm_backend_herdr_container_ensure /proj' "$ROOT" > "$dir/out" 2>/dev/null ) & + bgpid=$! + sleep 1 + created=$(grep -c $'\x1f''workspace'$'\x1f''create' "$log" || true) + [ "$created" = 0 ] || fail "ensure must wait while a live holder owns the lock, but a workspace create ran" + jq '.workspaces += [{workspace_id:"w9",label:"firstmate"}]' "$state" > "$state.tmp" && mv "$state.tmp" "$state" + rm -rf "$lock" + wait "$bgpid" || fail "the waiting ensure failed after the lock was released" + out=$(cat "$dir/out") + case "$out" in + "fmtest:w9"$'\t') : ;; + *) fail "the waiter must adopt the winner's workspace with an empty seeded tab id, got '$out'" ;; + esac + created=$(grep -c $'\x1f''workspace'$'\x1f''create' "$log" || true) + [ "$created" = 0 ] || fail "the adopting waiter must never create, but a workspace create ran" + pass "fm_backend_herdr_workspace_ensure: waits on a live lock holder, then adopts the winner's workspace" +} + +test_workspace_ensure_breaks_provably_dead_lock_holder() { + # A lock dir whose recorded holder pid is provably dead (kill -0 fails) + # must be broken promptly so the spawn proceeds; fail-open never blocks + # dispatch on a crashed prior holder. + local dir log state fb key lock dead raw wscount + dir="$TMP_ROOT/lock-stale"; mkdir -p "$dir/locks"; log="$dir/log"; state="$dir/state.json"; : > "$log" + fb=$(make_herdr_statefake "$dir") + key=$(printf '%s' "$ROOT" | cksum | cut -d' ' -f1) + lock="$dir/locks/fm-herdr-ws.$key.fmtest.firstmate.lock" + sh -c 'echo $$ > "$1"' _ "$dir/deadpid" & + wait $! + dead=$(cat "$dir/deadpid") + mkdir "$lock" && printf '%s' "$dead" > "$lock/pid" + raw=$( PATH="$fb:$PATH" FM_HERDR_LOG="$log" FM_FAKE_HERDR_STATE="$state" HERDR_SESSION=fmtest \ + FM_HOME="$ROOT" TMPDIR="$dir/locks" \ + bash -c '. "$0/bin/backends/herdr.sh"; fm_backend_herdr_container_ensure /proj' "$ROOT" ) \ + || fail "container_ensure failed against a provably dead lock holder" + case "$raw" in fmtest:w*) : ;; *) fail "expected a normal container after breaking the dead holder's lock, got '$raw'" ;; esac + wscount=$(jq -r '[.workspaces[]|select(.label=="firstmate")]|length' "$state") + [ "$wscount" = 1 ] || fail "expected exactly 1 workspace after breaking the dead lock, got $wscount" + [ ! -d "$lock" ] || fail "the lock must be released after ensure completes" + pass "fm_backend_herdr_workspace_ensure: breaks a provably dead holder's lock and proceeds" +} + # --- created-vs-adopted default-tab-prune safety (2026-07-02 self-kill fix) - # # Root cause and fix are documented at @@ -2055,6 +2148,9 @@ test_container_ensure_creates_with_no_focus_flag test_container_ensure_uses_secondmate_home_label test_workspace_ensure_prunes_default_tab test_repeated_cycles_reuse_one_workspace_no_orphans +test_workspace_ensure_concurrent_first_spawns_mint_one_workspace +test_workspace_ensure_waits_for_live_lock_holder_then_adopts +test_workspace_ensure_breaks_provably_dead_lock_holder test_adopted_workspace_never_prunes_default_tab test_label_collision_startup_workspace_leaves_live_tab_alone test_prune_refuses_a_working_agent_pane_defense_in_depth