Skip to content

Commit e9e4fc6

Browse files
jmoseleyCopilot
andcommitted
[Rust] Report a refused kill instead of waiting on a live process
`start_kill` failures were logged and swallowed. That is right when the child is already gone — the reap then reports its real status — but wrong when the OS refuses the signal: nothing will make that process exit, so the reaper would poll forever, every `ForcedShutdown::wait()` would park indefinitely, and `force_stop()` would report success. Distinguish the two with `signal_refusal`, which treats a failed kill as benign only when the child has actually exited. A genuine refusal is published as a definitive error *before* the reaper starts, so waiters observe it rather than blocking. Ownership is unchanged: the reaper still takes the child and still overwrites the error with the truth if the process does go away, so a refusal degrades to an observable error rather than to a silent success or a hang. The decision is a pure function so it can be tested directly; fabricating a real refusal would mean signalling a process the test does not own. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f9d5ac7-9999-4f37-82b3-a5533bfbc1f6
1 parent 0601c94 commit e9e4fc6

1 file changed

Lines changed: 58 additions & 6 deletions

File tree

rust/src/child.rs

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -134,25 +134,36 @@ impl ChildLifecycle {
134134
match slot.take() {
135135
Some(mut child) => {
136136
let pid = child.id();
137-
if let Err(error) = child.start_kill() {
138-
// Usually just an already-exited child; the wait
139-
// below still reports its real status.
140-
warn!(pid = ?pid, error = %error, "kill signal not delivered to CLI process");
137+
let kill = child.start_kill();
138+
let refusal = signal_refusal(kill, || matches!(child.try_wait(), Ok(Some(_))));
139+
if let Some(error) = &refusal {
140+
warn!(pid = ?pid, error = %error, "kill signal refused by the OS");
141141
}
142142
self.state.send_replace(ReapState::Pending);
143-
(Some(child), self.handle())
143+
(Some((child, refusal)), self.handle())
144144
}
145145
// Either termination already started (the running reaper
146146
// will publish) or there was never a child (already
147147
// `Reaped(None)`).
148148
None => (None, self.handle()),
149149
}
150150
};
151-
let Some(child) = claimed else {
151+
let Some((child, refusal)) = claimed else {
152152
return handle;
153153
};
154154

155155
let pid = child.id();
156+
// Publish the refusal *before* the reaper starts, so waiters see it
157+
// rather than blocking on a process that was never signalled. The
158+
// reaper still takes ownership and still overwrites this with the
159+
// truth if the process does go away, so a refusal degrades to an
160+
// observable error rather than to a silent success or a hang.
161+
if let Some(error) = refusal {
162+
self.state.send_replace(ReapState::Failed {
163+
kind: error.kind(),
164+
message: format!("the CLI child could not be signalled: {error}"),
165+
});
166+
}
156167
info!(pid = ?pid, "terminating CLI process");
157168
// Reap on a dedicated thread with its own runtime rather than on a
158169
// caller's. A `tokio::spawn`ed task is cancelled when its runtime
@@ -241,6 +252,24 @@ impl Drop for ReapGuard {
241252
}
242253
}
243254

255+
/// Classify a `start_kill` result.
256+
///
257+
/// A failed kill is benign when the child is already gone — the reap
258+
/// reports its real status. Anything else means the signal was refused, so
259+
/// nothing will make the process exit and it must not be reported as a
260+
/// successful stop. `exited` is evaluated only when it matters, since it
261+
/// costs a `waitpid`.
262+
fn signal_refusal(
263+
kill: std::io::Result<()>,
264+
exited: impl FnOnce() -> bool,
265+
) -> Option<std::io::Error> {
266+
match kill {
267+
Ok(()) => None,
268+
Err(_) if exited() => None,
269+
Err(error) => Some(error),
270+
}
271+
}
272+
244273
/// Wait for the OS to release a child that has already been signalled.
245274
///
246275
/// Polls rather than awaiting `Child::wait`. The child is usually a zombie
@@ -725,6 +754,29 @@ mod tests {
725754
}
726755
}
727756

757+
/// A kill that fails because the child is already gone is benign —
758+
/// the reap reports its real status. A kill the OS refuses is not:
759+
/// nothing will make that process exit, so it must surface as an
760+
/// error rather than as a successful stop or an endless wait.
761+
#[test]
762+
fn signal_refusal_distinguishes_a_dead_child_from_a_refused_kill() {
763+
assert!(signal_refusal(Ok(()), || panic!("must not probe on success")).is_none());
764+
assert!(
765+
signal_refusal(
766+
Err(std::io::Error::from(std::io::ErrorKind::InvalidInput)),
767+
|| true
768+
)
769+
.is_none(),
770+
"an already-exited child is not a refusal"
771+
);
772+
let refusal = signal_refusal(
773+
Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)),
774+
|| false,
775+
)
776+
.expect("a refused kill on a live child must be reported");
777+
assert_eq!(refusal.kind(), std::io::ErrorKind::PermissionDenied);
778+
}
779+
728780
#[test]
729781
fn forced_shutdown_handle_is_send_and_static() {
730782
fn assert_send_static<T: Send + 'static>() {}

0 commit comments

Comments
 (0)