From 1f7f3895a710e7851aeb27bb8f8cec812015d851 Mon Sep 17 00:00:00 2001 From: l5y <220195275+l5yth@users.noreply.github.com> Date: Tue, 5 May 2026 08:58:03 +0200 Subject: [PATCH 1/3] fix bug where killed process linger in list --- src/runtime.rs | 12 +++++++- src/signal.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/src/runtime.rs b/src/runtime.rs index ac6a63a..0a66b33 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -427,7 +427,17 @@ pub fn run_interactive( }; let mut refresh_rows = |filter: Option<&process::FilterSpec>| process::refresh_rows(&mut sys, filter, user_only); - let mut sender = |pid, sig| signal::send_signal(pid, sig).map_err(|err| err.to_string()); + let mut sender = |pid, sig| { + let result = signal::send_signal(pid, sig).map_err(|err| err.to_string()); + if result.is_ok() { + let _ = signal::wait_for_pid_gone( + pid, + Duration::from_millis(200), + Duration::from_millis(20), + ); + } + result + }; let result = run_with_runtime( filter, compiled_filter, diff --git a/src/signal.rs b/src/signal.rs index f6790bc..c767ea7 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,39 @@ pub fn send_signal(pid: i32, signal: Signal) -> nix::Result<()> { kill(Pid::from_raw(pid), signal) } +/// 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. +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)); + } +} + #[cfg(test)] mod tests { - use super::{send_signal, signal_from_digit}; + use super::{send_signal, signal_from_digit, wait_for_pid_gone}; use nix::sys::signal::Signal; + use std::{ + process::Command, + time::{Duration, Instant}, + }; #[test] fn signal_from_digit_maps_expected_range() { @@ -64,4 +101,43 @@ 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), + )); + } } From 43cd4c486725491ade6cba62ab7273a5f44c9b07 Mon Sep 17 00:00:00 2001 From: l5y <220195275+l5yth@users.noreply.github.com> Date: Tue, 5 May 2026 11:36:59 +0200 Subject: [PATCH 2/3] address review comments --- src/app.rs | 15 ++- src/debug_tui.rs | 2 + src/runtime.rs | 237 ++++++++++++++++++++++++++++++++++++++++------- src/signal.rs | 34 ++++++- 4 files changed, 248 insertions(+), 40 deletions(-) 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 0a66b33..630fb90 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; } @@ -427,17 +433,8 @@ pub fn run_interactive( }; let mut refresh_rows = |filter: Option<&process::FilterSpec>| process::refresh_rows(&mut sys, filter, user_only); - let mut sender = |pid, sig| { - let result = signal::send_signal(pid, sig).map_err(|err| err.to_string()); - if result.is_ok() { - let _ = signal::wait_for_pid_gone( - pid, - Duration::from_millis(200), - Duration::from_millis(20), - ); - } - result - }; + let mut sender = |pid, sig| signal::send_signal(pid, sig).map_err(|err| err.to_string()); + let mut await_pid_gone = |pid: i32| signal::wait_for_pid_gone_default(pid); let result = run_with_runtime( filter, compiled_filter, @@ -445,6 +442,7 @@ pub fn run_interactive( &mut next_event, &mut refresh_rows, &mut sender, + &mut await_pid_gone, ); restore_terminal(terminal); result @@ -457,11 +455,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. @@ -605,7 +611,8 @@ mod tests { &mut app, Action::ConfirmPendingSignal, &mut refresh, - &mut sender + &mut sender, + &mut |_: i32| {}, ), ActionResult { should_quit: false, @@ -617,6 +624,51 @@ 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()); + let mut awaited = false; + let mut await_pid_gone = |_: i32| { + awaited = true; + }; + + apply_action( + &mut app, + Action::ConfirmPendingSignal, + &mut refresh, + &mut sender, + &mut await_pid_gone, + ); + + assert!(!awaited); + 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")]); @@ -629,7 +681,8 @@ mod tests { &mut app, Action::CancelPendingSignal, &mut refresh, - &mut sender + &mut sender, + &mut |_: i32| {}, ), ActionResult { should_quit: false, @@ -645,7 +698,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 |_: i32| {} + ), ActionResult { should_quit: true, needs_redraw: false @@ -659,7 +718,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -675,10 +740,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 |_: i32| {}, + ); assert_eq!( - apply_action(&mut app, Action::MoveDown, &mut refresh, &mut sender), + apply_action( + &mut app, + Action::MoveDown, + &mut refresh, + &mut sender, + &mut |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -687,7 +764,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -704,7 +787,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -713,7 +802,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -755,7 +850,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -764,7 +865,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: true @@ -784,7 +891,8 @@ mod tests { &mut app, Action::BeginSignalConfirmation(1), &mut refresh, - &mut sender + &mut sender, + &mut |_: i32| {}, ), ActionResult { should_quit: false, @@ -806,7 +914,8 @@ mod tests { &mut app, Action::ConfirmPendingSignal, &mut refresh, - &mut sender + &mut sender, + &mut |_: i32| {}, ), ActionResult { should_quit: false, @@ -823,7 +932,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 |_: i32| {} + ), ActionResult { should_quit: false, needs_redraw: false @@ -857,6 +972,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ) .expect("loop should terminate cleanly"); @@ -891,6 +1007,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ) .expect("loop should terminate cleanly"); @@ -923,6 +1040,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ) .expect("loop should terminate cleanly"); @@ -949,6 +1067,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ) .expect("loop should terminate cleanly"); } @@ -967,6 +1086,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ); assert!(result.is_err()); } @@ -987,6 +1107,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ); assert!(result.is_err()); } @@ -1019,6 +1140,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, + &mut |_: i32| {}, ) .expect("runtime should terminate cleanly"); @@ -1066,7 +1188,8 @@ mod tests { &mut app, Action::BeginInteractiveFilter, &mut refresh, - &mut sender + &mut sender, + &mut |_: i32| {}, ), ActionResult { should_quit: false, @@ -1093,6 +1216,7 @@ mod tests { Action::FilterInputChar('f'), &mut refresh, &mut sender, + &mut |_: i32| {}, ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, "f"); @@ -1114,6 +1238,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, + &mut |_: i32| {}, ); assert_eq!(app.filter_input.as_ref().unwrap().text, "f"); } @@ -1128,7 +1253,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 |_: i32| {}, + ); assert!(app.filter_input.is_none()); assert_eq!(app.filter.as_deref(), Some("foo")); assert!(app.compiled_filter.is_some()); @@ -1144,7 +1275,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 |_: i32| {}, + ); assert!(app.filter_input.is_none()); assert!(result.needs_redraw); } @@ -1207,6 +1344,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, + &mut |_: i32| {}, ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, "foo"); @@ -1224,6 +1362,7 @@ mod tests { Action::FilterInputChar('x'), &mut refresh, &mut sender, + &mut |_: i32| {}, ); assert!(!result.needs_redraw); // Rows must not change since filter mode is not active. @@ -1241,6 +1380,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, + &mut |_: i32| {}, ); assert!(!result.needs_redraw); assert_eq!(app.rows[0].pid, 11); @@ -1264,6 +1404,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, + &mut |_: i32| {}, ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, ""); @@ -1276,7 +1417,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 |_: 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()); @@ -1293,7 +1440,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 |_: i32| {}, + ); assert!(app.filter_input.is_none()); assert!(app.filter.is_none()); assert!(app.compiled_filter.is_none()); @@ -1305,7 +1458,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 |_: i32| {}, + ); assert!(!result.needs_redraw); // Rows must not change since there was nothing to cancel. assert_eq!(app.rows[0].pid, 11); @@ -1333,6 +1492,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, + &mut |_: i32| {}, ); // The row list must already reflect the pre-filled filter. assert_eq!(app.rows.len(), 1); @@ -1351,7 +1511,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 |_: i32| {}, + ); assert_eq!(app.table_state.selected(), Some(0)); } @@ -1372,6 +1538,7 @@ mod tests { Action::FilterInputChar('f'), &mut refresh, &mut sender, + &mut |_: i32| {}, ); assert_eq!(app.table_state.selected(), Some(0)); } diff --git a/src/signal.rs b/src/signal.rs index c767ea7..57be25f 100644 --- a/src/signal.rs +++ b/src/signal.rs @@ -48,6 +48,16 @@ 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)` @@ -58,6 +68,12 @@ pub fn send_signal(pid: i32, signal: Signal) -> nix::Result<()> { /// 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 { @@ -73,9 +89,17 @@ pub fn wait_for_pid_gone(pid: i32, timeout: Duration, interval: Duration) -> boo } } +/// 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, wait_for_pid_gone}; + 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, @@ -140,4 +164,12 @@ mod tests { 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); + } } From cbcad3ca15aa2b4f3dfeda8433aaa2c5f0504764 Mon Sep 17 00:00:00 2001 From: l5y <220195275+l5yth@users.noreply.github.com> Date: Tue, 5 May 2026 12:15:45 +0200 Subject: [PATCH 3/3] address review comments --- src/runtime.rs | 106 +++++++++++++++++++++++++++++-------------------- 1 file changed, 62 insertions(+), 44 deletions(-) diff --git a/src/runtime.rs b/src/runtime.rs index 630fb90..1586b39 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -434,7 +434,6 @@ pub fn run_interactive( let mut refresh_rows = |filter: Option<&process::FilterSpec>| process::refresh_rows(&mut sys, filter, user_only); let mut sender = |pid, sig| signal::send_signal(pid, sig).map_err(|err| err.to_string()); - let mut await_pid_gone = |pid: i32| signal::wait_for_pid_gone_default(pid); let result = run_with_runtime( filter, compiled_filter, @@ -442,7 +441,7 @@ pub fn run_interactive( &mut next_event, &mut refresh_rows, &mut sender, - &mut await_pid_gone, + &mut (signal::wait_for_pid_gone_default as fn(i32)), ); restore_terminal(terminal); result @@ -510,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, @@ -612,7 +633,7 @@ mod tests { Action::ConfirmPendingSignal, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -652,20 +673,17 @@ mod tests { 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()); - let mut awaited = false; - let mut await_pid_gone = |_: i32| { - awaited = true; - }; + // `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 await_pid_gone, + &mut (must_not_run as fn(i32)), ); - assert!(!awaited); assert!(app.status.contains("failed")); } @@ -682,7 +700,7 @@ mod tests { Action::CancelPendingSignal, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -703,7 +721,7 @@ mod tests { Action::Quit, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: true, @@ -723,7 +741,7 @@ mod tests { Action::Refresh, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -745,7 +763,7 @@ mod tests { Action::Refresh, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert_eq!( @@ -754,7 +772,7 @@ mod tests { Action::MoveDown, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -769,7 +787,7 @@ mod tests { Action::MoveUp, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -792,7 +810,7 @@ mod tests { Action::PageDown, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -807,7 +825,7 @@ mod tests { Action::PageUp, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -855,7 +873,7 @@ mod tests { Action::CollapseTree, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -870,7 +888,7 @@ mod tests { Action::ExpandTree, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -892,7 +910,7 @@ mod tests { Action::BeginSignalConfirmation(1), &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -915,7 +933,7 @@ mod tests { Action::ConfirmPendingSignal, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -937,7 +955,7 @@ mod tests { Action::Noop, &mut refresh, &mut sender, - &mut |_: i32| {} + &mut (noop_await as fn(i32)) ), ActionResult { should_quit: false, @@ -972,7 +990,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); @@ -1007,7 +1025,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); @@ -1040,7 +1058,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); @@ -1067,7 +1085,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ) .expect("loop should terminate cleanly"); } @@ -1086,7 +1104,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(result.is_err()); } @@ -1107,7 +1125,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(result.is_err()); } @@ -1140,7 +1158,7 @@ mod tests { &mut next_event, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ) .expect("runtime should terminate cleanly"); @@ -1189,7 +1207,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ), ActionResult { should_quit: false, @@ -1216,7 +1234,7 @@ mod tests { Action::FilterInputChar('f'), &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, "f"); @@ -1238,7 +1256,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert_eq!(app.filter_input.as_ref().unwrap().text, "f"); } @@ -1258,7 +1276,7 @@ mod tests { Action::FilterConfirm, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(app.filter_input.is_none()); assert_eq!(app.filter.as_deref(), Some("foo")); @@ -1280,7 +1298,7 @@ mod tests { Action::FilterCancel, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(app.filter_input.is_none()); assert!(result.needs_redraw); @@ -1344,7 +1362,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, "foo"); @@ -1362,7 +1380,7 @@ mod tests { Action::FilterInputChar('x'), &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(!result.needs_redraw); // Rows must not change since filter mode is not active. @@ -1380,7 +1398,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(!result.needs_redraw); assert_eq!(app.rows[0].pid, 11); @@ -1404,7 +1422,7 @@ mod tests { Action::FilterInputBackspace, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); let fi = app.filter_input.as_ref().unwrap(); assert_eq!(fi.text, ""); @@ -1422,7 +1440,7 @@ mod tests { Action::FilterConfirm, &mut refresh, &mut sender, - &mut |_: i32| {}, + &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()); @@ -1445,7 +1463,7 @@ mod tests { Action::FilterConfirm, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(app.filter_input.is_none()); assert!(app.filter.is_none()); @@ -1463,7 +1481,7 @@ mod tests { Action::FilterCancel, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert!(!result.needs_redraw); // Rows must not change since there was nothing to cancel. @@ -1492,7 +1510,7 @@ mod tests { Action::BeginInteractiveFilter, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); // The row list must already reflect the pre-filled filter. assert_eq!(app.rows.len(), 1); @@ -1516,7 +1534,7 @@ mod tests { Action::FilterCancel, &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert_eq!(app.table_state.selected(), Some(0)); } @@ -1538,7 +1556,7 @@ mod tests { Action::FilterInputChar('f'), &mut refresh, &mut sender, - &mut |_: i32| {}, + &mut (noop_await as fn(i32)), ); assert_eq!(app.table_state.selected(), Some(0)); }