Skip to content

fix(sandbox-e2b): reliable exec timeout with server-side kill #2976

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
chcodex wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(sandbox-e2b): reliable exec timeout with server-side kill #2976

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b64ed41
fix(sandbox-e2b): reliable exec timeout with server-side kill (#2974)
chcodex Sep 4, 2026
ec8cf44
fix(sandbox-e2b): address review: interrupt snapshot, error-frame kil…
chcodex Sep 20, 2026
d995191
fix(sandbox-e2b): deterministic server-wins race, pre-cancel fail fas…
chcodex Sep 21, 2026
a007a21
fix(sandbox-e2b): 5s fault-grading slack, timeout message without com…
chcodex Sep 28, 2026
298b138
fix(sandbox-e2b): drop proto end-stream decoder, frames are always JSON
chcodex Sep 28, 2026
da3eb1b
test(sandbox-e2b): socket-level coverage for readTimeout(0) and clien…
chcodex Sep 28, 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
30 changes: 24 additions & 6 deletions ...-sandbox-e2b/src/main/java/io/agentscope/extensions/sandbox/e2b/E2bEnvdProcessClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,19 @@ byte[] runShellBinaryStdout(
private ShellCapture runShellCapture(
E2bSandboxState state, String cwd, String shellCommand, int timeoutSeconds)
throws Exception {
if (timeoutSeconds <= 0) {
throw new IllegalArgumentException(
"timeoutSeconds must be positive: " + timeoutSeconds);

@oss-maintainer oss-maintainer Sep 16, 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.

[Warning] Fail-fast on timeoutSeconds <= 0 changes the effective contract, and it diverges from the sibling sandbox implementations.

AbstractBaseSandbox.exec() only substitutes a default when the argument is null (int timeout = timeoutSeconds != null ? timeoutSeconds : getDefaultExecTimeoutSeconds()), so an explicit 0 used to mean "no callTimeout" and now aborts before the request is issued. DaytonaHttp.execute() (Math.max(1, timeoutSeconds)) and AgentRunMcpChannel.exec() (Math.max(1, ...)) clamp instead of throwing, so the same input now behaves differently per backend.

It also escapes the tool-layer error mapping: SandboxBackedFilesystem.execute() catches SandboxException.ExecTimeoutException / ExecException only, so an IllegalArgumentException surfaces as an unmapped failure rather than an ExecuteResponse with a non-zero exit code.

Either clamp to keep the backends consistent, or throw a SandboxException so the harness renders it as a tool error:

if (timeoutSeconds <= 0) {
    timeoutSeconds = getDefaultExecTimeoutSeconds(); // or throw SandboxException.ExecException
}

If throwing is deliberate, it is worth a line in the PR description plus a javadoc note on runShell/runShellBinaryStdout, since both are called with hardcoded constants today.

}
OkHttpClient callClient =
timeoutSeconds > 0
? http.newBuilder().callTimeout(timeoutSeconds, TimeUnit.SECONDS).build()
: http;
http.newBuilder()
.callTimeout(timeoutSeconds, TimeUnit.SECONDS)
// Disable the idle read timeout: it fires on gaps between bytes and
// would preempt callTimeout (both surface as InterruptedIOException),
// misreporting a short idle stall as a full exec timeout (#2974).
// Total-duration semantics is owned solely by callTimeout.
.readTimeout(0, TimeUnit.SECONDS)

@oss-maintainer oss-maintainer Sep 16, 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.

[Info] readTimeout(0) is the actual fix for #2974, but nothing pins it: the new tests use application interceptors, which throw before OkHttp's timeouts engage, so the assertions really cover "header is set" and "interrupt is not swallowed". If someone restores the inherited readTimeout, the suite still passes.

A MockWebServer case that writes one frame, stalls longer than opt.getReadTimeoutSeconds(), and asserts ExecTimeoutException would lock the behaviour in; asserting callClient.readTimeoutMillis() == 0 && callClient.callTimeoutMillis() == timeoutSeconds * 1000 via a small package-private seam is the cheap version.

.build();
String host = envdHost(state);
String url = host + "/process.Process/Start";
byte[] envelope = encodeStartRequestEnvelope(shellCommand, cwd);
Expand All @@ -134,7 +143,10 @@ private ShellCapture runShellCapture(
.addHeader("User-Agent", "agentscope-java-e2b")
.addHeader("E2b-Sandbox-Id", state.getSandboxId())
.addHeader("E2b-Sandbox-Port", Integer.toString(ENVD_PORT))
.addHeader("Authorization", basicAuthUser(opt.getRunUser()));
.addHeader("Authorization", basicAuthUser(opt.getRunUser()))
// Ask envd to kill the remote process when this lapses; otherwise a
// client-side timeout only drops our HTTP stream and leaks the process.
.addHeader("Connect-Timeout-Ms", String.valueOf(timeoutSeconds * 1000L));

@oss-maintainer oss-maintainer Sep 16, 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.

[Warning] The server-side deadline and the client-side callTimeout are set to exactly the same instant, so on timeout the two abort races and the client usually wins — which means the process is reaped server-side (good, that is #2974) but the terminal frames from envd are still lost, and the caller sees a generic ExecTimeoutException without the partial output/exit status the server already flushed.

Give the server a small head start so its close frame can arrive first:

long timeoutMs = TimeUnit.SECONDS.toMillis(timeoutSeconds);
rb.addHeader("Connect-Timeout-Ms", String.valueOf(timeoutMs));
// client waits slightly longer than envd's own deadline
OkHttpClient callClient = http.newBuilder()
        .callTimeout(timeoutMs + 5_000L, TimeUnit.MILLISECONDS)
        .readTimeout(0, TimeUnit.SECONDS)
        .build();

Also timeoutSeconds * 1000L overflows silently for very large values from a caller-supplied timeout — TimeUnit.SECONDS.toMillis(...) saturates instead. Note TAR_TIMEOUT_SECONDS and the 120s workspace calls are unaffected; this only matters for the LLM-supplied path.

if (state.getEnvdAccessToken() != null && !state.getEnvdAccessToken().isBlank()) {
rb.addHeader("X-Access-Token", state.getEnvdAccessToken());
}
Expand All @@ -145,7 +157,7 @@ private ShellCapture runShellCapture(
int exit;
try (Response res = callClient.newCall(req).execute()) {
if (!res.isSuccessful()) {
String err = res.body() != null ? res.body().string() : "";
String err = res.body().string();
throw new SandboxException.SandboxRuntimeException(
SandboxErrorCode.WORKSPACE_START_ERROR,
"envd Start failed HTTP " + res.code() + ": " + err);
Expand All @@ -154,6 +166,12 @@ private ShellCapture runShellCapture(
exit = drainStartStream(in, stdout, stderr);
}
} catch (InterruptedIOException e) {
// External cancellation surfaces here too; don't misreport it as a timeout
// and don't swallow the interrupt bit.
if (Thread.currentThread().isInterrupted()) {

@oss-maintainer oss-maintainer Sep 16, 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.

[Warning] Restoring the interrupt bit and rethrowing is the right call, but the exception type now leaves the SandboxException vocabulary. runShellCapture is reached from E2bSandbox.doExec, and SandboxBackedFilesystem.execute() maps only ExecTimeoutException / ExecException; a raw InterruptedIOException propagates past both handlers.

Two knock-on effects worth deciding on explicitly:

  1. A cancelled exec produces an unstructured failure for the agent instead of an ExecuteResponse that says the command was interrupted.
  2. The check is a heuristic — any stale interrupt bit on a pooled thread (e.g. after shutdownNow(), or an interceptor that interrupts and does not clear it) turns a genuine callTimeout into "cancellation". The finally in the new test hints at how easy that is to leak. If the goal is to distinguish the two reliably, an explicit call.isExecuted()/canceled flag or a wrapper exception (SandboxException.ExecInterruptedException) is more robust than reading global thread state.

The second catch clause of DockerSandbox.doExec has the same shape, so a shared convention would keep the backends aligned.

Thread.currentThread().interrupt();
throw e;
}
throw new SandboxException.ExecTimeoutException(shellCommand, timeoutSeconds);
}
return new ShellCapture(exit, stdout, stderr);
Expand All @@ -179,7 +197,7 @@ private int drainStartStream(
break;
}
int len = ByteBuffer.wrap(lenB).order(ByteOrder.BIG_ENDIAN).getInt() & 0x7FFFFFFF;
if (len < 0 || len > 64 * 1024 * 1024) {
if (len > 64 * 1024 * 1024) {
throw new IOException("Invalid connect frame length: " + len);

@oss-maintainer oss-maintainer Sep 16, 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.

[Info] Dropping len < 0 is correct — the & 0x7FFFFFFF on line 199 already makes the value non-negative, so the branch was dead. But that same mask is why a corrupt header with the high bit set is silently truncated rather than rejected: 0x80000001 becomes 1, i.e. a plausible one-byte frame instead of an error.

If you want the length guard to be meaningful, validate the unsigned value before masking:

int raw = ByteBuffer.wrap(lenB).order(ByteOrder.BIG_ENDIAN).getInt();
long len = Integer.toUnsignedLong(raw);
if (len > 64L * 1024 * 1024) {
    throw new IOException("Invalid connect frame length: " + len);
}

Pre-existing behaviour, not introduced here — mentioning it only because this hunk touches the validation.

}
byte[] data = in.readNBytes(len);
Expand Down
77 changes: 77 additions & 0 deletions ...dbox-e2b/src/test/java/io/agentscope/extensions/sandbox/e2b/E2bEnvdProcessClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,30 @@

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

import com.google.protobuf.ByteString;
import com.google.protobuf.Descriptors;
import com.google.protobuf.DynamicMessage;
import io.agentscope.harness.agent.sandbox.SandboxException;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.net.SocketTimeoutException;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.concurrent.atomic.AtomicReference;
import okhttp3.Interceptor;
import okhttp3.MediaType;
import okhttp3.OkHttpClient;
import org.junit.jupiter.api.Test;

class E2bEnvdProcessClientTest {
Expand Down Expand Up @@ -307,4 +314,74 @@ private static E2bSandboxClientOptions options(E2bCodec codec) {
options.setCodec(codec);
return options;
}

@Test
void requestCarriesConnectTimeoutMsHeader() throws Exception {
AtomicReference<String> header = new AtomicReference<>();
Interceptor capture =
chain -> {
header.set(chain.request().header("Connect-Timeout-Ms"));
throw new SocketTimeoutException("timeout");
};
E2bEnvdProcessClient client = clientWithInterceptor(capture);

assertThrows(
SandboxException.ExecTimeoutException.class,
() -> client.runShell(state(), "/workspace", "sleep 1000", 3));
assertEquals("3000", header.get());
}

@Test
void interruptionIsRethrownNotWrappedAsTimeout() throws Exception {
Interceptor interrupting =
chain -> {
Thread.currentThread().interrupt();
throw new SocketTimeoutException("read timed out");
};
E2bEnvdProcessClient client = clientWithInterceptor(interrupting);
try {
SocketTimeoutException thrown =
assertThrows(
SocketTimeoutException.class,
() -> client.runShell(state(), "/workspace", "sleep 1000", 3));
assertEquals("read timed out", thrown.getMessage());
assertTrue(Thread.currentThread().isInterrupted(), "interrupt bit must be restored");
} finally {
// Do not leak the interrupt bit into other tests.
Thread.interrupted();
}
assertFalse(Thread.currentThread().isInterrupted());
}

@Test
void nonPositiveTimeoutFailsFastWithoutRequest() throws Exception {
AtomicReference<String> header = new AtomicReference<>();
Interceptor capture =
chain -> {
header.set(chain.request().header("Connect-Timeout-Ms"));
throw new SocketTimeoutException("must not be called");
};
E2bEnvdProcessClient client = clientWithInterceptor(capture);

assertThrows(
IllegalArgumentException.class,
() -> client.runShell(state(), "/workspace", "echo hi", 0));
assertThrows(
IllegalArgumentException.class,
() -> client.runShell(state(), "/workspace", "echo hi", -5));
assertNull(header.get(), "no HTTP request must be issued for invalid timeout");
}

private static E2bEnvdProcessClient clientWithInterceptor(Interceptor interceptor)
throws Exception {
E2bSandboxClientOptions opt = options(E2bCodec.PROTO);
opt.setHttpClient(new OkHttpClient.Builder().addInterceptor(interceptor).build());
return new E2bEnvdProcessClient(opt);
}

private static E2bSandboxState state() {
E2bSandboxState state = new E2bSandboxState();
state.setSandboxId("test-sandbox");
return state;
}
}
Loading