A configured upload command inherits cmux's blocked signals and waits out its own timeouts

Sources/TerminalCustomUploadRunner.swift

@@ -258,10 +258,26 @@ struct TerminalCustomUploadRunner {
             throw uploadError("Failed to prepare upload command.")
         }
         defer { posix_spawnattr_destroy(&attributes) }
+        // A signal mask survives exec, and this spawns from a libdispatch worker, whose
+        // threads run with most signals blocked. Without SETSIGMASK the command inherits
+        // that mask and so does everything it runs, including SIGCHLD. A command that
+        // watches its own children through SIGCHLD then never learns they exited and
+        // waits out its internal timeouts instead. Measured with an uploader that reaps
+        // that way: each phase took exactly its own budget, 10003ms on a ten second probe
+        // and 45004ms on a forty-five second copy, for work that takes about a second;
+        // 103ms and 784ms with the mask cleared. Only the mask is reset, not signal
+        // dispositions -- an inherited SIG_IGN on SIGPIPE is what lets a child see EPIPE
+        // instead of dying mid-cleanup.
+        var emptyMask = sigset_t()
+        sigemptyset(&emptyMask)
         // New process group led by the child (pgid == child pid) so the whole tree
         // can be signalled with kill(-pid, …). If this fails we must not spawn,
         // else timeout/cancel couldn't tear the group down.
-        guard posix_spawnattr_setflags(&attributes, Int16(POSIX_SPAWN_SETPGROUP)) == 0,
+        guard posix_spawnattr_setsigmask(&attributes, &emptyMask) == 0,
+              posix_spawnattr_setflags(
+                  &attributes,
+                  Int16(POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_SETSIGMASK)
+              ) == 0,
               posix_spawnattr_setpgroup(&attributes, 0) == 0 else {
             throw uploadError("Failed to prepare upload command.")
         }
@@ -302,46 +318,64 @@ struct TerminalCustomUploadRunner {
             stderrBuffer.set(drainPipe(stderrReadFD, cap: maxStderrBytes)); stderrDrained.signal()
         }
 
-        // `wakeup` is signalled by the reaper when the child exits AND by
-        // cancellation, so the waits below block on a real event — never a timer or
-        // a poll. On cancel/timeout the group gets SIGTERM, then SIGKILL after a
-        // bounded grace if it hasn't died. Signalling only happens while the leader
-        // is still alive (group non-empty), so the pgid can't have been reused.
+        // `wakeup` is signalled when the leader exits AND by cancellation, so the waits
+        // below block on a real event — never a timer or a poll. On cancel/timeout the
+        // group gets SIGTERM, then SIGKILL. Signalling only happens while the leader is
+        // unreaped, and an unreaped leader is still a member of its group, so the pgid
+        // cannot have been reused.
         let wakeup = DispatchSemaphore(value: 0)
+        // The leader is observed without being reaped, then reaped once teardown is
+        // over. `mayReap` is what says teardown is over, so the two stages stay on this
+        // one thread and the blocking waits never move onto the caller's.
+        let mayReap = DispatchSemaphore(value: 0)
+        let reaped = DispatchSemaphore(value: 0)
         DispatchQueue.global(qos: .userInitiated).async {
-            spawned.reap()
+            spawned.awaitLeaderExit()
             wakeup.signal()
+            mayReap.wait()
+            spawned.collectLeader()
+            reaped.signal()
         }
         operation.installCancellationHandler {
             spawned.signalGroup(SIGTERM)
             wakeup.signal()
         }
         defer { operation.clearCancellationHandler() }
+        // Whatever happens below, the leader gets reaped: an early return would otherwise
+        // park the reaper thread forever and leave a zombie. Signalling twice is fine,
+        // the reaper waits once.
+        defer { mayReap.signal() }
 
