feat(sandbox): retain E2B sandboxes between calls - #3413

[Warning] Priority 2 (user-supplied state) returns before the RETAIN requires a stable sandbox isolation key guard added below (line ~102), so RETAIN + externalSandboxState + no resolvable scope key is accepted here. SandboxManager.persistState resolves the scope key itself and returns early when it is absent, so the newly retained sandboxId is never written to the state store — the E2B instance stays alive and billed, but nothing can ever reconnect to it or delete it through the SDK. Either apply the same scope-key requirement to this branch, or document/enforce that callers using RETAIN with an explicit state must persist the returned state themselves.

[Warning] Preserving workspaceRootReady whenever sandboxCreatedDuringStart == null also covers failures that happen after the workspace was already mutated. Reaching super.start() with workspaceRootReady == true and a probe that returns true still runs applyWorkspaceSpec(...)/projection work; if that throws mid-way, the tree is partially initialized while the restored flag now claims it is intact. On the next call Branch A applies only ephemeral entries, so a half-restored workspace is silently treated as good. Could the preserve be narrowed to the pre-mutation phase (failure from ensureSandbox() or the probe) — e.g. a workspaceMutated flag set right before doSetupWorkspace()/doHydrateWorkspace()/applyWorkspaceSpec() — and keep the old setWorkspaceRootReady(false) behaviour for everything after that point?

[Warning] RETAIN's only cleanup paths are onRetained() (same JVM, same state object) and delete()/shutdown() with an in-memory handle. If the process dies, the state-store entry is evicted, or the isolation key changes, the retained instance is orphaned until provider expiry with no way to find it — a cost/leak risk for a feature aimed at long-running agents. A cheap mitigation would be to tag created sandboxes with the isolation key/metadata (E2B supports sandbox metadata) and expose a best-effort reconciliation/list-and-delete helper, or at minimum state this limitation in the enum javadoc + docs/harness/sandbox.md so operators know a crashed worker can leave billed instances behind.

[Info] delete() now delegates to sandbox.shutdown(), which returns immediately when !e2bState.isSandboxOwned(), so deleting a non-owned/external sandbox is a silent no-op while the method's javadoc reads as an unconditional destroy. Worth saying "only when the SDK owns the instance" in the javadoc. Minor: the failure is wrapped as a generic SandboxRuntimeException(WORKSPACE_STOP_ERROR, ...) although the existing SandboxException.WorkspaceStopException carries the same code plus the cause type — using it would keep error handling consistent with the rest of the backend.

[Info] The 10 here duplicates the private AbstractBaseSandbox.PROBE_TIMEOUT_SECONDS constant (also 10), and the javadoc of that constant ({@value #PROBE_TIMEOUT_SECONDS}) will drift from this override. Making the constant protected and reusing it would avoid the divergence. The shellSingleQuote(...) addition is a genuine improvement over the base implementation, which interpolates the root unquoted.

[Info] AbstractBaseSandbox.stop() already sets running = false, so a retained sandbox reports isRunning() == false right after a successful RETAIN release, and this override adds a second condition that is only false in the never-started case. If isRunning() is meant to mean "the remote instance is reachable", consider a distinct method (e.g. hasRemoteInstance()); as written the two semantics are conflated and callers cannot tell "retained and alive" from "gone".

[Warning] The only one of the three points from the previous pass that is now resolved by documentation instead of by code. Priority 2 still returns here, ahead of the RETAIN requires a stable sandbox isolation key guard at ~line 100, so RETAIN + externalSandboxState with no resolvable key is accepted and the harness deliberately never writes the record — the current sandboxId exists only in the caller's object.

I am no longer asking you to change the API shape: the new externalSandboxState() javadoc, the Lost state records bullet and the worked EN/ZH example state the contract unambiguously, and callerCanPersistExplicitStateResumeAndDeleteWithoutIsolationKey pins exactly the caller-side sequence the docs prescribe. That is a defensible v1.

The residual gap is observability, not behavior. If a caller ignores the contract, the symptom is a billed instance nothing can find plus a workspace the next call cannot resume, and the only trace of the decision is a paragraph in the docs — log.debug here would not even show it at default level, because this branch is a successful acquire. A single log.warn on the conjunction releasePolicy == RETAIN && externalSandboxState != null && scopeKey.isEmpty() converts a silent footgun into a diagnosable one, at the cost of one line. Non-blocking, but I would like it before this becomes the documented pattern other callers copy.

[Info] Correct fix for the over-broad metadata preservation, and the narrowing holds in both directions.

workspaceProbeFailed is reachable only from the catch in probeWorkspaceRootForPreservedResume(), i.e. from the read-only test -d before anything is written, so the preserve branch now covers exactly the failures that provably cannot have mutated the tree. Everything later (doSetupWorkspace, snapshot.restore, spec apply, projection apply) falls into the else branch, and nulling the projection hash there is the right pairing with the workspaceRootReady=false AbstractBaseSandbox.start() already applied — otherwise a retry could take Branch A off a stale hash and skip reapplying a projection over a restored snapshot.

Three edge cases I traced and found clean, so you do not have to re-check them:

• a non-0/1 probe exit throws ExecException inside the try, is caught by that same catch, so the flag is still set and abnormalProbeExitPreservesWorkspaceAndCanRetry keeps its meaning;
• hoisting ensureSandbox() out of the try does not strand a new allocation: if applyDefaultDomain() throws after sandboxCreatedDuringStart is set, SandboxLifecycleMiddleware's catch still routes RETAIN failures through SandboxManager.discard() → cleanupAfterStartFailure(), which kills only that id; and metadata is left untouched, which is precisely the outcome you want for a connect failure;
• if the 404-expiry recreate inside ensureSandbox() throws after already clearing ready/projection, the next attempt keeps the old expired id, re-connects, gets 404 again and recreates — it converges, and re-projection is forced rather than skipped.

failedProbeOfNewAllocationDoesNotRestoreStaleReadyMetadata is the case I was actually worried about (a fresh allocation inheriting ready=true from the instance it replaced) and the parameterized retryAfterWorkspaceMutationFailureRestoresInsteadOfTrustingPartialTree covers the three mutation stages separately, including the entries case where first.txt is deliberately asserted to survive. Good tests.

[Info] Accepting the documentation resolution for the orphan case. Saying plainly that there is no automatic instance discovery or reconciliation, next to concrete mitigations (durable record, finite sandboxTimeoutSeconds, delete before evicting the record), is worth more to an operator than an implied guarantee would have been, and EN and ZH say the same thing here.

If the maintainers later want this closed rather than documented, the seam is on the client, not in this enum: tag the instance at create time with agentId + isolation key and let a startup sweep reconcile GET /sandboxes against the state store, deleting what has no record. Worth a follow-up issue — the docs you added are the right place to link it from. Not something to change in this PR.

[Info] Closing the loop on the two Info items from the previous pass:

• this javadoc is the fix I was after for isRunning() — AbstractBaseSandbox.stop() clears running on a successful RETAIN, so the method reports local handle state and not remote existence, and saying so here is better than making the method try to be a liveness probe. The new retention test asserts the same semantics from outside (assertFalse(first.isRunning(), ...) with the remote instance still alive), which keeps the wording honest;
• PROBE_TIMEOUT_SECONDS going private → protected and being referenced here instead of a second 10 removes the silent-coupling risk, and AgentRunSandbox's override keeps its own literal, so nothing else moved.

delete()'s added Non-owned instances are left unchanged line answers the last one too.