diff --git a/crates/ghostty-termio/src/exec.rs b/crates/ghostty-termio/src/exec.rs index 341f5e0..622b4d1 100644 --- a/crates/ghostty-termio/src/exec.rs +++ b/crates/ghostty-termio/src/exec.rs @@ -1344,11 +1344,18 @@ fn getpgid(pid: Pid) -> Option { // We loop while pgid == my_pgid. The expectation if we have a valid // pid is that setsid will eventually be called because it is the // FIRST thing the child process does and as far as I can tell, - // setsid cannot fail. (Zig loops forever; we give up after ~5s.) - for _ in 0..500 { + // setsid cannot fail. Like Zig we wait without a cap, with one + // exception (DEVIANT_FIXES.md): if the child already exited without + // ever creating its own group (still an unreaped zombie, whose pgid + // stays ours forever) Zig would spin here forever. + loop { match rustix::process::getpgid(Some(pid)) { Ok(pgid) => { if Some(pgid) == my_pgid { + if child_exited(pid) { + log::warn!("child exited before creating its process group"); + return None; + } log::warn!("pgid is our own, retrying"); std::thread::sleep(Duration::from_millis(10)); continue; @@ -1364,7 +1371,20 @@ fn getpgid(pid: Pid) -> Option { } } } - None +} + +/// Whether `pid` has terminated (without reaping it). +fn child_exited(pid: Pid) -> bool { + use rustix::process::{WaitId, WaitIdOptions, waitid}; + match waitid( + WaitId::Pid(pid), + WaitIdOptions::EXITED | WaitIdOptions::NOHANG | WaitIdOptions::NOWAIT, + ) { + Ok(Some(_)) => true, + Ok(None) => false, + // Not our child (or already reaped): nothing more to wait for. + Err(_) => true, + } } /// Builds the argv array for the process we should exec for the @@ -1740,4 +1760,43 @@ mod tests { assert_eq!(exit_code(Exit::Exited(3)), 3); assert_eq!(exit_code(Exit::Signal(9)), 9); } + + #[cfg(unix)] + fn pid_of(child: &std::process::Child) -> Pid { + Pid::from_raw(i32::try_from(child.id()).unwrap()).unwrap() + } + + /// Like Zig, there is no cap on waiting for the child's `setsid`: a + /// child that creates its group later than the old ~5 s cap's + /// granularity is still found (a short delay here keeps the test fast). + #[cfg(unix)] + #[test] + fn getpgid_waits_for_late_setsid() { + let mut child = std::process::Command::new("sh") + .args(["-c", "sleep 0.4; exec setsid sleep 3"]) + .spawn() + .unwrap(); + let pid = pid_of(&child); + let start = std::time::Instant::now(); + let pgid = getpgid(pid); + assert_eq!(pgid, Some(pid)); + assert!(start.elapsed() >= Duration::from_millis(300)); + let _ = child.kill(); + let _ = child.wait(); + } + + /// Deviation from Zig (DEVIANT_FIXES.md): a child that exited without + /// ever calling `setsid` (unreaped zombie, pgid == ours) does not make + /// the wait spin forever. + #[cfg(unix)] + #[test] + fn getpgid_gives_up_on_exited_child_without_setsid() { + let mut child = std::process::Command::new("true").spawn().unwrap(); + let pid = pid_of(&child); + std::thread::sleep(Duration::from_millis(200)); + let start = std::time::Instant::now(); + assert_eq!(getpgid(pid), None); + assert!(start.elapsed() < Duration::from_secs(2)); + let _ = child.wait(); + } } diff --git a/docs/rewrite/DEVIANT_FIXES.md b/docs/rewrite/DEVIANT_FIXES.md index 8c7cb67..b0109ad 100644 --- a/docs/rewrite/DEVIANT_FIXES.md +++ b/docs/rewrite/DEVIANT_FIXES.md @@ -62,6 +62,7 @@ when this file was written. | 34 | Renderer (Metal): buffer reallocation size | `renderer/metal/buffer.zig:89` (`Buffer.sync`) and `:146` (`syncFromArrayLists`) | `size = req_bytes * 2` is already in bytes, then `size * @sizeOf(T)` is passed to `newBufferWithLength:options:`, so the new buffer is `sizeof(T)` times larger than intended (GPU-memory over-allocation). | The new buffer is exactly `2 * req_bytes` (`metal_layout::grown_buffer_bytes`); the element count is `2 * total_len`. | bab49ee; registered + test in `2040677` fix-deviant(renderer) | `metal_layout::tests::grown_buffer_bytes_is_double_the_required_bytes_not_scaled_by_size_of_t` (runs on any host; the Metal buffer itself is compile-checked only) | | 35 | Termio: PTY hangup drops unread output | `termio/Exec.zig` `ReadThread.threadMainPosix` (exits as soon as `POLL.HUP` is seen) | `POLLIN \| POLLHUP` can arrive together with unread final output (a Linux PTY check confirmed this); exiting immediately loses the tail of the child's output. | After HUP the read thread makes one more pass over the fd, so the remaining output is delivered before exit. | 6629fb0 | none (needs a live PTY; verified by RV18 on Linux) | | 36 | Font (wasm/browser Canvas): atlas pixel format | `font/face/web_canvas.zig` (atlas write branch on `.rgba`) | The branch tests a stale `.rgba` format against an enum that contains `.bgra`, so BGRA atlases are never converted. | The BGRA atlas conversion is implemented. No claim of equivalence with a real browser (not testable here). | d89691b | `ghostty-font` web_canvas tests (compile/Node level) | +| 37 | Termio: subprocess cleanup | `termio/Exec.zig:1188-1224` (`getpgid`) | Loops forever while `getpgid(child) == our pgid`, i.e. also when the child died before `setsid` and is an unreaped zombie (its pgid stays ours), hanging the surface close. | Still waits without a cap (no 5 s limit) while the child is alive, but stops (no SIGHUP, nothing to kill) once `waitid(WNOWAIT)` shows the child exited. | uncommitted (P3) | `getpgid_gives_up_on_exited_child_without_setsid`, `getpgid_waits_for_late_setsid` (`ghostty-termio/src/exec.rs`) | ## Upstream quirks deliberately kept (not fixed)