From babe4266c479bfb2b3bdbe9ea4025cb260728f2e Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Sat, 8 Aug 2026 15:20:07 -0700 Subject: [PATCH 1/2] test: cover distinct foreground process-group teardown --- src/termio/Exec.zig | 155 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 152 insertions(+), 3 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index e7c2b49751e..edf42950ba1 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -577,6 +577,7 @@ const Subprocess = struct { const c = @cImport({ @cInclude("errno.h"); @cInclude("signal.h"); + @cInclude("sys/ioctl.h"); @cInclude("unistd.h"); }); @@ -1135,12 +1136,20 @@ const Subprocess = struct { /// to SIGKILL and bounds that reap wait as well. /// This does not close the pty. pub fn stop(self: *Subprocess) void { + self.stopWithTimeouts(.{}); + } + + fn stopWithTimeouts(self: *Subprocess, timeouts: KillTimeouts) void { if (self.process) |*process| { switch (process.*) { .fork_exec => |*cmd| { // Note: this will also wait for the command to exit, so // DO NOT call cmd.wait. - killCommand(cmd, self.process_group_id) catch |err| + killCommandWithTimeouts( + cmd, + self.process_group_id, + timeouts, + ) catch |err| log.err("error stopping command: {}", .{err}); }, @@ -1155,7 +1164,7 @@ const Subprocess = struct { // The process watcher may already have consumed the direct child's // wait status. Its process group remains ours to terminate. if (self.process_group_id) |pgid| { - killProcessGroupWithTimeouts(pgid, null, .{}) catch |err| + killProcessGroupWithTimeouts(pgid, null, timeouts) catch |err| log.err("error stopping process group: {}", .{err}); } } @@ -1190,6 +1199,14 @@ const Subprocess = struct { /// Kill the underlying subprocess. POSIX process groups receive SIGHUP /// first and SIGKILL if they outlive the graceful shutdown budget. fn killCommand(command: *Command, process_group_id: ?c.pid_t) !void { + return killCommandWithTimeouts(command, process_group_id, .{}); + } + + fn killCommandWithTimeouts( + command: *Command, + process_group_id: ?c.pid_t, + timeouts: KillTimeouts, + ) !void { if (command.pid) |pid| { switch (builtin.os.tag) { .windows => { @@ -1203,7 +1220,7 @@ const Subprocess = struct { else => try killProcessGroupWithTimeouts( process_group_id orelse pid, pid, - .{}, + timeouts, ), } } @@ -2456,6 +2473,138 @@ test "subprocess stop kills descendants after the group leader was reaped" { try testing.expectEqual(posix.E.SRCH, group_probe_err); } +test "subprocess stop kills a distinct foreground process group" { + if (comptime builtin.os.tag == .windows or builtin.os.tag == .ios) { + return error.SkipZigTest; + } + + const testing = std.testing; + const c = Subprocess.c; + var pty = try Pty.open(.{}); + defer pty.deinit(); + + const ready_pipe = try internal_os.pipe(); + const job_ready_pipe = try internal_os.pipe(); + defer { + _ = posix.system.close(ready_pipe[0]); + _ = posix.system.close(ready_pipe[1]); + _ = posix.system.close(job_ready_pipe[0]); + _ = posix.system.close(job_ready_pipe[1]); + } + + const leader_pid: posix.pid_t = leader: { + const rc = posix.system.fork(); + switch (posix.errno(rc)) { + .SUCCESS => break :leader @intCast(rc), + .AGAIN, .NOMEM => return error.SystemResources, + else => |err| return posix.unexpectedErrno(err), + } + }; + if (leader_pid == 0) { + _ = posix.system.close(pty.master); + _ = posix.system.close(ready_pipe[0]); + if (c.setsid() < 0) c._exit(1); + if (c.ioctl(pty.slave, c.TIOCSCTTY, @as(c_ulong, 0)) < 0) c._exit(1); + + const job_pid = posix.system.fork(); + switch (posix.errno(job_pid)) { + .SUCCESS => {}, + else => c._exit(1), + } + if (job_pid == 0) { + _ = posix.system.close(ready_pipe[1]); + _ = posix.system.close(job_ready_pipe[0]); + if (c.setpgid(0, 0) < 0) c._exit(1); + + var action: posix.Sigaction = .{ + .handler = .{ .handler = posix.SIG.IGN }, + .mask = posix.sigemptyset(), + .flags = 0, + }; + posix.sigaction(posix.SIG.HUP, &action, null); + if (posix.system.write(job_ready_pipe[1], "j", 1) != 1) c._exit(1); + while (true) _ = c.pause(); + } + + _ = posix.system.close(job_ready_pipe[1]); + var job_ready: [1]u8 = undefined; + if (posix.system.read(job_ready_pipe[0], &job_ready, 1) != 1) c._exit(1); + if (c.tcsetpgrp(pty.slave, @intCast(job_pid)) < 0) c._exit(1); + if (posix.system.write(ready_pipe[1], "r", 1) != 1) c._exit(1); + while (true) _ = c.pause(); + } + + var leader_reaped = false; + var foreground_pgid: ?c.pid_t = null; + defer { + if (foreground_pgid) |pgid| _ = c.killpg(pgid, c.SIGKILL); + _ = c.killpg(leader_pid, c.SIGKILL); + if (!leader_reaped) { + var status: c_int = 0; + _ = posix.system.waitpid(leader_pid, &status, 0); + } + } + + _ = posix.system.close(ready_pipe[1]); + var ready: [1]u8 = undefined; + try testing.expectEqual(@as(usize, 1), try posix.read(ready_pipe[0], &ready)); + _ = posix.system.close(pty.slave); + + foreground_pgid = @intCast( + pty.getProcessInfo(.foreground_pid) orelse + return error.ForegroundProcessGroupUnavailable, + ); + try testing.expect(foreground_pgid.? != leader_pid); + + var arena = ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + var subprocess: Subprocess = .{ + .arena = arena, + .cwd = null, + .env = null, + .args = &.{}, + .grid_size = .{}, + .screen_size = .{ .width = 1, .height = 1 }, + .pty = pty, + .process = null, + .process_group_id = leader_pid, + .rt_pre_exec_info = undefined, + .rt_post_fork_info = undefined, + }; + var leader_status: c_int = 0; + var wait_result: posix.pid_t = 0; + const leader_reaper = try std.Thread.spawn(.{}, struct { + fn run( + pid: posix.pid_t, + status: *c_int, + result: *posix.pid_t, + ) void { + result.* = @intCast(posix.system.waitpid(pid, status, 0)); + } + }.run, .{ leader_pid, &leader_status, &wait_result }); + var leader_reaper_joined = false; + defer if (!leader_reaper_joined) { + _ = c.killpg(leader_pid, c.SIGKILL); + leader_reaper.join(); + leader_reaped = true; + }; + + subprocess.stopWithTimeouts(.{ + .sighup_grace = .fromMilliseconds(20), + .sigkill_grace = .fromSeconds(1), + }); + + leader_reaper.join(); + leader_reaper_joined = true; + leader_reaped = wait_result == leader_pid; + try testing.expectEqual(leader_pid, wait_result); + + const foreground_probe = c.killpg(foreground_pgid.?, 0); + const foreground_probe_err = posix.errno(foreground_probe); + try testing.expectEqual(@as(c_int, -1), foreground_probe); + try testing.expectEqual(posix.E.SRCH, foreground_probe_err); +} + /// Builds the argv array for the process we should exec for the /// configured command. This isn't as straightforward as it seems since /// we deal with shell-wrapping, macOS login shells, etc. From 47e9bd4c90dec35b55b550095eea14498a37508f Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Sat, 8 Aug 2026 15:24:35 -0700 Subject: [PATCH 2/2] fix: reap foreground job-control process groups --- src/termio/Exec.zig | 173 +++++++++++++++++++++++++++++++------------- 1 file changed, 123 insertions(+), 50 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index edf42950ba1..7fb4f39b455 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -1140,6 +1140,7 @@ const Subprocess = struct { } fn stopWithTimeouts(self: *Subprocess, timeouts: KillTimeouts) void { + const foreground_process_group_id = self.foregroundProcessGroupId(); if (self.process) |*process| { switch (process.*) { .fork_exec => |*cmd| { @@ -1148,6 +1149,7 @@ const Subprocess = struct { killCommandWithTimeouts( cmd, self.process_group_id, + foreground_process_group_id, timeouts, ) catch |err| log.err("error stopping command: {}", .{err}); @@ -1164,7 +1166,12 @@ const Subprocess = struct { // The process watcher may already have consumed the direct child's // wait status. Its process group remains ours to terminate. if (self.process_group_id) |pgid| { - killProcessGroupWithTimeouts(pgid, null, timeouts) catch |err| + killProcessGroupsWithTimeouts( + pgid, + foreground_process_group_id, + null, + timeouts, + ) catch |err| log.err("error stopping process group: {}", .{err}); } } @@ -1173,6 +1180,13 @@ const Subprocess = struct { self.process_group_id = null; } + fn foregroundProcessGroupId(self: *Subprocess) ?c.pid_t { + const pty = &(self.pty orelse return null); + const raw = pty.getProcessInfo(.foreground_pid) orelse return null; + const pgid = std.math.cast(c.pid_t, raw) orelse return null; + return if (pgid > 0) pgid else null; + } + /// Resize the pty subprocess. This is safe to call anytime. pub fn resize( self: *Subprocess, @@ -1199,12 +1213,13 @@ const Subprocess = struct { /// Kill the underlying subprocess. POSIX process groups receive SIGHUP /// first and SIGKILL if they outlive the graceful shutdown budget. fn killCommand(command: *Command, process_group_id: ?c.pid_t) !void { - return killCommandWithTimeouts(command, process_group_id, .{}); + return killCommandWithTimeouts(command, process_group_id, null, .{}); } fn killCommandWithTimeouts( command: *Command, process_group_id: ?c.pid_t, + foreground_process_group_id: ?c.pid_t, timeouts: KillTimeouts, ) !void { if (command.pid) |pid| { @@ -1217,8 +1232,9 @@ const Subprocess = struct { _ = try command.wait(false); }, - else => try killProcessGroupWithTimeouts( + else => try killProcessGroupsWithTimeouts( process_group_id orelse pid, + foreground_process_group_id, pid, timeouts, ), @@ -1246,68 +1262,120 @@ const Subprocess = struct { pgid: c.pid_t, direct_child_pid: ?c.pid_t, timeouts: KillTimeouts, + ) !void { + return killProcessGroupsWithTimeouts( + pgid, + null, + direct_child_pid, + timeouts, + ); + } + + fn killProcessGroupsWithTimeouts( + primary_pgid: c.pid_t, + foreground_pgid: ?c.pid_t, + direct_child_pid: ?c.pid_t, + timeouts: KillTimeouts, ) !void { // `Pty.childPreExec` calls setsid before exec, so the direct child pid // is the process-group id. Unlike getpgid(pid), that identity remains - // valid after the leader exits while descendants retain the group. + // valid after the leader exits while descendants retain the group. Job + // control may move the terminal's foreground process into a distinct + // group, so both groups share one graceful and forced-shutdown budget. + const distinct_foreground_pgid = if (foreground_pgid) |pgid| + if (pgid != primary_pgid) pgid else null + else + null; + const group_ids: [2]?c.pid_t = .{ + primary_pgid, + distinct_foreground_pgid, + }; + var group_gone: [2]bool = .{ false, distinct_foreground_pgid == null }; + var phase_signal_sent: [2]bool = .{ false, false }; var phase: enum { sighup, sigkill } = .sighup; - var phase_signal_sent = false; var deadline = std.Io.Timestamp.now(global.io(), .awake).addDuration( timeouts.sighup_grace, ); var direct_child_reaped = direct_child_pid == null; var direct_sigkill_sent = false; while (true) { - var process_group_exists = true; - if (!phase_signal_sent) { - const signal = switch (phase) { - .sighup => c.SIGHUP, - .sigkill => c.SIGKILL, - }; - switch (posix.errno(c.killpg(pgid, signal))) { - .SUCCESS => { - phase_signal_sent = true; - log.debug( - "process group signalled pgid={} signal={}", - .{ pgid, signal }, - ); - }, - .SRCH => { - // A just-forked child may not have called setsid yet. - // Retry until it creates the group or is reaped. - process_group_exists = false; - }, - else => |err| killpg: { - if ((comptime builtin.target.os.tag.isDarwin()) and - err == .PERM) - { - phase_signal_sent = true; + var primary_group_missing = false; + for (group_ids, 0..) |maybe_pgid, index| { + const pgid = maybe_pgid orelse continue; + if (group_gone[index]) continue; + + if (!phase_signal_sent[index]) { + const signal = switch (phase) { + .sighup => c.SIGHUP, + .sigkill => c.SIGKILL, + }; + switch (posix.errno(c.killpg(pgid, signal))) { + .SUCCESS => { + phase_signal_sent[index] = true; log.debug( - "killpg failed with EPERM, expected on Darwin and ignoring", - .{}, + "process group signalled pgid={} signal={}", + .{ pgid, signal }, ); - break :killpg; - } + }, + .SRCH => { + // A just-forked direct child may not have called + // setsid yet. Retry its primary group until the + // child creates it or is reaped. The foreground + // group was already observed through tcgetpgrp, so + // once it disappears it must not be targeted again. + if (index == 0 and !direct_child_reaped) { + primary_group_missing = true; + } else { + group_gone[index] = true; + } + }, + else => |err| killpg: { + if ((comptime builtin.target.os.tag.isDarwin()) and + err == .PERM) + { + phase_signal_sent[index] = true; + log.debug( + "killpg failed with EPERM, expected on Darwin and ignoring", + .{}, + ); + break :killpg; + } - log.warn("error signalling process group pgid={} err={}", .{ pgid, err }); + log.warn( + "error signalling process group pgid={} err={}", + .{ pgid, err }, + ); + return error.KillFailed; + }, + } + } else switch (posix.errno(c.killpg(pgid, 0))) { + .SUCCESS => {}, + .SRCH => { + if (index == 0 and !direct_child_reaped) { + primary_group_missing = true; + } else { + group_gone[index] = true; + } + }, + .PERM => {}, + else => |err| { + log.warn( + "error probing process group pgid={} err={}", + .{ pgid, err }, + ); return error.KillFailed; }, } - } else switch (posix.errno(c.killpg(pgid, 0))) { - .SUCCESS => {}, - .SRCH => process_group_exists = false, - .PERM => {}, - else => |err| { - log.warn("error probing process group pgid={} err={}", .{ pgid, err }); - return error.KillFailed; - }, } if (!direct_child_reaped) { direct_child_reaped = try reapExitedChild(direct_child_pid.?); } + if (direct_child_reaped and primary_group_missing) { + group_gone[0] = true; + } if (phase == .sigkill and - !process_group_exists and + primary_group_missing and !direct_child_reaped and !direct_sigkill_sent) { @@ -1325,25 +1393,30 @@ const Subprocess = struct { }, } } - if (direct_child_reaped and !process_group_exists) return; + if (direct_child_reaped and group_gone[0] and group_gone[1]) return; const now = std.Io.Timestamp.now(global.io(), .awake); if (now.toNanoseconds() >= deadline.toNanoseconds()) { if (phase == .sighup) { phase = .sigkill; - phase_signal_sent = false; + phase_signal_sent = .{ false, false }; deadline = now.addDuration(timeouts.sigkill_grace); log.warn( - "process group exceeded SIGHUP grace; escalating " ++ - "pgid={}", - .{pgid}, + "process groups exceeded SIGHUP grace; escalating " ++ + "primary_pgid={} foreground_pgid={?}", + .{ primary_pgid, distinct_foreground_pgid }, ); continue; } log.err( - "process group did not reap after SIGKILL pgid={} pid={?}", - .{ pgid, direct_child_pid }, + "process groups did not reap after SIGKILL " ++ + "primary_pgid={} foreground_pgid={?} pid={?}", + .{ + primary_pgid, + distinct_foreground_pgid, + direct_child_pid, + }, ); return error.ProcessTerminationTimedOut; }