fix(harness): drain local shell output concurrently#2151
Conversation
|
u010143241 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
added
bug
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR fixes the pipe deadlock issue in LocalFilesystemWithShell.execute() (Issue #1519). The old implementation called proc.waitFor() before reading stdout/stderr synchronously, causing a deadlock when subprocess output exceeds the OS pipe buffer (~64KB). The new implementation starts two guardian threads immediately after process launch to concurrently drain stdout/stderr, sharing a unified deadline for timeout control, and terminates the process tree on timeout/interruption. Six new test cases cover large output, mixed output, inherited pipe, and descendant termination scenarios. The fix correctly addresses the described issue with industry-standard concurrent drain patterns.
| Executors.newFixedThreadPool( | ||
| 2, | ||
| runnable -> { | ||
| Thread thread = new Thread(runnable, "local-shell-output-drainer"); |
There was a problem hiding this comment.
[minor] Each execute() call creates a dedicated ExecutorService (2-thread pool). In high-frequency command execution scenarios, this has overhead from frequent thread pool creation/destruction. While shutdownNow() in finally guarantees cleanup, consider maintaining a shared, bounded ExecutorService at the class level (e.g. Executors.newCachedThreadPool with daemon thread factory), closed in close()/shutdown(). If call frequency is low, current approach is acceptable.
| ExecutorService drainers = | ||
| Executors.newFixedThreadPool( | ||
| 2, | ||
| runnable -> { |
There was a problem hiding this comment.
[minor] All drainer threads share the same name local-shell-output-drainer. When multiple execute() calls run concurrently, thread dumps cannot distinguish which thread belongs to which command. Consider adding a unique identifier to the thread name (e.g. sandboxId or auto-incrementing counter): "local-shell-drainer-" + sandboxId + "-" + streamName. This is valuable for production deadlock/timeout troubleshooting.
| return timeoutResponse(effectiveTimeout, timeoutSeconds != null); | ||
| } | ||
| try { | ||
| stdoutCapture = awaitCapture(stdoutFuture, deadlineNanos); |
There was a problem hiding this comment.
[nitpick] When stdout awaitCapture succeeds but stderr times out, the collected stdout data is discarded and only a timeout message is returned. Consider attaching partial output to the timeout response — partial data is more useful than a pure timeout message for long-running command debugging.
| private static void terminateProcessTree( | ||
| Process process, Set<ProcessHandle> observedDescendants) { | ||
| List<ProcessHandle> handles = new ArrayList<>(observedDescendants); | ||
| process.toHandle().descendants().forEach(handles::add); |
There was a problem hiding this comment.
[nitpick] terminateProcessTree uses descending PID order to infer parent-child relationships (higher PID = created later = kill first). This works in most cases but can misorder on PID wrap-around. The subsequent destroyForcibly fallback sweep guarantees correctness, but consider adding a comment explaining this is a heuristic strategy with a fallback mechanism.
| return new CapturedOutput(captured.toString(StandardCharsets.UTF_8), truncated); | ||
| } | ||
|
|
||
| private static boolean waitForProcess( |
There was a problem hiding this comment.
[nitpick] The 50ms polling interval calls process.toHandle().descendants() which creates a new Stream and reads /proc (Linux) each time. For the default 120s timeout, this means ~2400 descendant scans. Current overhead is acceptable, but consider reducing descendant collection frequency (e.g. every 500ms) or collecting once after process exit.
| + "\n\n... Output truncated at " | ||
| + maxOutputBytes | ||
| + " bytes."; | ||
| boolean truncated = stdoutCapture.truncated() || stderrCapture.truncated(); |
There was a problem hiding this comment.
[nitpick] Behavioral change (positive): Old code only reported truncation when the final concatenated output exceeded maxOutputBytes; new code reports truncation when any individual stream is truncated. This is more accurate — old code could silently drop stream data without notifying the user. Consider documenting this semantic change in the PR description or commit message to avoid surprising downstream consumers of the old behavior.
3 participants
Summary:
Fixes #1519
Verification: upstream local shell 6/6 including 30K/90K, stderr, mixed output, inherited pipe, and descendant marker cleanup; broader filesystem/Harness tests 68/68; JCode upstream consumption 4/4; Spotless and git diff check passed.