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

[chcodex]

Fixes #2974.

• Per-call client disables the idle read timeout (readTimeout=0) so total-duration semantics is owned solely by callTimeout; previously a small base readTimeout preempted callTimeout and misreported a short idle stall as a full exec timeout (both surface as InterruptedIOException).
• Send Connect-Timeout-Ms so envd kills the remote process server-side; previously a client timeout only dropped our HTTP stream and leaked the process.
• External cancellation (Thread.interrupt()) is rethrown as-is with the interrupt bit restored instead of being wrapped as ExecTimeoutException.
• timeoutSeconds <= 0 now fails fast with IllegalArgumentException.
• Tests: E2bEnvdProcessClientTest 10/10 pass (4 new: header carried end-to-end, interruption rethrown with bit restored, fail-fast without request; plus existing codec/drain cases). Full reactor test BUILD SUCCESS.

[codecov]

Codecov Report

❌ Patch coverage is 75.36232% with 17 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...e/extensions/sandbox/e2b/E2bEnvdProcessClient.java	76.47%	11 Missing and 5 partials ⚠️
...tscope/harness/agent/sandbox/SandboxException.java	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[CryoThrust]

One small robustness concern: res.body().string() is now unconditional in the non-success response path. OkHttp permits a response with a null body (for example a 204/transport edge or a test double), so this can replace the intended SandboxRuntimeException with an NPE. Keeping the previous null-safe fallback (res.body() != null ? res.body().string() : "") would preserve the error contract while still including the server body when present. A regression test with a non-success response and no body would pin this.

[chcodex]

@CryoThrust Thanks for flagging this. The null guard was removed deliberately:

• Response.body is a non-null contract in OkHttp (Kotlin val body: ResponseBody). For a real Call.execute() response — the only path that reaches this code — an empty body (204/304, truncated gateway reply) arrives as EmptyResponseBody, so .string() returns "" rather than throwing NPE.
• A null body can only come from a hand-built Response.Builder.build() without setting a body, which cannot occur on this production path. IDEA's nullability inference (from OkHttp's annotations) agrees and reports res.body() != null as always-true, i.e. the guard is dead code.
• The suggested regression test (non-success response with no body) would only exercise such a hand-built double, not a real server reply, so it would pin behavior that cannot happen in production.

[oss-maintainer]

Summary

The timeout model is right: readTimeout(0) hands total-duration ownership to callTimeout, Connect-Timeout-Ms stops leaking the remote process, and the interrupt bit is no longer swallowed. CI is green, CLA signed, head mergeable — nothing blocking.

Five points, in priority order:

1. [Warning] Thread.currentThread().isInterrupted() (line 171) reads ambient thread state, so a stale interrupt bit can turn a genuine exec timeout into an unmapped InterruptedIOException. Snapshot/clear the bit around the call instead.
2. [Warning] timeoutSeconds <= 0 now throws IllegalArgumentException where it previously meant "unbounded"; that type is not mapped by SandboxBackedFilesystem.execute() and diverges from the daytona/agentrun backends, which clamp.
3. [Warning] Client callTimeout and the server Connect-Timeout-Ms deadline are identical, so on expiry the client still wins the race and the partial stdout/stderr already drained is discarded.
4. [Info] On the res.body() null-guard exchange with @CryoThrust: the production-path argument is convincing for real Call.execute() responses — Objects.requireNonNull(res.body(), ...) documents that without reintroducing the ternary, and survives a NullAway/ErrorProne rollout or a synthesising interceptor.
5. [Info] Good coverage on the header and the fail-fast path; a MockWebServer stall case is the one thing that would actually pin readTimeout(0) itself, since the app-level interceptors short-circuit before OkHttp timeouts engage.

---

Automated review by github-manager-bot

[oss-maintainer]

