From 86e80250bba45898a013c93637df4ac8a717e402 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 28 Oct 2026 21:52:59 +0100 Subject: [PATCH] remove the partial salvage dir when a salvage fails - a failure between creating the dir and the rename removes that dir only - give the ephemeral-rootfs vm test a 4 GiB disk so the space check passes --- docs/agent-lifecycle/persistence.md | 7 +- hive-priv/src/rootfs.rs | 120 +++++++++++++++++++++--- nix/vm-tests/agent-ephemeral-rootfs.nix | 3 + 3 files changed, 116 insertions(+), 14 deletions(-) diff --git a/docs/agent-lifecycle/persistence.md b/docs/agent-lifecycle/persistence.md index bcc95e6a..25be4475 100644 --- a/docs/agent-lifecycle/persistence.md +++ b/docs/agent-lifecycle/persistence.md @@ -423,9 +423,10 @@ anything, hive-priv sizes the root and compares it with the free space under leave 2 GiB free. One that has to copy data must leave 15 % of the filesystem free. When the free space is short, hive-priv skips the salvage, deletes nothing, logs `salvage skipped: need free `, and the -container starts persistent; the next start tries again. hive-priv checks a file that fails to -clone against the 2 GiB reserve again before it copies the data. After a -salvage, hive-priv logs `salvage: reflinked=/ files`. +container starts persistent; the next start tries again. hive-priv checks a file +that fails to clone against the 2 GiB reserve again before it copies the data. A +salvage that fails partway removes the directory it created, and leaves the root +and any earlier salvage as they were. After a salvage, hive-priv logs `salvage: reflinked=/ files`. **Concurrency.** hive-priv holds an `flock` on `/run/hive-priv/salvage-.lock` (the unit's `RuntimeDirectory`, which diff --git a/hive-priv/src/rootfs.rs b/hive-priv/src/rootfs.rs index 56d7f62e..25079f94 100644 --- a/hive-priv/src/rootfs.rs +++ b/hive-priv/src/rootfs.rs @@ -243,16 +243,8 @@ fn salvage( return Ok(Outcome::Skipped { need, free }); } let salvage = fresh_dir(work, &format!("salvaged-rootfs-{date}"))?; - lchown(&salvage, Some(uid), Some(gid)) - .with_context(|| format!("chown {}", salvage.display()))?; - let mut copier = Copier::new(env.backend, uid, gid); - copier - .tree(root, &salvage, Path::new("")) - .with_context(|| format!("copy {} to {}", root.display(), salvage.display()))?; - carry_nix_state(root, agent, profiles, env.backend, uid, gid)?; - let old_root = free_sibling(root, &format!("pre-ephemeral-{date}"))?; - std::fs::rename(root, &old_root) - .with_context(|| format!("move {} to {}", root.display(), old_root.display()))?; + let copied = copy_and_move_aside(root, &salvage, agent, profiles, date, env, (uid, gid)); + let (old_root, stats) = copied.inspect_err(|_| discard_partial(&salvage))?; std::fs::create_dir(root).with_context(|| format!("create {}", root.display()))?; if let Some(id) = machine_id { write_container_etc_file_in(root, "machine-id", &format!("{id}\n"))?; @@ -260,10 +252,47 @@ fn salvage( Ok(Outcome::Salvaged { salvage, old_root, - stats: copier.stats, + stats, }) } +/// Copy `root` into the fresh `salvage` dir, then rename `root` aside; returns +/// where it went. Up to the rename the old root is untouched, so a failure +/// anywhere in here leaves it where it was. +fn copy_and_move_aside( + root: &Path, + salvage: &Path, + agent: &str, + profiles: &Path, + date: &str, + env: &Env<'_>, + (uid, gid): (u32, u32), +) -> Result<(PathBuf, Stats)> { + lchown(salvage, Some(uid), Some(gid)) + .with_context(|| format!("chown {}", salvage.display()))?; + let mut copier = Copier::new(env.backend, uid, gid); + copier + .tree(root, salvage, Path::new("")) + .with_context(|| format!("copy {} to {}", root.display(), salvage.display()))?; + carry_nix_state(root, agent, profiles, env.backend, uid, gid)?; + let old_root = free_sibling(root, &format!("pre-ephemeral-{date}"))?; + std::fs::rename(root, &old_root) + .with_context(|| format!("move {} to {}", root.display(), old_root.display()))?; + Ok((old_root, copier.stats)) +} + +/// Remove the directory a failed salvage was filling: the one this attempt +/// created, never an earlier salvage. `remove_dir_all` does not follow links. +fn discard_partial(salvage: &Path) { + if let Err(e) = std::fs::remove_dir_all(salvage) { + tracing::warn!( + salvage = %salvage.display(), + error = %e, + "could not remove the partial salvage; remove it by hand" + ); + } +} + /// What [`measure`] found under a root. struct Tally { /// Everything outside [`CACHE`], directories and links included. @@ -752,6 +781,8 @@ mod tests { total: u64, mode: CloneMode, delay: Duration, + space_calls: AtomicU32, + space_fails_from: u32, calls: AtomicU32, started: AtomicBool, active: AtomicU32, @@ -766,6 +797,8 @@ mod tests { total: 1 << 51, mode: CloneMode::Fail(libc::EXDEV), delay: Duration::ZERO, + space_calls: AtomicU32::new(0), + space_fails_from: u32::MAX, calls: AtomicU32::new(0), started: AtomicBool::new(false), active: AtomicU32::new(0), @@ -784,6 +817,12 @@ mod tests { self } + /// Make the `n`th and later free-space reads fail, counting from 0. + fn space_fails_from(mut self, n: u32) -> Self { + self.space_fails_from = n; + self + } + fn delay(mut self, delay: Duration) -> Self { self.delay = delay; self @@ -792,6 +831,9 @@ mod tests { impl Backend for Fake { fn space(&self, _path: &Path) -> io::Result { + if self.space_calls.fetch_add(1, Ordering::SeqCst) >= self.space_fails_from { + return Err(io::Error::from_raw_os_error(libc::EIO)); + } Ok(FsSpace { free: self.free, total: self.total, @@ -1424,6 +1466,62 @@ mod tests { assert_eq!(stats.cloned, 0); } + /// A salvage that fails after it created its directory removes that + /// directory and nothing else: an earlier salvage stays, the old root is + /// as it was, and no marker is written. Once without room for a copy, once + /// with a free-space read that fails. + #[test] + fn a_failed_salvage_removes_its_partial_directory() { + let body = "x".repeat(1 << 20); + let cases = [ + ( + "no room for a file", + Fake::new() + .space(CLONE_RESERVE + (512 << 10), 100 * GIB) + .mode(CloneMode::FirstOnly), + ), + ("free space unreadable", Fake::new().space_fails_from(1)), + ]; + for (what, fake) in cases { + let f = Fixture::with(fake); + f.put("home/iris/big", &body); + f.put("home/iris/small", "s"); + // An earlier salvage of the same day, which must survive. + let earlier = f.salvage(); + std::fs::create_dir_all(earlier.join("home")).unwrap(); + std::fs::write(earlier.join("home/old"), "kept").unwrap(); + + assert!(f.try_rotate().is_err(), "{what}: must fail"); + assert_eq!( + Fixture::names(&f.work), + [format!("salvaged-rootfs-{DATE}")], + "{what}: a partial directory is left" + ); + assert_eq!( + std::fs::read_to_string(earlier.join("home/old")).unwrap(), + "kept" + ); + assert!( + f.root.join("home/iris/big").is_file(), + "{what}: root was touched" + ); + assert!(f.root.join("home/iris/small").is_file(), "{what}"); + assert!(!f.root.join("etc").join(MARKER).exists(), "{what}"); + assert!( + Fixture::names(&f.top) + .iter() + .all(|n| !n.contains("pre-ephemeral")), + "{what}: the root was moved" + ); + } + + // Control: the same root with room and a working space read succeeds. + let ok = Fixture::with(Fake::new()); + ok.put("home/iris/big", &body); + assert!(matches!(ok.rotate(), Outcome::Salvaged { .. })); + assert!(ok.salvage().join("home/iris/big").is_file()); + } + /// A second request for a root that is being salvaged waits, then finds /// the finished template: one salvage directory, one moved-aside root. #[test] diff --git a/nix/vm-tests/agent-ephemeral-rootfs.nix b/nix/vm-tests/agent-ephemeral-rootfs.nix index 3088a706..74a651aa 100644 --- a/nix/vm-tests/agent-ephemeral-rootfs.nix +++ b/nix/vm-tests/agent-ephemeral-rootfs.nix @@ -78,6 +78,9 @@ pkgs.testers.runNixOSTest { pkgs.hello pkgs.tree ]; + # The salvage skips when a copy would leave under 15% of the disk free, and + # this disk cannot clone files; the default 1 GiB leaves too little room. + virtualisation.diskSize = 4096; # The agent bridge hive-priv names in the container's conf. networking.bridges.${bridge}.interfaces = [ ]; networking.interfaces.${bridge}.ipv4.addresses = [