Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import io.agentscope.harness.agent.filesystem.model.FileUploadResponse;
import io.agentscope.harness.agent.sandbox.ExecResult;
import io.agentscope.harness.agent.sandbox.Sandbox;
import io.agentscope.harness.agent.sandbox.SandboxAcquireResult;
import io.agentscope.harness.agent.sandbox.SandboxAware;
import io.agentscope.harness.agent.sandbox.SandboxException;
import io.agentscope.harness.agent.sandbox.SandboxFileTransfer;
Expand All @@ -44,9 +45,17 @@
/**
* A {@link BaseSandboxFilesystem} that delegates execution to a live {@link Sandbox}.
*
* <p>Stable proxy created at agent build time; a fresh {@link Sandbox} is injected on each call
* via the volatile {@code sandbox} field by {@link
* io.agentscope.harness.agent.middleware.SandboxLifecycleMiddleware}.
* <p>Stable proxy created once per agent bean. The live {@link Sandbox} for a call is bound
* <em>per-call</em> on the invocation's {@link RuntimeContext} by {@link
* io.agentscope.harness.agent.middleware.SandboxLifecycleMiddleware} and resolved here via {@link
* #requireSandbox(RuntimeContext)} — this per-call binding takes precedence and is what keeps
* concurrent distinct-session calls on the same agent bean isolated (issue #2490). The legacy
* {@code volatile sandbox} field is retained only as a best-effort fallback for context-free
* internal callers that resolve the filesystem with a shared empty {@link RuntimeContext} (e.g.
* {@link io.agentscope.harness.agent.bus.WorkspaceMessageBus}, which carries no per-call binding).
* The middleware still maintains that field via {@link #setSandbox} on acquire and {@link
* #clearSandboxIfCurrent} on release, so it remains last-writer-wins under concurrency and must not
* be relied on for isolation.
*/
public class SandboxBackedFilesystem extends BaseSandboxFilesystem implements SandboxAware {

Expand All @@ -60,7 +69,7 @@ public SandboxBackedFilesystem() {
}

@Override
public void setSandbox(Sandbox sandbox) {
public synchronized void setSandbox(Sandbox sandbox) {
this.sandbox = sandbox;
}

Expand All @@ -69,6 +78,19 @@ public Sandbox getSandbox() {
return sandbox;
}

/**
* Clears the fallback {@code sandbox} field only if it still points at {@code expected}. Used by
* {@link io.agentscope.harness.agent.middleware.SandboxLifecycleMiddleware} on release so a
* finishing call never nulls a concurrent sibling call's fallback binding (issue #2490).
*
* @param expected the sandbox this call bound at acquire time
*/
public synchronized void clearSandboxIfCurrent(Sandbox expected) {
if (this.sandbox == expected) {
this.sandbox = null;
}
}

@Override
public String id() {
return fsId;
Expand All @@ -77,7 +99,7 @@ public String id() {
@Override
public ExecuteResponse execute(
RuntimeContext runtimeContext, String command, Integer timeoutSeconds) {
Sandbox active = requireSandbox();
Sandbox active = requireSandbox(runtimeContext);
try {
ExecResult result = active.exec(runtimeContext, command, timeoutSeconds);
return new ExecuteResponse(
Expand All @@ -100,7 +122,7 @@ public ExecuteResponse execute(
@Override
public List<FileUploadResponse> uploadFiles(
RuntimeContext runtimeContext, List<Map.Entry<String, byte[]>> files) {
Sandbox active = requireSandbox();
Sandbox active = requireSandbox(runtimeContext);
List<FileUploadResponse> results = new ArrayList<>(files.size());

for (Map.Entry<String, byte[]> file : files) {
Expand Down Expand Up @@ -137,7 +159,7 @@ public List<FileUploadResponse> uploadFiles(
@Override
public List<FileDownloadResponse> downloadFiles(
RuntimeContext runtimeContext, List<String> paths) {
Sandbox active = requireSandbox();
Sandbox active = requireSandbox(runtimeContext);
List<FileDownloadResponse> results = new ArrayList<>(paths.size());

for (String path : paths) {
Expand Down Expand Up @@ -182,8 +204,23 @@ public List<FileDownloadResponse> downloadFiles(
return results;
}

private Sandbox requireSandbox() {
Sandbox s = sandbox;
/**
* Resolves the {@link Sandbox} bound to the current call, preferring the per-call binding
* carried on {@code runtimeContext} (concurrency-safe under parallel distinct-session calls,
* issue #2490) and falling back to the legacy {@code sandbox} field for direct
* {@link #setSandbox} callers that do not thread a per-call context.
*/
private Sandbox requireSandbox(RuntimeContext runtimeContext) {
Sandbox s = null;
if (runtimeContext != null) {
SandboxAcquireResult bound = runtimeContext.get(SandboxAcquireResult.class);
if (bound != null) {
s = bound.getSandbox();
}
}
if (s == null) {
s = sandbox;
}
if (s == null) {
throw new SandboxException.SandboxConfigurationException(
"No active sandbox — sandbox filesystem used outside of a call context");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import io.agentscope.harness.agent.sandbox.SandboxAcquireResult;
import io.agentscope.harness.agent.sandbox.SandboxContext;
import io.agentscope.harness.agent.sandbox.SandboxManager;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -34,28 +33,32 @@
* <li>Read {@link SandboxContext} from the current {@link RuntimeContext}</li>
* <li>Acquire a session via {@link SandboxManager}</li>
* <li>Start the session (4-branch workspace init)</li>
* <li>Inject the live session into the {@link SandboxBackedFilesystem} proxy</li>
* <li>Bind the live session on the per-call {@link RuntimeContext} for the
* {@link SandboxBackedFilesystem} proxy to resolve</li>
* </ol>
*
* <h2>doFinally</h2>
* <ol>
* <li>Persist sandbox session state via {@link SandboxManager} and
* {@link io.agentscope.harness.agent.sandbox.SessionSandboxStateStore}</li>
* <li>Release the session via {@link SandboxManager} (stop + optional shutdown)</li>
* <li>Clear the session reference from the filesystem proxy</li>
* <li>Clear this call's session binding from the {@link RuntimeContext}</li>
* </ol>
*
* <p>Post-call failures (persist, release) are logged but do not propagate — this ensures
* the agent call result is always returned to the caller even if sandbox cleanup fails.
*
* <p>The sandbox is bound <em>per call</em> on the invocation's {@link RuntimeContext} rather than
* on a shared agent-level slot: distinct {@code (userId, sessionId)} sessions run in parallel on
* the same agent bean, so a shared slot would let concurrent calls corrupt each other's binding
* (issue #2490).
*/
public class SandboxLifecycleMiddleware implements HarnessRuntimeMiddleware {

private static final Logger log = LoggerFactory.getLogger(SandboxLifecycleMiddleware.class);

private final SandboxManager sandboxManager;
private final SandboxBackedFilesystem filesystemProxy;
private final AtomicReference<SandboxAcquireResult> currentAcquireResult =
new AtomicReference<>();
private volatile Consumer<RuntimeContext> beforeStartCallback;

public SandboxLifecycleMiddleware(
Expand Down Expand Up @@ -108,13 +111,20 @@ public void acquireForCall(RuntimeContext ctx) {
Sandbox sandbox = result.getSandbox();
try {
sandbox.start();
// Bind the acquired sandbox per-call on this invocation's RuntimeContext rather
// than only on a shared agent-level slot. Distinct (userId, sessionId) sessions
// run in parallel on the same agent bean, so a shared slot lets concurrent calls
// corrupt each other's binding (issue #2490). The filesystem proxy resolves the
// sandbox from this context first; the field below is a best-effort fallback for
// context-free callers (e.g. WorkspaceMessageBus) that do not thread a per-call.
ctx.put(SandboxAcquireResult.class, result);
filesystemProxy.setSandbox(sandbox);
currentAcquireResult.set(result);
log.debug(
"[sandbox-mw] Acquired sandbox {}",
sandbox.getState() != null ? sandbox.getState().getSessionId() : "?");
} catch (Exception e) {
filesystemProxy.setSandbox(null);
ctx.put(SandboxAcquireResult.class, null);
filesystemProxy.clearSandboxIfCurrent(sandbox);
try {
sandboxManager.release(result);
} catch (Exception releaseErr) {
Expand All @@ -139,11 +149,20 @@ public void acquireForCall(RuntimeContext ctx) {
* @param ctx the per-call RuntimeContext (captured at acquire time)
*/
public void releaseForCall(RuntimeContext ctx) {
SandboxAcquireResult result = currentAcquireResult.getAndSet(null);
if (ctx == null) {
return;
}
// Read back the binding this same call established in acquireForCall, so a call only ever
// tears down its own sandbox — never a concurrent session's (issue #2490).
SandboxAcquireResult result = ctx.get(SandboxAcquireResult.class);
if (result == null) {
return;
}
SandboxContext sandboxContext = ctx != null ? ctx.get(SandboxContext.class) : null;
ctx.put(SandboxAcquireResult.class, null);
// Compare-and-clear the fallback field so a releasing call never nulls a concurrent
// sibling's binding (issue #2490); it only clears the field when it still points here.
filesystemProxy.clearSandboxIfCurrent(result.getSandbox());
SandboxContext sandboxContext = ctx.get(SandboxContext.class);
try {
sandboxManager.persistState(result, sandboxContext, ctx);
} catch (Exception e) {
Expand All @@ -155,6 +174,5 @@ public void releaseForCall(RuntimeContext ctx) {
log.warn("[sandbox-mw] Failed to release sandbox session: {}", e.getMessage(), e);
}
result.getLease().close();
filesystemProxy.setSandbox(null);
}
}
Loading
Loading