From 2efb0d0d381c9d8f0334c96ce71abc5af1263081 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 11 Nov 2026 08:13:12 -0500 Subject: [PATCH 1/2] Remove a failed skill or Review acquisition's empty temp directory SkillWorktree kept its tempdir before acquisition, so a failed add left an empty thirdshift-skill-* or thirdshift-review-* directory behind. Remove only that still-empty allocated directory instance on failure, keeping retained checkouts and the acquisition error. Closes #1975 --- src/worktree.rs | 2 + src/worktree/skill.rs | 48 ++++++---- src/worktree/skill_tests.rs | 181 ++++++++++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 17 deletions(-) create mode 100644 src/worktree/skill_tests.rs diff --git a/src/worktree.rs b/src/worktree.rs index 9649e282b..66253c669 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -29,6 +29,8 @@ mod preservation_tests; #[cfg(test)] mod review_recovery_tests; #[cfg(test)] +mod skill_tests; +#[cfg(test)] mod synchronization_tests; #[derive(Clone, Copy)] diff --git a/src/worktree/skill.rs b/src/worktree/skill.rs index 2cb26ff52..f355b8d7e 100644 --- a/src/worktree/skill.rs +++ b/src/worktree/skill.rs @@ -1,5 +1,6 @@ //! Standalone skill checkouts start at the Launch directory's committed HEAD. +use std::os::unix::fs::MetadataExt; use std::path::Path; use anyhow::Result; @@ -44,13 +45,9 @@ impl SkillWorktree { let recovered = retained.is_some(); let checkout = match retained { Some(checkout) => checkout, - None => { - let path = tempfile::Builder::new() - .prefix("thirdshift-review-") - .tempdir()? - .keep(); - acquisition::add_review(launch, branch, &path, commit)? - } + None => allocate("thirdshift-review-", |path| { + acquisition::add_review(launch, branch, path, commit) + })?, }; Ok(Self { launch: Git::new(root), @@ -62,16 +59,13 @@ impl SkillWorktree { fn checkout(launch: &Git, commit: &str) -> Result { let root = launch.run(&["rev-parse", "--show-toplevel"])?; - // Cleanup belongs to the captured checkout, never TempDir's recursive Drop. - let path = tempfile::Builder::new() - .prefix("thirdshift-skill-") - .tempdir()? - .keep(); - progress::step(format_args!( - "creating skill worktree {} at {commit}", - path.display() - )); - let checkout = acquisition::add(launch, None, &path, commit)?; + let checkout = allocate("thirdshift-skill-", |path| { + progress::step(format_args!( + "creating skill worktree {} at {commit}", + path.display() + )); + acquisition::add(launch, None, path, commit) + })?; Ok(Self { launch: Git::new(root), checkout, @@ -130,6 +124,26 @@ impl SkillWorktree { } } +/// Cleanup belongs to the captured checkout, never TempDir's recursive Drop: +/// a failed acquisition can retain work there. Failure removes only the +/// allocated directory itself, still empty, and keeps acquisition's error. +fn allocate( + prefix: &str, + acquire: impl FnOnce(&Path) -> Result, +) -> Result { + let directory = tempfile::Builder::new().prefix(prefix).tempdir()?; + let allocated = std::fs::symlink_metadata(directory.path())?; + let path = directory.keep(); + acquire(&path).inspect_err(|_| { + // remove_dir refuses a directory with contents, such as a retained checkout. + if std::fs::symlink_metadata(&path).is_ok_and(|current| { + current.is_dir() && current.dev() == allocated.dev() && current.ino() == allocated.ino() + }) { + let _ = std::fs::remove_dir(&path); + } + }) +} + impl Drop for SkillWorktree { fn drop(&mut self) { self.checkout diff --git a/src/worktree/skill_tests.rs b/src/worktree/skill_tests.rs new file mode 100644 index 000000000..e4c3901ba --- /dev/null +++ b/src/worktree/skill_tests.rs @@ -0,0 +1,181 @@ +//! Skill and Review checkout allocation, each run in a child with its own temp root. + +use std::fs; +use std::path::Path; + +use super::*; + +const CHILD: &str = "THIRDSHIFT_SKILL_ALLOCATION_TEST"; +const MISSING: &str = "1234567890123456789012345678901234567890"; + +/// Run the calling test again in a child whose temp root holds only its +/// fixtures, so allocation leftovers can be counted. +fn isolated(configure: impl FnOnce(&Path, &mut std::process::Command)) -> bool { + if std::env::var_os(CHILD).is_some() { + return false; + } + let root = tempfile::tempdir().unwrap(); + let name = std::thread::current().name().unwrap().to_string(); + // The launch fixture's empty clone warns, so judge only the child's status. + let mut command = std::process::Command::new(std::env::current_exe().unwrap()); + command + .args(["--exact", &name, "--nocapture"]) + .env(CHILD, "1") + .env("TMPDIR", root.path()); + configure(root.path(), &mut command); + let output = command.output().unwrap(); + assert!( + output.status.success(), + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + true +} + +fn allocations() -> Vec { + fs::read_dir(std::env::temp_dir()) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| { + name.starts_with("thirdshift-review-") || name.starts_with("thirdshift-skill-") + }) + .collect() +} + +#[test] +fn a_failed_skill_checkout_leaves_no_allocation_behind() { + if isolated(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + + assert!(SkillWorktree::at(&launch, MISSING).is_err()); + + assert_eq!(allocations(), Vec::::new()); +} + +#[test] +fn a_failed_review_checkout_leaves_no_allocation_behind() { + if isolated(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + + assert!(SkillWorktree::on_pull_request(&launch, "feature", MISSING).is_err()); + + assert_eq!(allocations(), Vec::::new()); + assert_eq!(local_head(&launch, "feature").unwrap(), None); +} + +#[test] +fn a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation() { + if isolated(|root, command| { + // Occupy the branch once, between preflight and Git's add. + let real = String::from_utf8( + std::process::Command::new("sh") + .args(["-c", "command -v git"]) + .output() + .unwrap() + .stdout, + ) + .unwrap(); + let bin = root.join("bin"); + fs::create_dir(&bin).unwrap(); + crate::test_support::write_executable( + &bin.join("git"), + &format!( + "#!/bin/sh\nif [ \"$1 $2 $3\" = 'worktree add -b' ] && [ ! -e '{root}/occupier' ]; then\n '{real}' worktree add -q -b \"$4\" '{root}/occupier' \"$6\" || exit 97\nfi\nexec '{real}' \"$@\"\n", + root = root.display(), + real = real.trim(), + ), + ); + let path = std::env::join_paths( + std::iter::once(bin).chain(std::env::split_paths(&std::env::var_os("PATH").unwrap())), + ) + .unwrap(); + command.env("PATH", path); + }) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + + assert!(SkillWorktree::on_pull_request(&launch, "feature", &start).is_err()); + + assert_eq!(allocations(), Vec::::new()); + let occupier = std::env::temp_dir().join("occupier"); + assert_eq!( + Git::new(&occupier).run(&["rev-parse", "HEAD"]).unwrap(), + start + ); + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn a_branch_occupied_before_review_is_refused_before_allocation() { + if isolated(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + let occupier = std::env::temp_dir().join("occupier"); + launch + .run(&[ + "worktree", + "add", + "-q", + "-b", + "feature", + occupier.to_str().unwrap(), + ]) + .unwrap(); + + let Err(error) = SkillWorktree::on_pull_request(&launch, "feature", &start) else { + panic!("an occupied branch was acquired"); + }; + + assert!( + format!("{error:#}").contains("Occupied checkout"), + "{error:#}" + ); + assert_eq!(allocations(), Vec::::new()); + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); +} + +#[test] +fn hook_work_from_a_failed_skill_checkout_survives() { + if isolated(|_, _| {}) { + return; + } + let (temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + crate::test_support::write_executable( + &temp.path().join("hooks/post-checkout"), + "#!/bin/sh\necho hook work > hook.txt\nexit 1\n", + ); + + assert!(SkillWorktree::at(&launch, &start).is_err()); + + let retained = allocations(); + assert_eq!(retained.len(), 1, "{retained:?}"); + let path = std::env::temp_dir().join(&retained[0]); + assert_eq!( + fs::read_to_string(path.join("hook.txt")).unwrap(), + "hook work\n" + ); +} + +#[test] +fn successful_skill_and_review_checkouts_are_cleaned_up() { + if isolated(|_, _| {}) { + return; + } + let (_temp, launch) = super::tests::launch_directory(); + let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); + + drop(SkillWorktree::at(&launch, &start).unwrap()); + drop(SkillWorktree::on_pull_request(&launch, "feature", &start).unwrap()); + + assert_eq!(allocations(), Vec::::new()); +} From 4f17d9a0676674575062bafccb8bc4bee13747c8 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 11 Nov 2026 08:15:01 -0500 Subject: [PATCH 2/2] Name the allocation seam and tighten review checkout controls Cover hook work on the Review path too and assert the occupation error. --- src/worktree/skill.rs | 6 ++--- src/worktree/skill_tests.rs | 49 +++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/src/worktree/skill.rs b/src/worktree/skill.rs index f355b8d7e..168055221 100644 --- a/src/worktree/skill.rs +++ b/src/worktree/skill.rs @@ -45,7 +45,7 @@ impl SkillWorktree { let recovered = retained.is_some(); let checkout = match retained { Some(checkout) => checkout, - None => allocate("thirdshift-review-", |path| { + None => acquire_in_temp("thirdshift-review-", |path| { acquisition::add_review(launch, branch, path, commit) })?, }; @@ -59,7 +59,7 @@ impl SkillWorktree { fn checkout(launch: &Git, commit: &str) -> Result { let root = launch.run(&["rev-parse", "--show-toplevel"])?; - let checkout = allocate("thirdshift-skill-", |path| { + let checkout = acquire_in_temp("thirdshift-skill-", |path| { progress::step(format_args!( "creating skill worktree {} at {commit}", path.display() @@ -127,7 +127,7 @@ impl SkillWorktree { /// Cleanup belongs to the captured checkout, never TempDir's recursive Drop: /// a failed acquisition can retain work there. Failure removes only the /// allocated directory itself, still empty, and keeps acquisition's error. -fn allocate( +fn acquire_in_temp( prefix: &str, acquire: impl FnOnce(&Path) -> Result, ) -> Result { diff --git a/src/worktree/skill_tests.rs b/src/worktree/skill_tests.rs index e4c3901ba..db2989f4a 100644 --- a/src/worktree/skill_tests.rs +++ b/src/worktree/skill_tests.rs @@ -6,11 +6,11 @@ use std::path::Path; use super::*; const CHILD: &str = "THIRDSHIFT_SKILL_ALLOCATION_TEST"; -const MISSING: &str = "1234567890123456789012345678901234567890"; +const MISSING_COMMIT: &str = "1234567890123456789012345678901234567890"; /// Run the calling test again in a child whose temp root holds only its /// fixtures, so allocation leftovers can be counted. -fn isolated(configure: impl FnOnce(&Path, &mut std::process::Command)) -> bool { +fn ran_in_isolated_child(configure: impl FnOnce(&Path, &mut std::process::Command)) -> bool { if std::env::var_os(CHILD).is_some() { return false; } @@ -45,24 +45,24 @@ fn allocations() -> Vec { #[test] fn a_failed_skill_checkout_leaves_no_allocation_behind() { - if isolated(|_, _| {}) { + if ran_in_isolated_child(|_, _| {}) { return; } let (_temp, launch) = super::tests::launch_directory(); - assert!(SkillWorktree::at(&launch, MISSING).is_err()); + assert!(SkillWorktree::at(&launch, MISSING_COMMIT).is_err()); assert_eq!(allocations(), Vec::::new()); } #[test] fn a_failed_review_checkout_leaves_no_allocation_behind() { - if isolated(|_, _| {}) { + if ran_in_isolated_child(|_, _| {}) { return; } let (_temp, launch) = super::tests::launch_directory(); - assert!(SkillWorktree::on_pull_request(&launch, "feature", MISSING).is_err()); + assert!(SkillWorktree::on_pull_request(&launch, "feature", MISSING_COMMIT).is_err()); assert_eq!(allocations(), Vec::::new()); assert_eq!(local_head(&launch, "feature").unwrap(), None); @@ -70,7 +70,7 @@ fn a_failed_review_checkout_leaves_no_allocation_behind() { #[test] fn a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation() { - if isolated(|root, command| { + if ran_in_isolated_child(|root, command| { // Occupy the branch once, between preflight and Git's add. let real = String::from_utf8( std::process::Command::new("sh") @@ -101,8 +101,11 @@ fn a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation let (_temp, launch) = super::tests::launch_directory(); let start = launch.run(&["rev-parse", "HEAD"]).unwrap(); - assert!(SkillWorktree::on_pull_request(&launch, "feature", &start).is_err()); + let Err(error) = SkillWorktree::on_pull_request(&launch, "feature", &start) else { + panic!("a branch occupied after preflight was acquired"); + }; + assert!(format!("{error:#}").contains("feature"), "{error:#}"); assert_eq!(allocations(), Vec::::new()); let occupier = std::env::temp_dir().join("occupier"); assert_eq!( @@ -114,7 +117,7 @@ fn a_branch_occupied_after_review_preflight_keeps_the_occupier_and_no_allocation #[test] fn a_branch_occupied_before_review_is_refused_before_allocation() { - if isolated(|_, _| {}) { + if ran_in_isolated_child(|_, _| {}) { return; } let (_temp, launch) = super::tests::launch_directory(); @@ -144,8 +147,8 @@ fn a_branch_occupied_before_review_is_refused_before_allocation() { } #[test] -fn hook_work_from_a_failed_skill_checkout_survives() { - if isolated(|_, _| {}) { +fn hook_work_from_a_failed_skill_or_review_checkout_survives() { + if ran_in_isolated_child(|_, _| {}) { return; } let (temp, launch) = super::tests::launch_directory(); @@ -156,19 +159,29 @@ fn hook_work_from_a_failed_skill_checkout_survives() { ); assert!(SkillWorktree::at(&launch, &start).is_err()); + assert!(SkillWorktree::on_pull_request(&launch, "feature", &start).is_err()); - let retained = allocations(); - assert_eq!(retained.len(), 1, "{retained:?}"); - let path = std::env::temp_dir().join(&retained[0]); - assert_eq!( - fs::read_to_string(path.join("hook.txt")).unwrap(), - "hook work\n" + let mut retained = allocations(); + retained.sort_by_key(|name| name.starts_with("thirdshift-review-")); + assert!( + retained.len() == 2 + && retained[0].starts_with("thirdshift-skill-") + && retained[1].starts_with("thirdshift-review-"), + "{retained:?}" ); + for name in retained { + let path = std::env::temp_dir().join(name); + assert_eq!( + fs::read_to_string(path.join("hook.txt")).unwrap(), + "hook work\n" + ); + } + assert_eq!(local_head(&launch, "feature").unwrap(), Some(start)); } #[test] fn successful_skill_and_review_checkouts_are_cleaned_up() { - if isolated(|_, _| {}) { + if ran_in_isolated_child(|_, _| {}) { return; } let (_temp, launch) = super::tests::launch_directory();