worktree: isolate concurrent runs#16
Conversation
| /// present, so the worktree dirs don't show up as untracked in | ||
| /// `git status` of the main checkout. | ||
| pub fn sweep_stale(main_repo: &Path, command_label: &str, dest: &VerboseDest) -> Result<()> { | ||
| pub fn sweep_stale(main_repo: &Path, _command_label: &str, dest: &VerboseDest) -> Result<()> { |
There was a problem hiding this comment.
I think we're not properly cleaning stale worktrees now, sweep_stale() only runs git worktree prune, which removes registry entries only when their directories are already missing. In case of a crash/SIGKILL the worktree directory still exists. Can you check if that's the case / fix this? Thanks.
|
Can you add your Signed-off-by line to the commit message? Thanks. |
nirmoy
left a comment
nirmoy
left a comment
There was a problem hiding this comment.
Two cleanup regressions need addressing.
-
sweep_stale()atsrc/worktree.rs:146-159now leaves every existing directory and only runsgit worktree prune --expire=now. Prune retains registered worktrees whose directories still exist, exactly the state left by SIGKILL, a host crash, or failed Drop cleanup. Repeated interrupted build/test runs can therefore retain full worktrees and large in-tree build artifacts indefinitely. A shell reproducer confirmed the worktree remained both present and registered after the new sweep. -
The new layout creates
<command>/<run-id>/<sha>, but Drop removes only the<sha>worktree. Neither Drop nor the revised sweep removes the empty run-ID parent, so every successful boro invocation leaves one directory behind. A normal create/drop reproducer left the parent present.
Please introduce a safe ownership/liveness mechanism for stale run cleanup (for example, a per-run lease/lock plus conservative stale detection), remove empty run parents after successful cleanup, and cover both crash-orphan and normal-drop behavior with tests. Do not reintroduce deletion of another live process's worktree.
Also add the mandatory Signed-off-by trailer.
The three new worktree tests and current CI pass, but they currently assert preservation without covering eventual stale reclamation.
fallintoplace
force-pushed
the
fix/isolate-worktree-runs
branch
from
c99fd51 to
6c719cf
Compare
arighi
left a comment
arighi
left a comment
There was a problem hiding this comment.
Apart from the conflict with the latest main, with this change a cleanup may follow a symlink outside its intended directory and delete unrelated data. Cleanup should reject symlinked directories and verify that every deletion remains within the expected directory tree.
fallintoplace
force-pushed
the
fix/isolate-worktree-runs
branch
from
6c719cf to
35518a4
Compare
Generated worktrees now live under per-run directories so the same command can review, build, or test the same commit concurrently without reusing one shared <command>/<sha> checkout. Each run keeps a lease lock file. sweep_stale() only reclaims runs whose lease can be acquired, which lets us clean up crash-orphaned registered worktrees without deleting another live process's checkout. Unknown no-lock directories are still preserved conservatively. Drop and startup sweep now remove empty run and command directories when cleanup succeeds. The stale sweep also rejects symlinked cleanup paths and only deletes direct child directories under the expected command tree. The tests cover the new path layout, stale registered worktree reclamation, live-run preservation, empty parent cleanup, and the new symlink safety guardrails. Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
fallintoplace
force-pushed
the
fix/isolate-worktree-runs
branch
from
35518a4 to
15bf584
Compare
3 participants
Summary
<command>/<sha>paths.Testing