Skip to content

Commit 050d496

Browse files
AlexMikhalevAlexclaude
authored
feat(orchestrator): startup worktree sweep for SIGKILL residue Refs #1570 (Gitea) (#875)
Layer 2 of the worktree lifecycle defence in depth from epic terraphim/terraphim-ai#1567. Between Layer 1 (WorktreeGuard::Drop on the happy/cancelled path) and Layer 3 (root-privileged ExecStartPre), this sweep reconciles residue left behind when the previous orchestrator process died via SIGKILL, OOM, or a panic-across-runtime where Drop never ran. Changes ------- * scope.rs: - New `pub fn WorktreeManager::sweep_stale(&self, extra_roots: &[PathBuf]) -> SweepReport`. Synchronous on purpose so it can run inline in `AgentOrchestrator::new` before any tick thread is spawned. - Walks `worktree_base` for `WORKTREE_REVIEW_PREFIX`-prefixed children, walks every direct child of each `extra_roots` entry (no prefix filter -- the per-agent root convention has no prefix), tries `git worktree remove --force` and falls back to `std::fs::remove_dir_all`. EACCES/EPERM increments `root_owned_skipped` rather than `failed_count` (Layer 3 territory). Then runs `git worktree prune --verbose` and emits a single structured `info!` line with every report field plus a synthetic `backlog_count = swept_count + root_owned_skipped` for Quickwit alerting. - New `pub struct SweepReport` carrying `swept_count`, `failed_count`, `root_owned_skipped`, `prune_succeeded`, `duration_ms`. - Seven new unit tests covering empty/missing base, review prefix removal, non-review preservation, prune execution, extra-roots no-prefix-filter, and the Linux-only root-owned contract test (runtime gated by `getuid() == 0`; documents the contract even when it cannot run). * compound.rs: - New `pub fn CompoundReviewWorkflow::worktree_manager(&self) -> &WorktreeManager` trivial accessor for the field needed by the Layer 2 wiring. * lib.rs: - `AgentOrchestrator::new` now calls `compound_workflow.worktree_manager().sweep_stale(&[ PathBuf::from("/tmp/adf-worktrees") ])` immediately after the compound workflow is constructed, before `Ok(Self { ... })`. The /tmp/adf-worktrees literal mirrors lib.rs:5393 (the per-agent worktree root from `create_agent_worktree`). - Logs a `warn!` when the residue backlog at startup exceeds 10, which the research doc flagged as the prior-crash-storm threshold. * tests/sweep_on_startup_test.rs (NEW): - Three integration tests around an isolated TempDir git repo via `scope::test_support::setup_git_repo`. Verifies that `AgentOrchestrator::new` does sweep `review-*` residue, preserves non-review siblings, and that extra_roots children are removed regardless of prefix. * Cargo.toml: - Declares the new integration test with `required-features = ["test-helpers"]`, mirroring the Layer 1 `compound_cancellation_test` entry. Deviations from the brief ------------------------- 1. Struct name: the brief says `Orchestrator::new`; the actual symbol is `AgentOrchestrator::new` (lib.rs:661). Insertion site is between `compound_workflow` construction at :687 and the `Ok(Self { ... })` block at :786. 2. `#[cfg(not(test))]` gate on the sweep call. The in-lib `test_config()` (lib.rs:7926) points `repo_path` at the live terraphim-ai checkout. Without the gate, the sweep's `git worktree prune --verbose` ran against that real repo and raced against `test_orchestrator_compound_review_manual`'s concurrent `git worktree add` on the same shared `.git/worktrees/` admin registry. Production wiring is fully exercised by `tests/sweep_on_startup_test.rs` (a separate crate, so `cfg(test)` is not set on the lib's own code path when it links the integration test). The gate is documented inline with a pointer to the integration test. 3. SweepReport is placed at module scope in scope.rs (as the brief preferred), not wrapped in `WorktreeManager`. 4. The Linux/root-only contract test is included as the brief requested. It uses a raw `extern "C" getuid()` FFI to avoid pulling in `nix` or `libc` solely for a single gated test. Verification ------------ * `cargo test -p terraphim_orchestrator --features test-helpers --test sweep_on_startup_test -- --nocapture`: 3 passed. * `cargo test -p terraphim_orchestrator --features test-helpers`: 691 lib tests + all integration tests pass, including Layer 1 `compound_cancellation_test`. * `cargo clippy -p terraphim_orchestrator --lib --tests --features test-helpers -- -D warnings`: clean. * `cargo fmt`: clean. Branched from task/1569-worktree-guard (Layer 1) which provides the `WORKTREE_REVIEW_PREFIX` constant referenced by the sweep. Co-authored-by: Alex <alex@example.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 95e2b15 commit 050d496

