fix(kill): close stacked details view, guard PID reuse, scale settle with interval

Review fixes for the process-kill feature:

1. Killing from INSIDE the details view left it open forever, frozen on
   the dead process (reproduced live): the 'Signal sent' Info modal sits
   on top when the death is confirmed on the next tick, the old top-only
   close_process_details missed it, gone-PIDs are processed once, and the
   details-poll fallback was gated on the selection the kill had just
   cleared. The close now removes the dead PID's view wherever it sits in
   the stack — which also retires a dead parent's view from under a child
   in a navigation chain, so backing out lands on the process list rather
   than a frozen corpse view. Tests updated to the new semantics, plus a
   regression test for the Info-stacked case.

2. PID-reuse guard: the PID comes from an agent snapshot and the
   confirmation can sit open indefinitely, so by signal time the kernel
   may have recycled the number. kill_local_process now takes the name
   the user confirmed and refuses to signal a PID whose current owner
   does not match ('PID N now belongs to X, not Y').

3. A transient request error no longer closes the details view: with
   process_details_answered set, any Err was read as 'process gone',
   including socket blips. The view now closes only when the PID is also
   absent from the agent's own process list.

4. PROC_CACHE_SETTLE scales with the user's processes interval (floor at
   the old 1.6s default-TTL value), and the tombstone lifetime rides on
   top of it — users who raise the agent's Processes TTL raise the client
   interval to match, so the interval is the best client-side signal for
   how stale an agent snapshot can be.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

socktop/src/app.rs

