fix(core): isolate active tool groups per session - #2444
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Code Review — Approved ✅
Overall: Excellent fix for the concurrent tool group cross-session interference bug (#2439). The approach is architecturally sound and well-tested.
Key Change: Each CallExecution now gets its own copy of the agent's Toolkit instead of sharing a single mutable instance. This ensures that active tool groups are isolated per-call, preventing concurrent sessions from overwriting each other's state.
Analysis:
-
Root Cause Fix: The previous design had
toolkit.setActiveGroups(...)modifying a shared instance inactivateSlotForContext, which caused race conditions when multiple sessions used the same agent singleton. By copying the toolkit per-call (ReActAgent.this.toolkit.copy()), each execution context now has its own isolated toolkit state. -
Clean Refactoring:
syncToolkitToStatenow takesCallExecutioninstead ofAgentState, correctly syncing from the call-scoped toolkitdoStructuredCallusesscope.toolkit.getToolSchemas(scope.state.getToolContext().getActivatedGroups())instead of the shared toolkit- Removed the problematic
toolkit.setActiveGroups(...)fromactivateSlotForContext
-
Test Coverage: Added
ReActAgentPerSessionStateTestwith comprehensive scenarios for per-session state isolation, including tool group isolation verification. -
Thread Safety: The
final Toolkit toolkitfield inCallExecutionensures immutability of the reference within a call scope.
CI Status: ✅ All checks passed (Check License, Check Module Sync, build on ubuntu-latest, build on windows-latest, license/cla)
Minor Observations:
- The
toolkit.copy()approach assumesToolkit.copy()is a deep copy. If it's a shallow copy, there could still be shared mutable state in the tool implementations themselves. Worth verifying in theToolkitclass. - The performance impact of copying the toolkit per-call should be negligible for most use cases, but worth monitoring in high-throughput scenarios.
Verdict: This is a well-designed fix that correctly addresses the concurrency issue. The test coverage is thorough, and the refactoring is clean.
Summary
Testing
mvn -pl agentscope-core -Dtest=ReActAgentPerSessionStateTest testmvn -pl agentscope-core -DskipTests packagemvn spotless:applyFixes #2439