From d2e7209f6d6273b68ff66adca52ed23dae241b87 Mon Sep 17 00:00:00 2001 From: larry-zy Date: Wed, 12 Aug 2026 13:25:47 +0800 Subject: [PATCH 1/2] fix(harness): isolate sandbox binding per call to fix concurrent corruption (#2490) SandboxLifecycleMiddleware and SandboxBackedFilesystem held the acquired sandbox in a single agent-level slot (an AtomicReference plus the proxy's volatile field). Because AgentBase.serializeOnKey only serialises same (userId, sessionId) calls, distinct-session calls run in parallel on one agent bean and raced on that slot: one call would execute against another session's sandbox, and a finishing call's release would tear down a still-live sibling sandbox. Bind the acquired SandboxAcquireResult per call on the invocation's RuntimeContext and resolve it there first, so concurrent distinct-session calls stay isolated. releaseForCall reads back its own binding and tears down only its own sandbox. The volatile field is retained as a best-effort fallback for context-free internal callers that resolve the filesystem with a shared empty RuntimeContext (e.g. WorkspaceMessageBus). To keep that fallback from being clobbered across concurrent calls, the field is now maintained via a synchronized compare-and-clear (clearSandboxIfCurrent) so a releasing call never nulls a sibling's binding. Adds SandboxLifecycleConcurrencyReproTest, which drives the legal A.acquire -> B.acquire -> A.use -> A.release interleaving and asserts each call executes against its own sandbox and A's release affects only A. --- .../sandbox/SandboxBackedFilesystem.java | 55 ++++- .../SandboxLifecycleMiddleware.java | 38 ++- .../SandboxLifecycleConcurrencyReproTest.java | 219 ++++++++++++++++++ 3 files changed, 293 insertions(+), 19 deletions(-) create mode 100644 agentscope-harness/src/test/java/io/agentscope/harness/agent/middleware/SandboxLifecycleConcurrencyReproTest.java diff --git a/agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/SandboxBackedFilesystem.java b/agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/SandboxBackedFilesystem.java index 3cc435d936..df76a28cf3 100644 --- a/agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/SandboxBackedFilesystem.java +++ b/agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/SandboxBackedFilesystem.java @@ -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; @@ -44,9 +45,17 @@ /** * A {@link BaseSandboxFilesystem} that delegates execution to a live {@link Sandbox}. * - *

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}. + *

