fix(sandbox2): let the sandbox leader own its namespace

crates/common/src/lib.rs

@@ -12,6 +12,7 @@ pub use remote_storage::RemoteStorage;
 pub mod renameat2;
 
 pub mod repo_spec;
+pub mod sandbox_owner;
 mod spec_hash;
 pub use spec_hash::SpecHash;
 mod subsets;

crates/common/src/sandbox_owner.rs

@@ -0,0 +1,131 @@
+//! Which process owns a sandbox directory.
+//!
+//! A sandbox directory is reclaimable once the process it belongs to is gone.
+//! Answering "which process" reliably is the whole point of this module, and it
+//! is less obvious than it looks.
+//!
+//! The directory name carries a trailing `-<pid>`, but that is the pid of
+//! whatever *created* the sandbox, recorded for uniqueness. Creator and owner
+//! coincide only when the creating process is also the one whose lifetime the
+//! sandbox tracks:
+//!
+//! * Under the CLI they do. `mip` creates the sandbox, runs it, and exits, so
+//!   its pid dying is a faithful signal that the directory is finished with.
+//! * Under a daemon they do not. `minimald` creates sandboxes and outlives them
+//!   all, and inside the microVM it is **pid 1** — alive by definition, and
+//!   still pid 1 after a restart. Every directory it creates looks permanently
+//!   owned, so nothing can ever be reclaimed and nothing can tell a running
+//!   build from an abandoned one.
+//!
+//! So ownership is recorded explicitly instead, in a [`LEADER_PID_FILE`] beside
+//! the sandbox contents. It names the creating process while the sandbox is
+//! being set up — a sandbox mid-construction must not look abandoned — and is
+//! rewritten with the sandbox **leader**'s pid as soon as one is spawned.
+//!
+//! The contract lives here, rather than in `sandbox2` where the writing
+//! happens, so that readers which do not (and should not) depend on the sandbox
+//! implementation can still answer the question.
+
+use std::path::Path;
+
+/// Name of the file, inside a sandbox directory, recording the pid whose death
+/// makes that directory reclaimable.
+pub const LEADER_PID_FILE: &str = "leader.pid";
+
+/// Record `pid` as the owner of the sandbox directory at `dir`.
+///
+/// Best-effort: a sandbox that cannot write its own marker still runs, it just
+/// cannot be attributed later. Failing a build over a housekeeping file would
+/// be the wrong trade. Returns whether the marker was written.
+pub fn set_owning_pid(dir: &Path, pid: u32) -> bool {
+    std::fs::write(dir.join(LEADER_PID_FILE), pid.to_string()).is_ok()
+}
+
+/// The pid owning the sandbox directory at `dir`, if it recorded one.
+///
+/// `None` means *unknown owner*, not *no owner* — a directory created before
+/// this marker existed has none, and so does one abandoned before its leader
+/// spawned. Callers reclaiming directories must treat the two the same way they
+/// treat a live owner: leave it alone.
+#[must_use]
+pub fn owning_pid(dir: &Path) -> Option<u32> {
+    std::fs::read_to_string(dir.join(LEADER_PID_FILE))
+        .ok()?
+        .trim()
+        .parse()
+        .ok()
+}
+
+/// Whether the sandbox directory at `dir` is reclaimable: it names an owner,
+/// and that owner is gone.
+///
+/// A `/proc` lookup, so this is only meaningful on the host whose pids the
+/// marker refers to. An unreadable `/proc/<pid>` reads as "still alive", which
+/// keeps a reaper off directories whose owner it cannot rule out.
+#[must_use]
+pub fn owner_is_gone(dir: &Path) -> bool {
+    match owning_pid(dir) {
+        Some(pid) => !std::fs::exists(format!("/proc/{pid}")).unwrap_or(true),
+        None => false,
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn a_recorded_pid_round_trips() {
+        let tmp = tempfile::tempdir().unwrap();
+        assert!(set_owning_pid(tmp.path(), 4242));
+        assert_eq!(owning_pid(tmp.path()), Some(4242));
+    }
+
+    /// Rewriting is the handover from creating process to sandbox leader, so
+    /// the later write has to win outright rather than append.
+    #[test]
+    fn rewriting_the_marker_replaces_the_owner() {
+        let tmp = tempfile::tempdir().unwrap();
+        set_owning_pid(tmp.path(), 1);
+        set_owning_pid(tmp.path(), 99999);
+        assert_eq!(owning_pid(tmp.path()), Some(99999));
+    }
+
+    /// An unmarked directory is *unknown*, and unknown must never read as
+    /// reclaimable — that is what keeps a reaper off a sandbox still being
+    /// built, and off every directory created before this marker existed.
+    #[test]
+    fn an_unmarked_directory_is_never_reclaimable() {
+        let tmp = tempfile::tempdir().unwrap();
+        assert_eq!(owning_pid(tmp.path()), None);
+        assert!(!owner_is_gone(tmp.path()));
+    }
+
+    #[test]
+    fn a_malformed_marker_is_unknown_not_reclaimable() {
+        let tmp = tempfile::tempdir().unwrap();
+        std::fs::write(tmp.path().join(LEADER_PID_FILE), "not-a-pid").unwrap();
+        assert_eq!(owning_pid(tmp.path()), None);
+        assert!(!owner_is_gone(tmp.path()));
+    }
+
+    /// pid 0 is never a live process, so `/proc/0` never exists — the one pid
+    /// that lets this assert the reclaimable arm without racing a real process.
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn a_dead_owner_makes_the_directory_reclaimable() {
+        let tmp = tempfile::tempdir().unwrap();
+        set_owning_pid(tmp.path(), 0);
+        assert!(owner_is_gone(tmp.path()));
+    }
+
+    /// This process is alive by construction, so its own directory must not be
+    /// reclaimable — the case the daemon got wrong by recording its own pid.
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn a_live_owner_holds_the_directory() {
+        let tmp = tempfile::tempdir().unwrap();
+        set_owning_pid(tmp.path(), std::process::id());
+        assert!(!owner_is_gone(tmp.path()));
+    }
+}

crates/mip/src/cmd_cache.rs

@@ -102,14 +102,23 @@ pub async fn cmd_cache(args: CacheArgs, ctx: &mut Context) -> Result<(), Error>
     Ok(())
 }
 
