Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion crates/flare-git-core/src/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
165 changes: 156 additions & 9 deletions crates/flare-git-core/src/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -27,19 +42,22 @@ pub struct SnapshotMeta {

/// `git <args>` 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<String, String> {
///
/// `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<String, String> {
// 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
// (autocrlf=true is the common default on Windows).
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
Expand All @@ -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<SnapshotId, String> {
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"])?;
Expand All @@ -83,6 +102,72 @@ pub fn snapshot_before(repo_root: &Path, reason: &str) -> Result<SnapshotId, Str
result
}

/// Snapshots a *linked worktree's* contents into the main repo's object
/// store, returning a recoverable commit under the private snapshot ref.
///
/// [`snapshot_before`] cannot do this job: it stages from `repo_root`, and
/// `ensure_worktrees_ignored` puts `.worktrees/` in `.git/info/exclude`, so
/// `git add -A` there captures exactly nothing of the worktree about to be
/// deleted. Every "snapshot first, then remove" call site was therefore
/// writing an empty safety net and destroying uncommitted work anyway.
///
/// Works even when the worktree's own `.git` pointer is broken (the
/// orphan case): the object store is addressed explicitly via `--git-dir`
/// on the main repo rather than discovered from the worktree. The
/// worktree's own `.gitignore` still applies, so build artifacts
/// (`target/`) and the generated `.cargo/` stay out of the snapshot.
///
/// The commit is deliberately parentless — the worktree's branch tip is
/// not necessarily its content's ancestor (a broken-gitdir worktree may
/// not have a resolvable branch at all), and recovery only needs the tree.
pub fn snapshot_worktree_before(
repo_root: &Path,
worktree_path: &Path,
reason: &str,
) -> Result<SnapshotId, String> {
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()
));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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 `<repo_root>/.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.
Expand Down Expand Up @@ -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
Expand Down
73 changes: 69 additions & 4 deletions crates/flare-git-core/src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 <branch>`.
Expand Down Expand Up @@ -503,6 +512,60 @@ pub fn create_worktree(
}
}

/// Removes the stale `.git/worktrees/<name>` 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 "<checkout>/.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) {
Expand Down Expand Up @@ -981,7 +1044,9 @@ pub fn gc_orphans(repo_root: &Path, names: &[String]) -> Vec<String> {
// 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
Expand Down
Loading
Loading