diff --git a/crates/flare-git-core/src/doctor.rs b/crates/flare-git-core/src/doctor.rs index 85ccaccc..cf23ab50 100644 --- a/crates/flare-git-core/src/doctor.rs +++ b/crates/flare-git-core/src/doctor.rs @@ -390,8 +390,12 @@ pub fn reclaim_scoped( } let path = Path::new(&lane.path); if path.exists() { - if let Err(e) = crate::snapshot::snapshot_before( + // Snapshot the lane's OWN contents: `snapshot_before(repo_root)` + // stages from the main checkout, where `.worktrees/` is excluded, + // so it captured nothing of the directory about to be deleted. + if let Err(e) = crate::snapshot::snapshot_worktree_before( repo_root, + path, &format!("doctor reclaim {}", lane.name), ) { eprintln!( diff --git a/crates/flare-git-core/src/snapshot.rs b/crates/flare-git-core/src/snapshot.rs index 23b26250..b0ce1d31 100644 --- a/crates/flare-git-core/src/snapshot.rs +++ b/crates/flare-git-core/src/snapshot.rs @@ -10,11 +10,26 @@ use std::path::Path; use std::process::Command; +use std::sync::atomic::{AtomicU64, Ordering}; use crate::shell::run_in; const SNAPSHOT_REF_PREFIX: &str = "refs/agentflare/snapshots/"; +/// Process-local counter mixed into temporary index filenames so two +/// snapshot calls racing inside the same process (distinct threads, same +/// PID) never share a `GIT_INDEX_FILE` -- `std::process::id()` alone only +/// guarantees uniqueness across processes. +static SNAPSHOT_CALL_ID: AtomicU64 = AtomicU64::new(0); + +fn unique_index_suffix() -> String { + format!( + "{}-{}", + std::process::id(), + SNAPSHOT_CALL_ID.fetch_add(1, Ordering::Relaxed) + ) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct SnapshotId(pub String); @@ -27,11 +42,14 @@ pub struct SnapshotMeta { /// `git ` with a temporary `GIT_INDEX_FILE`, so staging for a /// snapshot never touches the caller's real index. -fn run_git_with_index( - repo_root: &Path, - index_file: &Path, - args: &[&str], -) -> Result { +/// +/// `cwd` is the directory git runs in. It is the repo root for a plain +/// repo-root snapshot, but `snapshot_worktree_before` points it at the +/// worktree being captured instead — `git add -A` resolves pathspecs +/// against the current directory, so running it from the repo root with +/// only `--work-tree` set makes git refuse the paths as outside the +/// repository. +fn run_git_with_index(cwd: &Path, index_file: &Path, args: &[&str]) -> Result { // A snapshot must capture exactly what's on disk right now -- `-c // core.autocrlf=false` stops git silently converting line endings // while staging, regardless of the caller's ambient/global git config @@ -39,7 +57,7 @@ fn run_git_with_index( let mut cmd = Command::new(crate::shell::git_binary()); cmd.args(["-c", "core.autocrlf=false"]) .args(args) - .current_dir(repo_root) + .current_dir(cwd) .env("GIT_INDEX_FILE", index_file); crate::shell::apply_filtered_path(&mut cmd); let out = cmd @@ -56,9 +74,10 @@ fn run_git_with_index( /// in a temporary index file, removed afterward regardless of outcome — /// the real index and working tree are never touched. pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result { - let tmp_index = repo_root - .join(".git") - .join(format!("agentflare-snapshot-index-{}", std::process::id())); + let tmp_index = repo_root.join(".git").join(format!( + "agentflare-snapshot-index-{}", + unique_index_suffix() + )); let result = (|| { run_git_with_index(repo_root, &tmp_index, &["add", "-A"])?; let tree = run_git_with_index(repo_root, &tmp_index, &["write-tree"])?; @@ -83,6 +102,72 @@ pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result Result { + if !worktree_path.exists() { + return Err(format!( + "worktree path does not exist: {}", + worktree_path.display() + )); + } + let common_dir = resolve_common_dir(repo_root); + let tmp_index = common_dir.join(format!( + "agentflare-worktree-snapshot-index-{}", + unique_index_suffix() + )); + let git_dir = common_dir.to_string_lossy().to_string(); + let result = (|| { + let stage_args = ["--git-dir", &git_dir, "add", "-A", "."]; + run_git_with_index(worktree_path, &tmp_index, &stage_args)?; + let tree = run_git_with_index( + worktree_path, + &tmp_index, + &["--git-dir", &git_dir, "write-tree"], + )?; + let sha = run_in(repo_root, &["commit-tree", &tree, "-m", reason])?; + let refname = format!("{SNAPSHOT_REF_PREFIX}{sha}"); + run_in(repo_root, &["update-ref", &refname, &sha])?; + Ok(SnapshotId(sha)) + })(); + let _ = std::fs::remove_file(&tmp_index); + result +} + +/// Absolute path to the repo's common git dir — the shared `.git` of the +/// main checkout, which is where linked worktrees' objects and admin +/// entries actually live. Falls back to `/.git` when git can't +/// answer, matching this module's best-effort style. +fn resolve_common_dir(repo_root: &Path) -> std::path::PathBuf { + match run_in( + repo_root, + &["rev-parse", "--path-format=absolute", "--git-common-dir"], + ) { + Ok(p) if !p.trim().is_empty() => std::path::PathBuf::from(p.trim()), + _ => repo_root.join(".git"), + } +} + /// Checks out `commit_ish`'s tracked files into the current working tree and /// index, without moving HEAD. Only restores paths that existed at /// `commit_ish` -- files created since are untouched and survive. @@ -240,6 +325,68 @@ mod tests { assert_eq!(after[0].id, id2); } + #[test] + fn concurrent_worktree_snapshots_do_not_share_an_index() { + // Regression: snapshot_worktree_before used to key its temporary + // GIT_INDEX_FILE on std::process::id() alone. Two threads in the + // same process snapshotting two different worktrees at once would + // stomp on each other's staging area, so a snapshot could end up + // capturing the wrong worktree's contents (or a mix of both). + let repo = init_repo_with_branch("master"); + let wt_a = repo.path.join("wt-a"); + let wt_b = repo.path.join("wt-b"); + run_in( + &repo.path, + &[ + "worktree", + "add", + "-b", + "wt-a", + wt_a.to_str().unwrap(), + "master", + ], + ) + .unwrap(); + run_in( + &repo.path, + &[ + "worktree", + "add", + "-b", + "wt-b", + wt_b.to_str().unwrap(), + "master", + ], + ) + .unwrap(); + std::fs::write(wt_a.join("sentinel-a.txt"), "only in a\n").unwrap(); + std::fs::write(wt_b.join("sentinel-b.txt"), "only in b\n").unwrap(); + + let repo_root_a = repo.path.clone(); + let wt_a_clone = wt_a.clone(); + let handle_a = std::thread::spawn(move || { + snapshot_worktree_before(&repo_root_a, &wt_a_clone, "concurrent snapshot a").unwrap() + }); + let repo_root_b = repo.path.clone(); + let wt_b_clone = wt_b.clone(); + let handle_b = std::thread::spawn(move || { + snapshot_worktree_before(&repo_root_b, &wt_b_clone, "concurrent snapshot b").unwrap() + }); + let id_a = handle_a.join().unwrap(); + let id_b = handle_b.join().unwrap(); + + let tree_a = run_in(&repo.path, &["ls-tree", "-r", "--name-only", &id_a.0]).unwrap(); + let tree_b = run_in(&repo.path, &["ls-tree", "-r", "--name-only", &id_b.0]).unwrap(); + assert!( + tree_a.contains("sentinel-a.txt") && !tree_a.contains("sentinel-b.txt"), + "snapshot a must capture only worktree a's contents: {tree_a}" + ); + assert!( + tree_b.contains("sentinel-b.txt") && !tree_b.contains("sentinel-a.txt"), + "snapshot b must capture only worktree b's contents: {tree_b}" + ); + } + #[test] fn snapshot_before_any_commit_exists_still_works() { // No parent commit to attach to -- must not error out on a diff --git a/crates/flare-git-core/src/worktree.rs b/crates/flare-git-core/src/worktree.rs index 0ecb446d..f0ba394a 100644 --- a/crates/flare-git-core/src/worktree.rs +++ b/crates/flare-git-core/src/worktree.rs @@ -409,9 +409,18 @@ pub fn create_worktree( // unconditionally refuses to check out a branch git still considers // checked out elsewhere ("already checked out" / prunable registration // -- confirmed live on item #331, regenerating on every dispatch - // attempt without this). Prune first so a stale registration for this - // branch never survives to block reuse. - crate::shell::prune_worktree_metadata_if(repo_root, true); + // attempt without this). Clear that registration first so it never + // survives to block reuse. + // + // Deliberately NOT `git worktree prune`: prune is repo-wide, and it + // drops the admin entry of ANY worktree whose `gitdir` file points + // somewhere non-existent -- even one whose directory is fully intact + // and holds uncommitted work. That turned one item's failed-dispatch + // retry into another item's data loss: the victim's `.git` pointer + // became dangling, `audit_orphans` then classified it as a broken- + // gitdir orphan, and `gc_orphans` deleted it. Scope the cleanup to + // this branch's own stale registration instead. + remove_stale_registration_for(repo_root, &branch); // Check it out as-is, no `-b` -- git auto-creates the local tracking // branch when only the remote-tracking ref exists, same as `git // checkout `. @@ -503,6 +512,60 @@ pub fn create_worktree( } } +/// Removes the stale `.git/worktrees/` admin entry that claims +/// `branch`, if there is one. Returns whether anything was removed. +/// +/// This is the narrow, per-branch equivalent of `git worktree prune`, and +/// exists because prune's blast radius is the whole repo. Prune deletes the +/// admin entry of *every* registration whose `gitdir` file points at a +/// missing path — including a worktree that is still fully present on disk +/// with uncommitted work in it, whose admin entry merely went stale (a +/// moved checkout, an interrupted operation, or a Windows path/locking +/// hiccup). The victim is left with a dangling `.git` pointer, which +/// `audit_orphans` reads as "broken gitdir" and `gc_orphans` then deletes. +/// +/// Two guards keep this scoped: only registrations whose `HEAD` names +/// `branch` are considered, and only ones whose checkout directory is +/// actually gone. A registration pointing at a live directory is never +/// touched, so another item's worktree can never be collateral damage. +fn remove_stale_registration_for(repo_root: &Path, branch: &str) -> bool { + let Ok(common_dir) = run_git_in(repo_root, &["rev-parse", "--git-common-dir"]) else { + return false; + }; + let admin_root = repo_root.join(common_dir.trim()).join("worktrees"); + let Ok(entries) = std::fs::read_dir(&admin_root) else { + return false; + }; + let wanted_head = format!("ref: refs/heads/{branch}"); + let mut removed = false; + for entry in entries.flatten() { + let admin = entry.path(); + if !admin.is_dir() { + continue; + } + // Does this registration claim our branch? + let head = std::fs::read_to_string(admin.join("HEAD")).unwrap_or_default(); + if head.trim() != wanted_head { + continue; + } + // `gitdir` holds "/.git" -- its parent is the checkout. + // Preserve the registration if that directory still exists (or if + // the file is unreadable): fail closed, since removing a live + // worktree's registration is the exact harm this function avoids. + let gitdir = std::fs::read_to_string(admin.join("gitdir")).unwrap_or_default(); + let still_live = std::path::Path::new(gitdir.trim()) + .parent() + .is_none_or(std::path::Path::exists); + if still_live { + continue; + } + if std::fs::remove_dir_all(&admin).is_ok() { + removed = true; + } + } + removed +} + /// Kills `child` and its whole process tree — not just the direct child — /// so a grandchild (e.g. a `git` credential helper) can't outlive a timeout. fn kill_tree(child: &mut std::process::Child) { @@ -981,7 +1044,9 @@ pub fn gc_orphans(repo_root: &Path, names: &[String]) -> Vec { // point a destructive delete has; deleting anyway would defeat the // whole point of snapshotting first. let reason = format!("gc orphan worktree {}", name); - if let Err(e) = crate::snapshot::snapshot_before(repo_root, &reason) { + if let Err(e) = + crate::snapshot::snapshot_worktree_before(repo_root, &worktree_path, &reason) + { eprintln!( "worktree: skipping orphan '{}', snapshot failed: {}", name, e diff --git a/crates/flare-git-core/src/worktree_tests.rs b/crates/flare-git-core/src/worktree_tests.rs index a98ba0d1..d3943084 100644 --- a/crates/flare-git-core/src/worktree_tests.rs +++ b/crates/flare-git-core/src/worktree_tests.rs @@ -462,6 +462,137 @@ fn audit_orphans_ignores_a_claimed_or_own_branch_worktree() { assert!(orphans.is_empty()); } +/// Cross-item data loss (vent, severity critical): dispatching several +/// items in quick succession caused worktree-creation retries, after which +/// two UNRELATED items' dirty worktrees vanished from disk — with no +/// `reclaim`/`force` anywhere in the picture. +/// +/// The mechanism, reproduced here: `create_worktree` used to run a +/// repo-wide `git worktree prune` to clear its own branch's stale +/// registration. Prune drops the admin entry of *any* registration whose +/// `gitdir` file points at a missing path — including a worktree that is +/// fully intact on disk with uncommitted work in it. The victim was left +/// with a dangling `.git`, which `audit_orphans` reads as a broken-gitdir +/// orphan and `gc_orphans` then deletes. +#[test] +fn create_worktree_does_not_orphan_another_items_live_worktree() { + let repo = init_repo(); + let target = resolve_default_branch(&repo.path); + + // Victim: another item's worktree, live on disk, with real uncommitted + // work in it. + let victim_item = test_item(110); + let victim = create_worktree(&victim_item, &repo.path, &target, None).unwrap(); + std::fs::write(victim.join("precious.txt"), "uncommitted work").unwrap(); + + // Make the victim's admin entry stale the way a partial/interrupted or + // path-shifted operation does -- its `gitdir` file now names a path + // that does not exist, while the checkout itself is untouched. + let admin = repo.path.join(".git").join("worktrees").join("110"); + assert!(admin.is_dir(), "victim registration missing at {admin:?}"); + std::fs::write( + admin.join("gitdir"), + "C:/nonexistent/path/.git +", + ) + .unwrap(); + + // Now drive the retry path on a DIFFERENT item: an already-existing + // branch, which is the branch_exists arm that used to prune repo-wide. + let other = test_item(475); + let other_branch = task_branch_name(&other); + assert!(crate::shell::run_in_ok( + &repo.path, + &["branch", &other_branch, &target] + )); + let created = create_worktree(&other, &repo.path, &target, None); + assert!(created.is_ok(), "dispatch failed: {created:?}"); + + // The victim must survive, registration and contents both. + assert!( + admin.is_dir(), + "another item's worktree registration was pruned as collateral damage" + ); + assert!( + victim.join("precious.txt").exists(), + "uncommitted work destroyed" + ); + assert!( + audit_orphans(&repo.path, Some(&std::collections::HashSet::new())) + .iter() + .all(|o| o.name != "110"), + "live worktree was reclassified as a prunable orphan" + ); +} + +/// The stale registration `create_worktree` *is* meant to clear (its own +/// branch, checkout genuinely gone) still gets cleared -- the scoping fix +/// must not regress the item #331 case it replaced. +#[test] +fn create_worktree_clears_its_own_stale_registration() { + let repo = init_repo(); + let target = resolve_default_branch(&repo.path); + let item = test_item(331); + let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); + let branch = resolve_worktree_branch(&item, &worktree_path); + + // Directory removed out-of-band (crash, manual cleanup): git still + // considers `branch` checked out here, which blocks `worktree add`. + std::fs::remove_dir_all(&worktree_path).unwrap(); + let admin = repo.path.join(".git").join("worktrees").join("331"); + assert!(admin.is_dir()); + assert!(remove_stale_registration_for(&repo.path, &branch)); + assert!(!admin.exists()); + + // And the re-dispatch it was blocking now succeeds. + let again = create_worktree(&item, &repo.path, &target, None); + assert!(again.is_ok(), "re-dispatch still blocked: {again:?}"); +} + +/// `gc_orphans` snapshots "before deletion" so a destructive sweep is +/// recoverable. That snapshot used to stage from `repo_root`, where +/// `ensure_worktrees_ignored` excludes `.worktrees/` -- so it captured +/// nothing of the directory being deleted and the work was simply gone. +/// A broken-gitdir orphan is also the one case `audit_orphans` never +/// dirty-checks (it cannot: `git status` needs a working gitdir), which is +/// exactly why the snapshot has to be real. +#[test] +fn gc_orphans_snapshot_actually_captures_the_deleted_worktrees_work() { + let repo = init_repo(); + let target = resolve_default_branch(&repo.path); + let item = test_item(473); + let worktree_path = create_worktree(&item, &repo.path, &target, None).unwrap(); + std::fs::write(worktree_path.join("precious.txt"), "uncommitted work").unwrap(); + + // Break the gitdir pointer -> audit_orphans sees a broken-gitdir orphan. + std::fs::write( + worktree_path.join(".git"), + "gitdir: C:/nonexistent/.git +", + ) + .unwrap(); + let orphans = audit_orphans(&repo.path, Some(&std::collections::HashSet::new())); + assert_eq!(orphans.len(), 1); + assert!(orphans[0].has_broken_gitdir); + + let deleted = gc_orphans(&repo.path, &["473".to_string()]); + assert_eq!(deleted, vec!["473".to_string()]); + assert!(!worktree_path.exists()); + + // The whole point: the work is still recoverable from the snapshot. + let snapshots = crate::snapshot::list(&repo.path); + let snap = snapshots + .iter() + .find(|m| m.reason.contains("gc orphan worktree 473")) + .expect("no snapshot recorded for the deleted orphan"); + let listing = + crate::shell::run_in(&repo.path, &["ls-tree", "-r", "--name-only", &snap.id.0]).unwrap(); + assert!( + listing.lines().any(|l| l.trim() == "precious.txt"), + "snapshot did not capture the deleted worktree's uncommitted work; got: {listing}" + ); +} + #[test] fn commit_uncommitted_commits_a_dirty_worktree() { let repo = init_repo();