fix(controller): detect and warn on a shared writable rootfs

CHANGELOG.md

@@ -33,6 +33,33 @@ snapshot is fully published, so a tag always holds a consistent
 src == dst guard comment duplication and makes the chain-unpack tail
 bail instead of resolving an empty path.
 
+### Snapshots record the rootfs drive's read-only flag
+
+`snapshot.json` now carries `rootfs_read_only`, the flag in effect when
+the vmstate was frozen. It matters because Firecracker serialises the
+drive's `path_on_host` and `is_read_only` into the vmstate and reopens
+both verbatim at restore, with no way to override either at load time —
+the API has no `drive_overrides` and the vmstate is a binary blob, not
+JSON. A snapshot baked with a writable rootfs therefore hands *every*
+restored child the same ext4 opened read-write, and two concurrent
+children are two guest kernels writing one filesystem with no
+coordinator: package files pick up other files' bytes, directory entries
+go `EBADMSG`, and the damage surfaces as a random build failure rather
+than a sandbox error.
+
+The controller detects this and warns when a restore would add a second
+writer to a writable-rootfs snapshot, naming the live holders and the
+remedies (re-bake read-only, or restore one child at a time). With
+`FORKD_REFUSE_SHARED_RW=1` the warning becomes a `409`. Refusing is
+opt-in because it rejects `fork -n N` on a writable snapshot — the flow
+`from-image` prints as its next step — and that behaviour change should
+be the operator's choice.
+
+Snapshots written before this field existed carry no flag;
+`Snapshot::rootfs_is_read_only` infers an `.ext4` rootfs as writable,
+the same convention the boot path uses, and leaves anything else unknown
+rather than asserting it is safe.
+
 ### Rootfs sidecar placement: recorded absolute path, validated
 
 Packs record the rootfs sidecar's target as the vmstate-frozen ABSOLUTE

crates/forkd-cli/src/hub.rs

@@ -1486,6 +1486,7 @@ mod tests {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         };
         std::fs::write(
             base_dir.join("snapshot.json"),
@@ -1505,6 +1506,7 @@ mod tests {
             parent_tag: Some("py-base".to_string()),
             parent_content_hash: Some(base_hash),
             rootfs: None,
+            rootfs_read_only: None,
         };
         std::fs::write(
             head_dir.join("snapshot.json"),
@@ -1530,6 +1532,7 @@ mod tests {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         };
         std::fs::write(
             base_dir.join("snapshot.json"),

crates/forkd-cli/src/main.rs

@@ -3046,6 +3046,12 @@ fn snapshot_cmd(
                 .unwrap_or_else(|_| cfg.rootfs.clone()),
         );
     }
+    // Record the drive's read-only flag alongside its path. The flag is
+    // frozen into the binary vmstate and reopens verbatim for every
+    // restored child, so a snapshot with a writable rootfs cannot be
+    // restored concurrently without those children sharing one ext4. The
+    // daemon reads this to warn — or refuse — instead of corrupting.
+    snap.rootfs_read_only = Some(!rw);
     eprintln!("    snapshot took {} ms", t.elapsed().as_millis());
 
     // Persist Snapshot metadata so subsequent `forkd fork` / `forkd run`
@@ -3306,6 +3312,7 @@ fn load_snapshot_meta(snap_dir: &std::path::Path) -> Result<Snapshot> {
         parent_tag: None,
         parent_content_hash: None,
         rootfs: None,
+        rootfs_read_only: None,
     })
 }
 

crates/forkd-controller/src/http.rs

@@ -749,6 +749,7 @@ async fn compact_snapshot(
         // Compaction flattens but doesn't change the rootfs — inherit
         // the head's so the compacted base stays hub-portable (#242).
         rootfs: head_snapshot.rootfs.clone(),
+        rootfs_read_only: None,
     };
     let staging_meta_path = staging.join("snapshot.json");
     let new_meta_json = match serde_json::to_vec_pretty(&new_meta) {
@@ -828,6 +829,7 @@ fn load_snapshot_with_fallback(snap_dir: &std::path::Path) -> forkd_vmm::Snapsho
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         })
 }
 
@@ -850,6 +852,7 @@ fn load_snapshot_for_restore(snap_dir: &std::path::Path) -> Result<forkd_vmm::Sn
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         })
     }
 }
@@ -883,6 +886,64 @@ fn snapshot_unbootable_reason(snap_dir: &std::path::Path) -> Option<String> {
     }
 }
 