Stable proxy created once per agent bean. The live {@link Sandbox} for a call is bound + * per-call 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 { @@ -60,7 +69,7 @@ public SandboxBackedFilesystem() { } @Override - public void setSandbox(Sandbox sandbox) { + public synchronized void setSandbox(Sandbox sandbox) { this.sandbox = sandbox; } @@ -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; @@ -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( @@ -100,7 +122,7 @@ public ExecuteResponse execute( @Override public List uploadFiles( RuntimeContext runtimeContext, List> files) { - Sandbox active = requireSandbox(); + Sandbox active = requireSandbox(runtimeContext); List results = new ArrayList<>(files.size()); for (Map.Entry file : files) { @@ -137,7 +159,7 @@ public List uploadFiles( @Override public List downloadFiles( RuntimeContext runtimeContext, List paths) { - Sandbox active = requireSandbox(); + Sandbox active = requireSandbox(runtimeContext); List results = new ArrayList<>(paths.size()); for (String path : paths) { @@ -182,8 +204,23 @@ public List 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"); diff --git a/agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/SandboxLifecycleMiddleware.java b/agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/SandboxLifecycleMiddleware.java index a33ff6edc5..202d5dbeae 100644 --- a/agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/SandboxLifecycleMiddleware.java +++ b/agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/SandboxLifecycleMiddleware.java @@ -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; @@ -34,7 +33,8 @@ *

  • Read {@link SandboxContext} from the current {@link RuntimeContext}
    • *
  • Acquire a session via {@link SandboxManager}
    • *
  • Start the session (4-branch workspace init)
    • - *
  • Inject the live session into the {@link SandboxBackedFilesystem} proxy
    • + *
  • Bind the live session on the per-call {@link RuntimeContext} for the + * {@link SandboxBackedFilesystem} proxy to resolve
    • * * *

    doFinally

    @@ -42,11 +42,16 @@ *
  • Persist sandbox session state via {@link SandboxManager} and * {@link io.agentscope.harness.agent.sandbox.SessionSandboxStateStore}
    • *
  • Release the session via {@link SandboxManager} (stop + optional shutdown)
    • - *
  • Clear the session reference from the filesystem proxy
    • + *
  • Clear this call's session binding from the {@link RuntimeContext}
    • * * *

    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. + * + *

    The sandbox is bound per call 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 { @@ -54,8 +59,6 @@ public class SandboxLifecycleMiddleware implements HarnessRuntimeMiddleware { private final SandboxManager sandboxManager; private final SandboxBackedFilesystem filesystemProxy; - private final AtomicReference currentAcquireResult = - new AtomicReference<>(); private volatile Consumer beforeStartCallback; public SandboxLifecycleMiddleware( @@ -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) { @@ -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) { @@ -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); } } diff --git a/agentscope-harness/src/test/java/io/agentscope/harness/agent/middleware/SandboxLifecycleConcurrencyReproTest.java b/agentscope-harness/src/test/java/io/agentscope/harness/agent/middleware/SandboxLifecycleConcurrencyReproTest.java new file mode 100644 index 0000000000..0cc9363489 --- /dev/null +++ b/agentscope-harness/src/test/java/io/agentscope/harness/agent/middleware/SandboxLifecycleConcurrencyReproTest.java @@ -0,0 +1,219 @@ +/* + * 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.middleware; + +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 static org.mockito.Mockito.mock; + +import io.agentscope.core.agent.RuntimeContext; +import io.agentscope.harness.agent.filesystem.sandbox.SandboxBackedFilesystem; +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.SandboxClient; +import io.agentscope.harness.agent.sandbox.SandboxContext; +import io.agentscope.harness.agent.sandbox.SandboxLease; +import io.agentscope.harness.agent.sandbox.SandboxManager; +import io.agentscope.harness.agent.sandbox.SandboxState; +import io.agentscope.harness.agent.sandbox.SessionSandboxStateStore; +import java.io.InputStream; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import org.junit.jupiter.api.Test; + +/** + * Reproduction for issue #2490 — concurrent calls on one agent corrupt each other's sandbox + * binding. + * + *

    {@link SandboxLifecycleMiddleware} and {@link SandboxBackedFilesystem} are built once per + * agent bean and hold single-slot state: one {@code currentAcquireResult} reference and + * one {@code volatile Sandbox} field. The framework explicitly allows calls for distinct + * {@code (userId, sessionId)} sessions to run in parallel on the same agent bean + * ({@code AgentBase.serializeOnKey} only serialises same-session calls), so two concurrent + * different-session calls race on these shared slots. + * + *

    The test drives the exact ordering a legal two-session interleaving produces — + * {@code A.acquire → B.acquire → A.use → A.release} — and asserts the correct, isolated + * behaviour every session is entitled to: each call executes against its own sandbox, and A's + * release tears down only A's sandbox while B's turn keeps working. On current {@code main} (single + * shared slot) these invariants are violated, so this test fails (red) — that failure is the + * reproduction of #2490. Once the sandbox binding is made per-call, the test goes green. No + * {@code sleep}/timing is used, so it is deterministic. + */ +class SandboxLifecycleConcurrencyReproTest { + + @Test + void differentSessionsMustNotShareSandboxBinding() { + SandboxBackedFilesystem proxy = new SandboxBackedFilesystem(); + + RecordingSandbox sandboxA = new RecordingSandbox("s1"); + RecordingSandbox sandboxB = new RecordingSandbox("s2"); + RecordingLease leaseA = new RecordingLease(); + RecordingLease leaseB = new RecordingLease(); + + Map sandboxBySession = + new ConcurrentHashMap<>(Map.of("s1", sandboxA, "s2", sandboxB)); + Map leaseBySession = + new ConcurrentHashMap<>(Map.of("s1", leaseA, "s2", leaseB)); + + SandboxManager manager = fakeManager(sandboxBySession, leaseBySession); + SandboxLifecycleMiddleware mw = new SandboxLifecycleMiddleware(manager, proxy); + + RuntimeContext ctxA = callContext("s1"); + RuntimeContext ctxB = callContext("s2"); + + // Call A (session s1) acquires its sandbox for its turn. + mw.acquireForCall(ctxA); + + // Concurrently, Call B (session s2) acquires. serializeOnKey does NOT gate this because + // s1 != s2, so both turns are legally in flight on the same agent bean at once. + mw.acquireForCall(ctxB); + + // INVARIANT 1 — isolation: each call must execute against its own sandbox. + // On single-slot main, A's exec is routed to B's sandbox and returns "s2" -> RED. + assertEquals( + "s1", + proxy.execute(ctxA, "whoami", null).output(), + "call A must execute against its own sandbox (s1), not another session's"); + assertEquals( + "s2", + proxy.execute(ctxB, "whoami", null).output(), + "call B must execute against its own sandbox (s2)"); + + // Call A finishes its turn and releases while B is still running. + mw.releaseForCall(ctxA); + + // INVARIANT 2 — release affects only the releasing call. + // On single-slot main, A's release tears down B and leaks A -> RED. + assertTrue(sandboxA.stopped, "A's release must tear down A's own sandbox"); + assertTrue(leaseA.closed, "A's release must close A's own lease"); + assertFalse(sandboxB.stopped, "A's release must NOT tear down B's still-live sandbox"); + assertFalse(leaseB.closed, "A's release must NOT close B's lease"); + + // INVARIANT 3 — B's turn keeps working after A releases. + // On single-slot main, A's release cleared the shared slot, so B now sees + // "No active sandbox" -> RED. + assertEquals( + "s2", + proxy.execute(ctxB, "whoami", null).output(), + "call B must still run against its own sandbox after A released"); + } + + private static RuntimeContext callContext(String sessionId) { + SandboxContext sandboxContext = SandboxContext.builder().build(); + return RuntimeContext.builder() + .sessionId(sessionId) + .put(SandboxContext.class, sandboxContext) + .build(); + } + + /** + * A {@link SandboxManager} whose {@code acquire} hands back a distinct fake sandbox per session + * and whose {@code release} records the teardown it is asked to perform. The client/state-store + * dependencies are unused because every consulted method is overridden. + */ + private static SandboxManager fakeManager( + Map sandboxBySession, + Map leaseBySession) { + SandboxClient client = mock(SandboxClient.class); + SessionSandboxStateStore stateStore = mock(SessionSandboxStateStore.class); + return new SandboxManager(client, stateStore, "repro-agent") { + @Override + public SandboxAcquireResult acquire( + SandboxContext sandboxContext, RuntimeContext runtimeContext) { + String sid = runtimeContext.getSessionId(); + return SandboxAcquireResult.selfManaged( + sandboxBySession.get(sid), leaseBySession.get(sid)); + } + + @Override + public void release(SandboxAcquireResult result) { + if (result != null && result.getSandbox() instanceof RecordingSandbox rs) { + rs.stopped = true; + } + } + + @Override + public void persistState( + SandboxAcquireResult result, + SandboxContext sandboxContext, + RuntimeContext runtimeContext) { + // no-op for the reproduction + } + }; + } + + /** Minimal {@link Sandbox} that records teardown and echoes its session id from exec. */ + private static final class RecordingSandbox implements Sandbox { + + private final String sessionId; + volatile boolean stopped; + + RecordingSandbox(String sessionId) { + this.sessionId = sessionId; + } + + @Override + public void start() {} + + @Override + public void stop() { + stopped = true; + } + + @Override + public void close() { + stopped = true; + } + + @Override + public boolean isRunning() { + return !stopped; + } + + @Override + public SandboxState getState() { + return null; + } + + @Override + public ExecResult exec( + RuntimeContext runtimeContext, String command, Integer timeoutSeconds) { + return new ExecResult(0, sessionId, "", false); + } + + @Override + public InputStream persistWorkspace() { + return InputStream.nullInputStream(); + } + + @Override + public void hydrateWorkspace(InputStream archive) {} + } + + /** {@link SandboxLease} that records whether it was closed. */ + private static final class RecordingLease implements SandboxLease { + + volatile boolean closed; + + @Override + public void close() { + closed = true; + } + } +} From 2d31aa7294fadc5aa1e0e8362b6b25be73108054 Mon Sep 17 00:00:00 2001 From: YuZhangLarry Date: Mon, 24 Aug 2026 15:20:57 +0800 Subject: [PATCH 2/2] ci: re-trigger flaky AguiMvcControllerTest on Windows