5 files changed

Lines changed: 702 additions & 0 deletions

File tree

crates/terraphim_orchestrator/Cargo.toml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,15 @@ name = "compound_cancellation_test"
101101
path = "tests/compound_cancellation_test.rs"
102102
required-features = ["test-helpers"]
103103

104+
# Layer 2 startup sweep integration test (epic #1567 / issue #1570).
105+
# Same test-helpers gate as Layer 1 -- the test re-uses the shared
106+
# `scope::test_support::setup_git_repo` fixture so the sweep never
107+
# runs against the live repository.
108+
[[test]]
109+
name = "sweep_on_startup_test"
110+
path = "tests/sweep_on_startup_test.rs"
111+
required-features = ["test-helpers"]
112+
104113

105114
[[bin]]
106115
name = "adf"

crates/terraphim_orchestrator/src/compound.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,18 @@ impl CompoundReviewWorkflow {
250250
Self::new(swarm_config)
251251
}
252252

253+
/// Borrow the inner [`WorktreeManager`].
254+
///
255+
/// Layer 2 (epic #1567, issue #1570) calls
256+
/// `worktree_manager().sweep_stale(...)` from
257+
/// `AgentOrchestrator::new` to reconcile stale `review-*` residue
258+
/// left behind by SIGKILL / OOM before any tick thread runs.
259+
/// Production code outside the startup sweep should prefer the
260+
/// higher-level workflow methods.
261+
pub fn worktree_manager(&self) -> &WorktreeManager {
262+
&self.worktree_manager
263+
}
264+
253265
/// Run a full compound review cycle.
254266
///
255267
/// 1. Get changed files between git_ref and base_ref

crates/terraphim_orchestrator/src/lib.rs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -693,6 +693,45 @@ impl AgentOrchestrator {
693693
let scheduler = TimeScheduler::new(&config.agents, Some(&config.compound_review.schedule))?;
694694
let compound_workflow =
695695
CompoundReviewWorkflow::from_compound_config(config.compound_review.clone());
696+
697+
// Layer 2 startup sweep (epic #1567, issue #1570).
698+
//
699+
// Reconcile any worktree residue left by a previous instance
700+
// before we accept ticks. Synchronous: must finish before the
701+
// tick thread is spawned in `run()` so a fresh review cycle
702+
// never races against half-killed `review-*` directories.
703+
//
704+
// `extra_roots` mirrors the per-agent worktree convention
705+
// from `lib.rs:5393`. If you change that literal there, change
706+
// it here too.
707+
//
708+
// `cfg(not(test))` gate: the in-lib `test_config()` (see
709+
// `lib.rs:7926`) points `repo_path` at the live terraphim-ai
710+
// checkout. Without this gate, the sweep's
711+
// `git worktree prune --verbose` races against
712+
// `test_orchestrator_compound_review_manual`'s concurrent
713+
// `git worktree add` on that shared real repo's
714+
// `.git/worktrees/` admin registry. The production wiring is
715+
// exercised end-to-end by `tests/sweep_on_startup_test.rs`,
716+
// which builds an isolated `TempDir` repo and asserts the
717+
// sweep DOES run from `AgentOrchestrator::new`. Do not remove
718+
// this gate without first migrating the in-lib `test_config()`
719+
// to a TempDir-based `repo_path`.
720+
#[cfg(not(test))]
721+
{
722+
let sweep_report = compound_workflow
723+
.worktree_manager()
724+
.sweep_stale(&[PathBuf::from("/tmp/adf-worktrees")]);
725+
if sweep_report.swept_count + sweep_report.root_owned_skipped > 10 {
726+
warn!(
727+
swept_count = sweep_report.swept_count,
728+
root_owned_skipped = sweep_report.root_owned_skipped,
729+
failed_count = sweep_report.failed_count,
730+
"large worktree backlog at startup -- prior crash storm likely"
731+
);
732+
}
733+
}
734+
696735
let handoff_buffer = HandoffBuffer::new(config.handoff_buffer_ttl_secs.unwrap_or(86400));
697736
let handoff_ledger = HandoffLedger::new(config.working_dir.join("handoff-ledger.jsonl"));
698737

0 commit comments

Comments
 (0)