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

@@ -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: <container> need <size> free <size>`, and the
-container starts persistent; the next start tries again. hive-priv checks a file that fails to
+container starts persistent; the next start tries again. hive-priv checks a file
-clone against the 2 GiB reserve again before it copies the data. After a
+that fails to clone against the 2 GiB reserve again before it copies the data. A
-salvage, hive-priv logs `salvage: <container> <size> reflinked=<n>/<m> files`.
+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: <container> <size> reflinked=<n>/<m> files`.
 
 **Concurrency.** hive-priv holds an `flock` on
 `/run/hive-priv/salvage-<container>.lock` (the unit's `RuntimeDirectory`, which

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))
+    let copied = copy_and_move_aside(root, &salvage, agent, profiles, date, env, (uid, gid));
-        .with_context(|| format!("chown {}", salvage.display()))?;
+    let (old_root, stats) = copied.inspect_err(|_| discard_partial(&salvage))?;
-    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()))?;
     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<FsSpace> {
+            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]

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 = [