From 8eba94f7dca7bff6c1358d48e9ece245acaab4e0 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Wed, 22 Jul 2026 10:46:11 -0400 Subject: [PATCH 1/2] fix(mcp): read pipe data before waitUntilExit to prevent deadlock Move readDataToEndOfFile() calls ahead of process.waitUntilExit() in ShellTool. When a child process writes more than the kernel pipe buffer (~64 KB on macOS), the write() blocks until someone drains the pipe. If the parent calls waitUntilExit() first, nobody is draining, so both parent and child block indefinitely. Reading pipes before waiting is the standard POSIX pattern and eliminates the deadlock. Add a regression test that generates 128 KB of output (via head -c 131072 /dev/zero | base64). Without the fix the test deadlocks (confirmed via 30-second timeout kill); with the fix it completes in ~0.08 seconds. --- .../MCP/Tools/ShellTool.swift | 7 ++++++- .../MCP/MCPSpecificToolTests.swift | 20 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift b/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift index d896a3e2b..46c058d72 100644 --- a/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift +++ b/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift @@ -65,10 +65,15 @@ public struct ShellTool: MCPTool { do { try process.run() - process.waitUntilExit() + // Read pipe data BEFORE waitUntilExit to avoid deadlock. + // If the child writes more than the pipe buffer (~64 KB), + // the kernel blocks the child's write() until the pipe is + // drained. Calling waitUntilExit() first means nobody is + // draining, so both parent and child block indefinitely. let outputData = outputPipe.fileHandleForReading.readDataToEndOfFile() let errorData = errorPipe.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() let output = String(data: outputData, encoding: .utf8) ?? "" let error = String(data: errorData, encoding: .utf8) ?? "" diff --git a/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift b/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift index 1c90015a7..a8a5ed0b6 100644 --- a/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift +++ b/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift @@ -429,6 +429,26 @@ struct MCPSpecificToolTests { try MCPAgentTool.validatedMaxSteps(101) } } + + // MARK: - Shell Tool Tests + + @Test(.timeLimit(.minutes(1))) + func `Shell tool does not deadlock on large output`() async throws { + let tool = makeTestTool(ShellTool.init) + + // Generate output larger than the macOS pipe buffer (~64 KB) + // to trigger a deadlock if waitUntilExit() precedes pipe reads. + let result = try await tool.execute(arguments: ToolArguments(raw: [ + "command": "head -c 131072 /dev/zero | base64", + ])) + + #expect(result.isError == false) + if case let .text(text, _, _) = result.content.first { + #expect(text.count > 100_000) + } else { + Issue.record("Expected text content from shell output") + } + } } @MainActor From ff2bffa5763a92b448a492e5960e4ee4a3245007 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 22 Jul 2026 20:35:14 -0700 Subject: [PATCH 2/2] fix(mcp): drain shell pipes concurrently --- .../MCP/Tools/ShellTool.swift | 11 ++++------- .../PeekabooTests/MCP/MCPSpecificToolTests.swift | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift b/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift index 46c058d72..3c30cd025 100644 --- a/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift +++ b/Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift @@ -66,13 +66,10 @@ public struct ShellTool: MCPTool { do { try process.run() - // Read pipe data BEFORE waitUntilExit to avoid deadlock. - // If the child writes more than the pipe buffer (~64 KB), - // the kernel blocks the child's write() until the pipe is - // drained. Calling waitUntilExit() first means nobody is - // draining, so both parent and child block indefinitely. - let outputData = outputPipe.fileHandleForReading.readDataToEndOfFile() - let errorData = errorPipe.fileHandleForReading.readDataToEndOfFile() + // Either pipe can fill while the other waits for EOF, so drain both concurrently. + async let outputRead = outputPipe.fileHandleForReading.readDataToEndOfFile() + async let errorRead = errorPipe.fileHandleForReading.readDataToEndOfFile() + let (outputData, errorData) = await (outputRead, errorRead) process.waitUntilExit() let output = String(data: outputData, encoding: .utf8) ?? "" diff --git a/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift b/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift index a8a5ed0b6..36eb0f8c8 100644 --- a/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift +++ b/Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift @@ -449,6 +449,22 @@ struct MCPSpecificToolTests { Issue.record("Expected text content from shell output") } } + + @Test(.timeLimit(.minutes(1))) + func `Shell tool does not deadlock when both pipes exceed their buffers`() async throws { + let tool = makeTestTool(ShellTool.init) + + let result = try await tool.execute(arguments: ToolArguments(raw: [ + "command": "head -c 131072 /dev/zero >&2; head -c 131072 /dev/zero", + ])) + + #expect(result.isError == false) + if case let .text(text, _, _) = result.content.first { + #expect(text.count == 131_072) + } else { + Issue.record("Expected text content from shell output") + } + } } @MainActor