fix(harness): fix concurrent sandbox isolation in multi-user singleton deployments#1964
fix(harness): fix concurrent sandbox isolation in multi-user singleton deployments#1964Buktal wants to merge 8 commits into
Conversation
…n deployments Two related bugs in HarnessAgent when used as a singleton serving concurrent sessions: Bug 1: SandboxBackedFilesystem used a single volatile Sandbox field and SandboxLifecycleMiddleware used a single AtomicReference<SandboxAcquireResult>. Concurrent sessions overwrote each other's bindings, causing cross-user sandbox contamination. Fixed by replacing both with ConcurrentHashMap<sessionId, ...>, keyed by sessionId so each session gets an isolated slot. SandboxAware interface updated: setSandbox/getSandbox replaced by bindSandbox/unbindSandbox/getSandbox(key). Bug 2: WorkspaceMessageBus and WorkspaceAsyncToolRegistry were constructed with the post-sandbox filesystem (SandboxBackedFilesystem) in HarnessAgent.build(). These are process-scoped infrastructure components; routing them through the sandbox filesystem causes RuntimeContext.empty() calls to fail or contaminate sandbox state. Fixed by capturing busFilesystem before the sandbox replacement and using it exclusively for bus/registry construction. Closes agentscope-ai#1896
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
itxaiohanglover
left a comment
itxaiohanglover
left a comment
There was a problem hiding this comment.
Good architectural fix — separating bus/registry filesystem from the sandbox filesystem prevents accidental routing through SandboxBackedFilesystem. The per-session ConcurrentHashMap approach for sandbox bindings looks correct for concurrent isolation. One question: is there a cleanup path for stale session entries when a session ends, or do entries persist for the proxy's lifetime?
Aligns the in-memory sandbox binding key with ReActAgent.slotKey() to prevent cross-user contamination when different users share the same sessionId in multi-tenant singleton deployments. Addresses review feedback from PR agentscope-ai#1964.
jujn
left a comment
jujn
left a comment
There was a problem hiding this comment.
The sandbox is still acquired in HarnessAgent.wrappedCall / wrappedStreamEvents before delegate.call() enters ReActAgent's per-slot serialization gate. If two calls for the same (userId, sessionId) are subscribed concurrently, both can run acquireForCall() first and write the same binding key. The second acquire overwrites the first sandbox/acquire result; the first call may then execute against the second sandbox and its cleanup can persist/release the wrong result, leaving the first sandbox leaked and the second call without a binding.
HarnessAgent.wrappedCall acquired the sandbox in Mono.using's resource supplier (subscription-time eager), which runs before ReActAgent's per-session serialization gate. Two concurrent same-session requests both ran acquireForCall(), the second overwrote the first's binding key in the ConcurrentHashMap — leaking the first sandbox and failing the second call with "No active sandbox". Fix: introduce a per-session async gate in SandboxLifecycleMiddleware (mirroring AgentBase.serializeOnKey) via serializedCall/serializedFlux. Same-session acquires now queue non-blocking; the second waits until the first's release completes before proceeding. Different sessions still run concurrently. HarnessAgent.wrappedCall/wrappedStream/wrappedStreamEvents delegate to the new methods instead of managing Mono.using/Flux.using directly.
…t-isolation # Conflicts: # agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/SandboxLifecycleMiddleware.java
Resolved, thanks! |
…adoc to internal methods
Buktal
force-pushed
the
fix/sandbox-concurrent-isolation
branch
from
c5d726f to
3f8f8ae
Compare
|
@Buktal I have sent my Dingtalk Id to you email address 1171971708@qq.com, thanks. |
Got it, thanks |
|
核心修复方向是对的—— 不过 建议保持原来 另外一个小点： |
…dingKey - Remove serializedCall/serializedFlux + sandboxGates (same-session concurrency serialization gate). Same-session concurrency is guarded at the business entry layer, not re-queued in the sandbox layer. - Restore acquireForCall/releaseForCall with Mono.using/Flux.using (aligned with main); internal acquire/release now operate per sessionKey. - Extract shared SandboxBindingKey to remove duplicated bindingKey impl in SandboxBackedFilesystem and SandboxLifecycleMiddleware.
4 participants
AgentScope-Java Version
2.0.0-SNAPSHOT
Description
Fixes two related concurrency bugs in
HarnessAgentwhen used as a singleton serving concurrent sessions (the standard multi-tenant deployment pattern documented in the class Javadoc and official examples).Bug 1 —
SandboxBackedFilesystemcross-session contamination (#1896)SandboxBackedFilesystemheld a singlevolatile Sandbox sandboxfield, andSandboxLifecycleMiddlewareheld a singleAtomicReference<SandboxAcquireResult>. Concurrent calls from different sessions overwrote each other's bindings — User A's tools could execute inside User B's sandbox.Fix: replace both single-value fields with
ConcurrentHashMap<sessionId, ...>.SandboxAwareinterface updated:setSandbox/getSandboxreplaced bybindSandbox(key, sandbox),unbindSandbox(key), andgetSandbox(key). The three public methods onSandboxBackedFilesystemalready receiveRuntimeContext, so the session key is always available at call time.Bug 2 —
WorkspaceMessageBus/WorkspaceAsyncToolRegistryusing sandbox filesystemHarnessAgent.build()constructed both components after replacingfilesystemwithSandboxBackedFilesystem. This caused their internalRuntimeContext.empty()file operations to callrequireSandbox(null), which would throw or contaminate sandbox state.Fix: capture
busFilesystembefore the sandbox replacement so bus and registry always use the non-sandbox (local or remote) filesystem. These are process-scoped infrastructure components with their own path-based isolation; they must not go through the sandbox.Testing
SandboxBackedFilesystemTest: updated existing 3 tests to new API; added 3 new tests including a concurrent 8-session isolation test usingCountDownLatchCompactionMiddlewareTest: already existed on thefeat/compaction-eventsbranch; no changes needed for this fixCloses #1896
Checklist
mvn spotless:applymvn test)