diff --git a/src/app.rs b/src/app.rs index 75d2a0b..cc529a1 100644 --- a/src/app.rs +++ b/src/app.rs @@ -292,10 +292,15 @@ impl App { } /// Confirm and execute a pending signal action. - pub fn confirm_signal(&mut self, sender: &mut dyn FnMut(i32, Signal) -> Result<(), String>) { - let Some(pending) = self.pending_confirmation.take() else { - return; - }; + /// + /// Returns `Some(pid)` when the sender accepted the signal so callers can + /// follow up (e.g. wait for the kernel to remove the entry from `/proc`). + /// Returns `None` when no confirmation was pending or the sender failed. + pub fn confirm_signal( + &mut self, + sender: &mut dyn FnMut(i32, Signal) -> Result<(), String>, + ) -> Option { + let pending = self.pending_confirmation.take()?; match sender(pending.pid, pending.signal) { Ok(()) => { @@ -303,9 +308,11 @@ impl App { "sent {:?} ({}) to pid {}", pending.signal, pending.digit, pending.pid ); + Some(pending.pid) } Err(err) => { self.status = format!("failed to signal pid {}: {}", pending.pid, err); + None } } } diff --git a/src/debug_tui.rs b/src/debug_tui.rs index b7c7c95..cf2943c 100644 --- a/src/debug_tui.rs +++ b/src/debug_tui.rs @@ -57,6 +57,7 @@ pub(crate) fn run() -> Result<()> { build_debug_rows(seed.get()) }; let mut sender = debug_signal_sender; + let mut await_pid_gone = |_pid: i32| {}; let mut app = App::with_rows(None, initial_rows); app.status = "debug tui: synthetic rows only".to_string(); let result = run_event_loop( @@ -65,6 +66,7 @@ pub(crate) fn run() -> Result<()> { &mut next_event, &mut refresh_rows, &mut sender, + &mut await_pid_gone, ); restore_terminal(terminal); result diff --git a/src/runtime.rs b/src/runtime.rs index ac6a63a..1586b39 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -146,6 +146,7 @@ pub fn apply_action( action: Action, refresh_rows: &mut dyn FnMut(Option<&process::FilterSpec>) -> Vec, sender: &mut dyn FnMut(i32, Signal) -> Result<(), String>, + await_pid_gone: &mut dyn FnMut(i32), ) -> ActionResult { match action { Action::Quit => ActionResult { @@ -218,7 +219,11 @@ pub fn apply_action( }; } - app.confirm_signal(sender); + if let Some(pid) = app.confirm_signal(sender) { + // Bridge the async-kill / proc-cleanup race so the upcoming + // refresh sees the dying process as gone instead of stale. + await_pid_gone(pid); + } refresh_with_selection_preserved(app, refresh_rows); ActionResult { should_quit: false, @@ -367,6 +372,7 @@ pub fn run_event_loop( next_event: &mut dyn FnMut(Duration) -> Result>, refresh_rows: &mut dyn FnMut(Option<&process::FilterSpec>) -> Vec, sender: &mut dyn FnMut(i32, Signal) -> Result<(), String>, + await_pid_gone: &mut dyn FnMut(i32), ) -> Result<()> { let mut needs_redraw = true; @@ -391,7 +397,7 @@ pub fn run_event_loop( app.pending_confirmation.is_some(), app.filter_input.is_some(), ); - let outcome = apply_action(app, action, refresh_rows, sender); + let outcome = apply_action(app, action, refresh_rows, sender, await_pid_gone); if outcome.should_quit { break; } @@ -435,6 +441,7 @@ pub fn run_interactive( &mut next_event, &mut refresh_rows, &mut sender, + &mut (signal::wait_for_pid_gone_default as fn(i32)), ); restore_terminal(terminal); result @@ -447,11 +454,19 @@ fn run_with_runtime( next_event: &mut dyn FnMut(Duration) -> Result>, refresh_rows: &mut dyn FnMut(Option<&process::FilterSpec>) -> Vec, sender: &mut dyn FnMut(i32, Signal) -> Result<(), String>, + await_pid_gone: &mut dyn FnMut(i32), ) -> Result<()> { let initial_rows = refresh_rows(compiled_filter.as_ref()); let mut app = App::with_rows(filter, initial_rows); app.compiled_filter = compiled_filter; - run_event_loop(&mut app, draw, next_event, refresh_rows, sender) + run_event_loop( + &mut app, + draw, + next_event, + refresh_rows, + sender, + await_pid_gone, + ) } /// Configure terminal raw mode and alternate screen for TUI rendering. @@ -494,6 +509,28 @@ mod tests { use std::time::Duration; use sysinfo::ProcessStatus; + /// Stand-in for the production `await_pid_gone` callback. Defined as a + /// real `fn` so the body is covered by `noop_await_runs` and every other + /// test can reference it without instantiating its own closure. + fn noop_await(_: i32) {} + + #[test] + fn noop_await_runs() { + noop_await(0); + } + + /// Panicking `await_pid_gone` stand-in for negative tests that assert the + /// callback is never invoked. Covered by `must_not_run_panics_when_called`. + fn must_not_run(_: i32) { + panic!("await_pid_gone must not be called when sender fails"); + } + + #[test] + #[should_panic(expected = "await_pid_gone must not be called when sender fails")] + fn must_not_run_panics_when_called() { + must_not_run(0); + } + fn row(pid: i32, name: &str) -> ProcRow { ProcRow { pid, @@ -595,7 +632,8 @@ mod tests { &mut app, Action::ConfirmPendingSignal, &mut refresh, - &mut sender + &mut sender, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -607,6 +645,48 @@ mod tests { assert!(app.pending_confirmation.is_none()); } + #[test] + fn apply_action_confirm_pending_signal_invokes_await_pid_gone_with_signaled_pid() { + let mut app = App::with_rows(None, vec![row(11, "foo")]); + app.begin_signal_confirmation(1); + let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo")]; + let mut sender = |_: i32, _: Signal| Ok(()); + let mut awaited: Option = None; + let mut await_pid_gone = |pid: i32| { + awaited = Some(pid); + }; + + apply_action( + &mut app, + Action::ConfirmPendingSignal, + &mut refresh, + &mut sender, + &mut await_pid_gone, + ); + + assert_eq!(awaited, Some(11)); + } + + #[test] + fn apply_action_confirm_pending_signal_skips_await_when_sender_fails() { + let mut app = App::with_rows(None, vec![row(11, "foo")]); + app.begin_signal_confirmation(1); + let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo")]; + let mut sender = |_: i32, _: Signal| Err("denied".to_string()); + + // `must_not_run` panics if invoked; reaching the assert means the await + // hook was correctly skipped on the sender-failure path. + apply_action( + &mut app, + Action::ConfirmPendingSignal, + &mut refresh, + &mut sender, + &mut (must_not_run as fn(i32)), + ); + + assert!(app.status.contains("failed")); + } + #[test] fn apply_action_cancel_pending_signal_clears_confirmation() { let mut app = App::with_rows(None, vec![row(11, "foo")]); @@ -619,7 +699,8 @@ mod tests { &mut app, Action::CancelPendingSignal, &mut refresh, - &mut sender + &mut sender, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -635,7 +716,13 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo")]; let mut sender = |_: i32, _: Signal| Ok(()); assert_eq!( - apply_action(&mut app, Action::Quit, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::Quit, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: true, needs_redraw: false @@ -649,7 +736,13 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(22, "bar")]; let mut sender = |_: i32, _: Signal| Ok(()); assert_eq!( - apply_action(&mut app, Action::Refresh, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::Refresh, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -665,10 +758,22 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| rows.clone(); let mut sender = |_: i32, _: Signal| Ok(()); // Exercise the refresh closure so its body is covered. - apply_action(&mut app, Action::Refresh, &mut refresh, &mut sender); + apply_action( + &mut app, + Action::Refresh, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); assert_eq!( - apply_action(&mut app, Action::MoveDown, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::MoveDown, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -677,7 +782,13 @@ mod tests { assert_eq!(app.table_state.selected(), Some(1)); assert_eq!( - apply_action(&mut app, Action::MoveUp, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::MoveUp, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -694,7 +805,13 @@ mod tests { let mut sender = |_: i32, _: Signal| Ok(()); assert_eq!( - apply_action(&mut app, Action::PageDown, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::PageDown, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -703,7 +820,13 @@ mod tests { assert_eq!(app.table_state.selected(), Some(10)); assert_eq!( - apply_action(&mut app, Action::PageUp, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::PageUp, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -745,7 +868,13 @@ mod tests { let mut sender = |_: i32, _: Signal| Ok(()); assert_eq!( - apply_action(&mut app, Action::CollapseTree, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::CollapseTree, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -754,7 +883,13 @@ mod tests { assert!(app.collapsed_pids.contains(&2)); assert_eq!( - apply_action(&mut app, Action::ExpandTree, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::ExpandTree, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: true @@ -774,7 +909,8 @@ mod tests { &mut app, Action::BeginSignalConfirmation(1), &mut refresh, - &mut sender + &mut sender, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -796,7 +932,8 @@ mod tests { &mut app, Action::ConfirmPendingSignal, &mut refresh, - &mut sender + &mut sender, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -813,7 +950,13 @@ mod tests { let mut sender = |_: i32, _: Signal| Ok(()); let selected = app.table_state.selected(); assert_eq!( - apply_action(&mut app, Action::Noop, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::Noop, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)) + ), ActionResult { should_quit: false, needs_redraw: false @@ -847,6 +990,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); @@ -881,6 +1025,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); @@ -913,6 +1058,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); @@ -939,6 +1085,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); } @@ -957,6 +1104,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); assert!(result.is_err()); } @@ -977,6 +1125,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); assert!(result.is_err()); } @@ -1009,6 +1158,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ) .expect("runtime should terminate cleanly"); @@ -1056,7 +1206,8 @@ mod tests { &mut app, Action::BeginInteractiveFilter, &mut refresh, - &mut sender + &mut sender, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -1083,6 +1234,7 @@ mod tests { Action::FilterInputChar('f'), &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, "f"); @@ -1104,6 +1256,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); assert_eq!(app.filter_input.as_ref().unwrap().text, "f"); } @@ -1118,7 +1271,13 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo")]; let mut sender = |_: i32, _: Signal| Ok(()); - apply_action(&mut app, Action::FilterConfirm, &mut refresh, &mut sender); + apply_action( + &mut app, + Action::FilterConfirm, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); assert!(app.filter_input.is_none()); assert_eq!(app.filter.as_deref(), Some("foo")); assert!(app.compiled_filter.is_some()); @@ -1134,7 +1293,13 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo")]; let mut sender = |_: i32, _: Signal| Ok(()); - let result = apply_action(&mut app, Action::FilterCancel, &mut refresh, &mut sender); + let result = apply_action( + &mut app, + Action::FilterCancel, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); assert!(app.filter_input.is_none()); assert!(result.needs_redraw); } @@ -1197,6 +1362,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, "foo"); @@ -1214,6 +1380,7 @@ mod tests { Action::FilterInputChar('x'), &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); assert!(!result.needs_redraw); // Rows must not change since filter mode is not active. @@ -1231,6 +1398,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); assert!(!result.needs_redraw); assert_eq!(app.rows[0].pid, 11); @@ -1254,6 +1422,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, ""); @@ -1266,7 +1435,13 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo")]; let mut sender = |_: i32, _: Signal| Ok(()); - apply_action(&mut app, Action::FilterConfirm, &mut refresh, &mut sender); + apply_action( + &mut app, + Action::FilterConfirm, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); // filter_input was None, compiled_filter stays None, rows refresh with None filter. assert!(app.filter_input.is_none()); assert!(app.compiled_filter.is_none()); @@ -1283,7 +1458,13 @@ mod tests { |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo"), row(22, "bar")]; let mut sender = |_: i32, _: Signal| Ok(()); - apply_action(&mut app, Action::FilterConfirm, &mut refresh, &mut sender); + apply_action( + &mut app, + Action::FilterConfirm, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); assert!(app.filter_input.is_none()); assert!(app.filter.is_none()); assert!(app.compiled_filter.is_none()); @@ -1295,7 +1476,13 @@ mod tests { let mut refresh = |_: Option<&crate::process::FilterSpec>| vec![row(22, "bar")]; let mut sender = |_: i32, _: Signal| Ok(()); - let result = apply_action(&mut app, Action::FilterCancel, &mut refresh, &mut sender); + let result = apply_action( + &mut app, + Action::FilterCancel, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); assert!(!result.needs_redraw); // Rows must not change since there was nothing to cancel. assert_eq!(app.rows[0].pid, 11); @@ -1323,6 +1510,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); // The row list must already reflect the pre-filled filter. assert_eq!(app.rows.len(), 1); @@ -1341,7 +1529,13 @@ mod tests { |_: Option<&crate::process::FilterSpec>| vec![row(11, "foo"), row(22, "bar")]; let mut sender = |_: i32, _: Signal| Ok(()); - apply_action(&mut app, Action::FilterCancel, &mut refresh, &mut sender); + apply_action( + &mut app, + Action::FilterCancel, + &mut refresh, + &mut sender, + &mut (noop_await as fn(i32)), + ); assert_eq!(app.table_state.selected(), Some(0)); } @@ -1362,6 +1556,7 @@ mod tests { Action::FilterInputChar('f'), &mut refresh, &mut sender, + &mut (noop_await as fn(i32)), ); assert_eq!(app.table_state.selected(), Some(0)); } diff --git a/src/signal.rs b/src/signal.rs index f6790bc..57be25f 100644 --- a/src/signal.rs +++ b/src/signal.rs @@ -16,8 +16,16 @@ //! Signal mapping and process signaling helpers. -use nix::sys::signal::{Signal, kill}; -use nix::unistd::Pid; +use std::{ + thread, + time::{Duration, Instant}, +}; + +use nix::{ + errno::Errno, + sys::signal::{Signal, kill}, + unistd::Pid, +}; /// Map keyboard digits 1-9 to Unix signals. pub fn signal_from_digit(digit: u8) -> Option { @@ -40,10 +48,63 @@ pub fn send_signal(pid: i32, signal: Signal) -> nix::Result<()> { kill(Pid::from_raw(pid), signal) } +/// Upper bound on how long we wait for a signaled process to disappear from +/// `/proc` before giving up and refreshing anyway. Sized to be short enough that +/// the event loop freeze is not perceptible for catchable signals (SIGHUP, +/// SIGINT, …) that the target may not honor, while still leaving comfortable +/// headroom for SIGKILL — which the kernel typically reaps within a few ms. +pub const SIGNAL_REAP_TIMEOUT: Duration = Duration::from_millis(100); + +/// Polling interval for the existence probe inside [`wait_for_pid_gone`]. +pub const SIGNAL_REAP_POLL_INTERVAL: Duration = Duration::from_millis(10); + +/// Poll until `pid` is absent from the process table or `timeout` elapses. +/// +/// Probes existence with `kill(pid, None)` (POSIX null-signal): `Err(ESRCH)` +/// means the pid no longer exists. Bridges the gap between `kill(2)` returning +/// (signal queued) and the kernel actually removing the entry from `/proc`, +/// so a refresh issued right after a signal send sees the updated state. +/// +/// Returns `true` if the process is gone before the deadline, `false` if it is +/// still alive after `timeout`. The first probe always runs, so a `timeout` of +/// `Duration::ZERO` still returns `true` for an already-absent pid. +/// +/// PID reuse: if the kernel reaps the original and recycles the pid for an +/// unrelated process inside the polling window, this returns `false`. The +/// caller must re-key on `(pid, start_time)` (see [`crate::app::App`]'s +/// `pending_target_matches_current_rows`) to detect that case after the +/// follow-up refresh. +pub fn wait_for_pid_gone(pid: i32, timeout: Duration, interval: Duration) -> bool { + let started = Instant::now(); + loop { + if matches!(kill(Pid::from_raw(pid), None::), Err(Errno::ESRCH)) { + return true; + } + let elapsed = started.elapsed(); + if elapsed >= timeout { + return false; + } + let remaining = timeout - elapsed; + thread::sleep(interval.min(remaining)); + } +} + +/// Convenience wrapper around [`wait_for_pid_gone`] using the project-default +/// timeout and polling interval. The boolean outcome is intentionally +/// discarded: the caller will refresh from `/proc` next, which surfaces the +/// truth (gone vs. still-alive after a non-fatal signal) either way. +pub fn wait_for_pid_gone_default(pid: i32) { + let _ = wait_for_pid_gone(pid, SIGNAL_REAP_TIMEOUT, SIGNAL_REAP_POLL_INTERVAL); +} + #[cfg(test)] mod tests { - use super::{send_signal, signal_from_digit}; + use super::{send_signal, signal_from_digit, wait_for_pid_gone, wait_for_pid_gone_default}; use nix::sys::signal::Signal; + use std::{ + process::Command, + time::{Duration, Instant}, + }; #[test] fn signal_from_digit_maps_expected_range() { @@ -64,4 +125,51 @@ mod tests { let pid = std::process::id() as i32; assert!(send_signal(pid, Signal::SIGCONT).is_ok()); } + + #[test] + fn wait_for_pid_gone_returns_true_for_nonexistent_pid() { + assert!(wait_for_pid_gone( + i32::MAX - 1, + Duration::from_millis(50), + Duration::from_millis(10), + )); + } + + #[test] + fn wait_for_pid_gone_times_out_for_living_process() { + let pid = std::process::id() as i32; + let started = Instant::now(); + let gone = wait_for_pid_gone(pid, Duration::from_millis(50), Duration::from_millis(10)); + assert!(!gone); + assert!(started.elapsed() >= Duration::from_millis(50)); + } + + #[test] + fn wait_for_pid_gone_zero_timeout_polls_at_least_once() { + assert!(wait_for_pid_gone( + i32::MAX - 1, + Duration::ZERO, + Duration::ZERO, + )); + } + + #[test] + fn wait_for_pid_gone_detects_child_exit() { + let mut child = Command::new("true").spawn().expect("spawn `true`"); + let pid = child.id() as i32; + child.wait().expect("reap child"); + assert!(wait_for_pid_gone( + pid, + Duration::from_millis(500), + Duration::from_millis(10), + )); + } + + #[test] + fn wait_for_pid_gone_default_returns_quickly_for_nonexistent_pid() { + let started = Instant::now(); + wait_for_pid_gone_default(i32::MAX - 1); + // Nonexistent pid resolves on the first probe, well below the cap. + assert!(started.elapsed() < super::SIGNAL_REAP_TIMEOUT); + } }