diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index a672d00e7..2f3dc4912 100644 --- a/crates/catalog/src/store.rs +++ b/crates/catalog/src/store.rs @@ -73,6 +73,16 @@ impl FsStore { } } +/// A new staging file at `tmp`: whatever is already there (a leftover from a crashed save, or a link) is removed first +/// (a link itself, never what it points to), then the file is created exclusively, so a write never follows a link +/// planted at that name into another file (issue #1502). +fn create_staging(tmp: &std::path::Path) -> io::Result { + if std::fs::symlink_metadata(tmp).is_ok() { + std::fs::remove_file(tmp)?; + } + std::fs::OpenOptions::new().write(true).create_new(true).open(tmp) +} + impl Store for FsStore { fn read(&mut self, name: &str) -> io::Result>> { match std::fs::read(self.path(name)) { @@ -89,7 +99,7 @@ impl Store for FsStore { } let tmp = self.path(&format!("{name}.tmp")); { - let mut f = std::fs::File::create(&tmp)?; + let mut f = create_staging(&tmp)?; f.write_all(data)?; f.sync_all()?; } @@ -105,7 +115,7 @@ impl Store for FsStore { } let tmp = self.path(&format!("{name}.tmp")); let written = (|| { - let mut w = Counter { inner: io::BufWriter::with_capacity(1 << 20, std::fs::File::create(&tmp)?), n: 0 }; + let mut w = Counter { inner: io::BufWriter::with_capacity(1 << 20, create_staging(&tmp)?), n: 0 }; fill(&mut w)?; w.flush()?; let f = w.inner.into_inner().map_err(|e| e.into_error())?; @@ -231,3 +241,29 @@ impl Store for MemStore { self.background.then(|| Box::new(self.clone()) as Box) } } + +#[cfg(test)] +mod tests_staging { + use super::*; + + /// A link planted at a staging name (`.tmp`) is removed, never followed: the file it points to keeps its + /// bytes and the save still lands (issue #1502). + #[cfg(unix)] + #[test] + fn a_link_at_the_staging_name_is_never_written_through() { + let dir = std::env::temp_dir().join(format!("lc-staging-link-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let mut store = FsStore::open(&dir).unwrap(); + let victim = dir.join("victim.txt"); + std::fs::write(&victim, b"keep").unwrap(); + std::os::unix::fs::symlink(&victim, dir.join("catalog.snap.tmp")).unwrap(); + store.write_atomic("catalog.snap", b"{}").unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + assert_eq!(std::fs::read(dir.join("catalog.snap")).unwrap(), b"{}"); + std::os::unix::fs::symlink(&victim, dir.join("catalog.log.tmp")).unwrap(); + store.write_atomic_with("catalog.log", &mut |w| w.write_all(b"[1]")).unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + assert_eq!(std::fs::read(dir.join("catalog.log")).unwrap(), b"[1]"); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/fetch/src/lib.rs b/crates/fetch/src/lib.rs index c5c96a902..12daff541 100644 --- a/crates/fetch/src/lib.rs +++ b/crates/fetch/src/lib.rs @@ -314,6 +314,15 @@ fn sha256_file(path: &Path) -> Result { Ok(h.finalize().iter().map(|b| format!("{b:02x}")).collect()) } +/// A new, empty partial download at `part`, created exclusively after removing whatever was there (a link itself, +/// never its target). +fn new_part(part: &Path) -> std::io::Result { + if std::fs::symlink_metadata(part).is_ok() { + std::fs::remove_file(part)?; + } + OpenOptions::new().write(true).create_new(true).open(part) +} + fn part_path(dir: &Path, name: &str) -> PathBuf { dir.join(format!("{name}.part")) } @@ -324,9 +333,10 @@ fn fetch_file(f: &FileSpec<'_>, url: &str, dir: &Path, opts: &Options, cancel: & let part = part_path(dir, f.name); let limit = f.size.unwrap_or(f.max); let disk = |e: std::io::Error| Fail::Disk(format!("{}: {e}", part.display())); - // resume only a pinned file (an unpinned one can't be checked after resuming) - let mut have = match std::fs::metadata(&part) { - Ok(m) if f.size.is_some() && m.len() <= limit => m.len(), + // resume only a pinned file (an unpinned one can't be checked after resuming), and only a plain file: anything + // else at that name (a link, which `metadata` would follow) is removed, never written through (issue #1502) + let mut have = match std::fs::symlink_metadata(&part) { + Ok(m) if m.is_file() && f.size.is_some() && m.len() <= limit => m.len(), Ok(_) => { std::fs::remove_file(&part).map_err(disk)?; 0 @@ -368,7 +378,7 @@ fn fetch_file(f: &FileSpec<'_>, url: &str, dir: &Path, opts: &Options, cancel: & return Err(Fail::NextMirror(format!("the server's file has the wrong size ({:?} bytes)", resp.content_length()))); } have = 0; - File::create(&part).map_err(disk)? + new_part(&part).map_err(disk)? } 206 if have > 0 => { let range = resp.header("content-range").unwrap_or_default(); @@ -376,6 +386,10 @@ fn fetch_file(f: &FileSpec<'_>, url: &str, dir: &Path, opts: &Options, cancel: & if start != have || total.is_some_and(|t| Some(t) != f.size) { return Err(Fail::NextMirror(format!("the server resumed at the wrong place ({range})"))); } + // (still the plain file checked above: a link put there meanwhile is not appended to) + if !std::fs::symlink_metadata(&part).is_ok_and(|m| m.is_file()) { + return Err(Fail::Retry("the partial download changed; starting over".into())); + } OpenOptions::new().append(true).open(&part).map_err(disk)? } 416 => { diff --git a/crates/fetch/src/tests.rs b/crates/fetch/src/tests.rs index 4a828c6d6..08f869dcc 100644 --- a/crates/fetch/src/tests.rs +++ b/crates/fetch/src/tests.rs @@ -210,6 +210,28 @@ fn resumes_partial_files_and_restarts_bad_ones() { let _ = std::fs::remove_dir_all(&dir); } +/// A link planted at the partial download's name is removed, never written or appended through: the file it points +/// to keeps its bytes and the download completes (issue #1502). +#[cfg(unix)] +#[test] +fn a_link_at_the_partial_name_is_never_written_through() { + let srv = server(); + let dir = tmp("part-link"); + std::fs::create_dir_all(&dir).unwrap(); + let victim = dir.join("victim.txt"); + for target_len in [10usize, 0] { + std::fs::write(&victim, vec![b'k'; target_len.max(1)]).unwrap(); + let before = std::fs::read(&victim).unwrap(); + let _ = std::fs::remove_file(dir.join("model.bin")); + let _ = std::fs::remove_file(part_path(&dir, "model.bin")); + std::os::unix::fs::symlink(&victim, part_path(&dir, "model.bin")).unwrap(); + run(&[pinned()], &[format!("{}/good", srv.base)], &dir, &AtomicBool::new(false)).0.unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), before, "the linked file is untouched"); + assert_eq!(std::fs::read(dir.join("model.bin")).unwrap(), payload()); + } + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn a_416_or_a_bad_complete_part_starts_over() { let srv = server(); diff --git a/docs/whats-new.md b/docs/whats-new.md index 1bf44c3f4..5db5d90dd 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -2,6 +2,11 @@ ## 2026-10-14 +### Reliability +- Saving the catalog and resuming a model download no longer write through a link left at the temporary file's + name (`catalog.snap.tmp`, `model.onnx.part`): the link is removed and a new file made in its place, so the file it + pointed to is never changed (issue #1502). + ### Hard-edged Heal and Clone spots - A spot with Feather at 0 no longer draws black pixels exactly on its radius. This also fixes spots at 0% Opacity changing pixels along their edge. The hard edge now leaves pixels on the radius unchanged.