Skip to content

Commit 0601c94

Browse files
jmoseleyCopilot
andcommitted
[Rust] Reap by polling instead of SIGCHLD wakeup
CI caught this on Ubuntu: dropping a client left its CLI process in `Z` for the full 5s the new test allows. The reaper awaited `Child::wait`, which on Unix falls back to the SIGCHLD driver once `try_wait` misses — and the signal registration belongs to the runtime that spawned the child, not to the reaper's own. The reaper could therefore sleep waiting for a wakeup that was delivered elsewhere. macOS happened to win the race locally; Linux did not. Poll `try_wait` on a short backoff (2ms doubling to 50ms) instead. That makes the reap independent of which runtime owns the signal registration, which is the whole premise of running it on its own thread. It costs a handful of wakeups once per client teardown. `enable_all()` on the reaper runtime stays load-bearing, now for the timer rather than the signal driver, and the test that guards it is unchanged: it reaps a child that is still running at the first check. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0f9d5ac7-9999-4f37-82b3-a5533bfbc1f6
1 parent 17b1a7f commit 0601c94

1 file changed

Lines changed: 36 additions & 11 deletions

File tree

rust/src/child.rs

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
2121
use std::process::ExitStatus;
2222
use std::sync::Arc;
23+
use std::time::Duration;
2324

2425
use parking_lot::Mutex;
2526
use tokio::process::Child;
@@ -171,10 +172,9 @@ impl ChildLifecycle {
171172
// Moved in, so it drops with the thread even if the body
172173
// panics before publishing.
173174
let mut guard = guard;
174-
// `enable_all` is load-bearing on Unix, not boilerplate:
175-
// once `try_wait` misses, readiness for `Child::wait`
176-
// comes from the signal driver. Without it, a child that
177-
// outlives the reaper's first poll — a process wedged in
175+
// `enable_all` is load-bearing, not boilerplate: `reap`
176+
// sleeps between polls, so without the timer a child that
177+
// outlives the first check — a process wedged in
178178
// uninterruptible I/O, exactly what `force_stop` exists
179179
// for — would never be reaped.
180180
match tokio::runtime::Builder::new_current_thread()
@@ -242,11 +242,36 @@ impl Drop for ReapGuard {
242242
}
243243

244244
/// Wait for the OS to release a child that has already been signalled.
245+
///
246+
/// Polls rather than awaiting `Child::wait`. The child is usually a zombie
247+
/// by the first check, but not always, and once `try_wait` misses,
248+
/// `wait` depends on the SIGCHLD driver waking *this* runtime — which is
249+
/// not guaranteed when the child was spawned under a different one, as it
250+
/// always is here. Polling keeps the reap independent of which runtime
251+
/// owns the signal registration. It runs once per client teardown, so the
252+
/// wakeups are immaterial.
245253
async fn reap(mut child: Child) -> ReapState {
254+
const FIRST_INTERVAL: Duration = Duration::from_millis(2);
255+
const MAX_INTERVAL: Duration = Duration::from_millis(50);
256+
246257
let pid = child.id();
247-
let state = ReapState::from_wait(child.wait().await);
248-
info!(pid = ?pid, outcome = ?state, "CLI process reaped");
249-
state
258+
let mut interval = FIRST_INTERVAL;
259+
loop {
260+
match child.try_wait() {
261+
Ok(Some(status)) => {
262+
info!(pid = ?pid, ?status, "CLI process reaped");
263+
return ReapState::Reaped(Some(status));
264+
}
265+
Ok(None) => {
266+
tokio::time::sleep(interval).await;
267+
interval = (interval * 2).min(MAX_INTERVAL);
268+
}
269+
Err(error) => {
270+
warn!(pid = ?pid, error = %error, "could not reap the CLI process");
271+
return ReapState::from_wait(Err(error));
272+
}
273+
}
274+
}
250275
}
251276
/// Owned handle to a CLI process being terminated.
252277
///
@@ -635,10 +660,10 @@ mod tests {
635660
/// load-bearing rather than boilerplate.
636661
///
637662
/// Every other test kills its child before the reaper runs, so the
638-
/// child is already a zombie and the first `try_wait` succeeds — the
639-
/// signal path is never touched, and the suite would pass even without
640-
/// a signal driver. This case waits on a child that is still running
641-
/// at the first poll, on a runtime other than the one that spawned it:
663+
/// child is already a zombie and the very first `try_wait` succeeds —
664+
/// the retry loop is never entered, and the suite would pass even
665+
/// without a timer. This case reaps a child that is still running at
666+
/// the first check, on a runtime other than the one that spawned it:
642667
/// the shape of the wedged process `force_stop` exists for.
643668
#[test]
644669
fn reap_completes_for_a_child_still_running_at_the_first_poll() {

0 commit comments

Comments
 (0)