diff --git a/docs/reference/engineer-worktree-isolation.md b/docs/reference/engineer-worktree-isolation.md index 423840049..589821f8e 100644 --- a/docs/reference/engineer-worktree-isolation.md +++ b/docs/reference/engineer-worktree-isolation.md @@ -9,6 +9,7 @@ related: - ../howto/spawn-engineers-from-ooda-daemon.md - ./engineer-loop-argv-sanitization.md - ./engineer-worktree-sweep-safety.md + - ./engineer-worktree-presence-guard.md - ../howto/run-ooda-daemon.md --- diff --git a/docs/reference/engineer-worktree-presence-guard.md b/docs/reference/engineer-worktree-presence-guard.md new file mode 100644 index 000000000..64e6b91c0 --- /dev/null +++ b/docs/reference/engineer-worktree-presence-guard.md @@ -0,0 +1,337 @@ +--- +title: Engineer Worktree Presence Guard +description: Reference for the cycle-start worktree presence check (EngineerWorktree::is_present) that closes the TOCTOU between engineer discovery/reuse and the worktree reaper, fixing the missing-workspace goal-session fault (issue #4578). +last_updated: 2026-07-24 +review_schedule: as-needed +owner: simard +doc_type: reference +related: + - ./engineer-worktree-isolation.md + - ./engineer-worktree-sweep-safety.md + - ../howto/spawn-engineers-from-ooda-daemon.md + - ../howto/run-ooda-daemon.md + - ../howto/diagnose-a-deferred-engineer-spawn.md +--- + +# Engineer Worktree Presence Guard + +Before the OODA daemon reuses an existing engineer worktree — or reports an +already-assigned / already-running engineer as done — it re-verifies that the +worktree directory the engineer was allocated (`-`) still exists +on disk. If the worktree has been reaped or removed between the check and the +use, the guard **re-provisions a fresh worktree** instead of returning a stale +success or crashing with a missing-workspace fault. + +This closes a time-of-check/time-of-use (TOCTOU) window between worktree +discovery/reuse and the concurrent worktree GC/reaper (see +[engineer-worktree-sweep-safety](./engineer-worktree-sweep-safety.md)) that +previously aborted goal-session cycles with a bare "missing workspace" +error. + +## Background — the fault this fixes + +`find_live_engineer_for_goal()` +(`src/ooda_actions/advance_goal/spawn.rs`) discovers a running engineer by +scanning `engineer-worktrees/-*` and returning the **first** worktree +whose liveness sentinel points at a still-alive PID. Separately, `advance_goal` +inserts each freshly allocated worktree into `state.engineer_worktrees` +(`typed_goal_session.rs:432`); later cycles reuse those entries via +`subordinate.rs:33` (the heartbeat/artifact path) and `cycle.rs:1613` (the +`worker_present` predicate). A third path short-circuits even earlier: when a +goal is already `assigned_to` an engineer, `advance_goal` returns immediately +(`typed_goal_session.rs:340`) without touching the filesystem at all. + +Both discovery and stored-map reuse historically returned or trusted a worktree +**path** without re-checking that the directory still existed at the moment of +use, and the `already_assigned` path never checked the filesystem. The reaper +subsystem runs concurrently and can remove a worktree between the check and the +use, so a cycle could: + +1. discover / look up a worktree path, +2. have the reaper delete it, +3. proceed to use the now-missing path, +4. abort the whole goal-session cycle with a missing-workspace crash. + +Symptom (issue #4578): goal-session cycles fail end-to-end with a +missing-workspace / no-worktree fault, blocking engineer execution. + +The presence guard moves the existence check to the **last step before use**, +and turns a hard crash into a logged, safe re-provision. + +## API + +### `EngineerWorktree::is_present` + +```rust +impl EngineerWorktree { + /// Returns `true` iff this worktree still holds a readable engineer-claim + /// sentinel (`.simard-engineer-claim`) on disk. + /// + /// Implemented as a single fail-closed claim read + /// ([`read_engineer_claim_full`](../../src/engineer_worktree/claim.rs)): if + /// the worktree directory was reaped, the read errors and the method + /// returns `false`. No separate `lstat` is needed — the claim read is the + /// one syscall that already distinguishes "present" from "gone", so this is + /// the smallest sufficient seam. + /// + /// Scope: this checks **worktree presence**, not engineer liveness. It does + /// not re-probe the claim PID with `kill(pid, 0)` — discovery + /// (`find_live_engineer_for_goal`) owns the liveness check. Reuse sites that + /// need both run discovery first, then presence-check at the point of use. + /// + /// Goal identity is bound by `self` (this `EngineerWorktree` was allocated + /// for one goal, and its directory name is `-`) and upstream + /// by `find_live_engineer_for_goal`'s `-*` glob — **not** by the + /// sentinel contents. The claim sentinel records only `\n` + /// (see [`format_engineer_claim`](../../src/engineer_worktree/claim.rs)); it + /// proves an engineer *claimed* the dir, not which goal owns it. + /// + /// This is the single TOCTOU seam for worktree reuse: call it immediately + /// before using `path()`, with no intervening `.await` / yield that would + /// widen the check-to-use window. + pub fn is_present(&self) -> bool; +} +``` + +| Property | Value | +| --- | --- | +| Cost | one claim-sentinel read (`read_engineer_claim_full`); the read fails when the dir is gone | +| Blocking | non-blocking; no git subprocess, no lock acquired | +| Idempotent | yes — pure read, safe to call repeatedly | +| Checks | worktree presence — a readable `.simard-engineer-claim`; absent/unreadable → `false` | +| Does not check | engineer PID liveness — that is discovery's responsibility | +| Goal binding | via `self` / the `-` dir name — the sentinel carries no goal-id | + +`is_present()` sits next to the existing accessors: + +- `path(&self) -> &Path` — the worktree location on disk. +- `branch(&self) -> &str` — the branch checked out in the worktree. +- `cleanup(&self) -> Result<(), SimardError>` — idempotent removal. + +The discovery path only holds a `PathBuf` (not an `EngineerWorktree`), so it +uses the same `read_engineer_claim_full(&path)` primitive that `is_present()` +wraps, rather than the method. + +### `find_live_engineer_for_goal` (unchanged; callers hardened) + +```rust +pub fn find_live_engineer_for_goal( + state_root: &std::path::Path, + goal_id: &str, +) -> Option; +``` + +Discovery already reads the claim sentinel and re-checks PID liveness +synchronously immediately before returning `Some(path)` (`spawn.rs:822-847`), so +it never returns a path whose sentinel was already gone at scan time. The +residual TOCTOU is **caller-side**: the reaper can delete the directory between +this return and the caller's use of the path. The feature closes that window by +having every reuse/report site presence-check the path at the moment of use (see +the table below). The signature and body of `find_live_engineer_for_goal` are +unchanged. + +## Behaviour at a goal cycle + +The guard is applied at the three sites where `advance_goal` and its per-cycle +consumers reuse or report an existing engineer worktree. In each case absence is +**not** an error — it is a logged, safe fall-through to a fresh allocation. + +| Reuse/report site | Location | On present | On absent (reaped) | +| --- | --- | --- | --- | +| Assigned-engineer short-circuit | `typed_goal_session.rs:340` (`already_assigned`) | keep the existing engineer (no re-spawn) | `tracing::warn!` + clear `goal.assigned_to` + drop the stale map entry + `allocate()` | +| Discovery reuse | `typed_goal_session.rs:362` (`find_live_engineer_for_goal`) | report the existing engineer as `Succeeded` | `tracing::warn!` + fall through to `allocate()` | +| Stored-map reuse | `subordinate.rs:33` (heartbeat/artifact path) and `cycle.rs:1613` (`worker_present`) | reuse the stored worktree | `tracing::warn!` + drop the stale `engineer_worktrees` entry + `allocate()` | + +Re-provisioning always produces a **clean** worktree via +`EngineerWorktree::allocate()` — it never partially reuses the reaped +directory's contents. + +### Structured logging + +Absence is surfaced through structured `tracing` (and OTel spans) only — there +are no `print!`/`println!` calls, per the repo's structured-observability +convention. Representative fields on the warning: + +```text +level=WARN +event="engineer_worktree.reaped_before_reuse" +goal_id= +worktree= +action="reprovision" +``` + +Operators can alert on `engineer_worktree.reaped_before_reuse` to track how +often the reaper races cycle reuse. A low, non-zero rate is expected and +healthy; a spike indicates the reaper is too aggressive relative to cycle +cadence. + +## Guarantees and limits + +- **Fail closed, visibly.** Absence never silently degrades into reusing the + wrong resource. It always becomes a `warn!` plus a clean re-provision. +- **No confused-deputy reuse.** Goal binding is enforced by + `find_live_engineer_for_goal`'s `-*` scan and by the fact that each + `EngineerWorktree` is allocated for exactly one goal — a cycle never adopts a + directory belonging to a different goal. (Note: the claim sentinel itself + carries no goal-id, so it is not the ownership authority — see + [Design decisions](#design-decisions).) +- **Residual window is bounded, not eliminated.** A worktree can still be + removed after the presence check returns `true` but before the engineer starts + writing. That remaining window is the engineer runtime's existing + fail-loud responsibility; the common reaped-before-reuse case is now a clean + re-provision instead of a crash. +- **No double-allocation storms.** Re-provision runs under the state lock: the + `assigned_to` field is cleared and re-set atomically, so only one cycle can win + the re-provision for a given goal and two cycles cannot both observe "absent" + and both allocate. Re-provision is not retried in a tight loop, avoiding a + reap ↔ re-provision livelock. + +## Configuration + +The presence guard has **no new configuration knobs** — it is always on and +additive. It reuses the existing worktree layout and env vars documented in +[engineer-worktree-isolation](./engineer-worktree-isolation.md): + +| Setting | Effect on the guard | +| --- | --- | +| `SIMARD_STATE_ROOT` | root under which `engineer-worktrees/-*` (and the checked claim sentinel) live | +| worktree reaper / sweep cadence | how often a worktree can disappear between cycles; see [sweep-safety](./engineer-worktree-sweep-safety.md) | + +## Examples + +### Re-provisioning an already-assigned engineer whose worktree was reaped + +```rust +// Assigned-engineer short-circuit (replaces the bare permanent error at +// typed_goal_session.rs:340). +if already_assigned { + let present = guard + .engineer_worktrees + .get(goal_id) + .map(|w| w.is_present()) + .unwrap_or(false); + if present { + // Genuinely running engineer — nothing to re-provision this cycle. + return Ok(/* existing-engineer outcome */); + } + // Assigned but reaped: clear the stale assignment and fall through. + tracing::warn!( + event = "engineer_worktree.reaped_before_reuse", + goal_id, + action = "reprovision", + ); + goal.assigned_to = None; + guard.engineer_worktrees.remove(goal_id); + // ...fall through to allocate() a clean replacement. +} +``` + +### Reusing a discovered engineer worktree safely + +```rust +// Discovery reuse path — we only hold a PathBuf here, so use the same +// claim-read primitive that is_present() wraps. +if let Some(path) = find_live_engineer_for_goal(&state_root, goal_id) { + if read_engineer_claim_full(&path).is_some() { + // Directory still present: report the existing engineer. + return Ok(EffectResult::Succeeded { /* existing engineer evidence */ }); + } + // Reaped between discovery and reuse: warn and fall through to allocate. + tracing::warn!( + event = "engineer_worktree.reaped_before_reuse", + goal_id, + worktree = %path.display(), + action = "reprovision", + ); +} +let worktree = + EngineerWorktree::allocate(&parent_repo, &state_root, goal_id)?; +``` + +### Guarding a stored worktree before reuse + +```rust +// Stored-map path (subordinate.rs / cycle.rs consumers). +if let Some(worktree) = guard.engineer_worktrees.get(goal_id) { + if worktree.is_present() { + // Safe: the worktree directory still holds a claim sentinel. + reuse(worktree); + } else { + tracing::warn!( + event = "engineer_worktree.reaped_before_reuse", + goal_id, + worktree = %worktree.path().display(), + action = "reprovision", + ); + guard.engineer_worktrees.remove(goal_id); // drop the stale entry + // ...then allocate() a clean replacement. + } +} +``` + +## Verifying the guard + +```bash +# Unit test: is_present() is true after allocate, false after cleanup. +cargo test engineer_worktree:: + +# Regression tests: reuse-after-reap and stored-map staleness re-provision +# cleanly instead of crashing with a missing-workspace fault. +cargo test ooda_actions::advance_goal +``` + +Expected results: + +- `is_present()` returns `true` immediately after `allocate()` and `false` + after `cleanup()` (or any external removal of the directory). +- A cycle whose worktree is reaped between discovery and reuse emits + `engineer_worktree.reaped_before_reuse` and completes by allocating a fresh + worktree — no missing-workspace error, issue #4578 no longer reproduces. + +## Design decisions + +These record why the guard is shaped the way it is. They are settled decisions +that the implementation follows — not open questions — and each is grounded in +the current code. + +1. **Presence-check the `already_assigned` short-circuit; don't keep the bare + error.** `advance_goal` (`typed_goal_session.rs:340`) currently returns a + **permanent** error (`"goal already has an assigned engineer"`) *before* + `find_live_engineer_for_goal` or any presence check runs. A goal whose + `assigned_to` is still set but whose worktree was reaped therefore fails + permanently instead of re-provisioning — the exact reaped-before-reuse case + this guard targets. The feature replaces that short-circuit with a presence + check: present → keep the engineer; absent → `warn!`, clear `assigned_to`, + drop the stale map entry, and re-provision. + +2. **Guard the real stored-map consumers, not the insert site.** In + `advance_goal` the `state.engineer_worktrees` map is **insert-only** + (`typed_goal_session.rs:432`). The actual per-cycle consumers of a stored + worktree are `advance_goal_with_subordinate` (`subordinate.rs:33`, which reads + `engineer_worktrees.get(goal_id)` for the heartbeat/artifact path and today + falls back to `"."`) and `cycle.rs:1613` (`worker_present = + engineer_worktrees.contains_key(goal_id)`). The stored-map presence check is + therefore sited there, not in the `advance_goal` insert branch. Note that + `cycle.rs:1613` currently only tests map-key membership (`contains_key`); the + guard upgrades that to an on-disk presence check so a reaped worktree no + longer counts as a live worker. + +3. **Do not extend the sentinel format.** The claim sentinel stays + `\n` (`claim.rs:37-45`). Goal ownership is already enforced by + the `-` directory name and the `-*` discovery glob; + adding a goal-id to the sentinel would duplicate that binding for no gain. + `is_present()` therefore verifies presence, not goal ownership. + +4. **`is_present()` is a single claim read, not lstat + read.** + `read_engineer_claim_full` already fails closed when the directory is gone + (its `read_to_string` errors → `None`), so an explicit extra `lstat` before + the claim read buys nothing. The single claim read is the smallest sufficient + seam (ruthless simplicity). + +## See also + +- [Per-Engineer Worktree Isolation](./engineer-worktree-isolation.md) — the + allocator, filesystem layout, and claim sentinel this guard builds on. +- [Engineer Worktree Sweep Safety](./engineer-worktree-sweep-safety.md) — the + reaper that races reuse and the safety guards it already honors. +- [Diagnose a deferred engineer spawn](../howto/diagnose-a-deferred-engineer-spawn.md). diff --git a/src/engineer_worktree/mod.rs b/src/engineer_worktree/mod.rs index f138fd304..04ff7b1a8 100644 --- a/src/engineer_worktree/mod.rs +++ b/src/engineer_worktree/mod.rs @@ -47,6 +47,8 @@ mod tests_extra; #[cfg(test)] mod tests_more; #[cfg(test)] +mod tests_presence_guard; +#[cfg(test)] mod tests_reaping_safety; /// Subdirectory under the supervisor state root that holds all engineer worktrees. @@ -70,8 +72,8 @@ pub const ENGINEER_CLAIM_FILE: &str = ".simard-engineer-claim"; mod claim; mod discovery; mod precommit; -use claim::{claim_is_live, format_engineer_claim, read_engineer_claim_full}; -pub use claim::{is_pid_alive_public, read_pid_starttime_public}; +use claim::{claim_is_live, format_engineer_claim}; +pub use claim::{is_pid_alive_public, read_engineer_claim_full, read_pid_starttime_public}; pub(crate) use discovery::goal_id_from_worktree_dir; pub use discovery::{ LiveEngineerWorktree, live_claimed_engineers, live_claimed_engineers_in_worktrees, @@ -448,6 +450,25 @@ impl EngineerWorktree { &self.branch } + /// Returns `true` iff this worktree still holds a readable engineer-claim + /// sentinel (`.simard-engineer-claim`) on disk. + /// + /// Side-effect-free, fail-closed presence probe that closes the worktree + /// reuse TOCTOU (issue #4578). Implemented as a single claim read via + /// [`read_engineer_claim_full`]: if the worktree directory was reaped out + /// of band, or the sentinel was removed/corrupted, the read errors and + /// this returns `false`. Reuse sites call it immediately before they + /// depend on `path()` still being on disk. + /// + /// Scope: worktree presence, not engineer PID liveness — discovery + /// (`find_live_engineer_for_goal`) owns the liveness check. The single + /// claim read is the smallest sufficient seam: `read_engineer_claim_full` + /// already fails closed when the directory is gone, so a separate `lstat` + /// buys nothing. + pub fn is_present(&self) -> bool { + read_engineer_claim_full(&self.path).is_some() + } + /// Remove the worktree, prune its registration, delete its branch. /// /// Idempotent — second and subsequent calls are `Ok(())` no-ops. diff --git a/src/engineer_worktree/tests_presence_guard.rs b/src/engineer_worktree/tests_presence_guard.rs new file mode 100644 index 000000000..be7d754c2 --- /dev/null +++ b/src/engineer_worktree/tests_presence_guard.rs @@ -0,0 +1,184 @@ +//! Red-phase TDD tests for the engineer-worktree **presence guard** +//! (issue #4578). +//! +//! Context: goal-session cycles crashed with a bare missing-workspace fault +//! because discovery / stored-map reuse handed back a worktree path that a +//! concurrent GC/reaper had already removed (a TOCTOU between "found a live +//! claim" and "use the checkout dir"). The fix introduces a single, +//! side-effect-free presence seam owned by the worktree module — +//! [`EngineerWorktree::is_present`] — that every reuse site consults +//! immediately before it depends on the checkout still being on disk. +//! +//! These tests specify the contract for that new accessor. They MUST fail in +//! the red phase (the method does not exist yet ⇒ the crate will not compile) +//! and MUST pass once `is_present()` lands, without further test edits. +//! +//! Contract (aligned with `docs/reference/engineer-worktree-presence-guard.md`): +//! `is_present()` performs a single **fail-closed** claim read of +//! `self.path()/.simard-engineer-claim`. It returns `true` iff the checkout +//! directory still exists AND its claim sentinel is readable; any absence +//! (dir reaped, sentinel gone, unreadable) returns `false`. It never mutates +//! the filesystem and never re-provisions. + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use tempfile::tempdir; + +use super::{ENGINEER_CLAIM_FILE, EngineerWorktree}; + +// --------------------------------------------------------------------------- +// Fixtures (self-contained: the sibling `tests` module's helpers are private). +// --------------------------------------------------------------------------- + +fn git_cmd(repo: &Path, args: &[&str]) -> Command { + let mut cmd = Command::new("git"); + cmd.args(args).current_dir(repo).env_clear(); + if let Ok(p) = std::env::var("PATH") { + cmd.env("PATH", p); + } + if let Ok(h) = std::env::var("HOME") { + cmd.env("HOME", h); + } + cmd +} + +fn run_git(repo: &Path, args: &[&str]) { + let out = git_cmd(repo, args).output().expect("spawn git"); + assert!( + out.status.success(), + "git {:?} failed in {}: {}", + args, + repo.display(), + String::from_utf8_lossy(&out.stderr) + ); +} + +/// A parent repo with a committed `main` so `EngineerWorktree::allocate` +/// (which branches off `main` HEAD) succeeds. +fn init_parent_repo(dir: &Path) -> PathBuf { + fs::create_dir_all(dir).expect("create parent repo dir"); + run_git(dir, &["init", "--initial-branch=main", "--quiet"]); + run_git(dir, &["config", "user.email", "test@example.com"]); + run_git(dir, &["config", "user.name", "test"]); + run_git(dir, &["config", "commit.gpgsign", "false"]); + fs::write(dir.join("README.md"), "seed\n").expect("seed file"); + run_git(dir, &["add", "README.md"]); + run_git(dir, &["commit", "-m", "seed", "--quiet"]); + dir.to_path_buf() +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +/// A freshly allocated worktree is present: `allocate()` creates the checkout +/// dir and writes the claim sentinel with the current PID, so the guard must +/// report `true`. +#[test] +fn is_present_true_after_allocate() { + let parent = tempdir().expect("tempdir"); + let state = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-present") + .expect("allocate engineer worktree"); + + assert!( + wt.path().is_dir(), + "precondition: freshly allocated worktree must exist on disk" + ); + assert!( + wt.is_present(), + "a freshly allocated worktree with its claim sentinel must be present" + ); +} + +/// After the worktree's own `cleanup()` removes the checkout, the guard must +/// report `false` — the core signal a reuse site needs to re-provision instead +/// of returning a stale success. +#[test] +fn is_present_false_after_cleanup() { + let parent = tempdir().expect("tempdir"); + let state = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-cleaned") + .expect("allocate engineer worktree"); + assert!(wt.is_present(), "precondition: present before cleanup"); + + wt.cleanup().expect("cleanup engineer worktree"); + + assert!( + !wt.is_present(), + "after cleanup removes the checkout dir, is_present() must be false" + ); +} + +/// The #4578 fault exactly: a concurrent reaper removes the checkout dir out +/// of band (no `cleanup()` call on this handle). The guard must still detect +/// the absence so callers never hand back / reuse a reaped worktree. +#[test] +fn is_present_false_when_dir_reaped_out_of_band() { + let parent = tempdir().expect("tempdir"); + let state = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-reaped") + .expect("allocate engineer worktree"); + assert!(wt.is_present(), "precondition: present before reap"); + + // Simulate the GC/reaper removing the worktree dir underneath us, WITHOUT + // going through this handle's cleanup(). This is the TOCTOU window. + fs::remove_dir_all(wt.path()).expect("simulate out-of-band reap"); + + assert!( + !wt.is_present(), + "a worktree whose dir was reaped out of band must report not-present" + ); +} + +/// Fail-closed: if the checkout dir survives but the claim sentinel is gone +/// (a partially reaped / corrupted worktree), the guard must return `false` +/// rather than optimistically treating the checkout as reusable. +#[test] +fn is_present_false_when_claim_sentinel_removed() { + let parent = tempdir().expect("tempdir"); + let state = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-noclaim") + .expect("allocate engineer worktree"); + assert!(wt.is_present(), "precondition: present with claim"); + + fs::remove_file(wt.path().join(ENGINEER_CLAIM_FILE)).expect("remove claim sentinel"); + assert!( + wt.path().is_dir(), + "precondition: only the sentinel is gone, the dir remains" + ); + + assert!( + !wt.is_present(), + "missing claim sentinel must fail closed to not-present" + ); +} + +/// The guard is a pure observation: calling it must not create, delete, or +/// otherwise mutate the worktree. Repeated calls are stable. +#[test] +fn is_present_is_side_effect_free_and_idempotent() { + let parent = tempdir().expect("tempdir"); + let state = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let wt = EngineerWorktree::allocate(parent.path(), state.path(), "goal-pure") + .expect("allocate engineer worktree"); + + assert!(wt.is_present()); + assert!(wt.is_present()); + assert!( + wt.path().is_dir() && wt.path().join(ENGINEER_CLAIM_FILE).exists(), + "is_present() must not have mutated the worktree or its sentinel" + ); +} diff --git a/src/ooda_actions/advance_goal/mod.rs b/src/ooda_actions/advance_goal/mod.rs index 8b0e2c2d7..40d7d3fe9 100644 --- a/src/ooda_actions/advance_goal/mod.rs +++ b/src/ooda_actions/advance_goal/mod.rs @@ -29,6 +29,8 @@ mod overlap; // so `spawn::dispatch_spawn_engineer` can invoke the gate. pub(crate) mod resource_admission; mod subordinate; +#[cfg(test)] +mod tests_presence_guard; #[cfg(not(test))] pub(crate) mod typed_goal_session; #[cfg(test)] diff --git a/src/ooda_actions/advance_goal/subordinate.rs b/src/ooda_actions/advance_goal/subordinate.rs index b7e185f49..2858d2510 100644 --- a/src/ooda_actions/advance_goal/subordinate.rs +++ b/src/ooda_actions/advance_goal/subordinate.rs @@ -29,11 +29,29 @@ pub fn advance_goal_with_subordinate( // available so artifact validation looks at the engineer's own scope, // not the parent checkout. Falls back to "." for legacy/manual paths // that pre-date worktree isolation. - let worktree_path = state - .engineer_worktrees - .get(goal_id) - .map(|w| w.path().to_path_buf()) - .unwrap_or_else(|| std::path::PathBuf::from(".")); + // + // Presence guard (issue #4578): a stored worktree can be reaped out of + // band between cycles. Consult is_present() before trusting the stored + // path; when the checkout is gone, warn, drop the stale entry (the next + // spawn cycle re-provisions a clean worktree) and fall back to the parent + // scope instead of dereferencing a missing path. + let worktree_path = match state.engineer_worktrees.get(goal_id) { + Some(worktree) if worktree.is_present() => worktree.path().to_path_buf(), + Some(worktree) => { + let stale_path = worktree.path().to_path_buf(); + tracing::warn!( + target: "simard::engineer_worktree", + event = "engineer_worktree.reaped_before_reuse", + goal_id, + worktree = %stale_path.display(), + action = "reprovision", + "stored engineer worktree reaped before reuse; dropping stale entry", + ); + state.engineer_worktrees.remove(goal_id); + std::path::PathBuf::from(".") + } + None => std::path::PathBuf::from("."), + }; let handle = crate::agent_supervisor::SubordinateHandle { pid: 0, agent_name: sub_name.to_string(), diff --git a/src/ooda_actions/advance_goal/tests_presence_guard.rs b/src/ooda_actions/advance_goal/tests_presence_guard.rs new file mode 100644 index 000000000..608a6eb3e --- /dev/null +++ b/src/ooda_actions/advance_goal/tests_presence_guard.rs @@ -0,0 +1,229 @@ +//! Red-phase TDD regression tests for the #4578 worktree presence guard at the +//! `advance_goal` reuse sites. +//! +//! Two reuse paths could hand back / reuse a reaped worktree and crash the +//! cycle with a missing-workspace fault: +//! +//! 1. **Discovery reuse** — `find_live_engineer_for_goal` scans +//! `engineer-worktrees/-*` for a live claim and returns its path. +//! The `typed_goal_session` reuse path returns `Succeeded` from that path +//! without re-verifying the dir still exists at the moment of use. +//! +//! 2. **Stored-map reuse** — consumers (`subordinate.rs`, `ooda_loop::cycle`) +//! read a worktree back out of `state.engineer_worktrees` and depend on +//! its `path()` being on disk. +//! +//! `typed_goal_session` is `#[cfg(not(test))]`, so these tests pin the two +//! **observable, always-compiled** contracts the fix relies on: +//! +//! * `find_live_engineer_for_goal` never returns a path for a worktree whose +//! dir has been reaped (the discovery-reuse regression guard). +//! * A worktree stored in `state.engineer_worktrees` can be detected as stale +//! via the new [`EngineerWorktree::is_present`] seam after an out-of-band +//! reap (the stored-map-staleness guard the consumers will call). +//! +//! The `is_present()`-based assertions MUST fail in the red phase (method does +//! not exist yet ⇒ crate will not compile) and MUST pass once the guard lands. + +#![cfg(test)] + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use tempfile::tempdir; + +use super::find_live_engineer_for_goal; +use crate::engineer_worktree::{ENGINEER_CLAIM_FILE, EngineerWorktree, WORKTREES_SUBDIR}; +use crate::goal_curation::GoalBoard; +use crate::ooda_loop::OodaState; + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +fn git_cmd(repo: &Path, args: &[&str]) -> Command { + let mut cmd = Command::new("git"); + cmd.args(args).current_dir(repo).env_clear(); + if let Ok(p) = std::env::var("PATH") { + cmd.env("PATH", p); + } + if let Ok(h) = std::env::var("HOME") { + cmd.env("HOME", h); + } + cmd +} + +fn run_git(repo: &Path, args: &[&str]) { + let out = git_cmd(repo, args).output().expect("spawn git"); + assert!( + out.status.success(), + "git {:?} failed in {}: {}", + args, + repo.display(), + String::from_utf8_lossy(&out.stderr) + ); +} + +fn init_parent_repo(dir: &Path) { + std::fs::create_dir_all(dir).expect("create parent repo dir"); + run_git(dir, &["init", "--initial-branch=main", "--quiet"]); + run_git(dir, &["config", "user.email", "test@example.com"]); + run_git(dir, &["config", "user.name", "test"]); + run_git(dir, &["config", "commit.gpgsign", "false"]); + std::fs::write(dir.join("README.md"), "seed\n").expect("seed file"); + run_git(dir, &["add", "README.md"]); + run_git(dir, &["commit", "-m", "seed", "--quiet"]); +} + +/// Allocate a real per-engineer worktree under `state_root` and register it in +/// `state.engineer_worktrees` keyed by `goal_id`. `allocate()` writes the claim +/// sentinel with the current process PID, so discovery treats it as live. +/// Returns the on-disk worktree path for reap simulation. +fn attach_engineer( + state: &mut OodaState, + parent_repo: &Path, + state_root: &Path, + goal_id: &str, +) -> PathBuf { + let wt = EngineerWorktree::allocate(parent_repo, state_root, goal_id) + .expect("allocate engineer worktree"); + let path = wt.path().to_path_buf(); + assert!( + path.is_dir(), + "freshly allocated worktree must exist on disk" + ); + state.engineer_worktrees.insert(goal_id.to_string(), wt); + path +} + +// --------------------------------------------------------------------------- +// Discovery-reuse regression: never return a reaped path +// --------------------------------------------------------------------------- + +/// Control: while the worktree exists and carries a live claim (this process's +/// PID), discovery finds it. This is the happy-path reuse the guard must not +/// break. +#[test] +fn discovery_finds_live_worktree_while_present() { + let parent = tempdir().expect("tempdir"); + let state_dir = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let mut state = OodaState::new(GoalBoard::new()); + let goal_id = "disc-live-goal"; + let wt_path = attach_engineer(&mut state, parent.path(), state_dir.path(), goal_id); + + let found = find_live_engineer_for_goal(state_dir.path(), goal_id); + assert_eq!( + found.as_deref(), + Some(wt_path.as_path()), + "discovery must return the live worktree path while it is present" + ); +} + +/// Reuse-after-reap: once the checkout dir is removed out of band, discovery +/// must NOT hand back a path (the reuse site would otherwise return a stale +/// `Succeeded` and the next cycle crashes with a missing-workspace fault). +#[test] +fn discovery_returns_none_after_worktree_reaped() { + let parent = tempdir().expect("tempdir"); + let state_dir = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let mut state = OodaState::new(GoalBoard::new()); + let goal_id = "disc-reaped-goal"; + let wt_path = attach_engineer(&mut state, parent.path(), state_dir.path(), goal_id); + + // Simulate a concurrent GC/reaper deleting the worktree dir (the claim + // sentinel goes with it). + std::fs::remove_dir_all(&wt_path).expect("simulate out-of-band reap"); + + let found = find_live_engineer_for_goal(state_dir.path(), goal_id); + assert!( + found.is_none(), + "discovery must not return a reaped worktree path, got {found:?}" + ); +} + +// --------------------------------------------------------------------------- +// Stored-map-staleness guard: consumers can detect a reaped stored worktree +// --------------------------------------------------------------------------- + +/// A worktree freshly stored in `state.engineer_worktrees` reports present, so +/// live consumers keep reusing it. +#[test] +fn stored_map_worktree_reports_present_while_live() { + let parent = tempdir().expect("tempdir"); + let state_dir = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let mut state = OodaState::new(GoalBoard::new()); + let goal_id = "stored-live-goal"; + attach_engineer(&mut state, parent.path(), state_dir.path(), goal_id); + + let stored = state + .engineer_worktrees + .get(goal_id) + .expect("worktree must be tracked in the stored map"); + assert!( + stored.is_present(), + "a live stored worktree must report present so consumers reuse it" + ); +} + +/// The stored-map-staleness contract: after the checkout dir is reaped out of +/// band, the worktree still sitting in `state.engineer_worktrees` must report +/// NOT present. This is the signal the stored-map consumers use to drop the +/// stale entry and re-provision instead of dereferencing a missing path. +#[test] +fn stored_map_worktree_reports_absent_after_reap() { + let parent = tempdir().expect("tempdir"); + let state_dir = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let mut state = OodaState::new(GoalBoard::new()); + let goal_id = "stored-reaped-goal"; + let wt_path = attach_engineer(&mut state, parent.path(), state_dir.path(), goal_id); + + // Reaper removes the checkout out of band; the map entry is now stale. + std::fs::remove_dir_all(&wt_path).expect("simulate out-of-band reap"); + assert!( + !wt_path.exists(), + "precondition: the stored worktree's dir has been reaped" + ); + + let stored = state + .engineer_worktrees + .get(goal_id) + .expect("stale entry is still in the map until a consumer drops it"); + assert!( + !stored.is_present(), + "a reaped stored worktree must report not-present so consumers re-provision" + ); +} + +/// Sanity anchor for the fixtures: the reaped dir really lived under the +/// managed `engineer-worktrees/-*` root, so the guards above exercise the +/// production layout rather than an arbitrary temp path. +#[test] +fn attached_worktree_lives_under_managed_root() { + let parent = tempdir().expect("tempdir"); + let state_dir = tempdir().expect("tempdir"); + init_parent_repo(parent.path()); + + let mut state = OodaState::new(GoalBoard::new()); + let goal_id = "layout-goal"; + let wt_path = attach_engineer(&mut state, parent.path(), state_dir.path(), goal_id); + + let managed_root = state_dir.path().join(WORKTREES_SUBDIR); + assert!( + wt_path.starts_with(&managed_root), + "worktree {} must live under managed root {}", + wt_path.display(), + managed_root.display() + ); + assert!( + wt_path.join(ENGINEER_CLAIM_FILE).exists(), + "allocate() must have written the claim sentinel used by discovery" + ); +} diff --git a/src/ooda_actions/advance_goal/typed_goal_session.rs b/src/ooda_actions/advance_goal/typed_goal_session.rs index 5adaa6635..724017b11 100644 --- a/src/ooda_actions/advance_goal/typed_goal_session.rs +++ b/src/ooda_actions/advance_goal/typed_goal_session.rs @@ -16,6 +16,7 @@ use super::repo_resolver; use super::spawn::{ derive_session_id, find_live_engineer_for_goal, lock_state, typed_ooda_state_root, }; +use crate::engineer_worktree::read_engineer_claim_full; use crate::ooda_actions::make_outcome; pub(crate) fn run( @@ -327,21 +328,69 @@ impl LiveGoalSessionEffects<'_, '_> { )); } self.require_goal_repository(goal_id, &spawn.repository)?; - let (goal_repo, already_assigned) = { - let guard = lock_state(self.state); - let goal = guard - .active_goals - .active - .iter() - .find(|goal| goal.id == goal_id) - .ok_or_else(|| EffectExecutionError::permanent("goal disappeared before spawn"))?; - (goal.repo.clone(), goal.assigned_to.is_some()) + let goal_repo = { + let mut guard = lock_state(self.state); + let (goal_repo, already_assigned) = { + let goal = guard + .active_goals + .active + .iter() + .find(|goal| goal.id == goal_id) + .ok_or_else(|| { + EffectExecutionError::permanent("goal disappeared before spawn") + })?; + (goal.repo.clone(), goal.assigned_to.is_some()) + }; + if already_assigned { + // Presence guard (issue #4578): an assigned goal's worktree can + // be reaped out of band by the concurrent GC/reaper. Only keep + // the existing engineer when its checkout is still on disk; + // otherwise clear the stale assignment + map entry and fall + // through to a clean re-provision instead of failing + // permanently with a missing-workspace fault. + // Derive the evidence session_id from the worktree checkout + // basename so both reuse branches (assigned short-circuit here + // and disk discovery below) report the engineer with the same + // identifier shape and `existing-engineer` fallback. + let existing_session_id = guard + .engineer_worktrees + .get(goal_id) + .filter(|worktree| worktree.is_present()) + .map(|worktree| { + worktree + .path() + .file_name() + .and_then(|value| value.to_str()) + .unwrap_or("existing-engineer") + .to_string() + }); + if let Some(session_id) = existing_session_id { + return Ok(EffectResult::Succeeded { + evidence: vec![EvidenceRef::EngineerRun { + session_id, + claim_key: spawn.claim_key.clone(), + }], + }); + } + tracing::warn!( + target: "simard::engineer_worktree", + event = "engineer_worktree.reaped_before_reuse", + goal_id, + action = "reprovision", + "assigned engineer worktree reaped before reuse; re-provisioning", + ); + if let Some(goal) = guard + .active_goals + .active + .iter_mut() + .find(|goal| goal.id == goal_id) + { + goal.assigned_to = None; + } + guard.engineer_worktrees.remove(goal_id); + } + goal_repo }; - if already_assigned { - return Err(EffectExecutionError::permanent( - "goal already has an assigned engineer", - )); - } // The spawn repository is already admitted against the normalized goal // repository by `require_goal_repository` above (which routes through // the single `goal_repository` -> `RepositoryRef::from_goal_slug` @@ -360,17 +409,33 @@ impl LiveGoalSessionEffects<'_, '_> { } let state_root = typed_ooda_state_root(); if let Some(path) = find_live_engineer_for_goal(&state_root, goal_id) { - let session_id = path - .file_name() - .and_then(|value| value.to_str()) - .unwrap_or("existing-engineer") - .to_string(); - return Ok(EffectResult::Succeeded { - evidence: vec![EvidenceRef::EngineerRun { - session_id, - claim_key: spawn.claim_key.clone(), - }], - }); + // Presence guard (issue #4578): discovery holds only a PathBuf, so + // re-read the claim sentinel with the same primitive is_present() + // wraps immediately before reporting the existing engineer. If the + // reaper removed the checkout between discovery and reuse, warn and + // fall through to allocate() a clean replacement rather than + // returning a stale Succeeded that crashes the next cycle. + if read_engineer_claim_full(&path).is_some() { + let session_id = path + .file_name() + .and_then(|value| value.to_str()) + .unwrap_or("existing-engineer") + .to_string(); + return Ok(EffectResult::Succeeded { + evidence: vec![EvidenceRef::EngineerRun { + session_id, + claim_key: spawn.claim_key.clone(), + }], + }); + } + tracing::warn!( + target: "simard::engineer_worktree", + event = "engineer_worktree.reaped_before_reuse", + goal_id, + worktree = %path.display(), + action = "reprovision", + "discovered engineer worktree reaped before reuse; re-provisioning", + ); } let parent_repo = repo_resolver::resolve_goal_repo(goal_repo.as_deref()).map_err(|error| { diff --git a/src/ooda_loop/cycle.rs b/src/ooda_loop/cycle.rs index fca6cf5f4..3258fd304 100644 --- a/src/ooda_loop/cycle.rs +++ b/src/ooda_loop/cycle.rs @@ -1610,7 +1610,15 @@ pub(crate) fn gather_per_goal_cycle_ctx( .filter(|w| w.kind.trim().eq_ignore_ascii_case("pr")) .map(|w| w.ref_id.clone()) .collect(); - let worker_present = state.engineer_worktrees.contains_key(goal_id); + // Presence guard (issue #4578): map membership alone can report a phantom + // live worker after the reaper removes a worktree out of band. Require the + // checkout to still be present on disk so a reaped worktree no longer + // counts as a live worker. + let worker_present = state + .engineer_worktrees + .get(goal_id) + .map(|worktree| worktree.is_present()) + .unwrap_or(false); // DEMOTED decider #1 — classify_standing_idle: a standing goal that looks // idle (no live in-flight ref) becomes a SIGNAL, not a roll.