fix(lifecycle): two signal/restart races in the daemonless monitor + kill paths (#144)

From an adversarial concurrency audit. Both are real multi-actor races (the
monitor daemon and CLI processes coordinate only via the per-modify flock, which
does NOT span an await), with concrete interleavings.

1. (HIGH) Monitor health-restart resurrects a box the user explicitly stopped.
   The unhealthy branch awaits graceful_stop (up to 10s) OUTSIDE any lock, then
   unconditionally marks the box dead and re-boots it. If the user runs
   `a3s-box stop` during that window (setting status=stopped, stopped_by_user),
   the monitor erases the stop and resurrects the box. Fix: after the await,
   re-validate the freshly-loaded record (new `health_restart_still_wanted`) and
   ABORT the restart if it was stopped/stopped_by_user in the meantime.

2. (MED) `kill` fallback host-signal fires with no PID-identity re-check. After a
   guest-delivery attempt that can block up to ~10s, the `!delivered` fallback
   calls a bare `send_signal(pid)` with no re-check — the one-shot identity check
   in require_live_pid is stale by then, so a concurrent force-kill that frees the
   shim PID (and a kernel PID reuse) could route the signal to an unrelated host
   process. Fix: re-check is_process_alive_with_identity immediately before the
   host signal; refuse rather than blind-kill on mismatch.

Test: health_restart_still_wanted unit test (neuter-verified — forcing it true
makes the test fail on the stopped case). #2 reuses the existing identity check
(a defensive re-check, not cleanly unit-testable end-to-end). Full a3s-box-cli
lib suite (598) green; fmt + clippy clean.

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

Author: ZhiXiao-Lin

src/cli/src/commands/kill.rs

@@ -131,6 +131,19 @@ async fn kill_one(
             process::deliver_signal_via_guest(&exec_socket, signal).await
         };
         if !delivered {
+            // The guest-delivery attempt above can block up to ~10s. Re-verify the
+            // PID is still THIS box's shim before a bare host kill: during that
+            // window a concurrent force-kill could have made the shim exit and the
+            // kernel reuse its PID for an unrelated process, which we must never
+            // signal. The one-shot identity check in require_live_pid is now stale.
+            if !process::is_process_alive_with_identity(pid, record.pid_start_time) {
+                return Err(format!(
+                    "box {} is no longer running its original shim (PID {pid} exited or was reused); \
+                     not sending {signal} to a possibly-reused PID",
+                    record.name
+                )
+                .into());
+            }
             process::send_signal(pid, signal).map_err(|err| {
                 format!(
                     "Failed to send signal {signal} to box {} (PID {pid}): {err}",

src/cli/src/commands/monitor.rs

@@ -290,17 +290,32 @@ async fn poll_once(tracker: &mut BackoffTracker) -> Result<(), Box<dyn std::erro
                 }
             }
             tracker.mark_dead(&box_id);
-            // Mark as dead so boot_from_record works; re-load fresh under the
-            // lock and touch only this box's fields.
-            StateFile::modify(|s| {
-                if let Some(rec) = s.find_by_id_mut(&box_id) {
-                    rec.status = "dead".to_string();
-                    rec.pid = None;
-                    rec.health_status = "none".to_string();
-                    rec.health_retries = 0;
-                }
-                Ok::<(), std::io::Error>(())
+            // Mark as dead so boot_from_record works; re-load fresh under the lock.
+            // graceful_stop above can take up to 10s, during which the user may have
+            // `stop`ped (or `rm`ed) the box. Re-validate fresh state and ABORT the
+            // restart if so — otherwise we silently resurrect a box the user
+            // explicitly stopped (overwriting their stopped/stopped_by_user record).
+            let proceed = StateFile::modify(|s| {
+                Ok::<bool, std::io::Error>(match s.find_by_id_mut(&box_id) {
+                    Some(rec) if health_restart_still_wanted(rec) => {
+                        rec.status = "dead".to_string();
+                        rec.pid = None;
+                        rec.health_status = "none".to_string();
+                        rec.health_retries = 0;
+                        true
+                    }
+                    // User stopped/removed the box during the graceful-stop window.
+                    _ => false,
+                })
             })?;
+            if !proceed {
+                println!(
+                    "monitor: box {name} ({short_id}) health-restart aborted — stopped by the user during shutdown",
+                    name = record.name,
+                    short_id = record.short_id,
+                );
+                continue;
+            }
         } else {
             tracker.mark_dead(&box_id);
             println!("{}", restart_log_line(&record, RestartReason::Dead));
@@ -366,6 +381,14 @@ fn is_unhealthy_restart_candidate(record: &BoxRecord) -> bool {
         && policy::should_restart(record)
 }
 
+/// Whether a health-restart should still proceed after the (up-to-10s)
+/// graceful-stop await, given the FRESHLY-loaded record. If the user `stop`ped
+/// the box (status=stopped or stopped_by_user) during that window, the restart
+/// must abort so we don't resurrect a box the user explicitly stopped.
+fn health_restart_still_wanted(record: &BoxRecord) -> bool {
+    record.status != "stopped" && !record.stopped_by_user
+}
+
 fn restart_log_line(record: &BoxRecord, reason: RestartReason) -> String {
     match reason {
         RestartReason::Dead => format!(
@@ -491,6 +514,21 @@ mod tests {
     use super::*;
     use crate::test_helpers::fixtures::make_record;
 
+    #[test]
+    fn health_restart_aborts_if_user_stopped_during_window() {
+        // Still running → a health-restart proceeds.
+        let running = make_record("id-1", "box", "running", Some(1));
+        assert!(health_restart_still_wanted(&running));
+        // User `stop`ped the box during the up-to-10s graceful-stop window → abort
+        // (do NOT resurrect a box the user explicitly stopped).
+        let stopped = make_record("id-1", "box", "stopped", None);
+        assert!(!health_restart_still_wanted(&stopped));
+        // stopped_by_user set (even if the status field still reads running) → abort.
+        let mut by_user = make_record("id-1", "box", "running", Some(1));
+        by_user.stopped_by_user = true;
+        assert!(!health_restart_still_wanted(&by_user));
+    }
+
     // --- BackoffTracker tests ---
 
     #[test]