@sigitcodereview / sigit / commits / 1422808

fix: kill the hook's whole process group on timeout

The Linux CI runner exposed a hole in the hook timeout: sh -c on dash forks the command instead of exec'ing it, so killing the shell at the deadline left the grandchild alive. It kept the stdout/stderr pipes open, and the drain threads' read_to_end blocked the caller until the grandchild exited on its own — 30 seconds for the test's sleep, unbounded in general. Hooks now run in their own process group, and the timeout kill signals the group, taking the whole tree with it. Output collection is also bounded: the drain threads report over a channel read with a short grace period, so even a daemonized straggler that escaped the group and still holds a pipe can only delay the caller briefly instead of hanging it. The test now uses a two-command hook to force the fork on every shell, not just dash.

paydii committed Jul 11, 2026 at 16:03 UTC 14228086412565adf6a141bd13513580911e6339

1 file changed +64 -23

src/hooks.rs

+64 -23

@@ -192,15 +192,22 @@ fn run_hook(cmd: &str, context: &HookContext, cwd: &Path, timeout: Duration) {

192 let (shell, flag, substituted) = ("cmd", "/C", context.substitute(cmd, windows_quote));

193

194 log::debug!("Running hook: {substituted}");

195 - let mut child = match Command::new(shell)

195 + let mut command = Command::new(shell);

196 + command

197 .arg(flag)

198 .arg(&substituted)

199 .current_dir(cwd)

200 .envs(context.env_vars())

201 .stdout(Stdio::piped())

201 - .stderr(Stdio::piped())

202 - .spawn()

203 - {

202 + .stderr(Stdio::piped());

203 + // Put the hook in its own process group so a timeout kill can take out the

204 + // whole tree. Killing just the shell is not enough: `sh -c` may fork

205 + // rather than exec (dash does), and the surviving grandchild would both

206 + // keep running and hold the pipe write ends open, blocking the drains

207 + // below until it exits on its own.

208 + #[cfg(unix)]

209 + std::os::unix::process::CommandExt::process_group(&mut command, 0);

210 + let mut child = match command.spawn() {

211 Ok(child) => child,

212 Err(err) => {

213 log::warn!("Failed to run hook: {err}");

@@ -208,21 +215,40 @@ fn run_hook(cmd: &str, context: &HookContext, cwd: &Path, timeout: Duration) {

215 }

216 };

217

218 + #[cfg(unix)]

219 + fn kill_hook(child: &mut std::process::Child) {

220 + // SIGKILL the process group (negative pid); the child is its leader.

221 + // Fall back to killing the child alone if that fails.

222 + if unsafe { libc::kill(-(child.id() as i32), libc::SIGKILL) } != 0 {

223 + let _ = child.kill();

224 + }

225 + let _ = child.wait();

226 + }

227 + #[cfg(windows)]

228 + fn kill_hook(child: &mut std::process::Child) {

229 + let _ = child.kill();

230 + let _ = child.wait();

231 + }

232 +

233 // Drain stdout/stderr on their own threads so a hook that writes more than

234 // the pipe buffer holds (a build, say) keeps making progress instead of

213 - // blocking on a full pipe and looking like a hang. The main thread polls

214 - // for exit and enforces the deadline.

215 - fn drain<R: Read + Send + 'static>(pipe: Option<R>) -> std::thread::JoinHandle<Vec<u8>> {

235 + // blocking on a full pipe and looking like a hang. The threads report back

236 + // over a channel rather than being joined: a receive can be bounded, while

237 + // a join could block forever on a pipe a detached grandchild still holds.

238 + let (out_tx, out_rx) = std::sync::mpsc::channel::<(u8, Vec<u8>)>();

239 + let drain = |tag: u8, pipe: Option<Box<dyn Read + Send>>| {

240 + let tx = out_tx.clone();

241 std::thread::spawn(move || {

242 let mut buf = Vec::new();

243 if let Some(mut p) = pipe {

244 let _ = p.read_to_end(&mut buf);

245 }

221 - buf

222 - })

223 - }

224 - let stdout_handle = drain(child.stdout.take());

225 - let stderr_handle = drain(child.stderr.take());

246 + let _ = tx.send((tag, buf));

247 + });

248 + };

249 + drain(0, child.stdout.take().map(|p| Box::new(p) as _));

250 + drain(1, child.stderr.take().map(|p| Box::new(p) as _));

251 + drop(out_tx);

252

253 let deadline = Instant::now() + timeout;

254 let status = loop {

@@ -230,25 +256,36 @@ fn run_hook(cmd: &str, context: &HookContext, cwd: &Path, timeout: Duration) {

256 Ok(Some(status)) => break Some(status),

257 Ok(None) => {

258 if Instant::now() >= deadline {

233 - let _ = child.kill();

234 - let _ = child.wait();

259 + kill_hook(&mut child);

260 break None;

261 }

262 std::thread::sleep(Duration::from_millis(50));

263 }

264 Err(err) => {

265 log::warn!("Failed to wait on hook: {err}");

241 - let _ = child.kill();

242 - let _ = child.wait();

266 + kill_hook(&mut child);

267 break None;

268 }

269 }

270 };

271

248 - // The child has exited (or been killed), so both pipes are closed and the

249 - // drain threads finish promptly.

250 - let stdout = stdout_handle.join().unwrap_or_default();

251 - let stderr = stderr_handle.join().unwrap_or_default();

272 + // Collect what the drains read, bounded: if something in the hook's tree

273 + // outlived the kill (or daemonized past the process group) and still holds

274 + // a pipe open, give up on its output after a short grace instead of

275 + // hanging the caller on it.

276 + let mut stdout = Vec::new();

277 + let mut stderr = Vec::new();

278 + let grace = Duration::from_secs(1);

279 + for _ in 0..2 {

280 + match out_rx.recv_timeout(grace) {

281 + Ok((0, buf)) => stdout = buf,

282 + Ok((_, buf)) => stderr = buf,

283 + Err(_) => {

284 + log::debug!("Hook output pipe still open after exit; skipping its output");

285 + break;

286 + }

287 + }

288 + }

289

290 match status {

291 None => log::warn!(

@@ -331,12 +368,16 @@ mod tests {

368 #[cfg(unix)]

369 #[test]

370 fn hook_that_hangs_is_killed_at_the_timeout() {

334 - // A hook that would sleep far past the deadline must be killed near it,

335 - // not block the caller for the full sleep.

371 + // A hook that would sleep far past the deadline must be killed near

372 + // it, not block the caller for the full sleep. The two-command form

373 + // forces the shell to fork `sleep` rather than exec it (what dash does

374 + // even for a single command), so this also proves the kill takes out

375 + // the whole process group: a surviving grandchild would hold the

376 + // output pipes open and stall the caller until its own exit.

377 let context = HookContext::new();

378 let start = Instant::now();

379 run_hook(

339 - "sleep 30",

380 + "sleep 30; echo done",

381 &context,

382 Path::new("."),

383 Duration::from_millis(300),