+/// Opt-in: refuse (409) instead of warning when a snapshot's rootfs is
+/// shared writable and a live sandbox already has it open.
+///
+/// Off by default because refusing rejects `fork -n N` on a writable
+/// snapshot, which is the flow `from-image` prints as the next step —
+/// making that a hard error is a behaviour change the operator should
+/// choose, not one to spring on them. On, it converts the current
+/// "one writer at a time" discipline into an enforced invariant instead
+/// of scheduling luck.
+fn refuse_shared_writable_rootfs() -> bool {
+    matches!(
+        std::env::var("FORKD_REFUSE_SHARED_RW").as_deref(),
+        Ok("1") | Ok("true") | Ok("yes")
+    )
+}
+
+/// `Some(reason)` when restoring this snapshot would add a second
+/// concurrent writer to a rootfs that is not read-only.
+///
+/// A restored child reopens the rootfs drive at the path AND the
+/// read-only flag frozen into the vmstate, and Firecracker offers no way
+/// to override either at load time — there is no `drive_overrides` on
+/// `/snapshot/load`, and the vmstate is a binary blob rather than JSON.
+/// So children of a writable-rootfs snapshot share one ext4: two of them
+/// are two guest kernels writing one filesystem with no coordinator,
+/// which corrupts package files and directory entries at random and
+/// surfaces as a broken build rather than a broken sandbox.
+fn shared_writable_rootfs_conflict(
+    s: &AppState,
+    tag: &str,
+    snapshot: &forkd_vmm::Snapshot,
+) -> Option<String> {
+    // `None` (not recorded, not inferable) cannot be asserted writable,
+    // so it does not block. Only a definite writable rootfs does.
+    if snapshot.rootfs_is_read_only() != Some(false) {
+        return None;
+    }
+    let live: std::collections::HashSet<String> = s.live_vms.lock().keys().cloned().collect();
+    let holders: Vec<String> = s
+        .registry
+        .list_sandboxes()
+        .into_iter()
+        .filter(|sb| sb.snapshot_tag == tag && live.contains(&sb.id))
+        .map(|sb| sb.id)
+        .collect();
+    if holders.is_empty() {
+        return None;
+    }
+    Some(format!(
+        "snapshot `{tag}` was baked with a WRITABLE rootfs, so every restored child reopens the \
+         same ext4 read-write and concurrent children corrupt each other's files. {} live \
+         sandbox(es) already hold it ({}). Re-bake the snapshot read-only, or restore one child \
+         at a time.",
+        holders.len(),
+        holders.join(", ")
+    ))
+}
+
 fn snapshot_info_from_disk_dir(tag: String, dir: PathBuf) -> SnapshotInfo {
     let created_at_unix = dir
         .metadata()
@@ -1165,6 +1226,19 @@ async fn create_sandbox(
             req.snapshot_tag
         ));
     }
+    // A writable-rootfs snapshot restored while another child already
+    // holds it means two writers on one ext4 — corruption, not a warning
+    // (see `shared_writable_rootfs_conflict`). Warn by default; refuse
+    // when the operator has opted in.
+    if let Some(reason) = shared_writable_rootfs_conflict(&s, &req.snapshot_tag, &snapshot) {
+        if refuse_shared_writable_rootfs() {
+            return conflict(&reason);
+        }
+        tracing::warn!(
+            snapshot = %req.snapshot_tag,
+            "{reason} Set FORKD_REFUSE_SHARED_RW=1 to refuse instead."
+        );
+    }
 
     // v0.5: if the loaded snapshot has parent_tag set, this is a
     // chained snapshot. Resolve the chain, verify parent content
@@ -2008,6 +2082,7 @@ async fn branch_sandbox(
                         parent_tag: None,
                         parent_content_hash: None,
                         rootfs: None,
+                        rootfs_read_only: None,
                     });
                 }
                 #[cfg(not(target_os = "linux"))]
