diff --git a/apps/macos/Sources/OpenClaw/ShellExecutor.swift b/apps/macos/Sources/OpenClaw/ShellExecutor.swift index 6c1c8e9742bc..ac7e21ab0527 100644 --- a/apps/macos/Sources/OpenClaw/ShellExecutor.swift +++ b/apps/macos/Sources/OpenClaw/ShellExecutor.swift @@ -11,34 +11,79 @@ enum ShellExecutor { var errorMessage: String? } + /// A background descendant may inherit stdout after its parent exits. + /// Seekable files let the parent result finish without waiting for that unrelated process. + private final class OutputFiles: @unchecked Sendable { + let stdout: FileHandle + let stderr: FileHandle + private let stdoutURL: URL + private let stderrURL: URL + + init() throws { + let directory = FileManager.default.temporaryDirectory + .appendingPathComponent("openclaw-shell-\(UUID().uuidString)", isDirectory: true) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + self.stdoutURL = directory.appendingPathComponent("stdout") + self.stderrURL = directory.appendingPathComponent("stderr") + FileManager.default.createFile(atPath: self.stdoutURL.path, contents: nil) + FileManager.default.createFile(atPath: self.stderrURL.path, contents: nil) + self.stdout = try FileHandle(forWritingTo: self.stdoutURL) + self.stderr = try FileHandle(forWritingTo: self.stderrURL) + } + + func readAndRemove() -> (stdout: String, stderr: String) { + try? self.stdout.close() + try? self.stderr.close() + let stdoutData = (try? Data(contentsOf: self.stdoutURL)) ?? Data() + let stderrData = (try? Data(contentsOf: self.stderrURL)) ?? Data() + try? FileManager.default.removeItem(at: self.stdoutURL.deletingLastPathComponent()) + return ( + String(bytes: stdoutData, encoding: .utf8) ?? "", + String(bytes: stderrData, encoding: .utf8) ?? "") + } + } + private final class CompletionBox: @unchecked Sendable { private let lock = NSLock() private var finished = false private let continuation: CheckedContinuation + private let output: OutputFiles - init(continuation: CheckedContinuation) { + init(continuation: CheckedContinuation, output: OutputFiles) { self.continuation = continuation + self.output = output } - func finish(_ result: ShellResult) { + func finish( + status: Int?, + timedOut: Bool, + errorMessage: String?, + beforeCapture: (@Sendable () -> Void)? = nil) + { self.lock.lock() - defer { self.lock.unlock() } - guard !self.finished else { return } + guard !self.finished else { + self.lock.unlock() + return + } self.finished = true - self.continuation.resume(returning: result) + self.lock.unlock() + beforeCapture?() + let captured = self.output.readAndRemove() + self.continuation.resume(returning: ShellResult( + stdout: captured.stdout, + stderr: captured.stderr, + exitCode: status, + timedOut: timedOut, + success: status == 0 && !timedOut && errorMessage == nil, + errorMessage: errorMessage ?? status.flatMap { $0 == 0 ? nil : "exit \($0)" })) } } - private static func completedResult( - status: Int, - outTask: Task, - errTask: Task) async -> ShellResult - { - let out = await outTask.value - let err = await errTask.value + private static func completedResult(status: Int, output: OutputFiles) -> ShellResult { + let captured = output.readAndRemove() return ShellResult( - stdout: String(bytes: out, encoding: .utf8) ?? "", - stderr: String(bytes: err, encoding: .utf8) ?? "", + stdout: captured.stdout, + stderr: captured.stderr, exitCode: status, timedOut: false, success: status == 0, @@ -71,60 +116,9 @@ enum ShellExecutor { process.environment = env } - let stdoutPipe = Pipe() - let stderrPipe = Pipe() - process.standardOutput = stdoutPipe - process.standardError = stderrPipe - - let outTask = Task { stdoutPipe.fileHandleForReading.readToEndSafely() } - let errTask = Task { stderrPipe.fileHandleForReading.readToEndSafely() } - - if let timeout, timeout > 0 { - return await withCheckedContinuation { continuation in - let completion = CompletionBox(continuation: continuation) - - process.terminationHandler = { terminatedProcess in - let status = Int(terminatedProcess.terminationStatus) - Task { - let result = await self.completedResult( - status: status, - outTask: outTask, - errTask: errTask) - completion.finish(result) - } - } - - do { - try process.run() - } catch { - completion.finish( - ShellResult( - stdout: "", - stderr: "", - exitCode: nil, - timedOut: false, - success: false, - errorMessage: "failed to start: \(error.localizedDescription)")) - return - } - - DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + timeout) { - guard process.isRunning else { return } - process.terminate() - completion.finish( - ShellResult( - stdout: "", - stderr: "", - exitCode: nil, - timedOut: true, - success: false, - errorMessage: "timeout")) - } - } - } - + let output: OutputFiles do { - try process.run() + output = try OutputFiles() } catch { return ShellResult( stdout: "", @@ -132,14 +126,57 @@ enum ShellExecutor { exitCode: nil, timedOut: false, success: false, + errorMessage: "failed to capture output: \(error.localizedDescription)") + } + process.standardOutput = output.stdout + process.standardError = output.stderr + + if let timeout, timeout > 0 { + return await withCheckedContinuation { continuation in + let completion = CompletionBox(continuation: continuation, output: output) + + process.terminationHandler = { terminatedProcess in + let status = Int(terminatedProcess.terminationStatus) + completion.finish(status: status, timedOut: false, errorMessage: nil) + } + + do { + try process.run() + } catch { + completion.finish( + status: nil, + timedOut: false, + errorMessage: "failed to start: \(error.localizedDescription)") + return + } + + DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + timeout) { + guard process.isRunning else { return } + // Claim timeout classification before SIGTERM can trigger the termination handler. + completion.finish( + status: nil, + timedOut: true, + errorMessage: "timeout", + beforeCapture: { process.terminate() }) + } + } + } + + do { + try process.run() + } catch { + let captured = output.readAndRemove() + return ShellResult( + stdout: captured.stdout, + stderr: captured.stderr, + exitCode: nil, + timedOut: false, + success: false, errorMessage: "failed to start: \(error.localizedDescription)") } process.waitUntilExit() - return await self.completedResult( - status: Int(process.terminationStatus), - outTask: outTask, - errTask: errTask) + return self.completedResult(status: Int(process.terminationStatus), output: output) } static func run(command: [String], cwd: String?, env: [String: String]?, timeout: Double?) async -> Response { diff --git a/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift b/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift index 8b9fae39aa51..811488c21b15 100644 --- a/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/LowCoverageHelperTests.swift @@ -54,8 +54,14 @@ struct LowCoverageHelperTests { } @Test func `shell executor times out`() async { - let result = await ShellExecutor.runDetailed(command: ["/bin/sleep", "1"], cwd: nil, env: nil, timeout: 0.05) - #expect(result.timedOut == true) + for _ in 0..<10 { + let result = await ShellExecutor.runDetailed( + command: ["/bin/sleep", "1"], + cwd: nil, + env: nil, + timeout: 0.01) + #expect(result.timedOut == true) + } } @Test func `shell executor drains stdout and stderr`() async { @@ -77,6 +83,19 @@ struct LowCoverageHelperTests { #expect(result.stderr.contains("stderr-1999")) } + @Test func `shell executor finishes when a descendant retains output handles`() async { + let startedAt = ContinuousClock.now + let result = await ShellExecutor.runDetailed( + command: ["/bin/sh", "-c", "sleep 5 & echo ready"], + cwd: nil, + env: nil, + timeout: 1) + + #expect(result.success == true) + #expect(result.stdout.contains("ready")) + #expect(ContinuousClock.now - startedAt < .seconds(2)) + } + @Test func `node info codable round trip`() throws { let info = NodeInfo( nodeId: "node-1",