diff --git a/socktop/src/app.rs b/socktop/src/app.rs index 9d86ee4..068015d 100644 --- a/socktop/src/app.rs +++ b/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 = 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 { - self.close_details_for_gone_process(pid); + 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)] diff --git a/socktop/src/proc_kill.rs b/socktop/src/proc_kill.rs index 3377ef2..9cf84a9 100644 --- a/socktop/src/proc_kill.rs +++ b/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()); } } diff --git a/socktop/src/ui/modal.rs b/socktop/src/ui/modal.rs index ed89278..8f57587 100644 --- a/socktop/src/ui/modal.rs +++ b/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) { @@ -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()); } }