fix(harness): prevent pipe deadlock in LocalFilesystemWithShell.execute - #2839
fix(harness): prevent pipe deadlock in LocalFilesystemWithShell.execute#2839zzz-ghost wants to merge 1 commit into
Conversation
Drain stdout/stderr on daemon threads concurrently with Process.waitFor. Previously a child writing more than the OS pipe buffer (~4 KB on Windows, 64 KB on Linux) blocked in write() while the parent blocked in waitFor(), deadlocking until the command was forcibly killed and misreported as a timeout (exit 124). Mirrors the fix already applied to ShellCommandTool in agentscope-core. Adds a regression test that prints ~70 KB from the shell and asserts the command completes with exit 0 and full output. Fixes agentscope-ai#2838
f8ae46b to
7fb5e9a
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note: the two earlier |
xiaoyuuuuuupeng
left a comment
There was a problem hiding this comment.
Deadlock fix LGTM.
Please open follow-up PR(s)/issues for:
- Cap writes inside
drainAsyncusingmaxOutputBytes(keep reading so the pipe stays drained; mark truncated). Post-hoc truncate can OOM before truncation runs. gemini-cli / qwen-code both bound capture during async drain. - Align reader teardown with
ShellCommandTool(Future.get+cancel(true), or afterjoinifisAlive()→ interrupt and discard the untrusted BAOS).
Optional: add a stderr / dual-stream regression test.
| int n; | ||
| try { | ||
| while ((n = in.read(chunk)) != -1) { | ||
| buf.write(chunk, 0, n); |
There was a problem hiding this comment.
nit / follow-up: please bound memory while draining, not only after assembling the final string.
maxOutputBytes currently truncates post-hoc, so a huge command can OOM before truncation. Cap buf.write once buf.size() hits the limit, but keep reading so the OS pipe stays drained (same idea as gemini-cli / qwen-code). Mark truncated when bytes were discarded.
|
|
||
| private static void joinQuietly(Thread t) { | ||
| try { | ||
| t.join(DRAIN_JOIN_TIMEOUT_MILLIS); |
There was a problem hiding this comment.
nit / follow-up: please align reader completion with ShellCommandTool.
join(5s) may return while the drainer is still alive; ByteArrayOutputStream is not thread-safe, so calling toString() concurrently with write is racy. Prefer Future.get(timeout) + cancel(true) and discard untrusted output on timeout, or after join check isAlive() → interrupt and do not trust the buffer.
|
Thanks! Filed #2850 to track these — will send a follow-up PR once this one lands. |
What this PR does
Fixes a pipe deadlock in
LocalFilesystemWithShell.execute(): the child process's stdout/stderr are now drained on daemon threads concurrently withProcess.waitFor(), instead of only after it returns.Root cause
Previously
execute()calledProcess.waitFor(timeout)first and readgetInputStream()/getErrorStream()only afterwards. If the child wrote more than the OS pipe buffer (≈4 KB on Windows, 64 KB default on Linux), it blocked inwrite()forever while the parent blocked inwaitFor()— a classic pipe deadlock. Every such command was misreported as a timeout (exit 124) and forcibly killed, even though it would have finished in well under a second.Fix
ByteArrayOutputStream) immediately afterProcessBuilder.start(), thenwaitFor(timeout), then join the drainers — destroying the process first on timeout so the drainers terminate.ShellCommandToolinagentscope-core("Start asynchronous stream readers immediately to prevent pipe buffer deadlock"), whichLocalFilesystemWithShellwas missed by.maxOutputBytestruncation.Testing
New regression test
execute_outputLargerThanOsPipeBufferCompletesWithoutDeadlockprints ~70 KB via shell builtins (works on bothshandcmd.exe, no Python needed):Verified on Windows 10 (≈4 KB pipe buffer); the threshold (68–72 KB) also exceeds the 64 KB Linux default pipe buffer, so the test covers both platforms.
Fixes #2838