Skip to content

Commit 3fe739f

Browse files
committed
fix(harness): stop orphan GC from deleting skills other calls still use
Fixes #2787 MarketplaceStager rebuilt its retain-list from a single call's visible skills and then deleted every other directory under a `.skills-cache` tree shared by every call against the workspace. With per-user skill visibility, call B's "orphan" was call A's live filesRoot, so B removed a directory A had just staged - often while A was still walking it, which surfaced as an UncheckedIOException out of onSystemPrompt that failed the whole agent call before its first model round. Two changes, matching the two defects in the issue. Traversal no longer aborts on a vanished entry. deleteRecursively uses Files.walkFileTree, so an entry removed mid-walk is reported through visitFileFailed instead of ending the traversal; removeUnexpected and garbageCollectOrphans also catch UncheckedIOException, which Files.walk and Files.list throw for mid-iteration IO errors and which is not an IOException; and the GC call is wrapped so cache hygiene can never fail an agent call, mirroring the fallback already applied per skill. Staging is scoped, following the issue's own suggestion to prefix `.skills-cache` with the namespace IsolationScope already applies to runtime data. Skills now materialise under `.skills-cache/<scope>/<source-ns>/<name>/`, where the scope is the call's userId (or sessionId, matching IsolationScope.USER's documented fallback). Calls that do not share a scope cannot address each other's subtree, so a white-list built from one call's visible skills is authoritative for everything its sweep can reach - the property the flat layout never had. A short grace window remains as a backstop for entries whose visibility changes within a scope. The scope must be at least as fine as the visibility dimension. The default, USER, matches the per-user visibility filters this failure was reported against; an agent that combines AGENT or GLOBAL scope with per-user visibility still shares one subtree and relies on the grace window alone. Tests: - MarketplaceStagerOrphanGcTest keeps a fresh orphan, still reclaims a stale one, survives concurrent callers with differing visible sets, degrades rather than throwing on an unreadable entry, and asserts that an aged directory in one scope is unreachable by another scope's sweep. - SharedWorkspaceSkillStagingE2ETest drives the real middleware path: two agents sharing one workspace, per-user visibility asserting that no prompt ever advertises a files-root missing from disk, and a deterministic case where one user's call must not delete another user's staged skill. Each of those fails against the unpatched stager. Full suite: 89/89 modules, 5983 tests, 0 failures.
1 parent 4ba433a commit 3fe739f

8 files changed

Lines changed: 885 additions & 38 deletions

File tree

agentscope-harness/src/main/java/io/agentscope/harness/agent/HarnessAgent.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2775,6 +2775,7 @@ public HarnessAgent build() {
27752775
visibilityFilter,
27762776
stager,
27772777
shellPolicy);
2778+
skillMiddleware.isolationScope(fsIsolationScope);
27782779
inner.middleware(skillMiddleware);
27792780

27802781
// Harness owns both the live and frozen repository paths.

agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/HarnessSkillMiddleware.java

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.agentscope.core.skill.SkillFilter;
2222
import io.agentscope.core.skill.repository.AgentSkillRepository;
2323
import io.agentscope.core.tool.Toolkit;
24+
import io.agentscope.harness.agent.IsolationScope;
2425
import io.agentscope.harness.agent.skill.LazyResourceCapable;
2526
import io.agentscope.harness.agent.skill.RuntimeContextSkillRepository;
2627
import io.agentscope.harness.agent.skill.SkillResources;
@@ -81,6 +82,34 @@ public class HarnessSkillMiddleware implements HarnessRuntimeMiddleware {
8182
private final SkillRuntime runtime;
8283
private final Map<AgentSkillRepository, String> sourceNamespaces;
8384
private final Map<String, RepoBound> frozenSkills;
85+
private IsolationScope isolationScope;
86+
87+
/**
88+
* Per-call cache scope, mirroring the identity {@link IsolationScope} already applies to
89+
* runtime data. Calls that share a scope share a {@code .skills-cache} subtree, and only
90+
* those calls can sweep it — which is what makes one call's visible-skill white-list
91+
* authoritative for everything the sweep can reach.
92+
*/
93+
private String scopeKeyFor(RuntimeContext ctx) {
94+
IsolationScope scope = isolationScope != null ? isolationScope : IsolationScope.USER;
95+
return switch (scope) {
96+
case USER -> {
97+
String uid = ctx != null ? ctx.getUserId() : null;
98+
if (uid != null && !uid.isBlank()) {
99+
yield uid;
100+
}
101+
// Mirrors IsolationScope.USER's documented fall back to the session identity.
102+
String sid = ctx != null ? ctx.getSessionId() : null;
103+
yield sid != null ? sid : MarketplaceStager.SHARED_SCOPE;
104+
}
105+
case SESSION -> {
106+
String sid = ctx != null ? ctx.getSessionId() : null;
107+
yield sid != null && !sid.isBlank() ? sid : MarketplaceStager.SHARED_SCOPE;
108+
}
109+
// The workspace is already per-agent, so these need no further separation.
110+
case AGENT, GLOBAL -> MarketplaceStager.SHARED_SCOPE;
111+
};
112+
}
84113

85114
public HarnessSkillMiddleware(List<AgentSkillRepository> repositories, Toolkit toolkit) {
86115
this(repositories, toolkit, null, null, null, ShellPathPolicy.noShell());
@@ -169,6 +198,7 @@ private HarnessSkillMiddleware(
169198
this.stager = stager;
170199
this.shellPathPolicy =
171200
shellPathPolicy != null ? shellPathPolicy : ShellPathPolicy.noShell();
201+
this.isolationScope = IsolationScope.USER;
172202
this.runtime = new SkillRuntime();
173203
// Pre-resolve source namespaces once at build time. The compose order is fixed for
174204
// the lifetime of the middleware, so this is safe and avoids repeated work per call.
@@ -186,6 +216,15 @@ public SkillRuntime runtime() {
186216
return runtime;
187217
}
188218

219+
/**
220+
* Overrides the isolation dimension used to separate {@code .skills-cache} subtrees.
221+
* Defaults to {@link IsolationScope#USER}, matching the default for runtime data.
222+
*/
223+
public HarnessSkillMiddleware isolationScope(IsolationScope scope) {
224+
this.isolationScope = scope;
225+
return this;
226+
}
227+
189228
/** Whether repository enumeration is frozen to the construction-time snapshot. */
190229
public boolean isFrozen() {
191230
return frozenSkills != null;
@@ -213,7 +252,7 @@ public void prestageMarketplaceSkills(RuntimeContext ctx) {
213252
List<RepoBound> visible = applyVisibility(merged.values(), ctx);
214253
List<RepoBound> enabled = applySkillFilter(visible, effectiveFilter(ctx));
215254
if (!enabled.isEmpty()) {
216-
stager.stage(enabled, sourceNamespaces);
255+
stager.stage(enabled, sourceNamespaces, scopeKeyFor(ctx));
217256
}
218257
}
219258

@@ -239,7 +278,9 @@ public Mono<String> onSystemPrompt(Agent agent, RuntimeContext ctx, String curre
239278
}
240279

241280
Map<String, StageResult> staged =
242-
stager != null ? stager.stage(enabled, sourceNamespaces) : Map.of();
281+
stager != null
282+
? stager.stage(enabled, sourceNamespaces, scopeKeyFor(ctx))
283+
: Map.of();
243284

244285
List<HarnessSkillEntry> entries = new ArrayList<>(enabled.size());
245286
for (RepoBound bound : enabled) {

0 commit comments

Comments
 (0)