-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(harness): prevent pipe deadlock in LocalFilesystemWithShell.execute #2839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,9 @@ | |
| import io.agentscope.harness.agent.filesystem.sandbox.AbstractSandboxFilesystem; | ||
| import io.agentscope.harness.agent.workspace.LocalFsMode; | ||
| import io.agentscope.harness.agent.workspace.PathPolicy; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.nio.charset.Charset; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
|
|
@@ -52,6 +54,15 @@ public class LocalFilesystemWithShell extends LocalFilesystem implements Abstrac | |
| /** Default timeout in seconds for shell command execution. */ | ||
| public static final int DEFAULT_EXECUTE_TIMEOUT = 120; | ||
|
|
||
| /** Read buffer size (bytes) for the stream drainer threads. */ | ||
| private static final int DRAIN_CHUNK_BYTES = 8192; | ||
|
|
||
| /** | ||
| * Safety net (millis) for joining drainer threads after the process exits or is destroyed. | ||
| * Never reached in practice: process exit closes the streams and ends the drainers at once. | ||
| */ | ||
| private static final long DRAIN_JOIN_TIMEOUT_MILLIS = 5000; | ||
|
|
||
| private final String sandboxId; | ||
| private final int defaultTimeout; | ||
| private final int maxOutputBytes; | ||
|
|
@@ -335,14 +346,27 @@ public ExecuteResponse execute( | |
|
|
||
| Process proc = pb.start(); | ||
|
|
||
| // stdout/stderr must be drained concurrently with waitFor: if the child writes | ||
| // more than the OS pipe buffer (~4 KB on Windows, 64 KB default on Linux) while | ||
| // the parent blocks in waitFor, both sides deadlock and every such command is | ||
| // misreported as a timeout (exit 124). | ||
| ByteArrayOutputStream stdoutBuf = new ByteArrayOutputStream(); | ||
| ByteArrayOutputStream stderrBuf = new ByteArrayOutputStream(); | ||
| Thread stdoutDrainer = drainAsync(proc.getInputStream(), stdoutBuf); | ||
| Thread stderrDrainer = drainAsync(proc.getErrorStream(), stderrBuf); | ||
|
|
||
| boolean finished = proc.waitFor(effectiveTimeout, TimeUnit.SECONDS); | ||
| if (!finished) { | ||
| proc.destroyForcibly(); | ||
| } | ||
| joinQuietly(stdoutDrainer); | ||
| joinQuietly(stderrDrainer); | ||
|
|
||
| Charset outputCharset = outputCharset(osName); | ||
| String stdout = new String(proc.getInputStream().readAllBytes(), outputCharset); | ||
| String stderr = new String(proc.getErrorStream().readAllBytes(), outputCharset); | ||
| String stdout = stdoutBuf.toString(outputCharset); | ||
| String stderr = stderrBuf.toString(outputCharset); | ||
|
|
||
| if (!finished) { | ||
| proc.destroyForcibly(); | ||
| String msg; | ||
| if (timeoutSeconds != null) { | ||
| msg = | ||
|
|
@@ -432,6 +456,38 @@ private Path resolveExecuteCwd(RuntimeContext rc) { | |
| return namespaced; | ||
| } | ||
|
|
||
| /** | ||
| * Continuously copies a subprocess stream into {@code buf} on a daemon thread so the child | ||
| * never blocks on a full OS pipe buffer. Read errors (e.g. the stream closing when the | ||
| * process is destroyed on timeout) end the drainer quietly. | ||
| */ | ||
| private static Thread drainAsync(InputStream in, ByteArrayOutputStream buf) { | ||
| Thread t = | ||
| new Thread( | ||
| () -> { | ||
| byte[] chunk = new byte[DRAIN_CHUNK_BYTES]; | ||
| int n; | ||
| try { | ||
| while ((n = in.read(chunk)) != -1) { | ||
| buf.write(chunk, 0, n); | ||
| } | ||
| } catch (IOException ignored) { | ||
| // Stream closed because the process was destroyed; nothing to do. | ||
| } | ||
| }); | ||
| t.setDaemon(true); | ||
| t.start(); | ||
| return t; | ||
| } | ||
|
|
||
| private static void joinQuietly(Thread t) { | ||
| try { | ||
| t.join(DRAIN_JOIN_TIMEOUT_MILLIS); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit / follow-up: please align reader completion with |
||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } | ||
|
|
||
| static Charset outputCharset(String osName) { | ||
| return outputCharset(osName, System.getProperty("native.encoding")); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit / follow-up: please bound memory while draining, not only after assembling the final string.
maxOutputBytescurrently truncates post-hoc, so a huge command can OOM before truncation. Capbuf.writeoncebuf.size()hits the limit, but keepreading so the OS pipe stays drained (same idea as gemini-cli / qwen-code). Marktruncatedwhen bytes were discarded.