agentscope-ai / agentscope-ai/agentscope-java

[Bug]: Per-path lock maps grow without bound — no eviction in SkillBox, WorkspaceManager and LocalFilesystem

Abierto
#2,486 2 comentarios 0 reacciones 0 asignados Ver en GitHub
area/core/agent area/core/tool area/harness bug
Lenguaje dominante
Java
Estrellas
5.6k
Forks
1.3k
Merge medio
4 d 12 h
PR fusionados (30 d)
77

Descripción

## Summary

Three lock maps are keyed by file path and never evict entries. In an agent deployment the paths are chosen by the model rather than by a fixed application config, so the key space is effectively unbounded.

Related to #2261 (`ReActAgent.stateCache` unbounded growth), but these are different maps and one of them is `static`.

## Affected code (current `main`)

| Location | Declaration | Key | Lifetime |
|---|---|---|---|
| `agentscope-core/.../skill/SkillBox.java:57` | `private static final ConcurrentHashMap FILE_LOCKS` | `targetPath.toString()` (line 834) | **JVM-wide, shared by every SkillBox instance** |
| `agentscope-harness/.../workspace/WorkspaceManager.java:116` | `private final Map pathLocks` | normalized / relative path (lines 376, 409, 456, 646) | instance |
| `agentscope-harness/.../filesystem/local/LocalFilesystem.java:93` | `private final ConcurrentHashMap fileLocks` | lock key (line 359) | instance |

Each is populated exclusively through `computeIfAbsent`, e.g.:

```java
// SkillBox.java:834
Object lock = FILE_LOCKS.computeIfAbsent(targetPath.toString(), k -> new Object());
```

Grepping each file for `remove` / `clear` on these maps returns **0 matches** — entries are only ever added.

## Why it matters here

`FILE_LOCKS` being `static` is the notable one: every distinct path any skill has ever written to retains an entry for the lifetime of the process, across all `SkillBox` instances.

The usual justification for never evicting a per-key lock map is that the key space is bounded by application configuration. That does not hold for an agent runtime — file paths come from model output, so a long-running process accumulates one entry per distinct path the model ever produced.

## Scale, stated honestly

The leaked values are small (`Object`, `ReentrantLock`), so this is a slow leak rather than a fast one — unlike #2261, which retains `AgentState` objects. Reaching memory pressure needs a large number of distinct paths. I am reporting it as a boundedness defect rather than claiming an imminent OOM.

## Possible directions

1. **Striped locking** — a fixed number of locks selected by `key.hashCode()`, so the map never grows (the standard fix; Guava's `Striped` does exactly this). Costs occasional false sharing between unrelated paths.
2. **Remove after release** — `finally { map.remove(key); }`, but that races with another thread that is about to acquire the same lock, so it needs care.
3. **Bounded cache with eviction** — matches whatever approach #2261 settles on, keeping the codebase consistent.

I have not opened a PR because the choice among these is a design call, and option 3 in particular should probably follow whatever #2261 lands on. Happy to implement whichever direction maintainers prefer, with a test asserting the map stays bounded across many distinct paths.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.