fix(harness): fix concurrent sandbox isolation in multi-user singleton deployments#1964
Open
Buktal wants to merge 1 commit into
Open
fix(harness): fix concurrent sandbox isolation in multi-user singleton deployments#1964Buktal wants to merge 1 commit into
Buktal wants to merge 1 commit 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
Contributor
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?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)