From 56af198d54b3c0eaf52ee8e8022c1278fd75081f Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Thu, 29 Oct 2026 22:42:12 +0100 Subject: [PATCH] fix(rootfs): make cache writes safe against concurrent runs Downloads, OCI bundle packing and rootfs packing all wrote through one fixed partial name, so two processes preparing the same entry interleaved bytes and renamed the wreckage into place for good. Each writer now gets a unique partial file (utils::partial_path) and the last atomic rename wins with whole bytes either way; the layer pointer is written the same way. --- crates/ecrou/src/download.rs | 9 ++++++--- crates/ecrou/src/rootfs.rs | 25 ++++++++++++++++++++----- crates/ecrou/src/utils.rs | 19 ++++++++++++++++++- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/crates/ecrou/src/download.rs b/crates/ecrou/src/download.rs index 0d8578b..482250a 100644 --- a/crates/ecrou/src/download.rs +++ b/crates/ecrou/src/download.rs @@ -302,12 +302,15 @@ async fn download_file_async( (total_size > 0).then_some(total_size), ); - let temp_dest = dest.with_extension("partial"); + // Unique per call: concurrent runs writing the same entry each + // complete their own file, and the last rename wins with whole + // bytes either way. + let temp_dest = crate::utils::partial_path(dest); // Download into the temp file, then atomically rename to the final path. // Verification runs before the rename so a corrupted download never // reaches the cache; on any failure after the temp file is created we - // remove it so stale .partial files don't accumulate. + // remove it so stale partial files don't accumulate. let mut hasher = DownloadHasher::new(); let result: anyhow::Result<()> = async { let mut file = tokio::fs::File::create(&temp_dest) @@ -579,7 +582,7 @@ async fn try_download_oci_image( // Bundle layers.manifest + all layer blobs into a single .tar.gz cache file // using the tar + flate2 crates — no external `tar` binary required. - let temp_dest = dest.with_extension("tar.partial"); + let temp_dest = crate::utils::partial_path(dest); let bundle_result: anyhow::Result<()> = (|| { use flate2::{write::GzEncoder, Compression}; diff --git a/crates/ecrou/src/rootfs.rs b/crates/ecrou/src/rootfs.rs index 0e4897b..6092848 100644 --- a/crates/ecrou/src/rootfs.rs +++ b/crates/ecrou/src/rootfs.rs @@ -136,8 +136,14 @@ impl RootfsCache { let dir = self.dir.join("layers"); std::fs::create_dir_all(&dir) .with_context(|| format!("Failed to create the layer directory {}", dir.display()))?; - std::fs::write(dir.join(name), cache_filename) + // Atomic write: a concurrent run reading the pointer must + // never see a truncated filename mid-write. + let pointer = dir.join(name); + let partial = crate::utils::partial_path(&pointer); + std::fs::write(&partial, cache_filename) .with_context(|| format!("Failed to write the layer pointer for {name}"))?; + std::fs::rename(&partial, &pointer) + .with_context(|| format!("Failed to move the layer pointer into place for {name}"))?; Ok(()) } @@ -522,7 +528,7 @@ fn drop_loopback_nameservers(config: &str) -> String { /// Write a tarball of `rootfs` at `dest`, compressed according to dest's /// extension (.tar.gz, .tar.xz, .tar.zst, plain .tar), atomically via a -/// `.partial` file. +/// unique partial file ([`crate::utils::partial_path`]). fn pack_rootfs(rootfs: &Path, dest: &Path) -> anyhow::Result<()> { // Packing the tree into a tarball placed inside it would recurse // forever: the walker keeps seeing the archive grow. @@ -541,7 +547,7 @@ fn pack_rootfs(rootfs: &Path, dest: &Path) -> anyhow::Result<()> { } let filename = dest.file_name().and_then(|n| n.to_str()).unwrap_or(""); - let partial = dest.with_extension("partial"); + let partial = crate::utils::partial_path(dest); let file = std::fs::File::create(&partial) .with_context(|| format!("Failed to create {}", partial.display()))?; @@ -848,12 +854,21 @@ mod tests { #[test] fn pack_failure_leaves_no_partial_file() { - // A nonexistent rootfs must fail cleanly and clean up the .partial + // A nonexistent rootfs must fail cleanly and clean up its + // partial file, whatever unique name it carried. let cache = tempfile::tempdir().unwrap(); let dest = cache.path().join("x.tar.gz"); assert!(pack_rootfs(Path::new("/nonexistent-rootfs"), &dest).is_err()); assert!(!dest.exists()); - assert!(!dest.with_extension("partial").exists()); + let leftovers: Vec = std::fs::read_dir(cache.path()) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| name.contains("partial")) + .collect(); + assert!( + leftovers.is_empty(), + "partial files left behind: {leftovers:?}" + ); } #[test] diff --git a/crates/ecrou/src/utils.rs b/crates/ecrou/src/utils.rs index d9da1e2..5711e22 100644 --- a/crates/ecrou/src/utils.rs +++ b/crates/ecrou/src/utils.rs @@ -1,7 +1,7 @@ use anyhow::anyhow; use crate::Result; -use std::path::Path; +use std::path::{Path, PathBuf}; // ============================================================================ // Constants @@ -35,6 +35,23 @@ pub fn random_suffix(len: usize) -> String { .collect() } +// ============================================================================ +// Unique scratch paths for atomic writes +// ============================================================================ + +/// Sequence counter for [`partial_path`], unique within the process. +static PARTIAL_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +/// A sibling scratch path for writing `dest` before an atomic rename: +/// the pid separates concurrent processes, the sequence separates +/// threads. Two writers racing on the same cache entry each complete +/// their own file, so whichever rename lands last, the bytes at `dest` +/// are whole. +pub fn partial_path(dest: &Path) -> PathBuf { + let seq = PARTIAL_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + dest.with_extension(format!("partial.{}-{seq}", std::process::id())) +} + // ============================================================================ // Architecture handling // ============================================================================