@@ -52,20 +52,21 @@ use socktop_connector::{
 const MIN_METRICS_INTERVAL_MS: u64 = 100;
 const MIN_PROCESSES_INTERVAL_MS: u64 = 200;
 
-/// How long to wait before forcing a process-list refresh after a kill. Just
-/// past the agent's default `Processes` cache TTL of 1500ms, so the answer
-/// reflects the kill instead of the cached snapshot taken before it.
-const PROC_CACHE_SETTLE: Duration = Duration::from_millis(1_600);
+/// Floor for the post-kill forced refresh delay: just past the agent's
+/// DEFAULT `Processes` cache TTL of 1500ms, so the answer reflects the kill
+/// instead of the cached snapshot taken before it. The effective delay scales
+/// with the user's processes interval — see [`App::proc_refresh_settle`].
+const PROC_CACHE_SETTLE_FLOOR: Duration = Duration::from_millis(1_600);
+
+/// Margin a tombstone outlives the settle window by. With default intervals
+/// this reproduces the original fixed 5s tombstone (1.6s + 3.4s).
+const TOMBSTONE_MARGIN: Duration = Duration::from_millis(3_400);
 
 /// How long to keep re-checking a signalled process for its exit. Long enough
 /// to cover a slow shutdown, short enough that a process which plainly ignored
 /// the signal keeps its row.
 const KILL_WATCH_FOR: Duration = Duration::from_secs(5);
 
-/// How long a confirmed-dead PID is remembered, so a cached agent snapshot
-/// taken before the kill cannot resurrect its row.
-const KILLED_TOMBSTONE_FOR: Duration = Duration::from_secs(5);
-
 /// Budget for one request/response round trip. Replies are matched to
 /// requests by order, so a request that never answers would otherwise hang
 /// `ws.next()` forever and freeze the TUI (raw mode even eats Ctrl+C).
@@ -401,7 +402,11 @@ impl App {
             return;
         };
         self.modal_manager.pop_modal();
-        let (title, message) = match kill_local_process(pid, signal) {
+        // The name shown in the confirmation doubles as the reuse guard: if
+        // the PID has been recycled since, the kill is refused. The "process"
+        // fallback from prompt_kill means "name unknown" — no guard possible.
+        let expected = (name != "process").then_some(name.as_str());
+        let (title, message) = match kill_local_process(pid, expected, signal) {
             Ok(()) => {
                 self.refresh_after_kill(pid);
                 (
@@ -434,7 +439,24 @@ impl App {
         // Check once right now: SIGKILL, and anything already exiting, is gone
         // by the time the confirmation is dismissed.
         self.poll_kill_watch();
-        self.procs_refresh_due_at = Some(Instant::now() + PROC_CACHE_SETTLE);
+        self.procs_refresh_due_at = Some(Instant::now() + self.proc_refresh_settle());
+    }
+
+    /// How long the post-kill forced refresh waits, and the base of the
+    /// tombstone lifetime. Scales with the user's processes interval: someone
+    /// who raised the agent's Processes TTL will have raised their client
+    /// interval to match (there is no point polling faster than the cache),
+    /// so the interval is the best client-side signal for how stale an agent
+    /// snapshot can be. Never below the default-TTL floor.
+    fn proc_refresh_settle(&self) -> Duration {
+        PROC_CACHE_SETTLE_FLOOR.max(self.procs_interval)
+    }
+
+    /// How long a confirmed-dead PID is remembered, so a cached agent snapshot
+    /// taken before the kill cannot resurrect its row. Must outlive the settle
+    /// window plus one round trip, hence settle + margin.
+    fn kill_tombstone_for(&self) -> Duration {
+        self.proc_refresh_settle() + TOMBSTONE_MARGIN
     }
 
     /// Re-check the processes we have signalled and retire the rows of any that
@@ -464,8 +486,9 @@ impl App {
             self.close_details_for_gone_process(pid);
             self.killed_gone.push((pid, now));
         }
+        let tombstone_for = self.kill_tombstone_for();
         self.killed_gone
-            .retain(|(_, at)| now.duration_since(*at) < KILLED_TOMBSTONE_FOR);
+            .retain(|(_, at)| now.duration_since(*at) < tombstone_for);
     }
 
     /// Drop rows for processes we have confirmed dead. Applied to every process
@@ -475,8 +498,9 @@ impl App {
             return;
         }
         let now = Instant::now();
+        let tombstone_for = self.kill_tombstone_for();
         self.killed_gone
-            .retain(|(_, at)| now.duration_since(*at) < KILLED_TOMBSTONE_FOR);
+            .retain(|(_, at)| now.duration_since(*at) < tombstone_for);
         let pids: Vec<u32> = self.killed_gone.iter().map(|(p, _)| *p).collect();
         for pid in pids {
             self.forget_process_row(pid);
@@ -1557,8 +1581,20 @@ impl App {
                                             // endpoint plainly works, so the PID is
                                             // the problem — close the view instead
                                             // of claiming the agent needs updating.
+                                            //
+                                            // Unless the process is still in the
+                                            // agent's own list: then this error is a
+                                            // transient (socket blip, torn frame),
+                                            // not a death — keep the view and let
+                                            // the next poll retry.
                                             if self.process_details_answered {
+                                                let still_listed =
+                                                    self.last_metrics.as_ref().is_some_and(|m| {
+                                                        m.top_processes.iter().any(|p| p.pid == pid)
+                                                    });
+                                                if !still_listed {
                                                     self.close_details_for_gone_process(pid);
+                                                }
                                             } else {
                                                 self.process_details_unsupported = true;
                                             }
@@ -2043,10 +2079,29 @@ mod kill_refresh_tests {
     #[test]
     fn the_forced_refresh_waits_out_the_agent_cache() {
         assert!(
-            PROC_CACHE_SETTLE >= Duration::from_millis(1_500),
+            PROC_CACHE_SETTLE_FLOOR >= Duration::from_millis(1_500),
             "agent serves Processes from a 1500ms cache by default"
         );
     }
+
+    /// Users who raise the agent's Processes TTL raise the client interval to
+    /// match, so the settle window (and the tombstone that must outlive it)
+    /// scales with the interval instead of assuming the default TTL.
+    #[test]
+    fn settle_and_tombstone_scale_with_the_processes_interval() {
+        // The default processes interval is 2s, which already exceeds the
+        // 1.6s floor — so the default settle is the interval itself.
+        let mut app = App::new();
+        assert_eq!(app.proc_refresh_settle(), Duration::from_secs(2));
+
+        app = app.with_intervals(None, Some(10_000));
+        assert_eq!(app.proc_refresh_settle(), Duration::from_secs(10));
+        assert!(app.kill_tombstone_for() > app.proc_refresh_settle());
+
+        // A tiny interval never drops the settle below the default-TTL floor.
+        app = app.with_intervals(None, Some(200));
+        assert_eq!(app.proc_refresh_settle(), PROC_CACHE_SETTLE_FLOOR);
+    }
 }
 
 #[cfg(test)]

socktop/src/proc_kill.rs

@@ -56,9 +56,19 @@ pub fn process_exists(pid: u32) -> bool {
 }
 
 /// Send `signal` to local process `pid`. Returns `Ok(())` on success, or an
-/// `Err` with a human-readable reason (process gone, permission denied,
-/// signal unsupported on this platform).
-pub fn kill_local_process(pid: u32, signal: KillSignal) -> Result<(), String> {
+/// `Err` with a human-readable reason (process gone, PID reused, permission
+/// denied, signal unsupported on this platform).
+///
+/// `expected_name`, when given, is compared against the process that owns the
+/// PID **right now**: the PID came from an agent snapshot and the confirmation
+/// dialog can sit open indefinitely, so by signal time the kernel may have
+/// recycled the number for an unrelated process. Both names come from the
+/// same sysinfo source, so a live, unchanged target compares equal.
+pub fn kill_local_process(
+    pid: u32,
+    expected_name: Option<&str>,
+    signal: KillSignal,
+) -> Result<(), String> {
     let spid = sysinfo::Pid::from_u32(pid);
 
     // Refresh just this one PID — we don't need a full process scan to signal it.
@@ -73,6 +83,16 @@ pub fn kill_local_process(pid: u32, signal: KillSignal) -> Result<(), String> {
         return Err(format!("Process {pid} no longer exists"));
     };
 
+    if let Some(expected) = expected_name {
+        let current = proc_.name().to_string_lossy();
+        if current != expected {
+            return Err(format!(
+                "PID {pid} now belongs to \"{current}\", not \"{expected}\" — \
+                 not signalling. Reselect the process and try again."
+            ));
+        }
+    }
+
     match proc_.kill_with(signal.as_sysinfo()) {
         Some(true) => Ok(()),
         Some(false) => Err(format!(
@@ -104,7 +124,7 @@ mod tests {
             .expect("spawn sleep for the test");
         let pid = child.id();
 
-        let result = kill_local_process(pid, KillSignal::Term);
+        let result = kill_local_process(pid, Some("sleep"), KillSignal::Term);
 
         // Reap on every path before asserting, so a failing assert cannot leak a
         // 30s sleep and cannot trip clippy's zombie_processes lint.
@@ -129,12 +149,33 @@ mod tests {
         );
     }
 
+    /// The reuse guard: a live PID whose owner does not match the name the
+    /// user confirmed must NOT be signalled. This also proves the name is
+    /// populated under ProcessRefreshKind::nothing() — if it weren't, the
+    /// matching-name test above would fail instead.
+    #[test]
+    fn refuses_a_pid_owned_by_a_different_process() {
+        let mut child = Command::new("sleep")
+            .arg("30")
+            .spawn()
+            .expect("spawn sleep");
+        let pid = child.id();
+
+        let result = kill_local_process(pid, Some("firefox"), KillSignal::Term);
+
+        let _ = child.kill();
+        let _ = child.wait();
+
+        let err = result.expect_err("signalled a process under the wrong name");
+        assert!(err.contains("firefox") && err.contains("sleep"), "{err}");
+    }
+
     #[test]
     fn reports_a_pid_that_is_gone() {
         let mut child = Command::new("true").spawn().expect("spawn true");
         let pid = child.id();
         child.wait().expect("reap");
         // The PID is now free; signalling it must fail cleanly, not panic.
-        assert!(kill_local_process(pid, KillSignal::Term).is_err());
+        assert!(kill_local_process(pid, None, KillSignal::Term).is_err());
     }
 }

socktop/src/ui/modal.rs

@@ -91,18 +91,40 @@ impl ModalManager {
         }
         m
     }
-    /// Close the details view for `pid` if that is what is currently on top.
+    /// Close the details view for `pid` WHEREVER it sits in the stack.
     /// Returns whether anything was closed.
     ///
-    /// Only the top modal, deliberately: with a parent-navigation chain, the
-    /// views underneath are other processes that may still be alive, and each
-    /// closes itself the same way once its own process goes.
+    /// Not just the top: killing from inside the details view stacks the
+    /// "Signal sent" Info modal on top of it, and a SIGKILL victim is usually
+    /// confirmed dead on the very next tick — while that Info is still up. A
+    /// top-only check missed the close, and since a gone PID is processed
+    /// once, the details view stayed open (frozen on the dead process's last
+    /// sample) with nothing left to ever close it.
+    ///
+    /// Per-PID matching keeps the parent-navigation property: only the dead
+    /// process's view goes; parent views underneath are other processes that
+    /// may still be alive and close themselves the same way.
     pub fn close_process_details(&mut self, pid: u32) -> bool {
-        if matches!(self.stack.last(), Some(ModalType::ProcessDetails { pid: p }) if *p == pid) {
-            self.pop_modal();
-            return true;
+        let was_top =
+            matches!(self.stack.last(), Some(ModalType::ProcessDetails { pid: p }) if *p == pid);
+        let before = self.stack.len();
+        self.stack
+            .retain(|m| !matches!(m, ModalType::ProcessDetails { pid: p } if *p == pid));
+        if self.stack.len() == before {
+            return false;
         }
-        false
+        // Mirror pop_modal's focus bookkeeping when the top changed.
+        if was_top && let Some(next) = self.stack.last() {
+            self.active_button = match next {
+                ModalType::ConnectionError { .. } => ModalButton::Retry,
+                ModalType::ProcessDetails { .. } => ModalButton::Ok,
+                ModalType::About => ModalButton::Ok,
+                ModalType::Help => ModalButton::Ok,
+                ModalType::Confirmation { .. } => ModalButton::Confirm,
+                ModalType::Info { .. } => ModalButton::Ok,
+            };
+        }
+        true
     }
 
     pub fn update_connection_error_countdown(&mut self, new_countdown: Option<u64>) {
@@ -1033,30 +1055,43 @@ mod close_details_tests {
         assert!(m.is_active());
     }
 
-    /// Walking up to a parent stacks details views. A child dying must close
-    /// only its own view and reveal the parent's, which is still valid.
+    /// Walking up to a parent stacks details views. Only the dead process's
+    /// view goes — whichever position it holds — and the survivor stays put.
     #[test]
-    fn only_closes_the_top_of_a_parent_chain() {
+    fn closes_only_the_dead_pids_view_in_a_parent_chain() {
         let mut m = ModalManager::new();
         m.push_modal(ModalType::ProcessDetails { pid: 100 }); // parent
         m.push_modal(ModalType::ProcessDetails { pid: 200 }); // child, on top
 
-        assert!(!m.close_process_details(100), "closed a view below the top");
-        assert!(m.close_process_details(200));
+        // Parent dies while the child is viewed: its view is removed from
+        // UNDER the top, so closing the child later lands on the process list
+        // instead of a frozen corpse view.
+        assert!(m.close_process_details(100));
         assert!(matches!(
             m.current_modal(),
-            Some(ModalType::ProcessDetails { pid: 100 })
+            Some(ModalType::ProcessDetails { pid: 200 })
         ));
+        assert!(m.close_process_details(200));
+        assert!(!m.is_active());
     }
 
+    /// The F1 regression: killing from inside the details view stacks the
+    /// "Signal sent" Info on top, and the death is usually confirmed while
+    /// that Info is still up. The details view must close anyway — a top-only
+    /// check left it open forever, frozen on the dead process.
     #[test]
-    fn does_nothing_when_another_modal_is_on_top() {
+    fn closes_details_beneath_a_stacked_info_modal() {
         let mut m = ModalManager::new();
         m.push_modal(ModalType::ProcessDetails { pid: 7 });
         m.push_modal(ModalType::Info {
-            title: "t".into(),
-            message: "m".into(),
+            title: "Signal sent".into(),
+            message: "Sent SIGKILL".into(),
         });
-        assert!(!m.close_process_details(7));
+
+        assert!(m.close_process_details(7));
+        // The Info survives on top; dismissing it lands on the process list.
+        assert!(matches!(m.current_modal(), Some(ModalType::Info { .. })));
+        m.pop_modal();
+        assert!(!m.is_active());
     }
 }