-
Notifications
You must be signed in to change notification settings - Fork 232
fix(copy): skip source files that vanish mid-copy instead of aborting #3744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -42,7 +42,9 @@ static COPY_POOL: LazyLock<rayon::ThreadPool> = LazyLock::new(|| { | |||||
| /// | ||||||
| /// Detects symlinks via `symlink_metadata` on the source. Returns `Some(bytes)` | ||||||
| /// when the entry was copied (reporting the source's logical byte size), or | ||||||
| /// `None` if skipped because the destination already exists. When `force` is | ||||||
| /// `None` if skipped because the destination already exists, or because the | ||||||
| /// source vanished after the caller's directory walk collected it (e.g. a | ||||||
| /// concurrent build deleting/replacing a build artifact). When `force` is | ||||||
| /// true, existing entries are removed before copying. | ||||||
| /// | ||||||
| /// When `root` is `Some`, refuses destination paths whose parent resolves | ||||||
|
|
@@ -67,15 +69,31 @@ pub fn copy_leaf( | |||||
| return Ok(None); | ||||||
| } | ||||||
|
|
||||||
| let src_meta = src | ||||||
| .symlink_metadata() | ||||||
| .with_context(|| format!("reading metadata for {}", src.display()))?; | ||||||
| let src_meta = match src.symlink_metadata() { | ||||||
| Ok(meta) => meta, | ||||||
| // The source can vanish between the caller's directory walk and this | ||||||
| // copy — e.g. a concurrent build rewriting `target/`. Skip rather | ||||||
| // than fail the whole batch over one file that's no longer there. | ||||||
| Err(e) if e.kind() == ErrorKind::NotFound => return Ok(None), | ||||||
| Err(e) => { | ||||||
| return Err( | ||||||
| anyhow::Error::from(e).context(format!("reading metadata for {}", src.display())) | ||||||
| ); | ||||||
| } | ||||||
| }; | ||||||
| let is_symlink = src_meta.file_type().is_symlink(); | ||||||
| let bytes = src_meta.len(); | ||||||
|
|
||||||
| if is_symlink { | ||||||
| let target = | ||||||
| fs::read_link(src).with_context(|| format!("reading symlink {}", src.display()))?; | ||||||
| let target = match fs::read_link(src) { | ||||||
| Ok(target) => target, | ||||||
| Err(e) if e.kind() == ErrorKind::NotFound => return Ok(None), | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the arm above — under
Suggested change
|
||||||
| Err(e) => { | ||||||
| return Err( | ||||||
| anyhow::Error::from(e).context(format!("reading symlink {}", src.display())) | ||||||
| ); | ||||||
| } | ||||||
| }; | ||||||
| create_symlink(&target, src, dest)?; | ||||||
| } else { | ||||||
| match reflink_copy::reflink_or_copy(src, dest) { | ||||||
|
|
@@ -100,6 +118,7 @@ pub fn copy_leaf( | |||||
| } | ||||||
| } | ||||||
| Err(e) if e.kind() == ErrorKind::AlreadyExists => return Ok(None), | ||||||
| Err(e) if e.kind() == ErrorKind::NotFound => return Ok(None), | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning: with Worth noting this arm is broader than the vanished-source case it's meant for —
Suggested change
|
||||||
| Err(e) => { | ||||||
| return Err(anyhow::Error::from(e).context(format!("copying {}", src.display()))); | ||||||
| } | ||||||
|
|
@@ -263,4 +282,19 @@ mod tests { | |||||
| let dir = std::env::temp_dir(); | ||||||
| assert!(remove_if_exists(&dir).is_err()); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn test_copy_leaf_skips_vanished_source() { | ||||||
| // Simulates a source file that existed during the caller's directory | ||||||
| // walk but is gone by copy time (e.g. a concurrent build rewriting | ||||||
| // `target/`). Should be skipped, not treated as a fatal error. | ||||||
| let dest_dir = tempfile::tempdir().unwrap(); | ||||||
| let src = dest_dir.path().join("does-not-exist"); | ||||||
| let dest = dest_dir.path().join("dest"); | ||||||
|
|
||||||
| let result = copy_leaf(&src, &dest, None, false).unwrap(); | ||||||
|
|
||||||
| assert_eq!(result, None); | ||||||
| assert!(!dest.exists()); | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
force,remove_if_exists(dest)has already run by the time we get here, so returningOk(None)leaves the destination deleted and reports a skip. Gate the skip on!forceso the batch-resilience win applies where it's needed without silently dropping a file the user asked to overwrite: