fix: close 2 review regressions + stale lockfile from the operability batch (#139)

Adversarial seam-review of the merged operability batch (v2.3.0..main)
surfaced two genuine incompletenesses plus a stale lockfile:

1. --memory-swap missed call-site (medium). The fail-closed `> i64::MAX`
   guard was added to the run/create path (common.rs) but NOT to
   `a3s-box update` (container_update.rs), so `update --memory-swap <huge>`
   still wrapped negative -> resize.rs maps swap < 0 to memory.swap.max=max
   -> silently UNLIMITED swap. Extract one shared `parse_memory_swap`
   helper (handles the -1 sentinel + the i64::MAX bound) and call it from
   both paths so they cannot diverge again.

2. OCI per-entry skip dropped data irrecoverably (low). On a per-entry
   deserialize failure, read_index_from_disk skipped the bad entry but —
   unlike the whole-file quarantine path — did NOT preserve it; the next
   save rewrites index.json with only the survivors, erasing the
   un-deserializable record (e.g. a schema mismatch after upgrade) with no
   backup. Add a copy-only `quarantine_copy` (preserve original in place,
   live catalog untouched) and back up index.json when entries are skipped.

3. Stale Cargo.lock. The monitor concurrent-probes fix added a `futures`
   dep to a3s-box-cli but didn't regenerate the lock, so the committed
   Cargo.lock omits the edge and `cargo build --locked` fails on main.
   Regenerate.

Also drops a stale `a3s-box reap` reference (a non-existent command) from a
state-file doc comment, matching the accurate operator-facing message.

Tests: parse_memory_swap (sentinel/normal/overflow), quarantine_copy
(preserve+backup) and quarantine_corrupt (move-aside).

Co-authored-by: Roy Lin <roylin@a3s.box>

Author: ZhiXiao-Lin

src/Cargo.lock

@@ -534,6 +534,26 @@ pub(crate) fn parse_memory_bytes(s: &str) -> Result<u64, String> {
         .ok_or_else(|| format!("size value too large: {s}"))
 }
 
+/// Parse a `--memory-swap` value into the cgroup `i64` representation, failing
+/// closed on overflow.
+///
+/// `-1` is Docker's sentinel for "unlimited" and maps to `-1`. Any other value
+/// is a byte size that MUST fit in `i64`: a value > `i64::MAX` would wrap
+/// negative on the `as i64` cast, and `resize.rs` maps `swap < 0` to
+/// `memory.swap.max=max` (unlimited) — so a fat-fingered huge value would
+/// silently become *unlimited* swap. Reject it instead. Shared by the
+/// run/create path and `a3s-box update` so the two cannot diverge.
+pub(crate) fn parse_memory_swap(s: &str) -> Result<i64, String> {
+    if s == "-1" {
+        return Ok(-1);
+    }
+    let bytes = parse_memory_bytes(s).map_err(|e| format!("Invalid --memory-swap: {e}"))?;
+    if bytes > i64::MAX as u64 {
+        return Err(format!("--memory-swap value too large: {s}"));
+    }
+    Ok(bytes as i64)
+}
+
 /// Build ResourceLimits from common box args.
 pub(crate) fn build_resource_limits(
     args: &CommonBoxArgs,
@@ -545,17 +565,7 @@ pub(crate) fn build_resource_limits(
         None => None,
     };
     let memory_swap = match &args.memory_swap {
-        Some(s) if s == "-1" => Some(-1i64),
-        Some(s) => {
-            let bytes = parse_memory_bytes(s).map_err(|e| format!("Invalid --memory-swap: {e}"))?;
-            // Reject before the lossy `as i64` cast: a value > i64::MAX would wrap
-            // negative, and resize.rs maps swap < 0 to memory.swap.max=max
-            // (unlimited) — so a fat-fingered huge value silently became unlimited.
-            if bytes > i64::MAX as u64 {
-                return Err(format!("--memory-swap value too large: {s}").into());
-            }
-            Some(bytes as i64)
-        }
+        Some(s) => Some(parse_memory_swap(s)?),
         None => None,
     };
 
@@ -644,6 +654,22 @@ mod tests {
         assert!(parse_memory_bytes("18446744073709551615k").is_err());
     }
 
+    #[test]
+    fn test_parse_memory_swap_sentinel_value_and_overflow() {
+        // -1 is Docker's "unlimited" sentinel and must survive verbatim.
+        assert_eq!(parse_memory_swap("-1").unwrap(), -1);
+        // A normal size parses to its byte count.
+        assert_eq!(parse_memory_swap("512m").unwrap(), 512 * 1024 * 1024);
+        // Fail closed: a value > i64::MAX must be rejected, NOT wrap negative
+        // (which resize.rs would map to memory.swap.max=max → silently unlimited).
+        // i64::MAX = 9223372036854775807; this is just above it.
+        let huge = "9300000000000000000";
+        let err = parse_memory_swap(huge).unwrap_err();
+        assert!(err.contains("too large"), "got: {err}");
+        // And the same guard via a suffixed overflow.
+        assert!(parse_memory_swap("9000000000g").is_err());
+    }
+
     #[test]
     fn test_validate_rejects_zero_and_excessive_cpus() {
         let mut args = default_common_args();

src/cli/src/commands/common.rs

@@ -102,12 +102,9 @@ pub async fn execute(args: ContainerUpdateArgs) -> Result<(), Box<dyn std::error
     }
 
     if let Some(ref swap) = args.memory_swap {
-        let val = if swap == "-1" {
-            -1i64
-        } else {
-            common::parse_memory_bytes(swap).map_err(|e| format!("Invalid --memory-swap: {e}"))?
-                as i64
-        };
+        // Same fail-closed parse as the run/create path so `update --memory-swap`
+        // can't silently grant unlimited swap on an overflowing value.
+        let val = common::parse_memory_swap(swap)?;
         update.limits.memory_swap = Some(val);
         record.resource_limits.memory_swap = Some(val);
         updated.push(format!("memory-swap={swap}"));

src/cli/src/commands/container_update.rs

@@ -92,8 +92,9 @@ impl StateFile {
     /// orphaning every running VM/overlay with no error and no recoverable
     /// record. Instead the corrupt file is quarantined to a timestamped sibling
     /// (`boxes.json.corrupt-<unix-secs>`) and a loud warning is emitted, so the
-    /// data is preserved for recovery (`a3s-box reap`, or manual repair) while
-    /// the process starts from a clean empty state rather than crashing.
+    /// data is preserved for recovery (restore it, then `a3s-box ps` re-reconciles;
+    /// otherwise repair manually) while the process starts from a clean empty
+    /// state rather than crashing.
     fn parse_or_quarantine(path: &Path, data: &str) -> Vec<BoxRecord> {
         match serde_json::from_str::<Vec<BoxRecord>>(data) {
             Ok(records) => records,

src/cli/src/state/file.rs

@@ -363,9 +363,15 @@ impl ImageStore {
             }
         }
         if skipped > 0 {
+            // Preserve the original (with the un-deserializable entries) before the
+            // next save rewrites index.json with only the survivors — otherwise the
+            // skipped records are erased with no backup, unlike the whole-file path.
+            let preserved = crate::store_io::quarantine_copy(&index_path)
+                .map(|p| p.display().to_string())
+                .unwrap_or_else(|| "<backup failed>".to_string());
             tracing::warn!(
-                "{skipped} image index entr{} skipped as unreadable; affected images \
-                 will be re-pulled on demand",
+                "{skipped} image index entr{} skipped as unreadable; preserved a copy at \
+                 {preserved}; affected images will be re-pulled on demand",
                 if skipped == 1 { "y" } else { "ies" },
             );
         }

src/runtime/src/oci/store.rs

@@ -33,3 +33,58 @@ pub(crate) fn quarantine_label(path: &Path) -> String {
         .map(|p| p.display().to_string())
         .unwrap_or_else(|| "<backup failed>".to_string())
 }
+
+/// Copy a store file aside to a timestamped `*.corrupt-<unix-secs>` sibling
+/// WITHOUT removing the original.
+///
+/// Unlike [`quarantine_corrupt`] (whole-file unreadable → move aside), this is
+/// for the *per-entry* skip path: the file still parses and its surviving
+/// entries stay live, but the next save rewrites it with only the survivors and
+/// would erase the un-deserializable entries (e.g. a schema mismatch after an
+/// upgrade) with no backup. Copying — not moving — preserves those entries for
+/// recovery while leaving the live catalog untouched. Returns the backup path,
+/// `None` if the copy failed.
+pub(crate) fn quarantine_copy(path: &Path) -> Option<PathBuf> {
+    let secs = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .map(|d| d.as_secs())
+        .unwrap_or(0);
+    let backup = path.with_extension(format!("json.corrupt-{secs}"));
+    std::fs::copy(path, &backup).ok().map(|_| backup)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn quarantine_copy_preserves_original_and_backs_up() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("index.json");
+        std::fs::write(&path, b"original contents").unwrap();
+
+        let backup = quarantine_copy(&path).expect("copy should succeed");
+
+        // The live file is untouched (per-entry skip keeps its survivors).
+        assert!(path.exists(), "original must remain in place");
+        assert_eq!(std::fs::read(&path).unwrap(), b"original contents");
+        // A recovery copy exists with the same bytes.
+        assert!(backup.exists(), "backup copy must exist");
+        assert_eq!(std::fs::read(&backup).unwrap(), b"original contents");
+        assert!(backup.to_string_lossy().contains(".corrupt-"));
+    }
+
+    #[test]
+    fn quarantine_corrupt_moves_original_aside() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("index.json");
+        std::fs::write(&path, b"corrupt").unwrap();
+
+        let backup = quarantine_corrupt(&path).expect("move should succeed");
+
+        // Whole-file path removes the original so the next save can't reuse it.
+        assert!(!path.exists(), "original must be moved aside");
+        assert!(backup.exists());
+        assert_eq!(std::fs::read(&backup).unwrap(), b"corrupt");
+    }
+}