Fix subprocess lifecycle cleanup

Sources/SwiftyShell/Core/MockExecutor.swift

@@ -119,7 +119,7 @@ public struct MockExecutor: CommandExecutor {
     }
 
     private func validateConfiguration(for command: Command, in context: ShellContext) throws {
-        if let timeout = command.timeoutOverride ?? context.defaultTimeout, timeout < 0 {
+        if let timeout = command.timeoutOverride ?? context.defaultTimeout, timeout < 0 || !timeout.isFinite {
             throw ShellError.invalidConfiguration(description: "Timeout must be greater than or equal to zero seconds")
         }
 

Sources/SwiftyShell/Internal/Execution/SubprocessExecutor.swift

@@ -200,12 +200,8 @@ private struct ResolvedCommand: Sendable {
     }
 }
 
-private struct TimeoutExceeded: Error, Sendable {}
-
 private struct CaptureLimitExceeded: Error, Sendable {}
 
-private let partialOutputFlushGrace: Duration = .milliseconds(75)
-
 private enum EarlyTerminationReason: Sendable {
     case outputLimitExceeded(command: String, limit: Int)
 }
@@ -360,9 +356,9 @@ private struct SingleCommandRunner {
             }
         } catch is CancellationError {
             processTask.cancel()
-            await waitForPartialOutputFlush()
-            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             await registry.teardownAll()
+            _ = await processTask.result
+            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             throw ShellError.canceled(command: resolved.displayCommand, partialOutput: output)
         }
     }
@@ -412,17 +408,6 @@ private struct SingleCommandRunner {
                             )
                             return .streamComplete
                         }