@@ -2139,6 +2214,7 @@ async fn branch_sandbox(
                         parent_tag: None,
                         parent_content_hash: None,
                         rootfs: None,
+                        rootfs_read_only: None,
                     }
                 } else {
                     let snap = vm.snapshot_to(
@@ -2939,6 +3015,7 @@ fn spawn_one_for_workspace(
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         },
     };
     let netns_reservation = if per_child_netns {
@@ -3242,6 +3319,7 @@ async fn suspend_workspace(
                     parent_tag: None,
                     parent_content_hash: None,
                     rootfs: None,
+                    rootfs_read_only: None,
                 }
             } else {
                 let snap = vm.snapshot_to(
@@ -3508,6 +3586,7 @@ mod tests {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         };
         std::fs::write(
             dir.join("snapshot.json"),
@@ -3535,6 +3614,7 @@ mod tests {
             parent_tag: Some(parent_tag.to_string()),
             parent_content_hash: Some(parent_hash.to_string()),
             rootfs: None,
+            rootfs_read_only: None,
         };
         std::fs::write(
             dir.join("snapshot.json"),
@@ -4335,6 +4415,7 @@ mod tests {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         };
         std::fs::write(
             state_dir.join("snapshot.json"),

crates/forkd-vmm/src/chain.rs

@@ -396,6 +396,7 @@ mod tests {
             parent_tag: parent.map(String::from),
             parent_content_hash: hash.map(String::from),
             rootfs: None,
+            rootfs_read_only: None,
         }
     }
 

crates/forkd-vmm/src/lib.rs

@@ -615,6 +615,44 @@ pub struct Snapshot {
     /// the source's rootfs.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub rootfs: Option<PathBuf>,
+    /// Whether the rootfs drive was opened read-only when this snapshot's
+    /// vmstate was frozen. Firecracker serialises the drive's
+    /// `path_on_host` + `is_read_only` into the vmstate and reopens both
+    /// verbatim at restore, and neither can be overridden at load time
+    /// (Firecracker has no `drive_overrides` on `/snapshot/load`, and the
+    /// vmstate is a binary blob, not JSON). So this flag is what every
+    /// restored child inherits, and it decides whether concurrent
+    /// children share one writable ext4 — which is mutual corruption, not
+    /// a warning.
+    ///
+    /// Recorded by the CLI bake. `None` for snapshots written before the
+    /// field existed and for daemon-side branches that inherit the
+    /// source's rootfs; see [`Snapshot::rootfs_is_read_only`].
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub rootfs_read_only: Option<bool>,
+}
+
+impl Snapshot {
+    /// Whether restored children open the rootfs read-write.
+    ///
+    /// `None` means "not recorded and not inferable" — a snapshot with no
+    /// rootfs path at all (daemon-side branches inherit the source's), or
+    /// one from before this field existed. Callers must not read `None`
+    /// as safe; it means they cannot vouch for the drive.
+    ///
+    /// For snapshots predating the field, an `.ext4` rootfs is inferred
+    /// writable, because that is exactly the convention the boot path uses
+    /// to choose `BootConfig::ext4_rw` (`rw_flag || extension ==
+    /// "ext4"`). Anything else stays unknown rather than being asserted
+    /// read-only.
+    pub fn rootfs_is_read_only(&self) -> Option<bool> {
+        self.rootfs_read_only.or_else(|| {
+            self.rootfs
+                .as_ref()
+                .filter(|p| p.extension().is_some_and(|e| e == "ext4"))
+                .map(|_| false)
+        })
+    }
 }
 
 /// Result of a Diff snapshot. `memory_diff` is a sparse file the same
@@ -1867,6 +1905,7 @@ impl Vm {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         })
     }
 
@@ -1976,6 +2015,7 @@ impl Vm {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         })
     }
 
@@ -2652,6 +2692,39 @@ mod tests {
         }
     }
 
+    /// The read-only flag decides whether a restored child is a second
+    /// writer on a shared ext4, so the resolution rules matter: a
+    /// recorded flag wins, and a pre-flag snapshot is inferred only where
+    /// the `.ext4` convention makes it unambiguous.
+    #[test]
+    fn rootfs_read_only_prefers_recorded_flag_then_infers_ext4() {
+        let snap = |rootfs: Option<&str>, flag: Option<bool>| Snapshot {
+            vmstate: PathBuf::from("/s/vmstate"),
+            memory: PathBuf::from("/s/memory.bin"),
+            volumes: Vec::new(),
+            parent_tag: None,
+            parent_content_hash: None,
+            rootfs: rootfs.map(PathBuf::from),
+            rootfs_read_only: flag,
+        };
+        // Recorded wins over the extension convention, both ways.
+        assert_eq!(
+            snap(Some("/r.ext4"), Some(true)).rootfs_is_read_only(),
+            Some(true)
+        );
+        assert_eq!(snap(None, Some(false)).rootfs_is_read_only(), Some(false));
+        // Pre-flag snapshot: `.ext4` is exactly the case the boot path
+        // treats as writable, so infer it rather than leaving it unknown.
+        assert_eq!(
+            snap(Some("/r.ext4"), None).rootfs_is_read_only(),
+            Some(false)
+        );
+        // Anything else stays unknown — never asserted read-only, and
+        // never assumed safe.
+        assert_eq!(snap(Some("/r.img"), None).rootfs_is_read_only(), None);
+        assert_eq!(snap(None, None).rootfs_is_read_only(), None);
+    }
+
     #[test]
     fn kernel_supports_kvm_clock_realtime_parses_versions() {
         assert!(!kernel_supports_kvm_clock_realtime("5.10.0-foo"));
@@ -2995,6 +3068,7 @@ mod tests {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         };
         let opts = ForkOpts {
             n: 2,
@@ -3028,6 +3102,7 @@ mod tests {
             parent_tag: None,
             parent_content_hash: None,
             rootfs: None,
+            rootfs_read_only: None,
         };
         let json = serde_json::to_string(&s).unwrap();
         let back: Snapshot = serde_json::from_str(&json).unwrap();
@@ -3055,6 +3130,7 @@ mod tests {
             parent_tag: Some("python-numpy".to_string()),
             parent_content_hash: Some("a".repeat(64)),
             rootfs: None,
+            rootfs_read_only: None,
         };
         let json = serde_json::to_string(&s).unwrap();
         let back: Snapshot = serde_json::from_str(&json).unwrap();