Bug: ToolGroupManager.activeGroups is not thread-safe, causing race conditions in concurrent agent scenarios

[Fruank4]

Bug Description

ToolGroupManager.activeGroups is declared as a plain ArrayList, while the other two maps in the same class use ConcurrentHashMap. In multi-agent or concurrent execution scenarios, multiple threads can read and write activeGroups simultaneously, leading to race conditions.

Affected File

agentscope-core/src/main/java/io/agentscope/core/tool/ToolGroupManager.java, line 36

Current Code

private final Map<String, ToolGroup> toolGroups = new ConcurrentHashMap<>(); // thread-safe ✓
private final Map<String, Set<String>> tools = new ConcurrentHashMap<>();    // thread-safe ✓
private List<String> activeGroups = new ArrayList<>();                       // NOT thread-safe ✗

activeGroups is accessed (read or written) from multiple methods without synchronization:

Line	Method	Operation
57–58	createToolGroup()	read + write
92–98	updateToolGroups()	read + write
132	removeToolGroups()	write
152	getActivatedNotes()	iterate
317–318	getActiveGroups()	read
327	setActiveGroups()	field reassignment (not volatile)
377	copyTo()	write

Additionally, setActiveGroups() replaces the field reference entirely (this.activeGroups = new ArrayList<>(...)), but the field is neither volatile nor final, so other threads may observe a stale reference.

Expected Behavior

All accesses to activeGroups should be thread-safe, consistent with the rest of the class.

Impact

When multiple agents share or concurrently modify a ToolGroupManager:

• ConcurrentModificationException during iteration in getActivatedNotes()
• Lost updates: tool group activation/deactivation silently dropped
• Stale reads: a thread may see the old list after setActiveGroups() replaces it
• Agents may execute with incorrect tool availability

Suggested Fix

Replace ArrayList with CopyOnWriteArrayList (appropriate for read-heavy workloads) and make the field final to prevent unsafe reference replacement:

// Before
private List<String> activeGroups = new ArrayList<>();

// After
private final List<String> activeGroups = new CopyOnWriteArrayList<>();

setActiveGroups() should then clear and re-add rather than reassign:

public void setActiveGroups(List<String> activeGroups) {
    this.activeGroups.clear();
    this.activeGroups.addAll(activeGroups);
    // ... rest of the method
}

Similarly, copyTo() should use the same pattern instead of direct field assignment.

[Alexxigang]

I traced the current implementation locally. ToolGroupManager keeps toolGroups in concurrent maps, but activeGroups is still a plain ArrayList, so concurrent activation/deactivation and read paths like getActiveGroups() / getActivatedNotes() can observe inconsistent state and are vulnerable to concurrent modification behavior. I am working on a fix that makes active-group state concurrency-safe without changing the public API, and I will add regression coverage for concurrent updates, snapshot reads, and toolkit copy/restore paths.

[Alexxigang]

Opened PR #1183 for the concurrent active-group state fix, with regression coverage for concurrent reads/writes, snapshot replacement, and copy isolation.