fix(sandbox-agentrun): keep MCP open when sandbox is not owned - #2302
fix(sandbox-agentrun): keep MCP open when sandbox is not owned#2302jialiuyang wants to merge 1 commit into
Conversation
AgentRunSandbox.shutdown() closed the MCP channel before checking isSandboxOwned(). For shared/non-owned sandboxes that left the cloud container alive, background SessionTree mirrors then failed with MCP session terminated. Skip close/delete when not owned, matching E2b and Daytona adapters. Fixes agentscope-ai#2259
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
added
bug
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR fixes the AgentRunSandbox.shutdown() lifecycle for non-owned (shared/reused) sandboxes. Previously, the MCP channel was unconditionally closed before the ownership check, meaning shared sandboxes lost their MCP connection even though background operations (e.g., SessionTree mirror uploads, ref #2259) might still need it. The fix moves the isSandboxOwned() guard before mcp.close(), aligning with the E2b/Daytona pattern where non-owned sandboxes trigger an immediate early return — neither closing the communication channel nor deleting the sandbox. The change is small, focused, and well-tested.
| public void shutdown() throws Exception { | ||
| // Match E2b/Daytona: a non-owned sandbox is shared/reused — keep the MCP channel open so | ||
| // background SessionTree mirrors can still uploadFiles() after agent teardown (#2259). | ||
| if (!arState.isSandboxOwned()) { |
There was a problem hiding this comment.
[nitpick] Observability: The early return for non-owned sandboxes is silent. Consider adding a log.debug before returning so that lifecycle issues (e.g., unexpected double-shutdown or leaked MCP connections) are easier to trace in production.
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR fixes the AgentRunSandbox.shutdown() lifecycle for non-owned (shared/reused) sandboxes. Previously, the MCP channel was unconditionally closed before the ownership check, meaning shared sandboxes lost their MCP connection even though background operations (e.g., SessionTree mirror uploads, ref #2259) might still need it. The fix moves the isSandboxOwned() guard before mcp.close(), aligning with the E2b/Daytona pattern where non-owned sandboxes trigger an immediate early return — neither closing the communication channel nor deleting the sandbox. The change is small, focused, and well-tested.
| public void shutdown() throws Exception { | ||
| // Match E2b/Daytona: a non-owned sandbox is shared/reused — keep the MCP channel open so | ||
| // background SessionTree mirrors can still uploadFiles() after agent teardown (#2259). | ||
| if (!arState.isSandboxOwned()) { |
There was a problem hiding this comment.
[nitpick] Observability: The early return for non-owned sandboxes is silent. Consider adding a log.debug before returning so that lifecycle issues (e.g., unexpected double-shutdown or leaked MCP connections) are easier to trace in production.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR contains 101 lines of changes. Review in progress.
Automated review by github-manager-bot
|
Could a maintainer merge when convenient? Happy to rebase or adjust if anything else is needed. |
3 participants
AgentScope-Java Version
2.0.x (main)
Description
AgentRunSandbox.shutdown()closed the MCP channel before checkingisSandboxOwned(). For shared/non-owned sandboxes (cloud container kept alive), backgroundSessionTreemirrors then failed withMCP session with server terminatedwhile uploading session files.This change returns early when the sandbox is not owned — keeping MCP open and skipping delete — matching
E2bSandbox/DaytonaSandbox. Owned sandboxes still close MCP and delete as before.Fixes #2259
Checklist
mvn spotless:apply