From 38fddc374b6b7e29500bee426a0f4d5c938544a9 Mon Sep 17 00:00:00 2001 From: broken-circle <252359939+broken-circle@users.noreply.github.com> Date: Sun, 31 May 2026 12:05:07 -0700 Subject: [PATCH] Support configurable Windows Job Object assignment On Windows, processes are currently assigned to Job Objects unconditionally. The drawback to this is that assignment can fail when the parent is already in a Job Object that doesn't allow nesting, or when the system otherwise cannot form a valid hierarchy. Add `PlatformOptions.JobObjectAssignment`, describing how Job Object assignment is handled. The default behavior of `.always` matches the current behavior. --- Sources/Subprocess/Error.swift | 42 +-- .../Platforms/Subprocess+Windows.swift | 254 ++++++++++++------ Sources/Subprocess/Teardown.swift | 16 +- Tests/SubprocessTests/WindowsTests.swift | 52 ++++ 4 files changed, 268 insertions(+), 96 deletions(-) diff --git a/Sources/Subprocess/Error.swift b/Sources/Subprocess/Error.swift index cd1d8e4d..496e5258 100644 --- a/Sources/Subprocess/Error.swift +++ b/Sources/Subprocess/Error.swift @@ -112,7 +112,7 @@ extension SubprocessError { private enum Context: Sendable, Hashable { case string(String) case int(Int) - case processControlOperation(ProcessControlOperation) + case processControlOperation(ProcessControlOperation, reason: String?) } internal enum ProcessControlOperation: Sendable, Hashable { @@ -199,22 +199,26 @@ extension SubprocessError: CustomStringConvertible, CustomDebugStringConvertible } case .processControlFailed: - if let context = self.context[self.code], - case .processControlOperation(let operation) = context - { - switch operation { - case .sendSignal(let signal): - return "Failed to send signal \(signal) to child process" - case .terminate: - return "Failed to terminate child process." - case .suspend: - return "Failed to suspend child process." - case .resume: - return "Failed to resume child process." - } - } else { + guard let context = self.context[self.code], + case .processControlOperation(let operation, let reason) = context + else { return "Failed to control child process state" } + var message: [String] + switch operation { + case .sendSignal(let signal): + message = ["Failed to send signal \(signal) to child process."] + case .terminate: + message = ["Failed to terminate child process."] + case .suspend: + message = ["Failed to suspend child process."] + case .resume: + message = ["Failed to resume child process."] + } + if let reason { + message.append("Reason: \(reason)") + } + return message.joined(separator: " ") } } @@ -290,11 +294,15 @@ extension SubprocessError { ) } - internal static func processControlFailed(_ operation: ProcessControlOperation, underlyingError: UnderlyingError?) -> Self { + internal static func processControlFailed( + _ operation: ProcessControlOperation, + reason: String? = nil, + underlyingError: UnderlyingError? + ) -> Self { return SubprocessError( code: .processControlFailed, underlyingError: underlyingError, - context: [.processControlFailed: .processControlOperation(operation)] + context: [.processControlFailed: .processControlOperation(operation, reason: reason)] ) } diff --git a/Sources/Subprocess/Platforms/Subprocess+Windows.swift b/Sources/Subprocess/Platforms/Subprocess+Windows.swift index e511652a..9e74646d 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Windows.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Windows.swift @@ -46,10 +46,13 @@ extension Configuration { // Create the Job Object up front. It persists across all candidate path // attempts, and is owned by this function until ownership transfers to // the `ProcessIdentifier` on the success path. - let jobHandle = try Self.createJobObject() - var jobHandleOwned = true + let jobHandle: HANDLE? = + self.platformOptions.jobObjectAssignment.createsJob + ? try Self.createJobObject() + : nil + var jobHandleOwned: Bool = jobHandle != nil defer { - if jobHandleOwned { + if jobHandleOwned, let jobHandle { _ = CloseHandle(jobHandle) } } @@ -124,11 +127,12 @@ extension Configuration { // child after assigning it to the Job Object. let userManagesResume = (createProcessFlags & DWORD(CREATE_SUSPENDED)) != 0 - // Subprocess assigns every spawned child to a Job Object - // before any user code runs in the child. `CREATE_SUSPENDED` - // makes the assignment atomic, so it is always set - // regardless of what the configurator did. - createProcessFlags |= DWORD(CREATE_SUSPENDED) + // CREATE_SUSPENDED makes Job Object assignment atomic, so it's + // forced on whenever a job will be created. With `.never` there's + // no assignment to protect, so the child isn't force-suspended. + if self.platformOptions.jobObjectAssignment.createsJob { + createProcessFlags |= DWORD(CREATE_SUSPENDED) + } let spawnContext = SpawnContext( startupInfo: startupInfo, @@ -237,11 +241,13 @@ extension Configuration { ) } + let assignedJobHandle: HANDLE? do { - try Self.assignChildToJobObjectAndResume( + assignedJobHandle = try Self.assignChildToJobObjectAndResume( jobHandle: jobHandle, processInfo: processInfo, - resumeThread: !userManagesResume + assignment: self.platformOptions.jobObjectAssignment, + resumeThread: self.platformOptions.jobObjectAssignment.createsJob && !userManagesResume ) } catch { try self.safelyCloseMultiple( @@ -255,14 +261,17 @@ extension Configuration { throw error } - // Transfer ownership of the job handle to the `ProcessIdentifier`. - jobHandleOwned = false + // Ownership of the job (if the child is in one) transfers to + // ProcessIdentifier. If a job was created but the child isn't in + // it (`.bestEffort` failure), the original handle stays owned here + // and is closed by `defer`. + jobHandleOwned = assignedJobHandle == nil && jobHandle != nil let pid = ProcessIdentifier( value: processInfo.dwProcessId, processDescriptor: processInfo.hProcess, threadHandle: processInfo.hThread, - jobHandle: jobHandle + jobHandle: assignedJobHandle ) do { @@ -388,6 +397,37 @@ public struct PlatformOptions: Sendable { public static let minimized: Self = .init(.minimized) } + /// Describes how Job Object assignment is handled. + public struct JobObjectAssignment: Sendable, Hashable { + internal enum Storage: Sendable, Hashable { + case always + case bestEffort + case never + } + + internal let storage: Storage + + /// Always assign the child to a Job Object, and throw + /// ``SubprocessError/spawnFailed`` if assignment fails. + /// + /// This is the default behavior. + public static let always: Self = .init(storage: .always) + + /// Attempt to assign the child to a Job Object, and proceed + /// without one if assignment fails. + /// + /// Descendant tracking and targeting the descendant-group + /// teardown are unavailable for subprocesses for which + /// assignment fails. + public static let bestEffort: Self = .init(storage: .bestEffort) + + /// Never assign the child to a Job Object. + /// + /// Use this when manually handling job management, or when + /// descendant-group teardown is not necessary. + public static let never: Self = .init(storage: .never) + } + /// The user credentials for starting the process as another user. internal var userCredentials: UserCredentials? = nil /// The console behavior of the new process. @@ -404,6 +444,11 @@ public struct PlatformOptions: Sendable { /// The process identifier of the new process group /// is the same as the process identifier. public var createProcessGroup: Bool = false + /// Describes how Job Object assignment is handled. + /// + /// Defaults to always assigning the child to a Job Object, and throwing + /// ``SubprocessError/spawnFailed`` if assignment fails. + public var jobObjectAssignment: JobObjectAssignment = .always /// An ordered list of steps to tear down the child /// process if the parent task is canceled before /// the child process terminates. @@ -424,23 +469,26 @@ public struct PlatformOptions: Sendable { /// and startup info `STARTUPINFOW` before /// they are sent to `CreateProcessW()`. /// - /// - Important: Subprocess assigns every spawned child to - /// an internal Job Object before any user code runs in the child, - /// so teardown sequences targeting the process group can terminate - /// the child and all of its descendants together. To make this - /// assignment atomic, Subprocess always spawns children with - /// `CREATE_SUSPENDED` set in `dwCreationFlags`, regardless of the - /// value the configurator leaves in `dwCreationFlags`. By default, - /// Subprocess resumes the child after the assignment completes. - /// If the configurator sets `CREATE_SUSPENDED` in `dwCreationFlags`, - /// Subprocess treats this as a signal that user code will resume - /// the child explicitly, and therefore skips the internal `ResumeThread` - /// call. In that case the caller is responsible for calling - /// `ResumeThread` on the thread handle exposed through - /// `Execution.processIdentifier.threadHandle`. Otherwise, the child - /// will remain suspended indefinitely. If user code separately - /// assigns the child to its own Job Object after spawn, that - /// user-supplied job is nested inside Subprocess's job. + /// - Important: When `jobObjectAssignment` is `.always` or `.bestEffort`, + /// Subprocess assigns the spawned child to an internal Job Object before + /// any user code runs in the child, so teardown sequences targeting the + /// process group can terminate the child and all of its descendants + /// together. (See ``JobObjectAssignment`` for how `.bestEffort` behaves + /// when assignment fails.) To make the assignment atomic, Subprocess + /// spawns such children with `CREATE_SUSPENDED` set in `dwCreationFlags`, + /// regardless of the value the configurator leaves there, and by default + /// resumes the child once assignment completes. If user code separately + /// assigns the child to its own Job Object after spawn, that user-supplied + /// job is nested inside Subprocess's job. When `jobObjectAssignment` is + /// `.never`, Subprocess creates no Job Object and does not force + /// `CREATE_SUSPENDED`; the configurator's `dwCreationFlags` are used as-is. + /// Regardless of `jobObjectAssignment`, if the configurator sets + /// `CREATE_SUSPENDED` in `dwCreationFlags`, Subprocess treats this as a + /// signal that user code will resume the child explicitly and skips its + /// internal `ResumeThread` call. The caller is then responsible for + /// calling `ResumeThread` on the thread handle exposed through + /// `Execution.processIdentifier.threadHandle`; otherwise the child remains + /// suspended indefinitely. public var preSpawnProcessConfigurator: ( @Sendable ( @@ -497,6 +545,35 @@ extension PlatformOptions.WindowStyle: CustomStringConvertible, CustomDebugStrin } } +extension PlatformOptions.JobObjectAssignment { + /// Whether this mode creates and attempts to assign a Job Object. + internal var createsJob: Bool { + switch self.storage { + case .always, .bestEffort: return true + case .never: return false + } + } +} + +extension PlatformOptions.JobObjectAssignment: CustomStringConvertible, CustomDebugStringConvertible { + /// A textual representation of the Job Object assignment mode. + public var description: String { + switch self.storage { + case .always: + return "always" + case .bestEffort: + return "bestEffort" + case .never: + return "never" + } + } + + /// A debug-oriented textual representation of the Job Object assignment mode. + public var debugDescription: String { + return self.description + } +} + extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible { internal func description(withIndent indent: Int) -> String { let indent = String(repeating: " ", count: indent * 4) @@ -506,6 +583,7 @@ extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible \(indent) consoleBehavior: \(self.consoleBehavior), \(indent) windowStyle: \(self.windowStyle), \(indent) createProcessGroup: \(self.createProcessGroup), + \(indent) jobObjectAssignment: \(self.jobObjectAssignment), \(indent) preSpawnProcessConfigurator: \(self.preSpawnProcessConfigurator == nil ? "not set" : "set") \(indent)) """ @@ -600,12 +678,25 @@ extension Execution { /// exit code is propagated to every process in the job. When `false` /// (the default), terminates only the immediate child process, leaving /// descendants unaffected. + /// - Throws: `SubprocessError` with error code `.processControlFailed`. + /// Passing `toProcessGroup: true` throws when the subprocess isn't assigned + /// to a Job Object (`jobObjectAssignment` was `.never`, or `.bestEffort` + /// and assignment failed). public func terminate( withExitCode exitCode: DWORD, toProcessGroup: Bool = false ) throws(SubprocessError) { if toProcessGroup { - guard TerminateJobObject(self.processIdentifier.jobHandle, exitCode) else { + guard let jobHandle = self.processIdentifier.jobHandle else { + // No Job Object assigned, so there's no descendant group to + // terminate. Throw rather than silently degrading to per-process. + throw SubprocessError.processControlFailed( + .terminate, + reason: "The subprocess is not assigned to a Job Object. Set PlatformOptions.jobObjectAssignment to .always or .bestEffort to enable process-group termination.", + underlyingError: nil + ) + } + guard TerminateJobObject(jobHandle, exitCode) else { throw SubprocessError.processControlFailed( .terminate, underlyingError: SubprocessError.WindowsError(win32Error: GetLastError()) @@ -912,14 +1003,17 @@ public struct ProcessIdentifier: Sendable, Hashable { public nonisolated(unsafe) let processDescriptor: HANDLE /// The main thread handle for this execution. public nonisolated(unsafe) let threadHandle: HANDLE - /// The Job Object handle that contains this process and any descendants. - internal nonisolated(unsafe) let jobHandle: HANDLE + /// The Job Object handle that contains this process and any descendants, + /// or `nil` if the process isn't assigned to a Job Object, either because + /// `PlatformOptions.jobObjectAssignment` was `.never`, or because it was + /// `.bestEffort` and assignment failed. + internal nonisolated(unsafe) let jobHandle: HANDLE? internal init( value: DWORD, processDescriptor: HANDLE, threadHandle: HANDLE, - jobHandle: HANDLE + jobHandle: HANDLE? ) { self.value = value self.processDescriptor = processDescriptor @@ -934,8 +1028,10 @@ public struct ProcessIdentifier: Sendable, Hashable { guard CloseHandle(self.processDescriptor) else { fatalError("Failed to close process HANDLE: \(SubprocessError.WindowsError(win32Error: GetLastError()))") } - guard CloseHandle(self.jobHandle) else { - fatalError("Failed to close job HANDLE: \(SubprocessError.WindowsError(win32Error: GetLastError()))") + if let jobHandle = self.jobHandle { + guard CloseHandle(jobHandle) else { + fatalError("Failed to close job HANDLE: \(SubprocessError.WindowsError(win32Error: GetLastError()))") + } } } } @@ -1169,58 +1265,66 @@ extension Configuration { return jobHandle } - /// Assigns the child process to the Job Object and, if requested, resumes - /// the child process thread. + /// Assigns the child process to the Job Object as specified by `assignment` + /// and, if requested, resumes the child process thread. /// /// When `resumeThread` is `false`, the caller is responsible for resuming /// the child using `ResumeThread` on the thread handle exposed through /// `Execution.processIdentifier.threadHandle`. The child remains /// suspended until then. + /// + /// The returned handle will be `nil` if `assignment` is `.never`, or if it + /// is `.bestEffort` and assignment failed. private static func assignChildToJobObjectAndResume( - jobHandle: HANDLE, + jobHandle: HANDLE?, processInfo: PROCESS_INFORMATION, + assignment: PlatformOptions.JobObjectAssignment, resumeThread: Bool - ) throws(SubprocessError) { - // `CreateProcessW` succeeded. The child is suspended. Assign it to - // the Job Object before resuming, so it cannot run any user code - // outside the job. - guard AssignProcessToJobObject(jobHandle, processInfo.hProcess) else { - let assignError = SubprocessError.WindowsError(win32Error: GetLastError()) - _ = TerminateProcess(processInfo.hProcess, 1) - _ = CloseHandle(processInfo.hThread) - _ = CloseHandle(processInfo.hProcess) - - // Detect the most common cause of assignment failure: the parent - // process itself is in a non-nestable Job Object. - var isParentInJob: WindowsBool = false - var reason: String = "Failed to assign child process to Job Object" - if IsProcessInJob(GetCurrentProcess(), nil, &isParentInJob), - isParentInJob.boolValue - { - reason += " (likely because the parent process is in a Job Object that does not allow nesting)" - } + ) throws(SubprocessError) -> HANDLE? { + var assignedJob: HANDLE? = nil + + // A job handle exists only if assignment is `.always` or `.bestEffort`. + if let jobHandle { + if AssignProcessToJobObject(jobHandle, processInfo.hProcess) { + assignedJob = jobHandle + } else if case .always = assignment.storage { + let assignError = SubprocessError.WindowsError(win32Error: GetLastError()) + _ = TerminateProcess(processInfo.hProcess, 1) + _ = CloseHandle(processInfo.hThread) + _ = CloseHandle(processInfo.hProcess) + + // Detect the most common cause of assignment failure: the parent + // process itself is in a non-nestable Job Object. + var isParentInJob: WindowsBool = false + var reason = "Failed to assign child process to Job Object" + if IsProcessInJob(GetCurrentProcess(), nil, &isParentInJob), + isParentInJob.boolValue + { + reason += " (likely because the parent process is in a Job Object that does not allow nesting)" + } - throw SubprocessError.spawnFailed( - withUnderlyingError: assignError, - reason: reason - ) + throw SubprocessError.spawnFailed( + withUnderlyingError: assignError, + reason: reason + ) + } + // `.bestEffort` failure: proceed without a job; fall through to resume. } - guard resumeThread else { - // The user opted into managing the resume themselves. - return + if resumeThread { + guard ResumeThread(processInfo.hThread) != DWORD(bitPattern: -1) else { + let resumeError = SubprocessError.WindowsError(win32Error: GetLastError()) + _ = TerminateProcess(processInfo.hProcess, 1) + _ = CloseHandle(processInfo.hThread) + _ = CloseHandle(processInfo.hProcess) + throw SubprocessError.spawnFailed( + withUnderlyingError: resumeError, + reason: "Failed to resume child process thread after Job Object assignment" + ) + } } - guard ResumeThread(processInfo.hThread) != DWORD(bitPattern: -1) else { - let resumeError = SubprocessError.WindowsError(win32Error: GetLastError()) - _ = TerminateProcess(processInfo.hProcess, 1) - _ = CloseHandle(processInfo.hThread) - _ = CloseHandle(processInfo.hProcess) - throw SubprocessError.spawnFailed( - withUnderlyingError: resumeError, - reason: "Failed to resume child process thread after Job Object assignment" - ) - } + return assignedJob } private func generateWindowsCommandAndArguments( diff --git a/Sources/Subprocess/Teardown.swift b/Sources/Subprocess/Teardown.swift index 7c52ec6d..9c2bd723 100644 --- a/Sources/Subprocess/Teardown.swift +++ b/Sources/Subprocess/Teardown.swift @@ -245,10 +245,18 @@ extension Execution { #endif // !os(Windows) case .kill: #if os(Windows) - try? self.terminate( - withExitCode: 0, - toProcessGroup: killProcessGroup - ) + // Teardown must always terminate the child. Group termination + // requires a Job Object; fall back to per-process termination + // when the child isn't in one, or when group termination fails. + if killProcessGroup { + do { + try self.terminate(withExitCode: 0, toProcessGroup: true) + } catch { + try? self.terminate(withExitCode: 0, toProcessGroup: false) + } + } else { + try? self.terminate(withExitCode: 0, toProcessGroup: false) + } #else try? self.send(signal: .kill, toProcessGroup: killProcessGroup) #endif diff --git a/Tests/SubprocessTests/WindowsTests.swift b/Tests/SubprocessTests/WindowsTests.swift index 5e1e8b36..fe3c03e4 100644 --- a/Tests/SubprocessTests/WindowsTests.swift +++ b/Tests/SubprocessTests/WindowsTests.swift @@ -435,6 +435,58 @@ extension SubprocessWindowsTests { } #expect(result.terminationStatus.isSuccess) } + + @Test func testJobObjectAssignmentNeverDoesNotSuspendChild() async throws { + // Complement of `testPreSpawnConfiguratorCanOptIntoManualResume`. With + // `.never`, Subprocess creates no Job Object and does not force + // `CREATE_SUSPENDED`, so the child runs immediately and `ResumeThread` + // reports a previous suspend count of `0`. + var platformOptions = PlatformOptions() + platformOptions.jobObjectAssignment = .never + + let result = try await Subprocess.run( + self.cmdExe, + arguments: ["/c", "type con"], + platformOptions: platformOptions, + input: .none, + output: .discarded, + error: .discarded + ) { execution in + let previousSuspendCount = ResumeThread(execution.processIdentifier.threadHandle) + #expect(previousSuspendCount == 0) + try execution.terminate(withExitCode: 0) + } + #expect(result.terminationStatus.isSuccess) + } + + @Test func testJobObjectAssignmentNeverDisablesGroupTermination() async throws { + var platformOptions = PlatformOptions() + platformOptions.jobObjectAssignment = .never + + let result = try await Subprocess.run( + self.cmdExe, + // Intentionally hangs so the child is alive for the body. + arguments: ["/c", "type con"], + platformOptions: platformOptions, + input: .none, + output: .discarded, + error: .discarded + ) { execution in + // No Job Object means group termination has no group to target. + let error = try #require(throws: SubprocessError.self) { + try execution.terminate(withExitCode: 0, toProcessGroup: true) + } + #expect(error.code == .processControlFailed) + + // Per-process termination still works. + try execution.terminate(withExitCode: 0, toProcessGroup: false) + } + guard case .exited(let code) = result.terminationStatus else { + Issue.record("Process should have exited") + return + } + #expect(code == 0) + } } // MARK: - Batch File Argument Escaping (BatBadBut)