Change runDetached to return an Execution instead of a ProcessIdentifier and make the HANDLE public

Currently runDetached returns a ProcessIdentifier, which on Unix is a pid and on Windows is a numeric process identifier. Similar to #92, there is a Windows-specific race condition here. On Unix the pid will be valid until someone waitid's it. But on Windows the ProcessIdentifier may already be invalid by the time runDetached returns.

This patch builds on #93 (where the HANDLE is stored in the Execution instead of being closed immediately), and now provides that HANDLE as part of the public Execution API. This allows callers to properly free the handle using CloseHandle, as well as not having to reverse engineer the handle from the numeric pid. #93 by itself would solve the race condition but introduce a resource leak. The API now also better matches the Unix behavior since it is the caller's responsibility to waitid the pid to clean up the resources, which is semantically similar to calling CloseHandle on Windows.

Closes #94

Author: jakepetroules

Sources/Subprocess/API.swift

@@ -614,7 +614,7 @@ public func runDetached(
     input: FileDescriptor? = nil,
     output: FileDescriptor? = nil,
     error: FileDescriptor? = nil
-) throws -> ProcessIdentifier {
+) throws -> Execution {
     let config: Configuration = Configuration(
         executable: executable,
         arguments: arguments,
@@ -643,7 +643,7 @@ public func runDetached(
     input: FileDescriptor? = nil,
     output: FileDescriptor? = nil,
     error: FileDescriptor? = nil
-) throws -> ProcessIdentifier {
+) throws -> Execution {
     switch (input, output, error) {
     case (.none, .none, .none):
         let processInput = NoInput()
@@ -653,7 +653,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.none, .none, .some(let errorFd)):
         let processInput = NoInput()
         let processOutput = DiscardedOutput()
@@ -665,7 +665,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.none, .some(let outputFd), .none):
         let processInput = NoInput()
         let processOutput = FileDescriptorOutput(
@@ -676,7 +676,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.none, .some(let outputFd), .some(let errorFd)):
         let processInput = NoInput()
         let processOutput = FileDescriptorOutput(
@@ -691,7 +691,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.some(let inputFd), .none, .none):
         let processInput = FileDescriptorInput(
             fileDescriptor: inputFd,
@@ -703,7 +703,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.some(let inputFd), .none, .some(let errorFd)):
         let processInput = FileDescriptorInput(
             fileDescriptor: inputFd, closeAfterSpawningProcess: false
@@ -717,7 +717,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.some(let inputFd), .some(let outputFd), .none):
         let processInput = FileDescriptorInput(
             fileDescriptor: inputFd,
@@ -732,7 +732,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     case (.some(let inputFd), .some(let outputFd), .some(let errorFd)):
         let processInput = FileDescriptorInput(
             fileDescriptor: inputFd,
@@ -750,7 +750,7 @@ public func runDetached(
             withInput: try processInput.createPipe(),
             outputPipe: try processOutput.createPipe(),
             errorPipe: try processError.createPipe()
-        ).execution.processIdentifier
+        ).execution
     }
 }
 

Sources/Subprocess/Configuration.swift

@@ -71,12 +71,11 @@ public struct Configuration: Sendable {
             outputPipe: output,
             errorPipe: error
         )
-        let pid = spawnResults.execution.processIdentifier
 
         var spawnResultBox: SpawnResult?? = consume spawnResults
         var _spawnResult = spawnResultBox!.take()!
 
-        let processIdentifier = _spawnResult.execution.processIdentifier
+        let execution = _spawnResult.execution
 
         let result: Swift.Result<Result, Error>
         do {
@@ -89,9 +88,8 @@ public struct Configuration: Sendable {
                 return try await body(_spawnResult.execution, inputIO, outputIO, errorIO)
             } onCleanup: {
                 // Attempt to terminate the child process
-                await Execution.runTeardownSequence(
-                    self.platformOptions.teardownSequence,
-                    on: pid
+                await execution.runTeardownSequence(
+                    self.platformOptions.teardownSequence
                 )
             })
         } catch {

Sources/Subprocess/Execution.swift

@@ -35,7 +35,7 @@ public struct Execution: Sendable {
     public let processIdentifier: ProcessIdentifier
 
     #if os(Windows)
-    internal nonisolated(unsafe) let processHandle: HANDLE
+    public nonisolated(unsafe) let processHandle: HANDLE
     internal let consoleBehavior: PlatformOptions.ConsoleBehavior
 
     init(

Sources/Subprocess/Platforms/Subprocess+Unix.swift

@@ -117,18 +117,6 @@ extension Execution {
     public func send(
         signal: Signal,
         toProcessGroup shouldSendToProcessGroup: Bool = false
-    ) throws {
-        try Self.send(
-            signal: signal,
-            to: self.processIdentifier,
-            toProcessGroup: shouldSendToProcessGroup
-        )
-    }
-
-    internal static func send(
-        signal: Signal,
-        to processIdentifier: ProcessIdentifier,
-        toProcessGroup shouldSendToProcessGroup: Bool
     ) throws {
         let pid = shouldSendToProcessGroup ? -(processIdentifier.value) : processIdentifier.value
         guard kill(pid, signal.rawValue) == 0 else {

Sources/Subprocess/Platforms/Subprocess+Windows.swift

@@ -501,29 +501,6 @@ extension Execution {
     /// Terminate the current subprocess with the given exit code
     /// - Parameter exitCode: The exit code to use for the subprocess.
     public func terminate(withExitCode exitCode: DWORD) throws {
-        try Self.terminate(self.processIdentifier, withExitCode: exitCode)
-    }
-
-    internal static func terminate(
-        _ processIdentifier: ProcessIdentifier,
-        withExitCode exitCode: DWORD
-    ) throws {
-        guard
-            let processHandle = OpenProcess(
-                // PROCESS_ALL_ACCESS
-                DWORD(STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE | 0xFFFF),
-                false,
-                processIdentifier.value
-            )
-        else {
-            throw SubprocessError(
-                code: .init(.failedToTerminate),
-                underlyingError: .init(rawValue: GetLastError())
-            )
-        }
-        defer {
-            CloseHandle(processHandle)
-        }
         guard TerminateProcess(processHandle, exitCode) else {
             throw SubprocessError(
                 code: .init(.failedToTerminate),
@@ -534,23 +511,6 @@ extension Execution {
 
     /// Suspend the current subprocess
     public func suspend() throws {
-        guard
-            let processHandle = OpenProcess(
-                // PROCESS_ALL_ACCESS
-                DWORD(STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE | 0xFFFF),
-                false,
-                self.processIdentifier.value
-            )
-        else {
-            throw SubprocessError(
-                code: .init(.failedToSuspend),
-                underlyingError: .init(rawValue: GetLastError())
-            )
-        }
-        defer {
-            CloseHandle(processHandle)
-        }
-
         let NTSuspendProcess: (@convention(c) (HANDLE) -> LONG)? =
             unsafeBitCast(
                 GetProcAddress(
@@ -575,23 +535,6 @@ extension Execution {
 
     /// Resume the current subprocess after suspension
     public func resume() throws {
-        guard
-            let processHandle = OpenProcess(
-                // PROCESS_ALL_ACCESS
-                DWORD(STANDARD_RIGHTS_REQUIRED | SYNCHRONIZE | 0xFFFF),
-                false,
-                self.processIdentifier.value
-            )
-        else {
-            throw SubprocessError(
-                code: .init(.failedToResume),
-                underlyingError: .init(rawValue: GetLastError())
-            )
-        }
-        defer {
-            CloseHandle(processHandle)
-        }
-
         let NTResumeProcess: (@convention(c) (HANDLE) -> LONG)? =
             unsafeBitCast(
                 GetProcAddress(

Sources/Subprocess/Teardown.swift

@@ -78,15 +78,8 @@ extension Execution {
     /// Teardown sequence always ends with a `.kill` signal
     /// - Parameter sequence: The  steps to perform.
     public func teardown(using sequence: some Sequence<TeardownStep> & Sendable) async {
-        await Self.runTeardownSequence(sequence, on: self.processIdentifier)
-    }
-
-    internal static func teardown(
-        using sequence: some Sequence<TeardownStep> & Sendable,
-        on processIdentifier: ProcessIdentifier
-    ) async {
         await withUncancelledTask {
-            await Self.runTeardownSequence(sequence, on: processIdentifier)
+            await runTeardownSequence(sequence)
         }
     }
 }
@@ -98,25 +91,10 @@ internal enum TeardownStepCompletion {
 }
 
 extension Execution {
-    internal static func gracefulShutDown(
-        _ processIdentifier: ProcessIdentifier,
+    internal func gracefulShutDown(
         allowedDurationToNextStep duration: Duration
     ) async {
         #if os(Windows)
-        guard
-            let processHandle = OpenProcess(
-                DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE),
-                false,
-                processIdentifier.value
-            )
-        else {
-            // Nothing more we can do
-            return
-        }
-        defer {
-            CloseHandle(processHandle)
-        }
-
         // 1. Attempt to send WM_CLOSE to the main window
         if _subprocess_windows_send_vm_close(
             processIdentifier.value
@@ -148,15 +126,13 @@ extension Execution {
         // Send SIGTERM
         try? self.send(
             signal: .terminate,
-            to: processIdentifier,
             toProcessGroup: false
         )
         #endif
     }
 
-    internal static func runTeardownSequence(
-        _ sequence: some Sequence<TeardownStep> & Sendable,
-        on processIdentifier: ProcessIdentifier
+    internal func runTeardownSequence(
+        _ sequence: some Sequence<TeardownStep> & Sendable
     ) async {
         // First insert the `.kill` step
         let finalSequence = sequence + [TeardownStep(storage: .kill)]
@@ -177,7 +153,6 @@ extension Execution {
                         }
                     }
                     await self.gracefulShutDown(
-                        processIdentifier,
                         allowedDurationToNextStep: allowedDuration
                     )
                     return await group.next()!
@@ -195,7 +170,7 @@ extension Execution {
                             return .processHasExited
                         }
                     }
-                    try? self.send(signal: signal, to: processIdentifier, toProcessGroup: false)
+                    try? self.send(signal: signal, toProcessGroup: false)
                     return await group.next()!
                 }
             #endif  // !os(Windows)
@@ -206,7 +181,7 @@ extension Execution {
                     withExitCode: 0
                 )
                 #else
-                try? self.send(signal: .kill, to: processIdentifier, toProcessGroup: false)
+                try? self.send(signal: .kill, toProcessGroup: false)
                 #endif
                 stepCompletion = .killedTheProcess
             }

Tests/SubprocessTests/SubprocessTests+Unix.swift

@@ -783,20 +783,20 @@ extension SubprocessUnixTests {
 extension SubprocessUnixTests {
     @Test func testRunDetached() async throws {
         let (readFd, writeFd) = try FileDescriptor.pipe()
-        let pid = try runDetached(
+        let execution = try runDetached(
             .path("/bin/bash"),
             arguments: ["-c", "echo $$"],
             output: writeFd
         )
         var status: Int32 = 0
-        waitpid(pid.value, &status, 0)
+        waitpid(execution.processIdentifier.value, &status, 0)
         #expect(_was_process_exited(status) > 0)
         try writeFd.close()
         let data = try await readFd.readUntilEOF(upToLength: 10)
         let resultPID = try #require(
             String(data: Data(data), encoding: .utf8)
         ).trimmingCharacters(in: .whitespacesAndNewlines)
-        #expect("\(pid.value)" == resultPID)
+        #expect("\(execution.processIdentifier.value)" == resultPID)
         try readFd.close()
     }
 

Tests/SubprocessTests/SubprocessTests+Windows.swift

@@ -684,27 +684,21 @@ extension SubprocessWindowsTests {
             DWORD(HANDLE_FLAG_INHERIT),
             0
         )
-        let pid = try Subprocess.runDetached(
+        let execution = try Subprocess.runDetached(
             .path("C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"),
             arguments: [
                 "-Command", "Write-Host $PID",
             ],
             output: writeFd
         )
-        // Wait for process to finish
-        guard
-            let processHandle = OpenProcess(
-                DWORD(PROCESS_QUERY_INFORMATION | SYNCHRONIZE),
-                false,
-                pid.value
-            )
-        else {
-            Issue.record("Failed to get process handle")
-            return
+        defer {
+            guard CloseHandle(execution.processHandle) else {
+                Issue.record("Failed to close process handle")
+            }
         }
 
         // Wait for the process to finish
-        WaitForSingleObject(processHandle, INFINITE)
+        WaitForSingleObject(execution.processHandle, INFINITE)
 
         // Up to 10 characters because Windows process IDs are DWORDs (UInt32), whose max value is 10 digits.
         let data = try await readFd.readUntilEOF(upToLength: 10)