fix(relocate): atomic no-overwrite rename for --clobber backup#2865
Merged
Conversation
`wt step relocate --clobber` backed up a blocking destination by checking `backup_path.exists()` and then calling `std::fs::rename`. The backup name is only second-resolution (`.bak-<YYYYmmdd-HHMMSS>`), so a process could create that path between the check and the rename — and `std::fs::rename` silently replaces an existing file or empty directory on Unix, destroying the just-created backup. A time-of-check/time-of-use race that defeats the intended fail-closed behavior. The backup now moves with `renamore::rename_exclusive` — an atomic no-overwrite rename (`renameat2(RENAME_NOREPLACE)` on Linux, `renamex_np(RENAME_EXCL)` on macOS, `MoveFileExW` on Windows). An `AlreadyExists` error maps to the existing fail-closed "Backup path already exists" error, so the command still refuses rather than overwriting. The atomic rename — not a pre-check — is what actually closes the race; `renamore` keeps the platform FFI and its `unsafe` inside the crate, so worktrunk stays `unsafe_code = "forbid"`. `renamore` is already a direct dependency, used the same way on the `wt switch --clobber` path (#2849). Adds a unit test asserting the rename refuses to replace an existing directory and leaves both source and destination intact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
worktrunk-bot
left a comment
worktrunk-bot
left a comment
Collaborator
There was a problem hiding this comment.
The swap to renamore::rename_exclusive is the right call — mirrors the wt switch --clobber path in src/commands/worktree/resolve.rs and closes the same TOCTOU window. One coverage gap on the non-AlreadyExists error arm; suggested test mirrors test_back_up_clobbered_path_errors_when_source_missing in worktree/resolve.rs.
`backup_blocking_path` had a test for the `AlreadyExists` fail-closed path but none for the generic `Err(err)` arm, leaving that line as a codecov/patch miss. Add a test that calls the function with a non-existent source: `renamore::rename_exclusive` returns a `NotFound` error, which flows through the generic arm and surfaces the "Failed to backup" context. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 21, 2026
`wt switch --clobber` and `wt step relocate --clobber` each had their own backup-the-blocker logic. They diverged: switch's `back_up_clobbered_path` iterates suffixes (`…-2`, `…-3`, …) on a name collision so it never fails, while relocate's `backup_blocking_path` failed closed on `AlreadyExists`. A relocate run could abort on a same-second clobber that switch handles gracefully. Both now go through `commands::backup` — `back_up_clobbered_path_now` computes the timestamp suffix and delegates to the suffix-iterating `back_up_clobbered_path`. relocate gains the suffix fallback, the timestamp computation is no longer duplicated across the two call sites, and `renamore::rename_exclusive` has a single caller. relocate's backup name changes from `.bak-<timestamp>` to the extension-aware `.bak.<timestamp>` form (e.g. `repo.feature` → `repo.feature.bak.<timestamp>`), matching switch. The `test_relocate_clobber_error_backup_exists` regression test asserted the old fail-closed behavior; it is rewritten as `test_relocate_clobber_falls_back_when_backup_taken` to assert the corrected `-N` fallback, mirroring the switch test. CLI help and the generated doc mirrors are updated for the new naming and fallback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
worktrunk-bot
approved these changes
May 21, 2026
max-sixty
added a commit
that referenced
this pull request
May 23, 2026
`wt switch --clobber` and `wt step relocate --clobber` each had their own logic for backing up a path blocking the target, and the two had diverged. relocate used an `exists()` check followed by `std::fs::rename` — a time-of-check/time-of-use race, since `std::fs::rename` silently replaces an existing file or empty directory on Unix and could destroy a just-created backup. switch (#2849) instead used `renamore::rename_exclusive`, an atomic no-overwrite rename, and counted up through suffixes (`…-2`, `…-3`, …) on a name collision rather than failing. This PR replaces both with one shared helper in `commands::backup`. `back_up_clobbered_path_now` computes the timestamped name and delegates to the suffix-iterating `back_up_clobbered_path`, which moves the blocker with `renamore::rename_exclusive` — atomic and no-overwrite, so an existing backup is never clobbered; a name collision just lands on the next free `-N` name. relocate gains the suffix fallback (previously it failed closed on `AlreadyExists`), the timestamp computation is no longer duplicated across the two call sites, and `renamore::rename_exclusive` now has a single caller. `renamore` is already a direct dependency and keeps the platform FFI and its `unsafe` inside that crate, so worktrunk stays `unsafe_code = "forbid"`. relocate's backup name changes from `.bak-<timestamp>` to the extension-aware `.bak.<timestamp>` form used by switch (e.g. `repo.feature` → `repo.feature.bak.<timestamp>`). The `test_relocate_clobber_error_backup_exists` regression test asserted the old fail-closed behavior and is rewritten as `test_relocate_clobber_falls_back_when_backup_taken`, mirroring the switch test. CLI help and the generated doc mirrors are updated for the new naming and fallback. The shared helper carries the suffix-iteration, collision, and missing-source unit tests (moved from `resolve.rs`); the relocate `--clobber` path keeps its integration coverage. > _This was written by Claude Code on behalf of Maximilian Roos_ --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wt switch --clobberandwt step relocate --clobbereach had their own logic for backing up a path blocking the target, and the two had diverged. relocate used anexists()check followed bystd::fs::rename— a time-of-check/time-of-use race, sincestd::fs::renamesilently replaces an existing file or empty directory on Unix and could destroy a just-created backup. switch (#2849) instead usedrenamore::rename_exclusive, an atomic no-overwrite rename, and counted up through suffixes (…-2,…-3, …) on a name collision rather than failing.This PR replaces both with one shared helper in
commands::backup.back_up_clobbered_path_nowcomputes the timestamped name and delegates to the suffix-iteratingback_up_clobbered_path, which moves the blocker withrenamore::rename_exclusive— atomic and no-overwrite, so an existing backup is never clobbered; a name collision just lands on the next free-Nname. relocate gains the suffix fallback (previously it failed closed onAlreadyExists), the timestamp computation is no longer duplicated across the two call sites, andrenamore::rename_exclusivenow has a single caller.renamoreis already a direct dependency and keeps the platform FFI and itsunsafeinside that crate, so worktrunk staysunsafe_code = "forbid".relocate's backup name changes from
.bak-<timestamp>to the extension-aware.bak.<timestamp>form used by switch (e.g.repo.feature→repo.feature.bak.<timestamp>). Thetest_relocate_clobber_error_backup_existsregression test asserted the old fail-closed behavior and is rewritten astest_relocate_clobber_falls_back_when_backup_taken, mirroring the switch test. CLI help and the generated doc mirrors are updated for the new naming and fallback.The shared helper carries the suffix-iteration, collision, and missing-source unit tests (moved from
resolve.rs); the relocate--clobberpath keeps its integration coverage.