fix(core/build): atomically promote git clones so a crashed build can self-heal

[phil-opp]

Summary

Fixes #2808.

ReuseOptions::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 the Reuse arm no longer deletes a clone whose HEAD fails to resolve (correctly, to avoid re-opening #2711 for a concurrently-written clone). But that turned a crash leftover into a permanent wedge: every subsequent build of the same repo@commit re-enters the Reuse arm, fails to resolve HEAD, and bail!s without cleanup — with no owning process left to recover it, only a manual rm -rf unblocks the build. For a branch/tag pin the outcome differs but is also bad: the HEAD check is skipped, so the broken checkout is silently reused.

Fix

Fix the regression at its root rather than restoring the risky delete-on-Err:

• Clone + checkout into a unique temporary sibling dir and rename it into clone_dir only after the checkout fully succeeds. rename within the same parent directory is atomic, so a crash only ever leaves a temp dir — never a directory at clone_dir. Any directory that exists at clone_dir is therefore, by construction, a complete clone, and clone_dir_ready / the Reuse arm can trust it again.
• Temp names are pid+counter unique, so no build ever writes into or reclaims another live build's temp dir — this keeps the change cross-process safe and does not re-open git clone reuse (#2482): cross-session HEAD-verify can remove_dir_all a directory a concurrent build session is still cloning into #2711.
• A best-effort, age-gated sweep reclaims temp dirs abandoned by crashed builds (only dirs older than 1h, which no live clone can be) so leftovers don't accumulate on disk.
• The git2::Repository handle is dropped before the rename so Windows (which refuses to rename a dir with open handles) is not broken.

This also fixes the branch/tag-pin variant, since a half-written checkout can no longer exist at clone_dir in the first place.

Tests

New unit tests in libraries/core/src/build/git.rs:

• new_clone_promotes_temp_into_target_and_cleans_up — clone lands at the target atomically, is a valid repo on the requested commit, and no temp sibling survives.
• new_clone_failure_leaves_no_target_dir — a failed clone leaves nothing at the target path (the property that prevents the wedge).
• partial_clone_path_is_a_unique_sibling — temp paths are distinct siblings, never the target.
• sweep_removes_abandoned_partial_dirs_only — the sweep removes stale temps and leaves the target / unrelated siblings alone.

All 16 git:: tests pass; cargo fmt and cargo clippy -p dora-core --features build --all-targets -- -D warnings are clean.

---

Generated by Claude Code

[trunk-io]

😎 Merged successfully - details.

[phil-opp]

Automated review by Claude — this is a fully automated review; no human has vetted it before posting.

No issues found. Cloning into a unique <pid>-<counter> temp sibling and renaming into place only after a successful checkout is a sound way to guarantee that any directory present at clone_dir is a complete clone, so a crashed build can self-heal on the next run. The concurrent-winner race (rename onto an already-populated target) is handled by dropping the local temp and reusing the existing clone, the git2::Repository is dropped before the rename (Windows handle safety), and the age-gated sweep won't touch a live clone because the in-progress temp dir's mtime stays fresh.

The tests cover the key invariants: new_clone_failure_leaves_no_target_dir (wedge prevention), sweep_removes_abandoned_partial_dirs_only (age gate in both directions), and partial_clone_path_is_a_unique_sibling (uniqueness / same-parent).

---

Generated by Claude Code

[phil-opp]

🤖 Automated review by Claude — fully automated, no human in the loop; it may contain mistakes.

The temp-dir + atomic-rename approach looks like the right root-cause fix — any dir at target_dir being complete-by-construction cleanly closes the #2808 wedge. Two non-blocking edge cases on the housekeeping side that might be worth a note:

1. The stale-sweep's age gate can misjudge a very long live clone. The comment on sweep_stale_partial_clones reasons that a temp being actively cloned into is "necessarily younger than a completed checkout." That holds for the temp dir's own mtime only while its top-level entries are being written — but git clone spends the bulk of its time writing into .git/objects/… (a subdir), which doesn't bump the top-level dir's mtime. For a large repo / slow network whose fetch exceeds PARTIAL_CLONE_MAX_AGE (1h), the top-level temp mtime can be >1h old while the clone is still live, and since the sweep matches every pid's .<commit>.partial-*, a second process cloning the same commit could remove_dir_all a live temp. It's self-healing (the interrupted clone just fails and retries, nothing lands at target_dir), but it does undercut the invariant the comment states. A pid-liveness check, or touching the temp dir periodically, would make the gate match the claim.

2. The sweep only runs in the NewClone arm. Once a good clone exists at target_dir, every later build of that repo@commit takes the Reuse arm and never sweeps — so a .<commit>.partial-* leftover from a concurrent build that crashed (while another finished the clone) never gets reclaimed for that commit. Minor, but it narrows the "so they don't accumulate on disk" guarantee to commits that still have no successful clone.

Neither blocks the fix.

---

Generated by Claude Code

[phil-opp]

Automated review by Claude — this is a fully automated review, not vetted by a human. I looked at the diff and the surrounding code, not the PR description.

I found one correctness gap: the atomic-promotion invariant only holds for the NewClone path, but the change documents it as a global property.

The temp-dir + atomic-rename approach is sound for NewClone — the temp is a sibling of target_dir (same filesystem, so rename is genuinely atomic), the concurrent-winner ENOTEMPTY reuse is handled, and the Repository is dropped before the rename for Windows. The tests cover that path well.

But the comment the change adds asserts:

Any directory that does exist at target_dir is, by construction, a complete checkout.

That isn't true for the other two arms that write clone_dir, and they're untouched here:

• CopyAndFetch calls std::fs::create_dir_all(&to) with to = target_dir and then copies content + fetches into that same directory — non-atomically. If the build is SIGKILLed / loses power during the copy phase (the exact threat model this PR targets), a partial directory with an incomplete .git is left sitting at target_dir. The next build then hits clone_dir_ready → dir.exists() → the Reuse arm, where git2::Repository::open / HEAD resolution fails, landing on the Err(err) => bail!(...) branch that intentionally leaves the dir in place (the git clone reuse (#2482): cross-session HEAD-verify can remove_dir_all a directory a concurrent build session is still cloning into #2711 tradeoff). That's precisely the permanent core/build: #2795 leaves a crashed build's half-written git clone permanently un-recoverable (Reuse HEAD-check bails without cleanup) #2808 wedge, repeating on every rebuild until someone manually removes the cache dir. (A crash during the fetch/checkout phase instead leaves a resolvable HEAD on the old commit, which the Reuse arm self-heals via the Ok(h)-mismatch cleanup — so the vulnerable window is specifically the copy phase.)
• RenameAndFetch is mostly self-healing for the same reason (its leftover is a complete clone at the previous commit), but a crash mid-checkout_tree can still corrupt HEAD into the same leave-in-place bail.

So the fix closes the wedge for NewClone but leaves CopyAndFetch exposed to the identical failure it means to eliminate "at the root," and the documented invariant over-claims. Consider routing CopyAndFetch (and the fetch/checkout mutation in RenameAndFetch) through the same temp-dir + atomic-rename promotion so the "complete-by-construction" property actually holds for every path that writes clone_dir.

Two prior points from earlier reviews still stand and aren't addressed by the current diff: the stale-sweep age gate can misjudge a very long live clone (the top-level temp mtime isn't bumped by writes into .git/objects/…), and the sweep only runs in the NewClone arm so .partial-* leftovers are never reclaimed once a good clone exists at target_dir.

---

Generated by Claude Code

[phil-opp]

The mechanics are right: the temp dir is a sibling so the rename is same-device and atomic, git2::Repository is dropped inside the spawn_blocking closure before the rename (Windows-safe), the concurrent-winner race is handled, and every error path calls cleanup_failed_clone.

The gap is scope, not mechanism. The invariant this writes into the code — "any directory that exists at clone_dir is, by construction, a complete clone" — only holds for NewClone. CopyAndFetch still does create_dir_all and copies in place (libraries/core/src/build/git.rs:314), and RenameAndFetch renames the old clone straight onto target_dir (git.rs:357). SIGKILL mid-fs_extra::dir::copy still leaves a partial .git at clone_dir, the Reuse arm's HEAD read fails, and you get the exact #2808 permanent wedge. Either extend temp-then-promote to those two arms, or narrow the claim in the comment.

Two smaller things:

• None of the four new tests goes RED without the fix — new_clone_failure_leaves_no_target_dir and new_clone_promotes_temp_into_target_and_cleans_up both pass pre-fix, because the old code already called cleanup_failed_clone(&target_dir). The actual property (crash mid-clone) is untested.
• sweep_stale_partial_clones reasons that "a live clone is necessarily younger" from the temp dir's top-level mtime, which git does not touch during the fetch phase (objects land under .git/). So a clone running longer than an hour can be remove_dir_all'd out from under a concurrent build of the same commit.

[phil-opp]

Thanks — all three are fair. Pushed a follow-up commit that addresses them:

1. Scope — extended temp-then-promote to all write arms. Factored the atomic promotion into a shared promote_clone(tmp, target) and routed CopyAndFetch and RenameAndFetch through a temp sibling too:

• CopyAndFetch now create_dir_all + copies + fetches + checks out in the temp dir, then promotes.
• RenameAndFetch renames the old clone into the temp dir (not straight onto target_dir), fetches + checks out there, then promotes.

So the "any dir at clone_dir is a complete clone" invariant now holds for every arm — a SIGKILL mid-fs_extra::dir::copy or mid-fetch leaves a temp dir, never a broken .git at clone_dir.

2. Racy sweep — removed entirely. You're right that the top-level mtime doesn't advance while git writes objects under .git/, so a long clone could be reclaimed out from under a concurrent build. I dropped sweep_stale_partial_clones rather than try to patch the heuristic. Orphaned temp dirs from a hard crash are harmless (unique names, never matched by clone_dir_ready/Reuse) and are left for external cleanup; a safe reclaimer would need a cross-process liveness signal (advisory lock / pid-liveness), which is out of scope here.

3. Tests. New tests that genuinely exercise the mechanism:

• promote_clone unit tests: absent-target move, and the concurrent-winner race (temp dropped, winner's clone reused untouched) — both red without the new code path.
• End-to-end promote tests for CopyAndFetch and RenameAndFetch via a local origin remote, asserting the checkout lands at target_dir and no temp sibling survives.
• Failure-path tests for all three arms asserting no target_dir and no temp leftover.

On the honest limit you flagged: a true SIGKILL-mid-clone can't be reproduced in an in-process unit test, because every in-process error path runs cleanup_failed_clone, which masks the difference between writing-in-place and writing-to-temp. That's exactly why the fix makes the invariant hold structurally — target_dir is only ever created by the final rename — rather than relying on cleanup. The promote_clone and per-arm tests lock that structural boundary.

The follow-up also merged cleanly with #2820 on the rebased base: a panic in the blocking clone/checkout task now routes through the temp-dir cleanup + bail! path.

---

Generated by Claude Code

[github-actions]

@phil-opp the Trunk merge queue failed for this PR.

See the Trunk merge-status comment for details.

Posted as a new comment so GitHub sends an email — Trunk's sticky comment is edited in place and won't trigger a notification.

[phil-opp]

Heads up on the merge-queue failure (error[E0063]: missing field \ready_barrier_released` in `state::RunningDataflow`atbinaries/coordinator/src/handlers.rs:911): that wasn't from this PR — it only touches libraries/core/src/build/git.rs. The branch had been rebased onto an older mainsnapshot (beforeready_barrier_releasedwas added to the coordinator), so the queue's merge with currentmain` produced a stale-base semantic conflict in the coordinator.

Fixed by rebasing the branch onto the latest origin/main (fb8c4e9) and force-pushing. Verified locally on rustc 1.95.0: dora-coordinator compiles (E0063 gone) and the 22 dora-core build::git tests pass. No code changes beyond the rebase.

---

Generated by Claude Code

[phil-opp]

🤖 Fully automated review by Claude — this review was generated end-to-end by an automated agent with no human vetting. Treat it accordingly.

The follow-up commit (b8a8383) resolves the scope gap from the earlier pass: promote_clone is now the shared tail of all three write arms, so target_dir is only ever created by the final atomic rename and the "any dir at clone_dir is a complete clone" invariant now holds for CopyAndFetch and RenameAndFetch, not just NewClone. The concurrent-winner reuse (Err(_) if target.exists()), the drop-Repository-before-rename ordering (Windows-safe), and the per-arm cleanup_failed_clone all look right, and the new promote_clone unit tests plus per-arm promote/failure tests exercise the real mechanism (the race test is genuinely red without the new path). The racy sweep_stale_partial_clones is gone from the net diff with no dangling caller.

Two small leftovers, neither blocking:

• Removing the sweep means temp dirs orphaned by a hard crash/SIGKILL now stay in the clone cache indefinitely (unique-named, never reused). That's a reasonable trade vs. the racy reclaimer, but it drops the earlier "so they don't accumulate on disk" guarantee — worth a note that external cleanup is now expected.
• The PR description still says "a best-effort, age-gated sweep reclaims temp dirs," which no longer matches the code now that the sweep is removed.

No new blocking issues.

---

Generated by Claude Code