+/// Remove a sandbox/task/temp directory whose owning process is gone.
+///
+/// Ownership comes from the marker the sandbox writes
+/// ([`common::sandbox_owner`]), not from the trailing `-<pid>` in the directory
+/// name. That suffix is the pid of whatever *created* the sandbox, which is the
+/// right answer only when the creator is also the thing whose lifetime the
+/// sandbox tracks — true for this CLI, false for a long-lived daemon. Reading
+/// the marker means one rule works for both.
+///
+/// A directory with no marker is left alone: it predates the marker, or was
+/// abandoned before its leader spawned, and neither is distinguishable here
+/// from a sandbox mid-construction.
 fn cleanup_stale(kind: &str, entry: std::fs::DirEntry) -> Result<(), Error> {
     let name = entry.file_name();
     let s = name.to_str().unwrap();
-    if s.contains("-")
-        && let Some(pid_str) = s.rsplit('-').next()
-        && let Ok(false) = std::fs::exists(format!("/proc/{}", pid_str))
-    {
-        // No such proc entry, therefore PID dead. Clean up directory.
+    if common::sandbox_owner::owner_is_gone(&entry.path()) {
+        // No such proc entry, therefore the owner is dead. Clean up directory.
         println!("Cleaning up stale {} {}", kind, s);
         common::remove_dir_all(entry.path())
             .map_err(|e| Error::IO("rm stale sandbox", entry.path(), e))?;

crates/sandbox2/src/config.rs

@@ -386,6 +386,12 @@ impl Config {
         // At this layer its plausible that there might be two packages of the same name
         // built at the same time, so we do an atomic directory creation dance /w an attempt
         // counter to make sure each sandbox gets its own folder.
+        //
+        // The pid here is for *uniqueness only*. Ownership — whose death makes
+        // this directory reclaimable — is recorded separately in
+        // [`common::sandbox_owner`], because the creating process is not the
+        // process whose lifetime the sandbox tracks. A reaper that reads this
+        // name instead sees the daemon (pid 1 in the guest), which never dies.
         use std::time::{SystemTime, UNIX_EPOCH};
         let timestamp = SystemTime::now()
             .duration_since(UNIX_EPOCH)
@@ -427,6 +433,13 @@ impl Config {
             }
         };
 
+        // Claim ownership for *this* process until a leader exists. Setup below
+        // (fs mappings, synth dir) runs before anything is spawned, and a
+        // concurrent reaper must not mistake a sandbox that is mid-construction
+        // for an abandoned one. `Sandbox::run` overwrites this with the leader's
+        // pid once there is one.
+        common::sandbox_owner::set_owning_pid(&build_base_dir, std::process::id());
+
         // Validate FS mappings, creating any non-existent files as we go.
         if let WdSetup::BoundDir { fs_mappings, .. } = &self.wd {
             for m in fs_mappings {

crates/sandbox2/src/lib.rs

@@ -875,6 +875,15 @@ impl<C: Channel> Sandbox<C> {
                 .spawn()
                 .map_err(|e| Error::Execution(ExecutionError::SpawnFailed(e)))?;
 
+            // Hand ownership of the directory to the process that now occupies
+            // it. Until this point the marker named whoever built the sandbox,
+            // which for the daemon is itself — a process that outlives every
+            // sandbox it creates, and inside the microVM is pid 1 and so alive
+            // by definition. The leader is the process whose death actually
+            // means this directory is finished with. Rewritten per invocation,
+            // so a sandbox running several execs always names the current one.
+            common::sandbox_owner::set_owning_pid(&self.base_dir, child.id());
+
             // Apply the configured per-sandbox network to this invocation's
             // freshly-unshared netns (own-IP switch attach). No-op for
             // HostNet/NoNet and when no custom `Network` is set, so existing