-        // Blocks on `wakeup` until the child is reaped or `grace` elapses.
-        func awaitExit(within grace: TimeInterval) -> Bool {
+        // Blocks on `wakeup` until the leader exits or `grace` elapses.
+        func awaitLeaderExit(within grace: TimeInterval) -> Bool {
             let deadline = DispatchTime.now() + grace
-            while !spawned.isReaped {
+            while !spawned.hasLeaderExited {
                 if wakeup.wait(timeout: deadline) == .timedOut { break }
             }
-            return spawned.isReaped
+            return spawned.hasLeaderExited
         }
 
         // First wait: exit, cancel, or the timeout budget — whichever comes first.
         let deadline = DispatchTime.now() + timeout
-        while !spawned.isReaped && !operation.isCancelled {
+        while !spawned.hasLeaderExited && !operation.isCancelled {
             if wakeup.wait(timeout: deadline) == .timedOut { break }
         }
 
-        var timedOut = false
-        if !spawned.isReaped {
-            timedOut = !operation.isCancelled
+        let timedOut = !spawned.hasLeaderExited && !operation.isCancelled
+        if timedOut || operation.isCancelled {
             spawned.signalGroup(SIGTERM)
-            if !awaitExit(within: 1) {
-                spawned.signalGroup(SIGKILL)
-                _ = awaitExit(within: 5)
-            }
-        }
+            // The leader exiting is not the group exiting. A descendant that ignores
+            // SIGTERM outlives it, keeps our pipes open, and would survive the whole
+            // teardown if the leader's exit were read as success — so SIGKILL goes to
+            // the group either way, as soon as the leader is gone or the grace runs out.
+            // Reaping waits until after that: a reaped leader frees its pgid for reuse,
+            // and the kill would then be addressed to whatever group inherits the id.
+            _ = awaitLeaderExit(within: 1)
+            spawned.signalGroup(SIGKILL)
+            _ = awaitLeaderExit(within: 5)
+        }
+        mayReap.signal()
+        _ = reaped.wait(timeout: .now() + 5)
 
         // The leader exited, but a descendant — possibly `setsid`'d out of the
         // group, so a group kill can't reach it — could still hold a write end
@@ -440,17 +474,40 @@ struct TerminalCustomUploadRunner {
 
     /// A spawned child process group, led by the `/bin/sh` child. A lock guards the
     /// wait status across the reaper thread and the caller; an actor can't own the
-    /// blocking `waitpid` this wraps.
+    /// blocking waits this wraps. The leader's exit and its reaping are separate steps
+    /// because teardown has to keep signalling the group after the leader is gone.
     private final class SpawnedProcess: @unchecked Sendable {
         private let lock = NSLock()
         private let groupID: pid_t
         private var rawStatus: Int32 = 0
-        private var reaped = false
+        private var leaderExited = false
+        private var collected = false
 
         init(pid: pid_t) { groupID = pid }
 
-        /// Blocks until the group leader (`/bin/sh`) exits and records its status.
-        func reap() {
+        /// Blocks until the group leader (`/bin/sh`) exits, leaving it unreaped. The
+        /// zombie is still a member of the process group, which keeps the pgid from
+        /// being handed to anything else while teardown is still signalling it.
+        func awaitLeaderExit() {
+            var info = siginfo_t()
+            while waitid(P_PID, id_t(groupID), &info, WEXITED | WNOWAIT) == -1 {
+                if errno == EINTR { continue }
+                break
+            }
+            lock.lock()
+            leaderExited = true
+            lock.unlock()
+        }
+
+        var hasLeaderExited: Bool {
+            lock.lock()
+            defer { lock.unlock() }
+            return leaderExited
+        }
+
+        /// Reaps the leader and records its status. Blocks, so callers hand this to the
+        /// same thread that did `awaitLeaderExit`.
+        func collectLeader() {
             var status: Int32 = 0
             while true {
                 let result = waitpid(groupID, &status, 0)
@@ -461,25 +518,20 @@ struct TerminalCustomUploadRunner {
             }
             lock.lock()
             rawStatus = status
-            reaped = true
+            leaderExited = true
+            collected = true
             lock.unlock()
         }
 
-        var isReaped: Bool {
-            lock.lock()
-            defer { lock.unlock() }
-            return reaped
-        }
-
         /// Signals the whole process group, unless the leader has been reaped — once
         /// reaped, the pgid may be empty and reusable, so signalling it could hit an
-        /// unrelated group. The reap flag is checked and the signal sent under the
-        /// same lock that `reap()` sets it with, so a signal never races past reap.
-        /// We only ever signal the group, never a bare pid.
+        /// unrelated group. The collected flag is checked and the signal sent under the
+        /// same lock that `collectLeader()` sets it with, so a signal never races past
+        /// the reap. We only ever signal the group, never a bare pid.
         func signalGroup(_ signal: Int32) {
             lock.lock()
             defer { lock.unlock() }
-            guard !reaped else { return }
+            guard !collected else { return }
             _ = kill(-groupID, signal)
         }
 

Sources/TerminalNotificationPolicy.swift

@@ -609,7 +609,17 @@ private final class NotificationHookProcessRun: @unchecked Sendable {
         var attributes: posix_spawnattr_t?
         try throwIfPOSIXError(posix_spawnattr_init(&attributes), operation: "initialize spawn attributes")
         defer { posix_spawnattr_destroy(&attributes) }
-        let flags = Int16(POSIX_SPAWN_SETPGROUP)
+        // Hooks are spawned from a dispatch queue, and a dispatch worker runs with most
+        // signals blocked. A mask survives exec, so without this the hook and everything
+        // it runs inherit that mask; see the longer note in TerminalCustomUploadRunner.
+        // Dispositions are left alone: this clears the mask, not an inherited SIG_IGN.
+        var emptyMask = sigset_t()
+        sigemptyset(&emptyMask)
+        try throwIfPOSIXError(
+            posix_spawnattr_setsigmask(&attributes, &emptyMask),
+            operation: "clear inherited signal mask"
+        )
+        let flags = Int16(POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_SETSIGMASK)
         try throwIfPOSIXError(posix_spawnattr_setflags(&attributes, flags), operation: "set spawn flags")
         try throwIfPOSIXError(posix_spawnattr_setpgroup(&attributes, 0), operation: "set process group")
         let arguments = ["/bin/sh", "-c", hook.command]
@@ -821,11 +831,16 @@ private final class NotificationHookProcessRun: @unchecked Sendable {
         let source = DispatchSource.makeTimerSource(queue: queue)
         source.schedule(deadline: .now() + .milliseconds(750))
         source.setEventHandler { [self] in
-            if self.processId > 0 {
-                self.signalProcessGroup(SIGKILL)
-            }
+            self.signalProcessGroup(SIGKILL)
             self.killSource?.cancel()
             self.killSource = nil
+            // A leader that exited during the grace period was left unreaped so its pgid
+            // would still be this group's when the SIGKILL above went out. Collect it now
+            // and finish. If it is still running, SIGKILL has just ended it and the exit
+            // source finishes the run instead.
+            if let status = self.reapProcessIfExited() {
+                self.finish(rawStatus: status)
+            }
         }
         killSource = source
         source.resume()
@@ -839,6 +854,11 @@ private final class NotificationHookProcessRun: @unchecked Sendable {
     }
 
     private func processExited() {
+        // The leader exiting is not the group exiting: a descendant that ignores SIGTERM
+        // outlives it. Reaping here would end the run and cancel the escalation timer,
+        // and would also free the pgid, so the SIGKILL that timer owes the group could
+        // land on a reused one. Leave the zombie in place and let the timer finish.
+        if didRequestTermination, killSource != nil { return }
         guard let status = waitForProcessExit() else { return }
         finish(rawStatus: status)
     }

cmuxTests/NotificationAndMenuBarTests.swift

@@ -46,6 +46,88 @@ final class TerminalNotificationPolicyEngineTests: XCTestCase {
         )
     }
 
+    func testHookCancellationKillsADescendantThatOutlivesTheLeader() async throws {
+        // The hook's own shell dies on SIGTERM while the descendant it left behind
+        // ignores it. Reaping the shell at that point would end the run and cancel the
+        // escalation, leaving the descendant running.
+        //
+        // Cancelling the evaluation is what starts the teardown here, rather than the
+        // hook's timeout. The descendant has to exist before there is anything to
+        // prove, and a one second budget can expire on a loaded machine before the
+        // shell is ever scheduled; the test would then fail having tested nothing.
+        // Both paths run the same termination code, so the regression is still
+        // covered. The timeout is only a backstop.
+        let pidPath = NSTemporaryDirectory() + "cmux-hook-teardown-\(UUID().uuidString).pid"
+        defer { try? FileManager.default.removeItem(atPath: pidPath) }
+
+        let request = TerminalNotificationPolicyRequest(
+            tabId: UUID(),
+            surfaceId: UUID(),
+            title: "Title",
+            subtitle: "Subtitle",
+            body: "Body",
+            cwd: FileManager.default.temporaryDirectory.path,
+            isAppFocused: false,
+            isFocusedPanel: false
+        )
+        let hook = CmuxResolvedNotificationHook(
+            id: "teardown",
+            command: "/bin/sh -c 'trap \"\" TERM; echo $$ > \(pidPath); exec /bin/sleep 30' & wait",
+            timeoutSeconds: 60,
+            sourcePath: nil,
+            cwd: FileManager.default.temporaryDirectory.path
+        )
+
+        let evaluation = Task {
+            await TerminalNotificationPolicyEngine.evaluate(request: request, hooks: [hook])
+        }
+        let recorded = await Self.waitForRecordedPID(atPath: pidPath, within: 20)
+        evaluation.cancel()
+
+        let result = await evaluation.value
+        guard case .failure = result else {
+            XCTFail("a cancelled hook must fail closed")
+            return
+        }
+
+        guard let descendant = recorded else {
+            XCTFail("the descendant never recorded its pid, so this proved nothing")
+            return
+        }
+        let died = Self.waitForExit(descendant, within: 5)
+        if !died { kill(descendant, SIGKILL) }
+        XCTAssertTrue(died, "a descendant that ignores SIGTERM must not survive teardown")
+    }
+
+    private static func recordedPID(atPath path: String) -> pid_t? {
+        guard let text = try? String(contentsOfFile: path, encoding: .utf8) else { return nil }
+        return pid_t(text.trimmingCharacters(in: .whitespacesAndNewlines))
+    }
+
+    /// The pid the spawned descendant wrote to `path`, waiting up to `seconds` for it
+    /// to appear. A partially written file reads back as nil, so keep polling. Sleeps
+    /// rather than spinning: the evaluation it is waiting on runs on the same pool.
+    private static func waitForRecordedPID(atPath path: String, within seconds: TimeInterval) async -> pid_t? {
+        let deadline = Date().addingTimeInterval(seconds)
+        while Date() < deadline {
+            if let pid = recordedPID(atPath: path) { return pid }
+            try? await Task.sleep(nanoseconds: 20_000_000)
+        }
+        return recordedPID(atPath: path)
+    }
+
+    /// Whether `pid` is gone within `seconds`. Polled rather than waited on: it is not
+    /// our child, so there is no exit to wait for — the reparented process is reaped by
+    /// launchd and `kill(pid, 0)` starts failing.
+    private static func waitForExit(_ pid: pid_t, within seconds: TimeInterval) -> Bool {
+        let deadline = Date().addingTimeInterval(seconds)
+        while Date() < deadline {
+            if kill(pid, 0) != 0 { return true }
+            usleep(20_000)
+        }
+        return kill(pid, 0) != 0
+    }
+
     func testHookCanDisableDesktopAndTransformBody() async throws {
         let request = TerminalNotificationPolicyRequest(
             tabId: UUID(),