Target process group during teardown when configured (#243)

Author: broken-circle

Sources/Subprocess/Teardown.swift

@@ -31,9 +31,9 @@ import Musl
 public struct TeardownStep: Sendable, Hashable {
     internal enum Storage: Sendable, Hashable {
         #if !os(Windows)
-        case sendSignal(Signal, allowedDuration: Duration)
+        case sendSignal(Signal, toProcessGroup: Bool, allowedDuration: Duration)
         #endif
-        case gracefulShutDown(allowedDuration: Duration)
+        case gracefulShutDown(toProcessGroup: Bool, allowedDuration: Duration)
         case kill
     }
     var storage: Storage
@@ -43,13 +43,22 @@ public struct TeardownStep: Sendable, Hashable {
     /// before proceeding to the next step.
     ///
     /// The final step in the sequence always sends a `.kill` signal.
+    ///
+    /// - Important: When sending the signal to the process group, unless you
+    /// also set `createSession` to `true`, or `processGroupID` to a
+    /// non-inherited value, the targeted process group includes the parent
+    /// process. This is almost never what you want. Pair `toProcessGroup`
+    /// with `createSession` to isolate the subprocess and its descendants in
+    /// their own session.
     public static func send(
         signal: Signal,
+        toProcessGroup: Bool = false,
         allowedDurationToNextStep: Duration
     ) -> Self {
         return Self(
             storage: .sendSignal(
                 signal,
+                toProcessGroup: toProcessGroup,
                 allowedDuration: allowedDurationToNextStep
             )
         )
@@ -64,11 +73,22 @@ public struct TeardownStep: Sendable, Hashable {
     ///   1. Sends `WM_CLOSE` if the child process is a GUI process.
     ///   2. Sends `CTRL_C_EVENT` to the console.
     ///   3. Sends `CTRL_BREAK_EVENT` to the process group.
+    ///
+    /// - Important: On Unix, when sending the signal to the process group,
+    /// unless you also set `createSession` to `true`, or `processGroupID`
+    /// to a non-inherited value, the targeted process group includes the parent
+    /// process. This is almost never what you want. Pair `toProcessGroup`
+    /// with `createSession` to isolate the subprocess and its descendants in
+    /// their own session. On Windows, the `toProcessGroup` parameter has no
+    /// effect; `WM_CLOSE` and `CTRL_C_EVENT` have no process-group equivalent,
+    /// and `CTRL_BREAK_EVENT` is always sent to the process group.
     public static func gracefulShutDown(
+        toProcessGroup: Bool = false,
         allowedDurationToNextStep: Duration
     ) -> Self {
         return Self(
             storage: .gracefulShutDown(
+                toProcessGroup: toProcessGroup,
                 allowedDuration: allowedDurationToNextStep
             )
         )
@@ -95,6 +115,7 @@ internal enum TeardownStepCompletion {
 
 extension Execution {
     internal func gracefulShutDown(
+        toProcessGroup: Bool,
         allowedDurationToNextStep duration: Duration
     ) async {
         #if os(Windows)
@@ -129,16 +150,30 @@ extension Execution {
         // Send SIGTERM
         try? self.send(
             signal: .terminate,
-            toProcessGroup: false
+            toProcessGroup: toProcessGroup
         )
         #endif
     }
 
     internal func runTeardownSequence(
         _ sequence: some Sequence<TeardownStep> & Sendable
     ) async {
-        // First insert the `.kill` step
-        let finalSequence = sequence + [TeardownStep(storage: .kill)]
+        // The implicit final `.kill` inherits `toProcessGroup` from the last
+        // explicit step in the sequence. This matches user intent: A sequence
+        // configured to target the process group should have its terminal kill
+        // also target the group, so descendants don't leak after teardown.
+        let steps = Array(sequence)
+        let killProcessGroup: Bool = {
+            guard let last = steps.last else { return false }
+            switch last.storage {
+            #if !os(Windows)
+            case .sendSignal(_, let toProcessGroup, _): return toProcessGroup
+            #endif
+            case .gracefulShutDown(let toProcessGroup, _): return toProcessGroup
+            case .kill: return false
+            }
+        }()
+        let finalSequence = steps + [TeardownStep(storage: .kill)]
         for step in finalSequence {
             let stepCompletion: TeardownStepCompletion
             guard self.isPotentiallyStillAlive() else {
@@ -147,7 +182,7 @@ extension Execution {
             }
 
             switch step.storage {
-            case .gracefulShutDown(let allowedDuration):
+            case .gracefulShutDown(let toProcessGroup, let allowedDuration):
                 stepCompletion = await withTaskGroup(of: TeardownStepCompletion.self) { group in
                     group.addTask {
                         do {
@@ -160,12 +195,13 @@ extension Execution {
                         }
                     }
                     await self.gracefulShutDown(
+                        toProcessGroup: toProcessGroup,
                         allowedDurationToNextStep: allowedDuration
                     )
                     return await group.next()!
                 }
             #if !os(Windows)
-            case .sendSignal(let signal, let allowedDuration):
+            case .sendSignal(let signal, let toProcessGroup, let allowedDuration):
                 stepCompletion = await withTaskGroup(of: TeardownStepCompletion.self) { group in
                     group.addTask {
                         do {
@@ -177,7 +213,7 @@ extension Execution {
                             return .processHasExited
                         }
                     }
-                    try? self.send(signal: signal, toProcessGroup: false)
+                    try? self.send(signal: signal, toProcessGroup: toProcessGroup)
                     return await group.next()!
                 }
             #endif // !os(Windows)
@@ -187,7 +223,7 @@ extension Execution {
                     withExitCode: 0
                 )
                 #else
-                try? self.send(signal: .kill, toProcessGroup: false)
+                try? self.send(signal: .kill, toProcessGroup: killProcessGroup)
                 #endif
                 stepCompletion = .killedTheProcess
             }

Tests/SubprocessTests/UnixTests.swift

@@ -382,6 +382,82 @@ extension SubprocessUnixTests {
         }
     }
 
+    @Test(.requiresBash)
+    func testTeardownSignalsProcessGroup() async throws {
+        do {
+            try await withThrowingTaskGroup { group in
+                let (readyStream, readyContinuation) = AsyncStream.makeStream(of: Void.self)
+
+                group.addTask {
+                    var platformOptions = PlatformOptions()
+                    // Creating a new session puts the shell (and its descendants,
+                    // absent further setsid calls) in their own process group, so
+                    // the teardown signal reaches everything spawned from the shell.
+                    platformOptions.createSession = true
+                    platformOptions.teardownSequence = [
+                        .send(signal: .terminate, toProcessGroup: true, allowedDurationToNextStep: .milliseconds(200))
+                    ]
+                    let result = try await Subprocess.run(
+                        .name("bash"),
+                        arguments: [
+                            "-c",
+                            """
+                            set -e
+                            # Spawn a grandchild that would otherwise outlive the shell.
+                            # Deliberately install NO trap: we want to verify that the
+                            # teardown signal reaches the grandchild directly via the
+                            # process group, not that bash cooperatively cleans up.
+                            /usr/bin/yes "Runaway process from \(#function), please file a SwiftSubprocess bug." > /dev/null &
+                            child_pid=$!
+                            echo "$child_pid"
+                            wait $child_pid
+                            """,
+                        ],
+                        platformOptions: platformOptions,
+                        error: .fileDescriptor(.standardError, closeAfterSpawningProcess: false),
+                        preferredBufferSize: 1
+                    ) { _, standardOutput in
+                        var grandChildPid: pid_t?
+                        for try await line in standardOutput.strings() {
+                            let trimmed = line.trimmingCharacters(in: .whitespacesAndNewlines)
+                            if let pid = pid_t(trimmed) {
+                                grandChildPid = pid
+                                readyContinuation.finish()
+                            }
+                        }
+                        return grandChildPid
+                    }
+                    #expect(result.terminationStatus == .signaled(SIGTERM))
+                    let grandChildPid = try #require(result.value)
+                    // Grandchild should have been signalled via the process group.
+                    // Allow a few iterations for signal propagation and reaping.
+                    for _ in 0..<10 {
+                        if kill(grandChildPid, 0) != 0 { break }
+                        try await Task.sleep(for: .milliseconds(100))
+                    }
+                    let finalRC = kill(grandChildPid, 0)
+                    let capturedError = errno
+                    #expect(finalRC != 0)
+                    #expect(capturedError == ESRCH)
+                }
+                group.addTask {
+                    for await _ in readyStream {}
+                }
+                // Wait for the ready signal
+                _ = try await group.next()
+                // Cancel child process to trigger teardown
+                group.cancelAll()
+                try await group.waitForAll()
+            }
+        } catch {
+            if error is CancellationError {
+                // We intentionally cancelled the task
+                return
+            }
+            throw error
+        }
+    }
+
     @Test func testSubprocessDoesNotInheritVeryHighFileDescriptors() async throws {
         var openedFileDescriptors: [CInt] = []
         // Open /dev/null to use as source for duplication