Skip to content

fix(harness): pass docker exec scripts via stdin on Windows (nested quotes survive) #3383

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.

Sign up for GitHub

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

Open
aiyili wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(harness): pass docker exec scripts via stdin on Windows (nested quotes survive) #3383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb08141
fix(harness): DockerSandbox.doExec 在 Windows 上经 stdin 传递脚本,嵌套双引号不再丢失
aiyili Oct 1, 2026
f48fc77
fix+test(harness): doExec stdin 写入移到 drainer 之后并提 seam 可测(评审跟进)
aiyili Oct 1, 2026
d240e15
fix(harness): drainer 池在 Windows 下扩为 3 线程,stdin 写不再被饿死(复审 Critical)
aiyili Oct 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 77 additions & 8 deletions ...-harness/src/main/java/io/agentscope/harness/agent/sandbox/impl/docker/DockerSandbox.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.nio.file.attribute.PosixFilePermissions;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
Expand Down Expand Up @@ -141,28 +142,68 @@ public void shutdown() throws Exception {
}
}

@Override
protected ExecResult doExec(RuntimeContext runtimeContext, String command, int timeoutSeconds)
throws Exception {
String containerId = dockerState.getContainerId();
String workspaceRoot = dockerState.getWorkspaceRoot();
/**
* Writes the script to the process stdin and closes the stream (EOF tells non-interactive
* {@code sh} to run the buffered script). Package-private seam so the stdin transport —
* the actual #2924 fix — is testable without a Docker daemon.
*/
static void writeScriptTo(java.io.OutputStream stdin, String command)
throws java.io.IOException {
try (OutputStream out = stdin) {
out.write(command.getBytes(StandardCharsets.UTF_8));
}
}

/**
* Builds the {@code docker exec} argument list for a shell command. On Windows the script
* travels via stdin ({@code -i} + plain {@code sh}, no {@code -c}): Java's {@link
* ProcessBuilder} quotes argv into a single command line for {@code CreateProcess}, and a
* native child (docker.exe) re-parses that line with MSVCRT-style rules, which strips the
* nested double quotes sandbox commands rely on (e.g. {@code "$(dirname ...)"}); a
* Linux/macOS host passes them through untouched (#2924).
*
* <p>Package-private for tests.
*/
static List<String> buildExecArgs(
String command, String workspaceRoot, String containerId, boolean windows) {
List<String> cmd = new ArrayList<>();
cmd.add("docker");
cmd.add("exec");
if (windows) {
cmd.add("-i");
}
cmd.add("-w");
cmd.add(workspaceRoot);
cmd.add(containerId);
cmd.add("sh");
cmd.add("-c");
cmd.add(command);
if (windows) {
// no "-c": sh executes the whole stdin as the script
} else {
cmd.add("-c");
cmd.add(command);
}
return cmd;
}

@Override
protected ExecResult doExec(RuntimeContext runtimeContext, String command, int timeoutSeconds)
throws Exception {
String containerId = dockerState.getContainerId();
String workspaceRoot = dockerState.getWorkspaceRoot();

boolean windows =
System.getProperty("os.name", "").toLowerCase(Locale.ROOT).contains("win");
List<String> cmd = buildExecArgs(command, workspaceRoot, containerId, windows);

ProcessBuilder pb = new ProcessBuilder(cmd);
Process process = pb.start();

// 3 threads on Windows: stdout drainer + stderr drainer + the stdin writer — the
// drainers only finish at process EOF, which never comes while sh waits for stdin,
// so a 2-thread pool would starve the write into a guaranteed deadlock (#2924 review).
ExecutorService drainer =
Executors.newFixedThreadPool(
2,
windows ? 3 : 2,
Comment on lines +201 to +206

@oss-maintainer oss-maintainer Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line that resolves the Critical from my previous review, and it is the one thing in the PR that no test can currently catch regressing. Reverting windows ? 3 : 2 back to 2 reintroduces the deterministic Windows deadlock (the stdin writer queues behind two EOF-blocked drainers), and every existing test still passes: buildExecArgs only pins argv shape, writeScriptTo only pins write-and-close on a ByteArrayOutputStream, and drainer sizing is inside doExec, which the test class never touches. build (windows-latest) is green here too, because CI does not run a Docker-backed doExec against a Windows host — the executor sizing is decided by the host OS, so on the Linux CI runner this branch never even executes.

I know the stub-docker orchestration test was deferred as follow-up (I can see it in your d240e15 comment) — flagging it as an accepted gap rather than a blocker. A cheap interim pin if you'd rather not carry a TODO: extract the capacity decision into the same kind of package-private seam as buildExecArgs/writeScriptTo, e.g. static int drainerPoolSize(boolean windows) { return windows ? 3 : 2; }, and assert drainerPoolSize(true) >= 3 next to the two existing arg tests. That doesn't cover real sequencing, but it does make the exact regression that caused this round of review fail a test instead of a user's Windows session.

r -> {
Thread t =
new Thread(
Expand All @@ -176,6 +217,34 @@ protected ExecResult doExec(RuntimeContext runtimeContext, String command, int t
drainer.submit(() -> readStream(process.getInputStream(), OUTPUT_TRUNCATE_BYTES));
Future<String> stderrFuture =
drainer.submit(() -> readStream(process.getErrorStream(), OUTPUT_TRUNCATE_BYTES));

if (windows) {
// The script travels via stdin (-i, no -c): write it, then close so sh runs the
// buffered script to completion and exits with its status (#2924). Written from
// the drainer pool AFTER the output drainers started, so early child output can
// never fill the stdout pipe and deadlock the write before waitFor() arms the
// timeout (#2924 review).
Future<?> stdinWrite =

@oss-maintainer oss-maintainer Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Windows stdin write is submitted as a third task to the same fixed 2-thread pool (Executors.newFixedThreadPool(2), unchanged by this PR) that already runs the stdout and stderr drainers, so the write task queues behind drainers that only complete at process EOF — which never comes because sh is blocked waiting for stdin EOF. Every Windows exec therefore deadlocks until stdinWrite.get(...) times out, then fails via destroyForcibly + SandboxException, making Windows strictly worse than before (all commands fail, not just quoted ones) — the previous review's stdin-write deadlock warning is still open in a new deterministic form. Size the pool for the third task (e.g. newFixedThreadPool(windows ? 3 : 2, ...)) or submit the write to its own single-thread executor, and add a test that runs doExec against a stub docker executable on PATH so the orchestration (submit order, pool capacity, write+close) is actually covered.

drainer.submit(
() -> {
try {
writeScriptTo(process.getOutputStream(), command);
return null;
} catch (IOException e) {
throw new java.util.concurrent.CompletionException(e);
}
});
try {
stdinWrite.get(timeoutSeconds, TimeUnit.SECONDS);
Comment on lines +237 to +238

@oss-maintainer oss-maintainer Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Budget nit, non-blocking: this get() consumes a full timeoutSeconds before the process.waitFor(timeoutSeconds, ...) at line 250 arms its own identical budget, so a Windows exec can now take up to ~2× the caller's timeout in the worst case (write stalls near the deadline, then the process still gets the full window). It is unlikely to bite after the pool fix, since the write should complete immediately. If you want the caller's contract to hold exactly, capture a deadline once and derive both waits from it:

long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(timeoutSeconds);
// ... then, for each wait:
long remainingNanos = deadline - System.nanoTime();
if (remainingNanos <= 0) { /* timeout path */ }
stdinWrite.get(remainingNanos, TimeUnit.NANOSECONDS);

Mentioning it only because doExec's timeout is a caller-visible contract (ShellExecuteTool → ExecTimeoutException → exit 124), not because this PR needs to fix it.

} catch (Exception stdinFailure) {
Comment on lines +237 to +239

@oss-maintainer oss-maintainer Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get(timeoutSeconds, SECONDS) throws three different things here — TimeoutException, InterruptedException, ExecutionException — and the catch-all flattens all of them into EXEC_NONZERO, "Failed to write script to docker exec stdin".

Two consequences:

  1. A stdin-write timeout is no longer reported as a timeout. Callers branch on the exception type, not the message: SandboxBackedFilesystem.java:107 and SharedSandboxFilesystem.java:78 catch SandboxException.ExecTimeoutException to return exit 124, but this throw is a plain SandboxException, so it falls through to their generic catch (Exception e) and surfaces as Internal sandbox error / exit -1. A hung write looks like an internal fault instead of a timeout. SandboxErrorCode already has EXEC_TIMEOUT for this, and ExecTimeoutException(command, timeoutSeconds) is what the waitFor failure below uses — worth matching here.
  2. InterruptedException is swallowed without restoring the flag. No Thread.currentThread().interrupt() before the rethrow, so an interrupt on the calling thread is consumed and lost.

Suggest splitting the catch, e.g.:

try {
    stdinWrite.get(timeoutSeconds, TimeUnit.SECONDS);
} catch (TimeoutException e) {
    process.destroyForcibly();
    drainer.shutdownNow();
    throw new SandboxException.ExecTimeoutException(command, timeoutSeconds);
} catch (InterruptedException e) {
    process.destroyForcibly();
    drainer.shutdownNow();
    Thread.currentThread().interrupt();
    throw new SandboxException(
            SandboxErrorCode.EXEC_NONZERO, "Interrupted writing script to docker exec stdin", e);
} catch (ExecutionException e) {
    process.destroyForcibly();
    drainer.shutdownNow();
    throw new SandboxException(
            SandboxErrorCode.EXEC_NONZERO,
            "Failed to write script to docker exec stdin",
            e.getCause() != null ? e.getCause() : e);
}

Also note ExecutionException wraps the CompletionException(IOException) from the lambda, so the real cause is currently two levels deep in the reported chain — unwrapping it keeps the I/O error visible. Non-blocking either way.

process.destroyForcibly();
drainer.shutdownNow();
throw new SandboxException(
SandboxErrorCode.EXEC_NONZERO,
"Failed to write script to docker exec stdin",
stdinFailure);
}
}
drainer.shutdown();

boolean exited = process.waitFor(timeoutSeconds, TimeUnit.SECONDS);
Expand Down
85 changes: 85 additions & 0 deletions .../test/java/io/agentscope/harness/agent/sandbox/impl/docker/DockerSandboxExecArgsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/*
* Copyright 2024-2026 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.agentscope.harness.agent.sandbox.impl.docker;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.ByteArrayOutputStream;
import java.io.OutputStream;
import java.util.List;
import org.junit.jupiter.api.Test;

/**
* Regression tests for the Windows nested-quote loss in {@code doExec} (#2924): on Windows the
* script must travel via stdin ({@code -i} + plain {@code sh}), because {@link ProcessBuilder}
* funnels arguments through {@code cmd.exe}, which strips the nested double quotes sandbox
Comment on lines +28 to +30

@oss-maintainer oss-maintainer Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale rationale, and it now contradicts the corrected Javadoc in the same PR. This comment still says {@link ProcessBuilder} funnels arguments through {@code cmd.exe}; the main-class Javadoc on buildExecArgs was updated in f48fc77 to the accurate mechanism (ProcessBuilder quotes argv into one CreateProcess command line, and docker.exe re-parses it with MSVCRT rules that strip the nested double quotes) — and docker.exe is a native binary, so cmd.exe is not involved at all. The assertion message at line 57 ("no -c on Windows: the script must not travel through cmd.exe") carries the same inaccuracy.

Worth aligning both to the buildExecArgs wording, since a future reader who trusts this file's explanation may conclude the fix is about shell metacharacters rather than argv re-parsing. Cosmetic — not blocking.

* commands rely on.
*/
class DockerSandboxExecArgsTest {

private static final String CMD =
"if [ -e 'test.txt' ]; then echo 'EXISTS'; exit 1; fi; "
+ "mkdir -p \"$(dirname 'test.txt')\" 2>&1";

@Test
void unixPath_usesShDashCWithScriptInline() {
List<String> args = DockerSandbox.buildExecArgs(CMD, "/workspace", "container-1", false);

assertEquals(
List.of("docker", "exec", "-w", "/workspace", "container-1", "sh", "-c", CMD),
args);
}

@Test
void windowsPath_passesScriptViaStdin() {

@oss-maintainer oss-maintainer Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests only assert on the new pure helper buildExecArgs; nothing exercises the actual fix in doExec (writing the script to process stdin and closing it, DockerSandbox.java:188-194). Deleting that block in doExec leaves all three tests green, so the regression is pinned only structurally, not behaviorally. windowsStdin_scriptSurvivesNestedDoubleQuotes is additionally trivially true because the command never appears in the Windows args by construction (its assertion is implied by the test above it and is effectively redundant). Consider extracting the stdin write into a package-private seam that takes a Process/OutputStream and testing it (script bytes are written, stream is closed), or make doExec's command injectable so the write path can be driven in a test.

List<String> args = DockerSandbox.buildExecArgs(CMD, "/workspace", "container-1", true);

// -i + plain sh: the script is fed on stdin, never placed on the Windows command line.
assertEquals(
List.of("docker", "exec", "-i", "-w", "/workspace", "container-1", "sh"), args);
assertFalse(
args.contains("-c"),
"no -c on Windows: the script must not travel through cmd.exe");
}

@Test
void windowsStdin_writeScriptTo_writesAndCloses() throws Exception {
// Behavioral pin of the actual #2924 fix: the seam writes the full script bytes and
// closes the stream (EOF tells non-interactive sh to run the buffered script).
ByteArrayOutputStream sink = new ByteArrayOutputStream();
OutputStream counted =
new OutputStream() {
@Override
public void write(int b) {
sink.write(b);
}

@Override
public void close() {
closed = true;
}
};

DockerSandbox.writeScriptTo(counted, CMD);

assertEquals(CMD, sink.toString("UTF-8"), "the script bytes must reach stdin verbatim");
assertTrue(closed, "stdin must be closed so sh sees EOF");
}

private volatile boolean closed;
}
Loading