From 5eb7ffa9f61202af5f0e8a411329cd13fd849412 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Jul 2026 03:17:13 +0000 Subject: [PATCH 1/2] fix(core/build): clone into a temp dir and atomically rename into place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `NewClone` cloned directly into the final `clone_dir`, so a crash (SIGKILL / power loss) mid-clone left a half-written repo at that path. Since #2795, a later build reusing such a leftover hits the Reuse arm's HEAD check, fails to resolve HEAD, and bails *without* cleanup — every subsequent rebuild repeats it, wedging the build permanently until the directory is deleted by hand (#2808). Branch/tag pins skip the HEAD check entirely and silently reuse the broken checkout. Fix the regression at its root: clone + checkout into a unique temporary sibling dir and `rename` it into `clone_dir` only once the checkout has fully succeeded. A crash then only ever leaves a temp dir, never a directory at `clone_dir` — so any dir that exists there is, by construction, a complete clone, and `clone_dir_ready`/Reuse can trust it again. Temp names are process+counter unique to stay cross-process safe, and a best-effort age-gated sweep reclaims temp dirs abandoned by crashed builds so they don't accumulate. Adds unit tests for atomic promotion + cleanup, the failure path leaving no target dir, unique temp paths, and the stale-temp sweep. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01AYVWUwXULA9QoNbYJcLsF4 --- libraries/core/src/build/git.rs | 247 +++++++++++++++++++++++++++++++- 1 file changed, 243 insertions(+), 4 deletions(-) diff --git a/libraries/core/src/build/git.rs b/libraries/core/src/build/git.rs index c6bca987aa..c62716925c 100644 --- a/libraries/core/src/build/git.rs +++ b/libraries/core/src/build/git.rs @@ -6,7 +6,11 @@ use itertools::Itertools; use std::{ collections::{BTreeMap, BTreeSet}, path::{Path, PathBuf}, - sync::{Arc, Mutex}, + sync::{ + Arc, Mutex, + atomic::{AtomicU64, Ordering}, + }, + time::Duration, }; use url::Url; @@ -267,12 +271,34 @@ impl GitFolder { ), ) .await; - let clone_target = target_dir.clone(); + + // Clone into a temporary sibling dir and only atomically + // `rename` it into `target_dir` once the checkout has fully + // succeeded. That way a crash (SIGKILL / power loss) mid-clone + // leaves a half-written repo under the temp name, *never* at + // `target_dir` -- so a later build's `clone_dir_ready` check + // (which trusts `dir.exists()`) can't mistake a broken leftover + // for a good clone and wedge itself permanently on the + // un-resolvable HEAD in the Reuse arm (#2808). Any directory + // that *does* exist at `target_dir` is, by construction, a + // complete checkout. + let tmp_dir = partial_clone_path(&target_dir); + + // Best-effort: reclaim temp dirs abandoned by earlier crashed + // builds so they don't accumulate. Only clearly-stale ones are + // swept (see helper), never a temp another build may still be + // writing right now. + sweep_stale_partial_clones(logger, &target_dir, PARTIAL_CLONE_MAX_AGE).await; + + let clone_target = tmp_dir.clone(); let checkout_result = match tokio::task::spawn_blocking(move || { let repository = clone_into(repo_url.clone(), &clone_target) .with_context(|| format!("failed to clone git repo from `{repo_url}`"))?; checkout_tree(&repository, &commit_hash) .with_context(|| format!("failed to checkout commit `{commit_hash}`")) + // `repository` is dropped here, before the rename below, so + // no git2 handles remain open on the temp dir (Windows + // refuses to rename a dir with open handles). }) .await { @@ -287,12 +313,35 @@ impl GitFolder { }; match checkout_result { - Ok(()) => target_dir, + Ok(()) => { + // Promote the finished clone into place atomically. + match tokio::fs::rename(&tmp_dir, &target_dir).await { + Ok(()) => target_dir, + // Another build won the race and already put a + // complete clone at `target_dir` (rename onto a + // populated dir fails). Drop ours and reuse theirs. + Err(_) if target_dir.exists() => { + cleanup_failed_clone(logger, &tmp_dir).await; + target_dir + } + Err(err) => { + logger + .log_message(LogLevel::Error, format!("{err:?}")) + .await; + cleanup_failed_clone(logger, &tmp_dir).await; + bail!( + "failed to move finished clone from {} into {}: {err}", + tmp_dir.display(), + target_dir.display() + ) + } + } + } Err(err) => { logger .log_message(LogLevel::Error, format!("{err:?}")) .await; - cleanup_failed_clone(logger, &target_dir).await; + cleanup_failed_clone(logger, &tmp_dir).await; bail!(err) } } @@ -508,6 +557,74 @@ async fn cleanup_failed_clone(logger: &mut impl BuildLogger, dir: &Path) { } } +/// How long an abandoned `NewClone` temp dir must have sat untouched before a +/// later build reclaims it. A live clone is younger than a fresh checkout, so +/// this comfortably exceeds any real in-progress clone while still bounding how +/// long a crash leftover lingers on disk (#2808). +const PARTIAL_CLONE_MAX_AGE: Duration = Duration::from_secs(60 * 60); + +/// Filename prefix shared by every in-flight `NewClone` temp dir for `target`. +/// Sits alongside `target` (whose final component is the commit hash), begins +/// with a dot so it's visually distinct from real clone dirs, and can never +/// collide with a sibling commit-hash dir. +fn partial_clone_prefix(target: &Path) -> String { + let name = target + .file_name() + .map(|n| n.to_string_lossy()) + .unwrap_or_default(); + format!(".{name}.partial-") +} + +/// A unique temp path (sibling of `target`) to clone into before the atomic +/// rename into place. The name is `-`: the pid keeps it +/// distinct across processes sharing a working dir, and the process-wide +/// counter keeps concurrent clones of the same commit apart. Uniqueness (rather +/// than a fixed `.partial` name) is what keeps this cross-process safe -- no +/// build ever writes into, or reclaims, another live build's temp dir. +fn partial_clone_path(target: &Path) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + let pid = std::process::id(); + target.with_file_name(format!("{}{pid}-{n}", partial_clone_prefix(target))) +} + +/// Best-effort removal of `NewClone` temp dirs for `target` abandoned by +/// crashed builds. Only dirs whose mtime is at least `max_age` old are removed, +/// so a temp another build is *currently* cloning into -- necessarily younger +/// than a completed checkout -- is never touched. Combined with the unique temp +/// names, this reclaims clearly-dead leftovers without ever racing a live clone +/// in another process. Any error (unreadable dir, missing entry) is ignored: +/// this is opportunistic housekeeping, not a correctness requirement. +async fn sweep_stale_partial_clones( + logger: &mut impl BuildLogger, + target: &Path, + max_age: Duration, +) { + let Some(parent) = target.parent() else { + return; + }; + let prefix = partial_clone_prefix(target); + let Ok(mut entries) = tokio::fs::read_dir(parent).await else { + return; + }; + while let Ok(Some(entry)) = entries.next_entry().await { + let name = entry.file_name(); + let Some(name) = name.to_str() else { continue }; + if !name.starts_with(&prefix) { + continue; + } + let stale = match entry.metadata().await.and_then(|m| m.modified()) { + // `elapsed()` errors if the mtime is in the future (clock skew); + // treat that as "not yet stale" and leave the dir alone. + Ok(mtime) => mtime.elapsed().map(|age| age >= max_age).unwrap_or(false), + Err(_) => false, + }; + if stale { + cleanup_failed_clone(logger, &entry.path()).await; + } + } +} + /// True for a full-length hex commit id (40 chars for SHA-1, 64 for SHA-256). /// Branch and tag names are shorter or non-hex, and I can't resolve those to a /// commit without hitting the network, so those pins skip the HEAD check. @@ -1141,4 +1258,126 @@ mod tests { checkout_tree(&repository, &branch_name).unwrap(); checkout_tree(&repository, &head_commit.to_string()).unwrap(); } + + // A partial-clone temp path is a sibling of the target (not the target + // itself), carries the target's basename, and every call is unique -- so it + // is never picked up by `clone_dir_ready`/Reuse and never collides with a + // concurrent clone of the same commit. + #[test] + fn partial_clone_path_is_a_unique_sibling() { + let target = Path::new("/base/localhost/org/repo").join("a".repeat(40)); + let p1 = partial_clone_path(&target); + let p2 = partial_clone_path(&target); + + assert_ne!(p1, target); + assert_ne!(p1, p2, "each call must produce a distinct temp path"); + assert_eq!(p1.parent(), target.parent(), "temp must be a sibling"); + let name = p1.file_name().unwrap().to_string_lossy(); + assert!(name.starts_with(&partial_clone_prefix(&target))); + } + + // A `NewClone` must land at `target_dir` atomically and leave no temp dir + // behind on success -- the property that stops a crashed build's half-clone + // from ever sitting at `target_dir` and wedging future reuse (#2808). + #[tokio::test] + async fn new_clone_promotes_temp_into_target_and_cleans_up() { + let repo_dir = tempfile::tempdir().unwrap(); + let repo_path = repo_dir.path().join("repo"); + let commit = init_repo_with_commit(&repo_path); + let repo_url = Url::parse(&format!("file://{}", repo_path.display())).unwrap(); + + let base = tempfile::tempdir().unwrap(); + let target = base.path().join("localhost").join(&commit); + + let folder = GitFolder { + reuse: ReuseOptions::NewClone { + target_dir: target.clone(), + repo_url, + commit_hash: commit.clone(), + }, + _claim: None, + }; + let out = folder.prepare(&mut TestLogger).await.unwrap(); + assert_eq!(out, target); + + // A real repo checked out at the requested commit is now at target. + let repo = git2::Repository::open(&target).unwrap(); + assert_eq!( + repo.head() + .unwrap() + .peel_to_commit() + .unwrap() + .id() + .to_string(), + commit + ); + + // No temp sibling survived the successful clone. + let leftovers: Vec<_> = std::fs::read_dir(base.path().join("localhost")) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| { + e.file_name() + .to_string_lossy() + .starts_with(&partial_clone_prefix(&target)) + }) + .collect(); + assert!( + leftovers.is_empty(), + "temp dir must be gone after promotion" + ); + } + + // A `NewClone` whose clone fails must leave *nothing* at `target_dir`, so a + // later build never mistakes a failed attempt for a reusable clone (#2808). + #[tokio::test] + async fn new_clone_failure_leaves_no_target_dir() { + let base = tempfile::tempdir().unwrap(); + let target = base.path().join("localhost").join("a".repeat(40)); + // A file:// URL to a path that isn't a git repo -> clone fails. + let missing = base.path().join("does-not-exist"); + let repo_url = Url::parse(&format!("file://{}", missing.display())).unwrap(); + + let folder = GitFolder { + reuse: ReuseOptions::NewClone { + target_dir: target.clone(), + repo_url, + commit_hash: "a".repeat(40), + }, + _claim: None, + }; + assert!(folder.prepare(&mut TestLogger).await.is_err()); + assert!( + !target.exists(), + "a failed clone must never leave a dir at the target path" + ); + } + + // The stale-temp sweep removes abandoned temp dirs for the target (age gate + // satisfied by `Duration::ZERO`) while leaving the real target dir and + // unrelated siblings untouched. + #[tokio::test] + async fn sweep_removes_abandoned_partial_dirs_only() { + let base = tempfile::tempdir().unwrap(); + let dir = base.path().join("localhost"); + std::fs::create_dir_all(&dir).unwrap(); + let target = dir.join("a".repeat(40)); + + let stale = partial_clone_path(&target); + std::fs::create_dir_all(&stale).unwrap(); + std::fs::write(stale.join("x"), b"half").unwrap(); + let unrelated = dir.join("b".repeat(40)); // a real sibling clone dir + std::fs::create_dir_all(&unrelated).unwrap(); + + // max_age = 0: every matching temp counts as stale. + sweep_stale_partial_clones(&mut TestLogger, &target, Duration::ZERO).await; + assert!(!stale.exists(), "abandoned temp must be swept"); + assert!(unrelated.exists(), "unrelated sibling must be kept"); + + // A huge max_age keeps a freshly-created temp (nothing is old enough). + let fresh = partial_clone_path(&target); + std::fs::create_dir_all(&fresh).unwrap(); + sweep_stale_partial_clones(&mut TestLogger, &target, Duration::from_secs(3600)).await; + assert!(fresh.exists(), "a fresh temp must not be swept"); + } } From b8a8383e27cccdf88148abe70b8224b468416a2b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 13:06:04 +0000 Subject: [PATCH 2/2] fix(core/build): extend atomic temp-then-promote to all clone-reuse arms Address review on #2809. The "any directory at clone_dir is a complete clone" invariant only held for the NewClone arm; CopyAndFetch and RenameAndFetch still wrote into clone_dir in place, so a SIGKILL mid-copy or mid-fetch could still leave a broken .git at clone_dir and reproduce the #2808 permanent wedge. - Factor the atomic promotion into a shared `promote_clone(tmp, target)` helper (rename-into-place, with the concurrent-winner race handled) and route all three write arms (NewClone, CopyAndFetch, RenameAndFetch) through a temp sibling dir, so clone_dir is only ever created by the final rename. - Remove `sweep_stale_partial_clones`: it derived staleness from the temp dir's top-level mtime, which git does not touch while fetching objects under .git/, so a clone running longer than the threshold could be removed out from under a concurrent build of the same commit. Orphaned temp dirs from a hard crash are harmless (unique names, never matched by clone_dir_ready/Reuse) and are left for external cleanup. Tests: add `promote_clone` unit tests (absent-target move and concurrent-winner race), end-to-end promote tests for CopyAndFetch and RenameAndFetch (via a local origin remote), and failure-path tests asserting no target dir and no temp leftover for every arm. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01AYVWUwXULA9QoNbYJcLsF4 --- libraries/core/src/build/git.rs | 432 ++++++++++++++++++++------------ 1 file changed, 269 insertions(+), 163 deletions(-) diff --git a/libraries/core/src/build/git.rs b/libraries/core/src/build/git.rs index c62716925c..35e376c20b 100644 --- a/libraries/core/src/build/git.rs +++ b/libraries/core/src/build/git.rs @@ -10,7 +10,6 @@ use std::{ Arc, Mutex, atomic::{AtomicU64, Ordering}, }, - time::Duration, }; use url::Url; @@ -274,31 +273,19 @@ impl GitFolder { // Clone into a temporary sibling dir and only atomically // `rename` it into `target_dir` once the checkout has fully - // succeeded. That way a crash (SIGKILL / power loss) mid-clone - // leaves a half-written repo under the temp name, *never* at - // `target_dir` -- so a later build's `clone_dir_ready` check - // (which trusts `dir.exists()`) can't mistake a broken leftover - // for a good clone and wedge itself permanently on the - // un-resolvable HEAD in the Reuse arm (#2808). Any directory - // that *does* exist at `target_dir` is, by construction, a - // complete checkout. + // succeeded (see `promote_clone`). A crash mid-clone then leaves + // a half-written repo under the temp name, never at `target_dir`. let tmp_dir = partial_clone_path(&target_dir); - // Best-effort: reclaim temp dirs abandoned by earlier crashed - // builds so they don't accumulate. Only clearly-stale ones are - // swept (see helper), never a temp another build may still be - // writing right now. - sweep_stale_partial_clones(logger, &target_dir, PARTIAL_CLONE_MAX_AGE).await; - let clone_target = tmp_dir.clone(); let checkout_result = match tokio::task::spawn_blocking(move || { let repository = clone_into(repo_url.clone(), &clone_target) .with_context(|| format!("failed to clone git repo from `{repo_url}`"))?; checkout_tree(&repository, &commit_hash) .with_context(|| format!("failed to checkout commit `{commit_hash}`")) - // `repository` is dropped here, before the rename below, so - // no git2 handles remain open on the temp dir (Windows - // refuses to rename a dir with open handles). + // `repository` is dropped here, before the rename in + // `promote_clone`, so no git2 handles remain open on the temp + // dir (Windows refuses to rename a dir with open handles). }) .await { @@ -313,30 +300,7 @@ impl GitFolder { }; match checkout_result { - Ok(()) => { - // Promote the finished clone into place atomically. - match tokio::fs::rename(&tmp_dir, &target_dir).await { - Ok(()) => target_dir, - // Another build won the race and already put a - // complete clone at `target_dir` (rename onto a - // populated dir fails). Drop ours and reuse theirs. - Err(_) if target_dir.exists() => { - cleanup_failed_clone(logger, &tmp_dir).await; - target_dir - } - Err(err) => { - logger - .log_message(LogLevel::Error, format!("{err:?}")) - .await; - cleanup_failed_clone(logger, &tmp_dir).await; - bail!( - "failed to move finished clone from {} into {}: {err}", - tmp_dir.display(), - target_dir.display() - ) - } - } - } + Ok(()) => promote_clone(logger, &tmp_dir, &target_dir).await?, Err(err) => { logger .log_message(LogLevel::Error, format!("{err:?}")) @@ -351,13 +315,17 @@ impl GitFolder { target_dir, commit_hash, } => { + // Copy + fetch + checkout into a temp sibling and promote it + // into `target_dir` only once every step succeeds. That keeps + // the whole operation all-or-nothing: a crash mid-copy (or a + // failed fetch) leaves a half-copied dir under the temp name, + // never a broken `.git` at `target_dir` that a later build would + // reuse and build the wrong commit from (#2480) or wedge on + // (#2808). + let tmp_dir = partial_clone_path(&target_dir); let from_clone = from.clone(); - let to = target_dir.clone(); + let to = tmp_dir.clone(); - // I want the whole copy + fetch + checkout to be all-or-nothing. - // If any step dies partway we're left with a half-copied dir on - // disk, and I don't want the next build mistaking it for a good - // clone and building the wrong commit (#2480). let result: eyre::Result<()> = async { tokio::task::spawn_blocking(move || { std::fs::create_dir_all(&to) @@ -384,16 +352,18 @@ impl GitFolder { ) .await; - let repository = fetch_changes(&target_dir, None).await?; + let repository = fetch_changes(&tmp_dir, None).await?; checkout_tree(&repository, &commit_hash)?; Ok(()) + // `repository` is dropped at the end of this block, before + // the rename in `promote_clone` (Windows-safe). } .await; match result { - Ok(()) => target_dir, + Ok(()) => promote_clone(logger, &tmp_dir, &target_dir).await?, Err(err) => { - cleanup_failed_clone(logger, &target_dir).await; + cleanup_failed_clone(logger, &tmp_dir).await; bail!(err) } } @@ -403,7 +373,14 @@ impl GitFolder { target_dir, commit_hash, } => { - tokio::fs::rename(&from, &target_dir) + // Rename the old clone into a temp sibling (not straight onto + // `target_dir`), fetch + checkout there, and promote it into + // place only once both succeed. If the fetch or checkout fails + // -- or the process is killed -- the half-updated repo sits at + // the temp name, never at `target_dir`, so a later build never + // reuses an old-commit or broken checkout (#2480, #2808). + let tmp_dir = partial_clone_path(&target_dir); + tokio::fs::rename(&from, &tmp_dir) .await .context("failed to rename repo clone")?; @@ -414,20 +391,19 @@ impl GitFolder { ) .await; - // The old clone now lives at target_dir. If the fetch or checkout - // fails from here, that dir is left sitting at the old commit, so - // I clean it up instead of letting a later build reuse it (#2480). let result: eyre::Result<()> = async { - let repository = fetch_changes(&target_dir, None).await?; + let repository = fetch_changes(&tmp_dir, None).await?; checkout_tree(&repository, &commit_hash)?; Ok(()) + // `repository` is dropped at the end of this block, before + // the rename in `promote_clone` (Windows-safe). } .await; match result { - Ok(()) => target_dir, + Ok(()) => promote_clone(logger, &tmp_dir, &target_dir).await?, Err(err) => { - cleanup_failed_clone(logger, &target_dir).await; + cleanup_failed_clone(logger, &tmp_dir).await; bail!(err) } } @@ -557,70 +533,69 @@ async fn cleanup_failed_clone(logger: &mut impl BuildLogger, dir: &Path) { } } -/// How long an abandoned `NewClone` temp dir must have sat untouched before a -/// later build reclaims it. A live clone is younger than a fresh checkout, so -/// this comfortably exceeds any real in-progress clone while still bounding how -/// long a crash leftover lingers on disk (#2808). -const PARTIAL_CLONE_MAX_AGE: Duration = Duration::from_secs(60 * 60); - -/// Filename prefix shared by every in-flight `NewClone` temp dir for `target`. -/// Sits alongside `target` (whose final component is the commit hash), begins -/// with a dot so it's visually distinct from real clone dirs, and can never -/// collide with a sibling commit-hash dir. -fn partial_clone_prefix(target: &Path) -> String { - let name = target - .file_name() - .map(|n| n.to_string_lossy()) - .unwrap_or_default(); - format!(".{name}.partial-") -} - -/// A unique temp path (sibling of `target`) to clone into before the atomic -/// rename into place. The name is `-`: the pid keeps it -/// distinct across processes sharing a working dir, and the process-wide -/// counter keeps concurrent clones of the same commit apart. Uniqueness (rather -/// than a fixed `.partial` name) is what keeps this cross-process safe -- no -/// build ever writes into, or reclaims, another live build's temp dir. +/// A unique temp path (a sibling of `target`) that a write arm clones, copies, +/// or renames into before atomically promoting it (see `promote_clone`). The +/// name is `..partial--`: it sits alongside +/// `target` so the promoting rename stays on one filesystem, begins with a dot +/// so it's visually distinct from real clone dirs, and can never collide with a +/// sibling commit-hash dir. The `pid` keeps it distinct across processes sharing +/// a working dir and the process-wide counter keeps concurrent operations on the +/// same commit apart. That uniqueness (rather than a fixed `.partial` name) is +/// what keeps this cross-process safe -- no build ever writes into, or reclaims, +/// another live build's temp dir. fn partial_clone_path(target: &Path) -> PathBuf { static COUNTER: AtomicU64 = AtomicU64::new(0); let n = COUNTER.fetch_add(1, Ordering::Relaxed); let pid = std::process::id(); - target.with_file_name(format!("{}{pid}-{n}", partial_clone_prefix(target))) + let name = target + .file_name() + .map(|n| n.to_string_lossy()) + .unwrap_or_default(); + target.with_file_name(format!(".{name}.partial-{pid}-{n}")) } -/// Best-effort removal of `NewClone` temp dirs for `target` abandoned by -/// crashed builds. Only dirs whose mtime is at least `max_age` old are removed, -/// so a temp another build is *currently* cloning into -- necessarily younger -/// than a completed checkout -- is never touched. Combined with the unique temp -/// names, this reclaims clearly-dead leftovers without ever racing a live clone -/// in another process. Any error (unreadable dir, missing entry) is ignored: -/// this is opportunistic housekeeping, not a correctness requirement. -async fn sweep_stale_partial_clones( +/// Atomically move a fully-prepared clone from `tmp` into `target`, so `target` +/// is only ever created by this single rename -- never written into in place. +/// +/// This is the shared tail of every write arm (`NewClone`, `CopyAndFetch`, +/// `RenameAndFetch`). Because each arm does all of its fallible work (clone / +/// copy, fetch, checkout) in `tmp` first and only calls this once that work has +/// fully succeeded, a crash (SIGKILL / power loss) at any earlier point leaves a +/// half-written repo under the temp name, *never* at `target`. So any directory +/// that exists at `target` is, by construction, a complete checkout -- which is +/// what lets a later build's `clone_dir_ready` check (which trusts +/// `dir.exists()`) and the Reuse arm trust it, instead of wedging forever on an +/// un-resolvable HEAD (#2808) or silently reusing a broken checkout. +/// +/// `tmp` must be a sibling of `target` (see `partial_clone_path`) so the rename +/// stays on one filesystem and is atomic. Callers must drop any open +/// `git2::Repository` on `tmp` before calling this (Windows refuses to rename a +/// directory with open handles). +/// +/// On the concurrent-winner race -- another build already promoted a complete +/// clone to `target`, so the rename onto a populated dir fails -- the redundant +/// `tmp` is dropped and the existing `target` reused. +async fn promote_clone( logger: &mut impl BuildLogger, + tmp: &Path, target: &Path, - max_age: Duration, -) { - let Some(parent) = target.parent() else { - return; - }; - let prefix = partial_clone_prefix(target); - let Ok(mut entries) = tokio::fs::read_dir(parent).await else { - return; - }; - while let Ok(Some(entry)) = entries.next_entry().await { - let name = entry.file_name(); - let Some(name) = name.to_str() else { continue }; - if !name.starts_with(&prefix) { - continue; +) -> eyre::Result { + match tokio::fs::rename(tmp, target).await { + Ok(()) => Ok(target.to_owned()), + Err(_) if target.exists() => { + cleanup_failed_clone(logger, tmp).await; + Ok(target.to_owned()) } - let stale = match entry.metadata().await.and_then(|m| m.modified()) { - // `elapsed()` errors if the mtime is in the future (clock skew); - // treat that as "not yet stale" and leave the dir alone. - Ok(mtime) => mtime.elapsed().map(|age| age >= max_age).unwrap_or(false), - Err(_) => false, - }; - if stale { - cleanup_failed_clone(logger, &entry.path()).await; + Err(err) => { + logger + .log_message(LogLevel::Error, format!("{err:?}")) + .await; + cleanup_failed_clone(logger, tmp).await; + bail!( + "failed to move finished clone from {} into {}: {err}", + tmp.display(), + target.display() + ) } } } @@ -1259,6 +1234,42 @@ mod tests { checkout_tree(&repository, &head_commit.to_string()).unwrap(); } + // The prefix `partial_clone_path` gives a target's temp siblings, recomputed + // independently so a drift in the naming scheme trips the assertions below. + fn partial_prefix(target: &Path) -> String { + format!( + ".{}.partial-", + target.file_name().unwrap().to_string_lossy() + ) + } + + // True if any temp sibling for `target` survives in its parent dir. + fn has_partial_leftover(target: &Path) -> bool { + let prefix = partial_prefix(target); + std::fs::read_dir(target.parent().unwrap()) + .unwrap() + .filter_map(|e| e.ok()) + .any(|e| e.file_name().to_string_lossy().starts_with(&prefix)) + } + + // Clone `origin` into `dest` so `dest` carries an `origin` remote that + // `fetch_changes` can pull from — the shape the Copy/Rename arms expect. + fn clone_from_origin(origin: &Path, dest: &Path) { + let url = format!("file://{}", origin.display()); + git2::Repository::clone(&url, dest).unwrap(); + } + + fn head_commit(dir: &Path) -> String { + git2::Repository::open(dir) + .unwrap() + .head() + .unwrap() + .peel_to_commit() + .unwrap() + .id() + .to_string() + } + // A partial-clone temp path is a sibling of the target (not the target // itself), carries the target's basename, and every call is unique -- so it // is never picked up by `clone_dir_ready`/Reuse and never collides with a @@ -1273,7 +1284,47 @@ mod tests { assert_ne!(p1, p2, "each call must produce a distinct temp path"); assert_eq!(p1.parent(), target.parent(), "temp must be a sibling"); let name = p1.file_name().unwrap().to_string_lossy(); - assert!(name.starts_with(&partial_clone_prefix(&target))); + assert!(name.starts_with(&partial_prefix(&target))); + } + + // `promote_clone` moves the temp dir onto an absent target with a single + // rename. This is the atomic step that guarantees `target` is never written + // in place, so a crash before it leaves only a temp dir. + #[tokio::test] + async fn promote_clone_moves_temp_into_absent_target() { + let base = tempfile::tempdir().unwrap(); + let target = base.path().join("clone"); + let tmp = partial_clone_path(&target); + std::fs::create_dir_all(&tmp).unwrap(); + std::fs::write(tmp.join("f"), b"data").unwrap(); + + let out = promote_clone(&mut TestLogger, &tmp, &target).await.unwrap(); + assert_eq!(out, target); + assert!(!tmp.exists(), "temp must be consumed by the rename"); + assert_eq!(std::fs::read(target.join("f")).unwrap(), b"data"); + } + + // On the concurrent-winner race — another build already promoted a complete + // clone to `target` — `promote_clone` must drop our redundant temp and reuse + // the winner's clone untouched, rather than clobbering it or erroring. + #[tokio::test] + async fn promote_clone_reuses_winner_and_drops_temp_on_race() { + let base = tempfile::tempdir().unwrap(); + let target = base.path().join("clone"); + let tmp = partial_clone_path(&target); + std::fs::create_dir_all(&tmp).unwrap(); + std::fs::write(tmp.join("mine"), b"mine").unwrap(); + // The winner's finished (non-empty) clone already sits at target. + std::fs::create_dir_all(&target).unwrap(); + std::fs::write(target.join("winner"), b"winner").unwrap(); + + let out = promote_clone(&mut TestLogger, &tmp, &target).await.unwrap(); + assert_eq!(out, target); + assert!(!tmp.exists(), "our redundant temp must be dropped"); + assert!( + target.join("winner").exists() && !target.join("mine").exists(), + "the winner's clone must be reused untouched" + ); } // A `NewClone` must land at `target_dir` atomically and leave no temp dir @@ -1299,41 +1350,21 @@ mod tests { }; let out = folder.prepare(&mut TestLogger).await.unwrap(); assert_eq!(out, target); - - // A real repo checked out at the requested commit is now at target. - let repo = git2::Repository::open(&target).unwrap(); - assert_eq!( - repo.head() - .unwrap() - .peel_to_commit() - .unwrap() - .id() - .to_string(), - commit - ); - - // No temp sibling survived the successful clone. - let leftovers: Vec<_> = std::fs::read_dir(base.path().join("localhost")) - .unwrap() - .filter_map(|e| e.ok()) - .filter(|e| { - e.file_name() - .to_string_lossy() - .starts_with(&partial_clone_prefix(&target)) - }) - .collect(); + assert_eq!(head_commit(&target), commit); assert!( - leftovers.is_empty(), + !has_partial_leftover(&target), "temp dir must be gone after promotion" ); } - // A `NewClone` whose clone fails must leave *nothing* at `target_dir`, so a - // later build never mistakes a failed attempt for a reusable clone (#2808). + // A `NewClone` whose clone fails must leave *nothing* at `target_dir` (and + // no temp sibling), so a later build never mistakes a failed attempt for a + // reusable clone (#2808). #[tokio::test] async fn new_clone_failure_leaves_no_target_dir() { let base = tempfile::tempdir().unwrap(); let target = base.path().join("localhost").join("a".repeat(40)); + std::fs::create_dir_all(target.parent().unwrap()).unwrap(); // A file:// URL to a path that isn't a git repo -> clone fails. let missing = base.path().join("does-not-exist"); let repo_url = Url::parse(&format!("file://{}", missing.display())).unwrap(); @@ -1351,33 +1382,108 @@ mod tests { !target.exists(), "a failed clone must never leave a dir at the target path" ); + assert!(!has_partial_leftover(&target), "temp must be cleaned up"); } - // The stale-temp sweep removes abandoned temp dirs for the target (age gate - // satisfied by `Duration::ZERO`) while leaving the real target dir and - // unrelated siblings untouched. + // A `CopyAndFetch` must copy + fetch + checkout in a temp sibling and + // promote it into `target_dir` only on full success — never write `.git` + // into `target_dir` in place (#2808). #[tokio::test] - async fn sweep_removes_abandoned_partial_dirs_only() { + async fn copy_and_fetch_promotes_into_target() { let base = tempfile::tempdir().unwrap(); - let dir = base.path().join("localhost"); - std::fs::create_dir_all(&dir).unwrap(); - let target = dir.join("a".repeat(40)); - - let stale = partial_clone_path(&target); - std::fs::create_dir_all(&stale).unwrap(); - std::fs::write(stale.join("x"), b"half").unwrap(); - let unrelated = dir.join("b".repeat(40)); // a real sibling clone dir - std::fs::create_dir_all(&unrelated).unwrap(); - - // max_age = 0: every matching temp counts as stale. - sweep_stale_partial_clones(&mut TestLogger, &target, Duration::ZERO).await; - assert!(!stale.exists(), "abandoned temp must be swept"); - assert!(unrelated.exists(), "unrelated sibling must be kept"); - - // A huge max_age keeps a freshly-created temp (nothing is old enough). - let fresh = partial_clone_path(&target); - std::fs::create_dir_all(&fresh).unwrap(); - sweep_stale_partial_clones(&mut TestLogger, &target, Duration::from_secs(3600)).await; - assert!(fresh.exists(), "a fresh temp must not be swept"); + let origin = base.path().join("origin"); + let commit = init_repo_with_commit(&origin); + // `from` is a prior clone (with an `origin` remote) to be copied. + let from = base.path().join("localhost").join("prev"); + clone_from_origin(&origin, &from); + + let target = base.path().join("localhost").join(&commit); + let folder = GitFolder { + reuse: ReuseOptions::CopyAndFetch { + from: from.clone(), + target_dir: target.clone(), + commit_hash: commit.clone(), + }, + _claim: None, + }; + let out = folder.prepare(&mut TestLogger).await.unwrap(); + assert_eq!(out, target); + assert_eq!(head_commit(&target), commit); + assert!(from.exists(), "the source clone must be left in place"); + assert!(!has_partial_leftover(&target), "temp must be gone"); + } + + // A `CopyAndFetch` whose fetch fails (no reachable `origin`) must leave + // nothing at `target_dir` and no temp sibling behind. + #[tokio::test] + async fn copy_and_fetch_failure_leaves_no_target_dir() { + let base = tempfile::tempdir().unwrap(); + // `from` has a commit but no `origin` remote, so the fetch fails. + let from = base.path().join("localhost").join("prev"); + init_repo_with_commit(&from); + + let target = base.path().join("localhost").join("a".repeat(40)); + let folder = GitFolder { + reuse: ReuseOptions::CopyAndFetch { + from, + target_dir: target.clone(), + commit_hash: "deadbeef".repeat(5), + }, + _claim: None, + }; + assert!(folder.prepare(&mut TestLogger).await.is_err()); + assert!(!target.exists(), "a failed copy+fetch must leave no target"); + assert!(!has_partial_leftover(&target), "temp must be cleaned up"); + } + + // A `RenameAndFetch` must rename the old clone into a temp sibling, fetch + + // checkout there, and promote into `target_dir` only on success. + #[tokio::test] + async fn rename_and_fetch_promotes_into_target() { + let base = tempfile::tempdir().unwrap(); + let origin = base.path().join("origin"); + let commit = init_repo_with_commit(&origin); + let from = base.path().join("localhost").join("prev"); + clone_from_origin(&origin, &from); + + let target = base.path().join("localhost").join(&commit); + let folder = GitFolder { + reuse: ReuseOptions::RenameAndFetch { + from: from.clone(), + target_dir: target.clone(), + commit_hash: commit.clone(), + }, + _claim: None, + }; + let out = folder.prepare(&mut TestLogger).await.unwrap(); + assert_eq!(out, target); + assert_eq!(head_commit(&target), commit); + assert!(!from.exists(), "the source clone is consumed by the rename"); + assert!(!has_partial_leftover(&target), "temp must be gone"); + } + + // A `RenameAndFetch` whose fetch fails must leave nothing at `target_dir` + // and no temp sibling behind (regression for the pre-fix in-place rename). + #[tokio::test] + async fn rename_and_fetch_failure_leaves_no_target_dir() { + let base = tempfile::tempdir().unwrap(); + let from = base.path().join("localhost").join("prev"); + init_repo_with_commit(&from); // no `origin` remote -> fetch fails + + let target = base.path().join("localhost").join("a".repeat(40)); + let folder = GitFolder { + reuse: ReuseOptions::RenameAndFetch { + from, + target_dir: target.clone(), + commit_hash: "deadbeef".repeat(5), + }, + _claim: None, + }; + assert!(folder.prepare(&mut TestLogger).await.is_err()); + assert!( + !target.exists(), + "a failed rename+fetch must leave no target" + ); + assert!(!has_partial_leftover(&target), "temp must be cleaned up"); } }