Skip to content

Commit dd44245

Browse files
authored
Retry mechanisms on Windows for copy_atomic and write_atomic (#10026)
Hello! 🙂 ## Summary After submitting retry mechanisms on scripts installation for windows: #9543 , I noticed that some other functions were using the same `persist` features of temporary files. This could lead to the same issue spotted before (temporary lock by AV/EDR software). I validated that it was possible. So I updated them to go through the same function on Windows, which is using the retry mechanisms if needed. In order to do so, I add to add an async version of the `persist_with_retry`. There is a little trick to make the borrow-checker happy line 306, curious of your opinion on it? This is just a pointer move so it should not induce some performance regression if I'm not mistaking. I also updated them to use `fs_err` on Unix for better error messages. Also, one of the error messages I introduced was badly formatted, I fixed it. 🙂 ## Test Plan The changes should be iso functional and covered with the existing test-suite.
1 parent e65a273 commit dd44245

File tree

1 file changed

+78
-34
lines changed

1 file changed

+78
-34
lines changed

crates/uv-fs/src/lib.rs

Lines changed: 78 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -165,17 +165,7 @@ pub async fn write_atomic(path: impl AsRef<Path>, data: impl AsRef<[u8]>) -> std
165165
.expect("Write path must have a parent"),
166166
)?;
167167
fs_err::tokio::write(&temp_file, &data).await?;
168-
temp_file.persist(&path).map_err(|err| {
169-
std::io::Error::new(
170-
std::io::ErrorKind::Other,
171-
format!(
172-
"Failed to persist temporary file to {}: {}",
173-
path.user_display(),
174-
err.error
175-
),
176-
)
177-
})?;
178-
Ok(())
168+
persist_with_retry(temp_file, path.as_ref()).await
179169
}
180170

181171
/// Write `data` to `path` atomically using a temporary file and atomic rename.
@@ -186,34 +176,14 @@ pub fn write_atomic_sync(path: impl AsRef<Path>, data: impl AsRef<[u8]>) -> std:
186176
.expect("Write path must have a parent"),
187177
)?;
188178
fs_err::write(&temp_file, &data)?;
189-
temp_file.persist(&path).map_err(|err| {
190-
std::io::Error::new(
191-
std::io::ErrorKind::Other,
192-
format!(
193-
"Failed to persist temporary file to {}: {}",
194-
path.user_display(),
195-
err.error
196-
),
197-
)
198-
})?;
199-
Ok(())
179+
persist_with_retry_sync(temp_file, path.as_ref())
200180
}
201181

202182
/// Copy `from` to `to` atomically using a temporary file and atomic rename.
203183
pub fn copy_atomic_sync(from: impl AsRef<Path>, to: impl AsRef<Path>) -> std::io::Result<()> {
204184
let temp_file = tempfile_in(to.as_ref().parent().expect("Write path must have a parent"))?;
205185
fs_err::copy(from.as_ref(), &temp_file)?;
206-
temp_file.persist(&to).map_err(|err| {
207-
std::io::Error::new(
208-
std::io::ErrorKind::Other,
209-
format!(
210-
"Failed to persist temporary file to {}: {}",
211-
to.user_display(),
212-
err.error
213-
),
214-
)
215-
})?;
216-
Ok(())
186+
persist_with_retry_sync(temp_file, to.as_ref())
217187
}
218188

219189
#[cfg(windows)]
@@ -311,6 +281,80 @@ pub fn rename_with_retry_sync(
311281
}
312282
}
313283

284+
/// Persist a `NamedTempFile`, retrying (on Windows) if it fails due to transient operating system errors, in a synchronous context.
285+
pub async fn persist_with_retry(
286+
from: NamedTempFile,
287+
to: impl AsRef<Path>,
288+
) -> Result<(), std::io::Error> {
289+
#[cfg(windows)]
290+
{
291+
// On Windows, antivirus software can lock files temporarily, making them inaccessible.
292+
// This is most common for DLLs, and the common suggestion is to retry the operation with
293+
// some backoff.
294+
//
295+
// See: <https://github.com/astral-sh/uv/issues/1491> & <https://github.com/astral-sh/uv/issues/9531>
296+
let to = to.as_ref();
297+
298+
// the `NamedTempFile` `persist` method consumes `self`, and returns it back inside the Error in case of `PersistError`
299+
// https://docs.rs/tempfile/latest/tempfile/struct.NamedTempFile.html#method.persist
300+
// So we will update the `from` optional value in safe and borrow-checker friendly way every retry
301+
// Allows us to use the NamedTempFile inside a FnMut closure used for backoff::retry
302+
let mut from = Some(from);
303+
304+
let backoff = backoff_file_move();
305+
let persisted = backoff::future::retry(backoff, move || {
306+
// Needed because we cannot move out of `from`, a captured variable in an `FnMut` closure, and then pass it to the async move block
307+
let mut from = from.take();
308+
309+
async move {
310+
if let Some(file) = from.take() {
311+
file.persist(to).map_err(|err| {
312+
let error_message = err.to_string();
313+
warn!(
314+
"Retrying to persist temporary file to {}: {}",
315+
to.display(),
316+
error_message
317+
);
318+
319+
// Set back the NamedTempFile returned back by the Error
320+
from = Some(err.file);
321+
322+
backoff::Error::transient(std::io::Error::new(
323+
std::io::ErrorKind::Other,
324+
format!(
325+
"Failed to persist temporary file to {}: {}",
326+
to.display(),
327+
error_message
328+
),
329+
))
330+
})
331+
} else {
332+
Err(backoff::Error::permanent(std::io::Error::new(
333+
std::io::ErrorKind::Other,
334+
format!(
335+
"Failed to retrieve temporary file while trying to persist to {}",
336+
to.display()
337+
),
338+
)))
339+
}
340+
}
341+
})
342+
.await;
343+
344+
match persisted {
345+
Ok(_) => Ok(()),
346+
Err(err) => Err(std::io::Error::new(
347+
std::io::ErrorKind::Other,
348+
err.to_string(),
349+
)),
350+
}
351+
}
352+
#[cfg(not(windows))]
353+
{
354+
async { fs_err::rename(from, to) }.await
355+
}
356+
}
357+
314358
/// Persist a `NamedTempFile`, retrying (on Windows) if it fails due to transient operating system errors, in a synchronous context.
315359
pub fn persist_with_retry_sync(
316360
from: NamedTempFile,
@@ -369,7 +413,7 @@ pub fn persist_with_retry_sync(
369413
Ok(_) => Ok(()),
370414
Err(err) => Err(std::io::Error::new(
371415
std::io::ErrorKind::Other,
372-
format!("{err:?}"),
416+
err.to_string(),
373417
)),
374418
}
375419
}

0 commit comments

Comments
 (0)