[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.

[oss-maintainer]

[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.

[oss-maintainer]

[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.

[oss-maintainer]

[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.

[oss-maintainer]

[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.

[chcodex]

@oss-maintainer Thanks for the thorough review — all five points addressed in ec8cf44 (19/19 tests green, spotless clean). Point by point:

1. Stale interrupt bit — fixed. Snapshot/clear with Thread.interrupted() before the call, restore in finally; the catch only treats a bit set during the call as cancellation.

2. <=0 fail-fast — kept deliberately, diverges from daytona/agentrun. Clamping to 1s would silently convert a caller bug (0/-5 is never intentional) into a 1-second execution — masking real bugs in production. fail-fast stays, but the message is now actionable and survives the generic-catch surfacing path ("Internal sandbox error: ..."): [e2b] timeoutSeconds must be positive, got 0 for command "...". This is a caller bug, not a sandbox failure; check the timeout argument. Normal agent paths always pass positive values (default 120), so this only fires on direct API misuse.

3. Identical deadlines — kept, with evidence, plus deterministic attribution. Live-verified against a real sandbox: sleep 30 + timeout=5 → ExecTimeoutException at ~5s, and ps confirms the server killed the process (no leak, no retry overlap). Rationale for identical values: the client timer starts at call initiation while the server timer starts at handler-begin (~1 RTT later), so the client wins by construction; no slack constant needed in either direction. For the residual case (server wins — client-timer delivery jitter past the ~RTT margin), the client now reads the kill reason instead of guessing:

   • end-stream error frames are parsed (JSON + PROTO via minimal CodedInputStream read, no new descriptors); deadline_exceeded → ExecTimeoutException, other codes → stream error carrying the code;
   • no error frame (fix(e2b): reject incomplete process streams without exit code #2828 truncation path) → new MissingExitCodeException extends IOException (upstream assertThrows(IOException) still passes) mapped by elapsed time: elapsed >= timeout → ExecTimeoutException, earlier → stream error as before.
     Partial stdout/stderr attached to the timeout exception needs a harness exception change — left as follow-up.

4. requireNonNull — accepted, one line documenting the non-null body contract instead of the ternary.

5. Socket-level stall test — agreed but blocked. envdHost is hardcoded https with no port/host injection, so MockWebServer cannot be pointed at it; app-level interceptors short-circuit before OkHttp timeouts engage (the old sleep-based test proved nothing and was removed). Making the host injectable is a separate refactor — follow-up.

[oss-maintainer]

Summary

Re-review after the force-push that addresses my previous five points. All five landed: the interrupt bit is snapshotted and restored instead of being read ambiently, the fail-fast path now explains itself and is pinned by a "no HTTP request issued" assertion, Objects.requireNonNull settles the res.body() debate, and the new Connect-Timeout-Ms → deadline_exceeded mapping plus mapPrematureEnd close the "server killed it but we call it a stream error" gap with real coverage on both codecs (JSON and hand-decoded proto). CI green, CLA signed, mergeable; the two points below are not blockers for me, they are the race and the pre-existing-bit case that the new logic still leaves open.

• [Warning] E2bEnvdProcessClient.java:167 — clearing a pre-existing interrupt bit means an already-cancelled caller now blocks in execute() for the full timeoutSeconds before the bit is restored; suggest failing fast when preInterrupted is true, plus a test for that path.
• [Warning] E2bEnvdProcessClient.java:190 — client callTimeout and the server deadline are the same duration, so the deadline_exceeded branch will rarely win the race in production (the canned-body tests bypass it), and partial stdout/stderr drained before the kill is still discarded by ExecTimeoutException. Offsetting one side by ~500 ms and carrying the captured output would make the new mapping actually observable.
• [Info] :129 — good resolution of the IllegalArgumentException debate, but the cross-backend divergence (daytona/agentrun clamp) is now documented rather than removed, and the message embeds raw model-generated command text that may carry inline tokens; worth a redaction pass or a why-not-clamp comment.
• [Info] :281 — the hand-rolled proto end-stream decode is correct for EndStreamResponse{error=1}; keep the broad catch and consider a debug log when an unparsable frame falls through to MissingExitCodeException.

---

Automated review by github-manager-bot

[oss-maintainer]

Snapshot-and-restore fixes the misclassification from my previous point 1, but it also removes the only fast-abort a caller has: Thread.interrupted() clears the bit, so a thread that was already cancelled before entering runShellCapture now issues the HTTP call and blocks in execute() for up to timeoutSeconds (default is a full exec timeout) before the bit is re-set in finally. OkHttp's blocking execute() does not poll the interrupt bit, so restoring it afterwards is not an early return — it's a delayed one.

Suggest failing fast on a pre-existing bit instead of absorbing it:

boolean preInterrupted = Thread.interrupted();
if (preInterrupted) {
    Thread.currentThread().interrupt();
    throw new InterruptedIOException("caller thread already interrupted; aborting exec before Start");
}

That keeps the guarantee you documented (a stale bit must not fake a cancellation of this call) while not making an already-cancelled caller wait out the timeout. Either way, a short test asserting the pre-existing-bit path's behaviour would be good — interruptionIsRethrownNotWrappedAsTimeout only covers an interrupt raised from inside the call.

[oss-maintainer]

The deadline_exceeded mapping is the right shape, but as long as client callTimeout (line 135) and server Connect-Timeout-Ms (line 156) are the same duration, this branch is mostly unreachable: the client aborts the stream at T and the server writes the end-stream frame at T too, so in practice the connection is closed before the frame is read and InterruptedIOException (or the new MissingExitCodeException heuristic) wins the race. Two consequences:

1. The error code you are mapping — and the mapPrematureEnd fallback below — will fire rarely in production and mostly in tests (the canned-body tests bypass the race entirely, since the frame is already in memory).
2. The partial stdout/stderr already drained at that point is still discarded: ExecTimeoutException carries only the command and the timeout, so the operator sees no output for a long-running command that produced megabytes before being killed. That was the substance of my previous point 3 and it is not yet addressed.

Cheap improvement: give the server a slightly shorter deadline (e.g. timeoutSeconds * 1000L - 500) or extend callTimeout by a small grace (timeoutSeconds + 1s) so the terminal frame can arrive, and consider a ExecTimeoutException(cmd, timeout, stdout, stderr) variant (or attaching the captured output to the existing type) so the killed run's partial output survives to the harness error path.

[oss-maintainer]

Fair resolution of the previous point 2 — the [e2b] prefix plus "this is a caller bug, not a sandbox failure" makes the intent unmistakable, and pinning "no HTTP request must be issued" in the test is the part that actually matters.

One thing to note for whoever reads this later: the IllegalArgumentException type is still not mapped by SandboxBackedFilesystem.execute(), and daytona/agentrun clamp instead of throwing, so the cross-backend divergence you and @CryoThrust discussed is now documented in the message rather than removed. If that is the deliberate call, a one-line comment at the throw site saying why clamping was rejected would save the next reviewer re-litigating it.

Also mind that the message now embeds up to 200 chars of the raw command. Shell commands in this path are model-generated and routinely contain inline tokens (curl -H "Authorization: Bearer ..."), and an IllegalArgumentException message is exactly what lands in logs and exception metrics. If the goal is just operator context, consider logging the command at warn behind the existing redaction helper (or quoting only the first argv word) instead of putting it in the throwable.

[oss-maintainer]

Two small robustness notes on the hand-rolled wire decode:

1. in.skipField(tag) on an outer tag of unknown wire type, combined with the WIRETYPE_LENGTH_DELIMITED guard, is safe for EndStreamResponse{ Error error = 1 }, but if the field is ever reused with a different type the loop silently skipFields and returns null, which turns a real server error back into MissingExitCodeException. A logger.debug("unparsable connect end-stream error ({} bytes)", data.length) before return null would make a server-side format change diagnosable.
2. readRawVarint32() as the sub-message limit followed by pushLimit is correct, but a hostile/truncated len yields NegativeRuntimeLengthException — an InvalidProtocolBufferException that is not an IOException subclass here... actually it is, but it escapes through parseProtoEndStreamError's catch (Exception e) { return null; } only because the catch is broad. It is fine today; please keep the broad catch, since a future narrowing to InvalidProtocolBufferException would let a malformed frame abort the drain loop.

[chcodex]

@oss-maintainer Re-review addressed in d995191 (23/23 tests green, spotless clean). Point by point:

1. Pre-cancelled caller no longer blocks — fail fast. Thread.interrupted() snapshot is now followed by an immediate throw (InterruptedIOException("exec cancelled before start"), bit restored) when set, so an already-cancelled caller never enters execute(). Pinned by alreadyCancelledCallFailsFastWithoutBlocking: asserts immediate return, restored bit, non-timeout message, and no HTTP request issued. The now-dead finally restore was removed.

2. Race made deterministic: server wins by 500ms. Connect-Timeout-Ms stays exactly timeout*1000 (kill lands on time, timeout promise stays accurate); client callTimeout is timeout*1000+500ms as pure backup. Since the server timer starts ~1 RTT after the client timer, the server now deterministically kills first and the client observes it via deadline_exceeded — so a retry can never overlap a still-running process (the exact failure mode from the earlier discussion). The client backup only fires on server misbehavior. Partial stdout/stderr is no longer silently dropped: both timeout-mapping paths emit a debug log with elapsed time, sizes, and 512-char tails. Carrying output in ExecTimeoutException needs a harness exception change — left as follow-up.

3. Command text removed from the fail-fast message. IAE now carries only the value + caller-bug guidance, plus a why-not-clamp comment. (ExecTimeoutException itself still embeds the full command — pre-existing behavior, out of scope here.)

4. Debug log on unparsable proto end-stream frames — added.

Coverage follow-ups included: malformed JSON end frame, proto frame with unknown top-level fields (skip path), empty proto end frame, and both mapPrematureEnd branches were already pinned. The socket-level stall test remains blocked on hardcoded envdHost (separate refactor).

[oss-maintainer]

Summary

Delta re-review of d9951910 (my previous review was COMMENTED on ec8cf440). Both non-blocking warnings from that review are now closed, and the fixes match what was asked:

• Pre-cancel fail fast (:167) — the interrupt-bit snapshot moved ahead of the call; it re-arms the bit and throws InterruptedIOException("exec cancelled before start") instead of letting an already-cancelled caller block for the full timeoutSeconds. alreadyCancelledCallFailsFastWithoutBlocking pins both the fast return and that no HTTP request is issued.
• Server-wins race (:190) — callTimeout is now timeoutSeconds*1000 + 500ms while Connect-Timeout-Ms keeps the exact server deadline, so deadline_exceeded deterministically wins and a retry never overlaps a still-running process. timeoutSeconds * 1000L promotes before the add, so no int overflow at realistic values.
• truncateCommand removal — command text is gone from the invalid-timeout error, with the rationale (model-generated commands can carry inline tokens) documented at the call site; grep over head confirms no dangling references.
• Debug-only partial-stream logging — logPartialStreams is guarded by isDebugEnabled() and capped at a 512-byte tail, so the hot path allocates nothing when debug is off, and the added parseProtoEndStreamError debug line makes the silent fall-through observable.

Findings

• [Info] E2bEnvdProcessClient.java:204 — only the deadline_exceeded and MissingExitCodeException paths log partial stdout/stderr tails; the generic ConnectStreamException path still surfaces code + message only. Reasonable scope choice, noting it just so the asymmetry is deliberate.
• [Info] CI: build (ubuntu-latest) and build (windows-latest) are still queued on d9951910 as of this review, so I am not approving on this commit yet. license/cla, Check License, Check Module Sync are green and the branch is MERGEABLE/CLEAN.

The remaining check is just CI going green; nothing here needs another code change from my side.

---

Automated review by github-manager-bot

[chcodex]

Thanks for the careful read — P1's mechanism is correct, P2's premise needs a correction, and both are addressed in a007a21:

P1 (backup can fire before the server deadline) — accepted, and the fix is already shaped by it. You're right about the two clocks: callTimeout runs from call initiation (DNS/TCP/TLS included) while Connect-Timeout-Ms only starts when envd receives the request. One correction: the 'request never arrives' half of the scenario is harmless — OkHttp cancels the call on timeout, so bytes in flight during setup either never arrive (zero overlap, retry is the only request in the world) or already arrived (the real overlap window setup − slack). There is no middle state where a cancelled request arrives late.

The slack is now documented as a fault-grading threshold rather than a race constant: within T the server kills on time and the client observes deadline_exceeded (slack never consumed, zero cost); within T+5s, engineering tolerance absorbing setup jitter so the kill lands before any retry; beyond T+5s, treated as network-layer fault and the client backup fires to guarantee a local return. 5s covers the setup distribution short of extreme degradation; larger values would approach the agent retry interval (model round-trip, seconds) and buy nothing.

P2 (timeout still embeds the command) — the premise doesn't hold, but the conclusion is now implemented anyway. No code, PR description, or comment in this PR ever stated 'errors carry no command text' as a goal — the IAE comment was scoped to that single new message. That said, the official SDKs settle it: Python's format_execution_timeout_error() is a fixed string with no command, JS likewise. ExecTimeoutException now emits fixed "Command timed out after Ns" (signature kept, Docker benefits too), pinned by timeoutMessageCarriesNoCommandText asserting a secret-bearing command never reaches the message.

[chcodex]

@oss-maintainer Follow-up in a007a21 (25/25 green, spotless clean), addressing your re-review plus a local review round:

• Race: settled on server-wins with a 5s fault-grading slack (was 500ms). Connect-Timeout-Ms stays exact; callTimeout is T+5s pure backup. Rationale documented on the constant: within T the slack costs nothing (server kills, client observes deadline_exceeded); T..T+5s absorbs setup jitter so the kill lands before any retry; beyond that it's a network fault and the backup guarantees a local return. Pinned by clientBackupToleratesServerKillMargin (1s timeout + 1.5s stall completes normally). An initial identical-deadlines version was reverted after review: with agent-loop retries in the low seconds, a client-first win could overlap a not-yet-killed process.
• Timeout message: now fixed "Command timed out after Ns" with no command text, matching the official SDKs' format_execution_timeout_error() — signature kept for compatibility (Docker inherits the fix). Pinned by timeoutMessageCarriesNoCommandText with a secret-bearing command.
• IAE: message kept actionable; comment rewritten — 0/unbounded is rejected because this OkHttp transport cannot model an unbounded stream (unlike connect-native SDKs where 0 disables the deadline), not merely 'caller bug'.
• Partial-output debug log and proto end-frame debug log from the last round are unchanged.

[oss-maintainer]

Summary

Delta re-review of a007a211 (previous COMMENTED review was on d9951910). Both previous warnings stay closed and this round adds the two things the timeout path needed: Connect-Timeout-Ms so envd kills the remote process instead of leaking it, an end-stream deadline_exceeded decode (JSON and proto) that maps a server-side kill to ExecTimeoutException, a T + 5s client backup with the fault-grading rationale written down, and ExecTimeoutException no longer echoing the command text (secret hygiene). readTimeout(0) plus the ConnectStreamException / MissingExitCodeException split are covered by new tests, including the no-HTTP-request-on-invalid-timeout and interrupt-bit cases.

LGTM — approving. Remaining items are non-blocking notes: the requireNonNull fail-loud shape on the non-2xx branch, the timeoutSeconds + 5s wall-clock tail for callers above doExec, and the truncating elapsed-time boundary in mapPrematureEnd. CI is green on a007a211 (build ubuntu + windows, codecov/patch) and CLA is signed.

---

Automated review by github-manager-bot

[oss-maintainer]

Dropping the command text from ExecTimeoutException is the right call — model-generated commands routinely inline API keys, and the message ends up in logs and error events. Keeping command as an unused constructor parameter (documented as signature compatibility) is acceptable, but a reader of SandboxException.ExecTimeoutException may still assume getMessage() is diagnostic; the javadoc paragraph added here mitigates that.

One behavioural note on this line though: Objects.requireNonNull(res.body(), ...) inside the non-2xx branch turns a hypothetically null body into an NullPointerException that is not caught by the InterruptedIOException / ConnectStreamException / MissingExitCodeException handlers below, so it escapes as an unexpected error instead of SandboxRuntimeException(WORKSPACE_START_ERROR). OkHttp never returns a null body for a real network response, so this is theoretical — but the previous res.body() != null ? res.body().string() : "" shape preserved the classification. If the intent is fail-loud, consider throwing the typed SandboxRuntimeException with "<no body>" instead of relying on requireNonNull.

[oss-maintainer]

The 500ms → 5000ms slack plus the T / T+slack / >T+slack fault-grading rationale is exactly the reasoning I asked for in the previous round, and clientBackupToleratesServerKillMargin pins the ordering so the server deadline wins the race.

Worth remembering the tail cost: with readTimeout(0) the call timeout is the only guard, so a hung-but-open stream now blocks the calling thread for up to timeoutSeconds + 5s. That is a deliberate trade-off versus misreporting an idle stall as an exec timeout, but the sandbox-level wall-clock budget (and any tool-level timeout above doExec) should be > timeoutSeconds + slack or the caller gives up first and observes a cancellation instead of an ExecTimeoutException.

[oss-maintainer]

mapPrematureEnd grades by elapsed time only, so a stream that dies at T + 3s (inside the slack window, server already gone) is reported as ExecTimeoutException rather than a transport failure. That is the documented intent of the slack band, and it keeps callers getting an exec timeout where the process did run to its deadline — just flagging that elapsedSeconds() truncates, so a stream ending at T - 0.4s classifies as a premature end rather than a timeout. Using the same TimeUnit.MILLISECONDS comparison as the call-timeout boundary would make the boundary exact.

[oss-maintainer]

Summary

This PR makes the E2B exec timeout deterministic: readTimeout(0) so total duration is owned solely by callTimeout, a Connect-Timeout-Ms header so envd kills the remote process server-side, a 5s client-backup slack so the server deadline deterministically wins the race, fail-fast on non-positive timeouts, and Thread.interrupt() cancellations rethrown instead of being misreported as exec timeouts. I verified the concurrency fix empirically against the repo's OkHttp 5.3.2 and against connect-go/envd sources: callTimeout surfaces as InterruptedIOException("timeout") with the interrupt bit clear (so the new catch maps it to ExecTimeoutException as intended), an application-interceptor stall is subject to callTimeout (so clientBackupToleratesServerKillMargin really does pin the T vs T+5s ordering), the interrupt snapshot/restore touches only the current thread -- no shared mutable state, no locks, no visibility hazard -- and envd reads Connect-Timeout-Ms as milliseconds and uses it as the process kill deadline, so the server-wins design holds. The one real defect is that the end-stream error frame is decoded as protobuf under the default E2bCodec.PROTO although the Connect protocol always sends it as JSON, which makes the new deadline_exceeded mapping and its two proto tests inert for the codec that ships by default (I confirmed by feeding real frames through the method: it returns null). Otherwise the racing path has no wire-level coverage -- every test uses in-memory canned frames -- so a MockWebServer-style case that stalls a real socket past T and asserts the server-wins classification would still be worth adding.

Re-reviewed at head a007a21136; CLA: signed.

---

Automated review by github-manager-bot

[oss-maintainer]

The end-stream error frame is decoded as protobuf whenever the codec is PROTO, but the Connect protocol always sends that frame as JSON — so the new deadline_exceeded mapping never fires for the default codec.

parseEndStreamError (line 272) switches on the request codec, and both codec() and E2bSandboxClientOptions default to E2bCodec.PROTO, so the default path goes to parseProtoEndStreamError (line 296). The Connect protocol does not use the RPC's codec for the end-of-stream envelope: connect-go (which serves process.Process/Start in envd — connectrpc.com/connect v1.18.1 in e2b-dev/infra's envd go.mod) marshals it with json.Marshal in connectStreamingMarshaler.MarshalEndStream from connectEndStreamMessage, a struct that has JSON tags only (error, metadata) and no protobuf tags, and connect-go's own client parses it with json.Unmarshal unconditionally. connect-python (_protocol_connect.py: json.dumps on send, json.loads on receive) and connect-es (protocol-connect/end-stream.ts, JSON-only serialization) behave the same. A real application/connect+proto stream therefore ends with a 0x02 frame whose payload is {"error":{"code":"deadline_exceeded","message":"context deadline exceeded"}}.

I fed those exact frames through this method: the leading { is read as tag field 15 / wire-type 3, the loop skips, and the broad catch (Exception) swallows the resulting InvalidProtocolBufferException and returns null (verified for deadline_exceeded, unavailable and empty-error frames). Consequences for the default codec:

1. parseEndStreamError always returns null, so ConnectStreamException is never thrown and the deadline_exceeded → ExecTimeoutException branch added by this PR is dead code; classification falls back to mapPrematureEnd's whole-second elapsedSeconds >= timeoutSeconds heuristic (which usually still reports a timeout because the server kills at T, but not reliably).
2. Non-timeout server errors (unavailable, internal, "sandbox gone") lose their code and message and surface as the generic "envd process stream ended before receiving a process exit code" — the case jsonNonTimeoutErrorFrameKeepsStreamError pins for JSON is unreported and differently classified for PROTO.
3. The two new proto tests (protoDeadlineExceededEndFrameMapsToExecTimeout, line 464, and protoEndFrameWithUnknownFieldsStillMaps, line 538) build their frames via protoEndStreamError (line 618) using the same error = 1 assumption, so decoder and fixture share one wrong premise and pass while production stays broken.

Suggested fix: decode the end-stream payload as JSON for both codecs (always call parseJsonEndStreamError), delete parseProtoEndStreamError and the protoEndStreamError fixture, and pin the behaviour with a test that feeds a JSON end-stream frame to a PROTO-coded client — the configuration that ships by default. If a proto fallback is kept for robustness it needs a captured frame from envd rather than a hand-built one. The rest of the frame handling is correct: END_STREAM_FLAG = 0x02 matches connect-go's connectFlagEnvelopeEndStream, and the per-call callTimeout + readTimeout(0) + interrupt-snapshot design verifies out against OkHttp 5.3.2.

[chcodex]

@oss-maintainer You found a real defect — confirmed and fixed in 298b138 (24/24 green, spotless clean).

Root cause. MarshalEndStream in connect-go hardcodes json.Marshal(end) — the end-stream message is never routed through the negotiated codec. So parseProtoEndStreamError could never match a real frame under any codec, not just the default one; its two tests passed only because I fed the method hand-rolled protobuf bytes, which was worse than no coverage.

Verified live, not just from source. Ran a real envd (0.6.10) with Connect-Timeout-Ms: 2000 + sleep 10, both content types:

application/connect+proto -> end-stream flags=0x02 len=76
  7b226572726f72223a7b22636f6465223a22646561646c696e655f6578636565646564222c...
  {"error":{"code":"deadline_exceeded","message":"context deadline exceeded"}}

application/connect+json  -> end-stream flags=0x02 len=76  (byte-identical)

Fix. Deleted the proto decoder, the codec dispatch, the CodedInputStream/WireFormat imports, and the two fabricated proto tests (net -77). parseEndStreamError is now a single JSON path with the protocol fact documented on the method. Replaced coverage with protoCodecStillDecodesJsonEndStreamFrame, which asserts the E2bCodec.PROTO client decodes the exact 76-byte frame captured from envd — so anyone reintroducing a codec-dispatched decoder turns the test red.

No behavior regression. When the server kills on time the client still reports ExecTimeoutException; only the attribution path shifts from "server said so" to the MissingExitCodeException + elapsed fallback, which was already covered by both branches.

On the wire-level socket test you also raised: agreed it is the real gap, but it needs envdHost to stop hardcoding https with no port injection — a separate refactor, tracked as follow-up rather than attempted here.

[chcodex]

@oss-maintainer You were right that the racing path had no wire-level coverage, and that the app-level interceptors couldn't provide it. Added in da3eb1b — E2bEnvdProcessClientSocketTest, 3 cases, no production changes.

I first tried the dns() override and it failed: it only redirects resolution, not the port, and envdHost hardcodes https://49983-<id>.<domain> so OkHttp still dialled 443 (no root here to bind it). What works is a URL-rewriting application interceptor that repoints scheme/host/port and then calls proceed() — the request still traverses the real network stack, so callTimeout and readTimeout are enforced by OkHttp itself, unlike the earlier interceptor tests that short-circuited before the timeout machinery engaged.

The cases, with the base client deliberately set to readTimeout=1s and T=1s:

• idleSocketIsNotKilledByInheritedReadTimeout — server sends a start frame then holds the socket silent past T. Asserts the call survives past T and ends as ExecTimeoutException("Command timed out after 1s"), not a SocketTimeoutException. This is the direct proof of readTimeout(0); with the base value inherited it would die at ~1s as a socket timeout. Runs ~6s (1s T + 5s slack).
• clientBackupFailsafeEndsHungSocket — server accepts and never responds. Asserts the failure is still ExecTimeoutException, bounded below 30s. Runs ~6s.
• normalStreamOverRealSocketSucceeds — a well-formed PROTO StartResponse end event over a real socket returns exit 0, confirming the harness itself is wired correctly rather than trivially failing.

While writing this I hit a genuine bug in my own first draft worth flagging: I initially wrapped the end event in a Connect end-stream frame (flags 0x02). The end event is a regular data frame (0x00); 0x02 is the Connect error frame flag, so it was correctly ignored and the call failed with MissingExitCodeException. The test caught it — noting it because it's exactly the class of mistake the socket-level test was meant to surface.

[oss-maintainer]

Summary

Delta re-review at head da3eb1b8 (previous round was on a007a211). Both open items from the last round are closed, and closed in the way the protocol actually works:

• End-stream decoding is now JSON-only. parseEndStreamError no longer branches on codec(), and the proto decoder is gone. The javadoc records why — connect-go's MarshalEndStream hardcodes json.Marshal, so the end-stream frame is not routed through the negotiated codec — and reports it verified against a live envd, where application/connect+proto and application/connect+json return the same 76-byte {"error":{"code":"deadline_exceeded",…}} frame. That was the defect: under the default E2bCodec.PROTO the new deadline_exceeded mapping could never fire, so the server-wins classification was inert for the codec that ships. Dropping the branch also deletes ~44 lines of hand-rolled wire parsing that nothing else needed.
• The racing path now has wire-level coverage. E2bEnvdProcessClientSocketTest (210 lines, 3 cases) drives a real socket through OkHttp rather than canned in-memory frames: a start event followed by silence past T asserts the ExecTimeoutException message and classification; a second case pins the client-backup ordering (elapsed >= T and < 30 s, so the backup bounds the call without preempting the server deadline); a third keeps the normal completion path honest. This is exactly the gap I flagged — every previous test fed pre-encoded buffers, so nothing exercised how callTimeout(0-read-timeout) behaves against an actual stalled peer.

CI is fully green on this head (build (ubuntu-latest) 19m18s, build (windows-latest) 27m27s, License, Module Sync, codecov), CLA is signed, and the PR is MERGEABLE/CLEAN. Nothing in the delta reopens the earlier analysis: the interrupt-bit handling, Connect-Timeout-Ms propagation and the mapPrematureEnd fault grading are unchanged since the last round.

Approving. Thanks for the live-envd verification note — that is the kind of detail that saves the next person from re-introducing a "symmetric" proto decoder.

One forward-looking note (not a blocker, no action needed here): #3341, #3343 and #3344 are a split of this same area by the same author and carry 064c77b5 / 8fd39b32 in their branches. Whichever lands first, the others will need re-anchoring, and #3341 currently still logs envdAccessToken in E2bEnvdProcessClientIntegrationTest — worth a glance when you reconcile the series.

---

Automated review by github-manager-bot