-                        if let timeout = resolved.timeout {
-                            group.addTask {
-                                do {
-                                    try await Task.sleep(for: durationFromSeconds(timeout))
-                                    return .timedOut
-                                } catch {
-                                    return .streamComplete
-                                }
-                            }
-                        }
-
                         var completedStreams = 0
                         do {
                             while let result = try await group.next() {
@@ -433,10 +418,6 @@ private struct SingleCommandRunner {
                                         group.cancelAll()
                                         return
                                     }
-                                case .timedOut:
-                                    await execution.swiftyShellTeardown()
-                                    group.cancelAll()
-                                    throw TimeoutExceeded()
                                 }
                             }
                         } catch is CaptureLimitExceeded {
@@ -469,19 +450,8 @@ private struct SingleCommandRunner {
             }
 
             return output
-        } catch is TimeoutExceeded {
-            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
-            throw ShellError.timeout(
-                command: resolved.displayCommand,
-                duration: resolved.timeout ?? 0,
-                partialOutput: output
-            )
         } catch is CaptureLimitExceeded {
-            let output = try decodeOutput(
-                command: resolved.displayCommand,
-                snapshot: store.snapshot(),
-                exitCode: -1
-            )
+            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             throw ShellError.outputLimitExceeded(
                 command: resolved.displayCommand,
                 limit: resolved.outputLimit,
@@ -512,31 +482,28 @@ private struct SingleCommandRunner {
             return try await processTask.value
         case .timedOut:
             processTask.cancel()
-            await waitForPartialOutputFlush()
-            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             await registry.teardownAll()
+            _ = await processTask.result
+            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             throw ShellError.timeout(
                 command: resolved.displayCommand,
                 duration: resolved.timeout ?? 0,
                 partialOutput: output
             )
         case let .earlyTerminated(reason):
             processTask.cancel()
+            await registry.teardownAll()
+            _ = await processTask.result
             switch reason {
             case let .outputLimitExceeded(command, limit):
-                let output = try decodeOutput(
-                    command: command,
-                    snapshot: store.snapshot(),
-                    exitCode: -1
-                )
-                await registry.teardownAll()
+                let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
                 throw ShellError.outputLimitExceeded(command: command, limit: limit, partialOutput: output)
             }
         case .canceled:
             processTask.cancel()
-            await waitForPartialOutputFlush()
-            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             await registry.teardownAll()
+            _ = await processTask.result
+            let output = lossyOutput(snapshot: store.snapshot(), exitCode: -1)
             throw ShellError.canceled(command: resolved.displayCommand, partialOutput: output)
         }
     }
@@ -568,28 +535,18 @@ private func runWithProcessGroupTeardownOnCancellation<Value: Sendable>(
 }
 
 private extension Execution {
-    /// Kills the subprocess immediately by sending SIGKILL.
-    ///
-    /// Sends the signal directly to the process (not the process group) to avoid the TOCTOU
-    /// race on Linux where a recycled PGID could let the signal reach an unrelated process.
-    /// Each spawned subprocess is its own process group leader
-    /// (``subprocessPlatformOptions`` sets `processGroupID = 0`), so targeting the process
-    /// directly is equivalent to targeting its group.
-    ///
-    /// No SIGTERM grace period is used. Partial output is captured in ``OutputCaptureStore``
-    /// before teardown is triggered, so there is nothing to flush. A delayed SIGKILL from a
-    /// grace period causes cross-test signal bleed on Linux: under `--no-parallel` the next
-    /// test can spawn a subprocess that recycles the same PID within the grace window and
-    /// then receives the stale SIGKILL.
+    /// Kills the subprocess and every descendant that remains in its process group.
     ///
+    /// Every subprocess is its own process-group leader. Callers await `Subprocess.run` after
+    /// teardown, keeping that leader owned and unreaped until the group signal has been sent;
+    /// this prevents its PID/PGID from being recycled during teardown.
     func swiftyShellTeardown() async {
-        try? send(signal: .kill, toProcessGroup: false)
+        try? send(signal: .kill, toProcessGroup: true)
     }
 }
 
 private enum SingleCommandTaskResult: Sendable {
     case streamComplete
-    case timedOut
 }
 
 private enum SpawnedCommandTaskResult: Sendable {
@@ -1024,9 +981,9 @@ private struct PipelineRunner {
             }
         } catch is CancellationError {
             processTask.cancel()
-            await waitForPartialOutputFlush()
-            let snapshot = pipelineSnapshot(stores: stores)
             await registry.teardownAll()
+            _ = await processTask.result
+            let snapshot = pipelineSnapshot(stores: stores)
             throw ShellError.canceled(
                 command: finalCommand.displayCommand,
                 partialOutput: lossyOutput(snapshot: snapshot, exitCode: -1)
@@ -1214,31 +1171,28 @@ private struct PipelineRunner {
             return try await processTask.value
         case let .timedOut(duration):
             processTask.cancel()
-            await waitForPartialOutputFlush()
-            let snapshot = pipelineSnapshot(stores: stores)
             await registry.teardownAll()
+            _ = await processTask.result
+            let snapshot = pipelineSnapshot(stores: stores)
             throw ShellError.timeout(
                 command: finalCommand.displayCommand,
                 duration: duration,
                 partialOutput: lossyOutput(snapshot: snapshot, exitCode: -1)
             )
         case let .earlyTerminated(reason):
             processTask.cancel()
+            await registry.teardownAll()
+            _ = await processTask.result
             switch reason {
             case let .outputLimitExceeded(command, limit):
-                let output = try decodeOutput(
-                    command: command,
-                    snapshot: pipelineSnapshot(stores: stores),
-                    exitCode: -1
-                )
-                await registry.teardownAll()
+                let output = lossyOutput(snapshot: pipelineSnapshot(stores: stores), exitCode: -1)
                 throw ShellError.outputLimitExceeded(command: command, limit: limit, partialOutput: output)
             }
         case .canceled:
             processTask.cancel()
-            await waitForPartialOutputFlush()
-            let snapshot = pipelineSnapshot(stores: stores)
             await registry.teardownAll()
+            _ = await processTask.result
+            let snapshot = pipelineSnapshot(stores: stores)
             throw ShellError.canceled(
                 command: finalCommand.displayCommand,
                 partialOutput: lossyOutput(snapshot: snapshot, exitCode: -1)
@@ -1259,12 +1213,6 @@ private enum PipelineRunEvent: Sendable {
     case canceled
 }
 
-private func waitForPartialOutputFlush() async {
-    await Task.detached {
-        try? await Task.sleep(for: partialOutputFlushGrace)
-    }.value
-}
-
 private struct PipelinePipe: Sendable {
     let readEnd: FileDescriptor
     let writeEnd: FileDescriptor
@@ -1496,6 +1444,8 @@ private func runPipelineStageWithStreamedStdout<Input: InputProtocol>(
 /// the nanosecond-based overload that requires manual overflow handling.
 private func durationFromSeconds(_ seconds: TimeInterval) -> Duration {
     if seconds <= 0 { return .zero }
+    let maximumNanosecondDuration = Double(Int64.max) / 1_000_000_000
+    if seconds >= maximumNanosecondDuration { return .nanoseconds(Int64.max) }
     let wholeSeconds = Int64(seconds)
     let fractionalNanoseconds = Int64((seconds - Double(wholeSeconds)) * 1_000_000_000)
     return .seconds(wholeSeconds) + .nanoseconds(fractionalNanoseconds)

Sources/SwiftyShell/SwiftyShell.docc/Articles/CoreConcepts.md

@@ -199,7 +199,7 @@ public protocol CommandExecutor: Sendable {
 }
 ```
 
-``SubprocessExecutor`` is the default production executor and is backed by the `swift-subprocess` package. It resolves executables from ``ShellContext/searchPaths``, runs single commands and pipelines as subprocesses, and preserves captured partial output for timeout, cancellation, and output-limit failures. `run()` closes stdin and immediately sends `SIGKILL` directly to registered processes when it must stop them. Explicitly spawned processes instead use their configured ``TeardownStrategy`` when the caller requests teardown.
+``SubprocessExecutor`` is the default production executor and is backed by the `swift-subprocess` package. It resolves executables from ``ShellContext/searchPaths``, runs single commands and pipelines as subprocesses, closes stdin for `run()` calls, and preserves captured partial output for timeout, cancellation, and output-limit failures. On Unix platforms each process started for `run()` executes as its own process-group leader; early termination sends `SIGKILL` to the process group, including descendants, and waits for process completion before returning. Explicitly spawned processes instead use their configured ``TeardownStrategy`` when the caller requests teardown.
 
 ``MockExecutor`` is the test double. You can also implement your own — for example, to add structured logging around every command:
 

Sources/SwiftyShell/SwiftyShell.docc/Articles/ErrorHandling.md

@@ -124,7 +124,7 @@ try await Command("swift", arguments: "test", "--filter", "CommandTests")
 
 #### `outputLimitExceeded`
 
-Captured output exceeded the configured limit. For a single command, stdout and stderr share one limit. In a pipeline, each stage has its own limit: intermediate stdout is piped rather than captured, while each stage's captured stderr and the final stage's captured stdout count against that stage. The error includes captured partial output up to the cap. Increase the limit via ``Command/outputLimit(_:)`` or redirect output to a file with ``OutputDestination/file(path:append:)``.
+Captured output exceeded the configured limit. For a single command, stdout and stderr share one limit. In a pipeline, each stage has its own limit: intermediate stdout is piped rather than captured, while each stage's captured stderr and the final stage's captured stdout count against that stage. The error includes captured partial output up to the cap. Because a byte limit can split a UTF-8 scalar, truncated partial output uses lossy UTF-8 decoding and may end with the replacement character. Increase the limit via ``Command/outputLimit(_:)`` or redirect output to a file with ``OutputDestination/file(path:append:)``.
 
 ```swift
 // Redirect verbose output to a file instead of capturing it
@@ -146,7 +146,7 @@ The captured output could not be decoded as UTF-8. This happens with binary outp
 
 The Swift `Task` enclosing the `run()` call was canceled. SwiftyShell rethrows the cancellation as ``ShellError/canceled(command:partialOutput:)`` and attaches partial output captured so far.
 
-For `run()`, SwiftyShell immediately sends `SIGKILL` directly to each registered process on timeout or cancellation. This is separate from ``TeardownStrategy``, which controls caller-requested teardown for a process started with `spawn`.
+For `run()`, SwiftyShell tears down each subprocess and descendants in its dedicated process group when it must stop execution. Timeout, cancellation, and output-limit failures wait for process completion and reaping before returning. The process-group leader remains owned until signaling completes, preventing its PID and group ID from being recycled during teardown. This is separate from ``TeardownStrategy``, which controls caller-requested teardown for a process started with `spawn`.
 
 ```swift
 let task = Task {

Tests/SwiftyShellTests/Core/CommandTests.swift

@@ -2,6 +2,12 @@ import Foundation
 import Testing
 @testable import SwiftyShell
 
+#if canImport(Darwin)
+import Darwin
+#elseif canImport(Glibc)
+import Glibc
+#endif
+
 private func waitForFile(at path: String) async throws {
     for _ in 0..<500 {
         if FileManager.default.fileExists(atPath: path) {
@@ -12,6 +18,35 @@ private func waitForFile(at path: String) async throws {
     Issue.record("Timed out waiting for marker file at \(path)")
 }
 
+private func waitForProcessExit(processIdentifier: Int32) async throws {
+    for _ in 0..<500 {
+        if !processIsRunning(processIdentifier) {
+            return
+        }
+        try await Task.sleep(for: .milliseconds(10))
+    }
+    Issue.record("Timed out waiting for descendant process exit for pid \(processIdentifier)")
+}
+
+private func processIsRunning(_ processIdentifier: Int32) -> Bool {
+    if kill(processIdentifier, 0) == -1, errno == ESRCH {
+        return false
+    }
+
+    #if canImport(Glibc)
+    if let stat = try? String(contentsOfFile: "/proc/\(processIdentifier)/stat", encoding: .utf8),
+        let closeParen = stat.lastIndex(of: ")")
+    {
+        let remainder = stat[stat.index(after: closeParen)...].trimmingCharacters(in: .whitespacesAndNewlines)
+        if remainder.first == "Z" {
+            return false
+        }
+    }
+    #endif
+
+    return true
+}
+
 struct CommandTests {
     @Test func shellPlatformCurrentProvidesDefaultSearchPaths() {
         #expect(ShellPlatform.current.defaultSearchPaths.isEmpty == false)
@@ -134,6 +169,22 @@ struct CommandTests {
         }
     }
 
+    @Test func outputLimitTruncatingUTF8UsesLossyPartialDecoding() async throws {
+        do {
+            _ = try await Command("/bin/sh", arguments: "-c", "printf '\\360\\237\\230\\200'")
+                .outputLimit(2)
+                .run()
+            Issue.record("Expected outputLimitExceeded")
+        } catch let error as ShellError {
+            guard case let .outputLimitExceeded(_, limit, partialOutput) = error else {
+                Issue.record("Unexpected error: \(error)")
+                return
+            }
+            #expect(limit == 2)
+            #expect(partialOutput.stdout == "\u{FFFD}")
+        }
+    }
+
     @Test func outputLimitExceededDoesNotHangOnLargeOutput() async throws {
         let context = ShellContext(defaultTimeout: 1.0, defaultOutputLimit: 4)
 
@@ -198,6 +249,53 @@ struct CommandTests {
         }
     }
 
+    @Test func mockExecutorRejectsNonFiniteTimeout() async throws {
+        do {
+            _ = try await Command("fake-tool")
+                .timeout(.nan)
+                .run(in: ShellContext(executor: MockExecutor()))
+            Issue.record("Expected invalidConfiguration")
+        } catch let error as ShellError {
+            guard case let .invalidConfiguration(description) = error else {
+                Issue.record("Unexpected error: \(error)")
+                return
+            }
+            #expect(description == "Timeout must be greater than or equal to zero seconds")
+        }
+    }
+
+    @Test func extremeFiniteTimeoutDoesNotTrap() async throws {
+        let output = try await Command("echo", arguments: "hello")
+            .timeout(.greatestFiniteMagnitude)
+            .run()
+        #expect(output.stdout == "hello\n")
+    }
+
+    @Test func timeoutKillsDescendantsAndReapsLeaderBeforeReturning() async throws {
+        let childPIDPath = "/tmp/swiftyshell-descendant-\(UUID().uuidString)"
+        defer { try? FileManager.default.removeItem(atPath: childPIDPath) }
+
+        do {
+            _ = try await Command(
+                "/bin/sh",
+                arguments: "-c",
+                "sleep 30 & child=$!; printf '%s' \"$child\" > '\(childPIDPath)'; wait"
+            )
+            .timeout(5)
+            .run()
+            Issue.record("Expected timeout")
+        } catch let error as ShellError {
+            guard case .timeout = error else {
+                Issue.record("Unexpected error: \(error)")
+                return
+            }
+        }
+
+        let pidString = try String(contentsOfFile: childPIDPath, encoding: .utf8)
+        let childPID = try #require(Int32(pidString))
+        try await waitForProcessExit(processIdentifier: childPID)
+    }
+
     @Test func negativeOutputLimitIsRejected() async throws {
         do {
             _ = try await Command("echo", arguments: "hello")