fix: retry rename on Windows transient file locks during install#10300
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFilesystem rename operations now use an internal ChangesFilesystem Rename Retry Mechanism
Sequence Diagram(s)sequenceDiagram
participant Caller
participant rename
participant do_rename
participant fs_rename
Caller->>rename: rename(from, to)
rename->>do_rename: do_rename(from, to)
do_rename->>fs_rename: fs::rename(from, to)
fs_rename-->>do_rename: Ok / Err(raw_os_error)
do_rename->>do_rename: retry on 5 or 32 with exponential backoff
do_rename-->>rename: Ok / Err(last_error)
rename-->>Caller: Result (with existing error wrapping)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR wraps
Confidence Score: 5/5Safe to merge — the change is Windows-only and isolated to a single helper; non-Windows builds compile through the trivial one-liner branch. The retry logic is small and self-contained: the back-off guard correctly avoids a pointless final sleep, non-retried errors are returned immediately without delay, and the unwrap() on last_err cannot panic. No behaviour changes on Linux or macOS. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(install): retry rename on Windows tr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/file.rs (1)
169-187: 💤 Low valueConsider adding trace logging for retry attempts.
The shims module logs when it encounters locked files (
trace!("cannot delete locked shim...")inshims.rs). Adding similar logging here would aid debugging when installs are slow due to antivirus interference:🔧 Suggested logging addition
#[cfg(windows)] fn do_rename(from: &Path, to: &Path) -> std::io::Result<()> { const MAX_ATTEMPTS: u32 = 5; let mut last_err = None; for attempt in 0..MAX_ATTEMPTS { match fs::rename(from, to) { Ok(()) => return Ok(()), Err(e) if matches!(e.raw_os_error(), Some(5) | Some(32)) => { // ERROR_ACCESS_DENIED (5) or ERROR_SHARING_VIOLATION (32): // likely a transient lock from antivirus or the OS. // Exponential backoff: 50ms, 100ms, 200ms, 400ms, 800ms + trace!( + "rename {} -> {} failed with OS error {}, retrying ({}/{})", + from.display(), + to.display(), + e.raw_os_error().unwrap_or(-1), + attempt + 1, + MAX_ATTEMPTS + ); last_err = Some(e); std::thread::sleep(std::time::Duration::from_millis(50 * (1 << attempt))); } Err(e) => return Err(e), } } Err(last_err.unwrap()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/file.rs` around lines 169 - 187, The do_rename function retries on Windows but doesn't log retry attempts; add trace-level logging inside the Err(e) branch that matches ERROR_ACCESS_DENIED/ERROR_SHARING_VIOLATION so each retry reports the attempt number, the from and to paths, the error and the backoff duration (use the same 50ms * (1 << attempt) calculation) using the trace! macro (referencing do_rename, the loop's attempt variable, from and to Path values, and the local e) and also add a final trace or debug log before returning Err(last_err.unwrap()) to record the ultimate failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/file.rs`:
- Around line 169-187: The do_rename function retries on Windows but doesn't log
retry attempts; add trace-level logging inside the Err(e) branch that matches
ERROR_ACCESS_DENIED/ERROR_SHARING_VIOLATION so each retry reports the attempt
number, the from and to paths, the error and the backoff duration (use the same
50ms * (1 << attempt) calculation) using the trace! macro (referencing
do_rename, the loop's attempt variable, from and to Path values, and the local
e) and also add a final trace or debug log before returning
Err(last_err.unwrap()) to record the ultimate failure.
jhult
force-pushed
the
fix/retry-rename-on-windows
branch
2 times, most recently
from
58e5028 to
6705cd4
Compare
When installing core tools like node on Windows, mise extracts archives to a temp directory then renames into place. Windows/antivirus can briefly lock files in the extracted directory, causing fs::rename to fail with ERROR_ACCESS_DENIED (5) or ERROR_SHARING_VIOLATION (32). This is the same class of transient lock that the shims module already handles for locked .exe files. jdx#4528
jhult
force-pushed
the
fix/retry-rename-on-windows
branch
from
6705cd4 to
20c91d2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/file.rs (1)
169-194: 💤 Low valueConsider adding trace logging on retry attempts.
When transient lock failures trigger retries, there's no visibility into the retry loop. Adding a
trace!log (similar to the pattern inshims.rsat line 271) would help diagnose intermittent Windows installation failures.🔧 Suggested trace logging
Err(e) if matches!(e.raw_os_error(), Some(5) | Some(32)) => { // ERROR_ACCESS_DENIED (5) or ERROR_SHARING_VIOLATION (32): // likely a transient lock from antivirus or the OS. // Exponential backoff: 50ms, 100ms, 200ms, 400ms, 800ms last_err = Some(e); if attempt + 1 < MAX_ATTEMPTS { + trace!( + "rename {} -> {} blocked by transient lock, retrying ({}/{})", + from.display(), + to.display(), + attempt + 1, + MAX_ATTEMPTS + ); std::thread::sleep(std::time::Duration::from_millis(50 * (1 << attempt))); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/file.rs` around lines 169 - 194, Add trace-level logging inside the Windows retry branch of do_rename to surface each transient retry: in the Err(e) if matches!(e.raw_os_error(), Some(5) | Some(32)) arm (inside fn do_rename), call trace! with the attempt number, MAX_ATTEMPTS, the error (e), and the from/to paths and computed backoff duration (follow the trace pattern used in shims.rs around the retry on line 271) before sleeping so each retry and its reason are visible for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/file.rs`:
- Around line 169-194: Add trace-level logging inside the Windows retry branch
of do_rename to surface each transient retry: in the Err(e) if
matches!(e.raw_os_error(), Some(5) | Some(32)) arm (inside fn do_rename), call
trace! with the attempt number, MAX_ATTEMPTS, the error (e), and the from/to
paths and computed backoff duration (follow the trace pattern used in shims.rs
around the retry on line 271) before sleeping so each retry and its reason are
visible for diagnostics.
2 participants
Problem:
file::renameon Windows fails withAccess is denied (os error 5)during tool installation (see #4528). When installing core tools likenode,miseextracts an archive to a temp directory and thenfs::renamesit into place. Antivirus (Windows Defender) can hold handles on recently-extracted files, causing the rename to fail transiently.Fix: The
file::renamefunction now retries up to 5 times on Windows when the error isERROR_ACCESS_DENIED (5)orERROR_SHARING_VIOLATION (32), with exponential backoff (50ms → 800ms). These are the same transient lock codes that the shims module already handles for locked.exefiles .The
move_filefunction also benefits since it now calls the retry wrapper instead of rawfs::rename.On non-Windows, behavior is unchanged - a single
fs::renamecall as before.Summary by CodeRabbit