fix(harness): stop skill-cache orphan GC from deleting live directories - #2840
fix(harness): stop skill-cache orphan GC from deleting live directories#2840wangchenxuya wants to merge 1 commit into
Conversation
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
37db971 to
bf91a83
Compare
| } | ||
| deleteRecursively(skillDir); |
There was a problem hiding this comment.
[P1] 旧状态检查和删除不是原子操作
另一位调用者可在 isStale 返回 true 后刷新此目录，但该调用者仍会继续对其进行删除操作。因此，同时被重新激活的失效缓存项可能会被标记为“已缓存”状态，随后又被移除。这需要一个跨实例/跨进程的租赁、锁定或不可变生成协议；仅通过额外的 mtime 读取操作仍有可能引发竞态问题。
There was a problem hiding this comment.
You are right, and it is worse than "narrow" — thanks for catching it.
I replayed exactly the stale-to-live reactivation you describe: age a 121-file skill past the grace window, then re-stage it while another caller sweeps it as an orphan. Against the previous commit that corrupts the directory in 379 of 400 rounds, with as few as 52 of the 121 files surviving. stage() still returns Cached. So the grace window narrowed when the race is reachable, but once a stale entry is being reactivated concurrently, losing content was the normal outcome, not the exception.
Fixed in 77b4b73 by serialising staging against GC on the cache root:
- Staging is shared — concurrent calls still stage side by side, so the common path is unchanged.
- GC is exclusive — which is what makes the staleness reading and the delete that follows one atomic step.
- Two layers, because
FileLockis owned by the JVM rather than by a thread and cannot separate threads inside one process: aReentrantReadWriteLockcovers threads, and a ref-counted advisory lock on.gc.lockcovers other replicas sharing the workspace volume. - A GC pass is skipped, never queued, when anyone is staging. Reclaiming an orphan is optional; deleting a directory a live call points at is not.
- Mounts that cannot lock degrade to the in-JVM layer plus the grace window rather than losing GC altogether.
invalidateAll()takes the same exclusive path.
I went with the lock rather than a lease or an immutable-generation scheme: a lease has to be held for the whole call to be meaningful (the staged path is handed to the model in the system prompt and used much later), which needs renewal and crash recovery, and content-addressed generations do not help here because GC can still delete the generation a caller just refreshed. The grace window covers the long horizon, the lock covers the instant — together there is no window left.
Also added the deterministic aged-directory test you asked for: agedDirectoryReactivatedDuringGcIsNotDeleted replays the reactivation and asserts that a Cached result always means a complete directory on disk, rather than only asserting that staging does not throw. It fails on round 0 without the lock.
Full suite green: 89/89 modules, 5982 tests, 0 failures.
There was a problem hiding this comment.
JVM 内竞态已修复，但跨进程场景仍可复现
There was a problem hiding this comment.
Both cross-process holes are fixed in 27ca82b (details in the two threads below), and the real two-JVM regression test you asked for is in: crossProcessReactivationIsNotDeleted launches a second JVM that ages the shared entry and sweeps it while this one re-stages, asserting that a Cached result never names a partly-deleted directory. It fails on round 15 against the previous commit. A READY handshake makes a sweeper that failed to launch fail the test rather than let it pass unopposed — worth having, since it immediately caught the child JVM writing logger chatter ahead of the signal.
Your point about the missing cross-process test was the root cause of both bugs, not a side note: every test I had was same-process, the in-JVM read/write lock covered those, so the suite was green while the cross-process path had never been executed once.
Reviewing my own locking afterwards also turned up three regressions I had introduced with it:
invalidateAll()went through the same gate as automatic GC, so it became a no-op on exactly the mounts where it is the documented escape hatch.- Acquiring the lock created the cache root, so
invalidateAll()materialised the tree it was asked to clear, and an agent with only workspace-native skills grew an empty.skills-cache— a sandbox projection root — plus a lock file.
Each is now covered by a test that fails against the version before it. The first needed a test that actually reaches the unlockable branch (chmod the cache root to force it), because my first attempt passed against both versions and proved nothing.
Full suite green: 89/89 modules, 5986 tests, 0 failures.
guslegend0510
left a comment
guslegend0510
left a comment
There was a problem hiding this comment.
遍历硬化过程看起来效果不错，但 stale 检查与递归删除操作仍有可能出现 TOCTOU 竞态。另一个调用方在 isStale () 返回 true 之后可能实际创建或触及一个 stale 目录，而 GC 调用方却继续对其进行删除，并留下一个指向已丢失路径的 cached 结果。当前的测试仅能确保 staging 过程不会抛出异常，但并未涵盖 stale 至 live 的重新激活过程。我们能否添加一个具有确定性的 aged 目录测试，并在合并前采用跨实例 / 进程租赁、锁定或不可变生成机制？
guslegend0510
left a comment
guslegend0510
left a comment
There was a problem hiding this comment.
这次新增的 JVM 内读写锁已经修复了同一 CacheGuard 下的竞态，但跨进程文件锁仍然是 fail-open。另一个进程持有 exclusive GC lock 时，tryLock(shared) 失败后 staging 仍会继续；同时，exclusive lock 获取异常时也会继续执行 GC，因此 stale→live 竞态在多副本场景下仍可复现。建议 staging 必须在成功取得 shared lock 后才能执行，而 GC 无法取得 exclusive lock 时应直接跳过，并补充真实双 JVM 的回归测试。
| } catch (IOException | RuntimeException e) { | ||
| // Locking unsupported on this mount; the in-JVM write lock still holds. | ||
| log.debug( | ||
| "Cross-process cache lock unavailable under {}, running GC in-JVM" | ||
| + " only: {}", | ||
| cacheRoot, | ||
| e.getMessage()); | ||
| crossProcess = false; | ||
| } | ||
| try { | ||
| if (crossProcess && exclusive == null) { | ||
| log.debug( | ||
| "Orphan GC under {} skipped: another process is staging", | ||
| cacheRoot); | ||
| return; | ||
| } | ||
| gc.run(); |
There was a problem hiding this comment.
[P1] 独占锁失败时应跳过 GC
获取 exclusive lock 抛异常并不能证明没有其他进程正在 staging。当前 fallback 会继续执行破坏性 GC，重新引入跨进程竞态。锁获取失败时应保守地跳过 GC。
There was a problem hiding this comment.
Fixed in 27ca82b — GC now runs only while it demonstrably holds the exclusive lock, and skips in every other case (contended, threw, or the mount cannot lock).
You are right that an exception is not evidence that nobody is staging. My reasoning for the fallback was "otherwise a mount without lock support never reclaims anything", which does not survive contact with the trade-off: a few KB of stale cache against deleting a directory a live call points at. I had written exactly that principle a few lines away — "reclaiming an orphan is optional, deleting a directory a live call points at is not" — and then violated it here.
invalidateAll() stays available as the explicit purge on such mounts; it deliberately does not share this gate.
| sharedLock = sharedChannel.tryLock(0L, Long.MAX_VALUE, true); | ||
| if (sharedLock == null) { | ||
| // Another replica is mid-GC. Staging still has to proceed; the grace window | ||
| // is what keeps that GC off anything staged recently. | ||
| closeShared(); | ||
| } |
There was a problem hiding this comment.
[P1] 获取不到共享锁时不能继续 staging
另一个进程持有 exclusive GC lock 时，tryLock(shared) 会返回 null，但当前代码仍继续 stageAll，导致 GC 可以删除正在恢复的目录。这里应等待 exclusive GC 完成并成功取得 shared lock 后再 staging，不能无锁继续。
There was a problem hiding this comment.
Fixed in 27ca82b — staging now blocks for the shared lock instead of carrying on without it.
The comment I had written there ("staging still has to proceed; the grace window is what keeps that GC off anything staged recently") was simply wrong, and worth naming: the directory being reactivated is by definition already past the grace window — that is why the sweeper picked it. So the grace window was never covering this case.
sharedChannel.lock(0L, Long.MAX_VALUE, true) now blocks. This cannot deadlock: a GC pass is bounded (two directory listings plus any deletes) and never itself waits — it takes the exclusive lock with tryLock and skips when it cannot get it — and the OS releases the lock if the holder dies.
The one case where staging still proceeds unlocked is a mount that cannot lock at all, because a failure to coordinate must not fail an agent call. That path now disables automatic GC process-wide instead, so nothing sweeps uncoordinated.
| } | ||
| deleteRecursively(skillDir); |
There was a problem hiding this comment.
JVM 内竞态已修复，但跨进程场景仍可复现
| if (Files.isDirectory(betaDir)) { | ||
| Files.setLastModifiedTime( | ||
| betaDir, FileTime.from(Instant.now().minus(Duration.ofDays(7)))); | ||
| } |
There was a problem hiding this comment.
P1] The cross-process test introduces a false race
The child process repeatedly rewinds beta’s mtime outside the cache-lock protocol. This can happen after the parent has already touched the directory, causing a legitimate GC pass to delete a newly reactivated entry. This has failed in repeated targeted runs. Please use a per-round handshake and age the directory only while neither process is staging.
| if (retained.contains(skillDir) || !isStale(skillDir, cutoff)) { | ||
| continue; | ||
| } | ||
| deleteRecursively(skillDir); |
There was a problem hiding this comment.
[P1] Grace retention bypasses per-call skill visibility
Skills excluded from the current call’s retained set remain cached for six hours, while the default sandbox projection recursively includes the entire .skills-cache directory. A user may therefore read or execute skills hidden by the visibility filter. Please project only the current visible set or isolate the cache per user/session.
| private static CacheGuard guardFor(Path cacheRoot) { | ||
| return GUARDS.computeIfAbsent(cacheRoot.toAbsolutePath().normalize(), CacheGuard::new); | ||
| } |
There was a problem hiding this comment.
[P1] The guard key does not resolve the real filesystem path
toAbsolutePath().normalize() does not unify aliases such as symlinked paths or /tmp and /private/tmp. The same cache can therefore receive multiple CacheGuard instances. An overlapping JVM FileLock then falls back to unlocked staging, reopening the GC race. Please key guards by a canonical/real path and add an alias-path regression test.
27ca82b to
3fe739f
Compare
|
Rewritten. The previous three commits are replaced by a single one that follows the issue rather than the mechanism I had been building on top of it. What changed and why I had picked the grace-window option from the issue and then, under review, kept adding machinery on top of it — a TOCTOU fix, then a cross-process lock, then fail-closed handling for that lock. Each round closed the hole you pointed at and opened a new one somewhere else. All three of your P1 findings were correct, and so were the two you filed after them. Stepping back: the issue already offered the alternative, and it is the one that removes the whole class rather than defending against it —
Skills now stage under That deletes the entire locking layer: Your findings against the mechanism, and what happened to each:
The one I have not closed. You are right that Known limit, stated in the commit and the class Javadoc: the cache scope must be at least as fine as the visibility dimension. The default Tests are deterministic rather than probabilistic now — the key one ages a directory in one scope and asserts another scope's sweeps cannot touch it. Each fails against the unpatched stager. Full suite: 89/89 modules, 5983 tests, 0 failures. |
3fe739f to
ed82c51
Compare
Fixes agentscope-ai#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. Identities map to segments injectively: sanitising alone would let alice@corp.com and alice#corp.com share a subtree, so anything that is not already a distinct, filesystem-safe segment keeps a readable prefix and is disambiguated by a digest of the original. 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.
3 participants
Fixes #2787
AgentScope-Java Version
2.0.3-SNAPSHOT(branched frommain@4ba433a9). The issue reproduces unchanged onmain:MarketplaceStager.javahas not been touched sincev2.0.1.Description
Background
MarketplaceStagerrebuilds its retain-list from a single call's visible skills and then deletes every other directory under.skills-cache. That tree is shared — one stager instance serves every concurrent call against a workspace root, and other replicas write into it over a shared volume. With per-user skill visibility, call B's "orphan" is call A's livefilesRoot, so B removes a directory A just staged, often while A is still walking it.Two defects combine, exactly as described in #2787:
The exception escapes.
Files.walkwraps mid-iteration IO errors inUncheckedIOException, which is not anIOException.garbageCollectOrphans/deleteRecursively/removeUnexpectedonly caughtIOException. The GC call also sits outside the per-skilltry/catchinstage(), so the failure propagates throughHarnessSkillMiddleware.onSystemPrompt→ReActAgent.seedSystemMsgand aborts the call before the first model round — matching the reportedtotalEvents=1, ~3 s signature.The deletion itself is wrong. Even when no exception is thrown, one call silently reclaims a directory whose absolute path another call has already handed to the model in its system prompt. That failure mode is quieter and arguably worse: the skill simply stops working, with nothing in the logs.
Changes
agentscope-harness/.../skill/runtime/MarketplaceStager.java:deleteRecursivelynow usesFiles.walkFileTree, so entries that vanish mid-traversal are reported throughvisitFileFailedinstead of aborting the whole walk.removeUnexpectedandgarbageCollectOrphansalso catchUncheckedIOException, and individual deletes are best-effort.stage()guards the GC call. Cache hygiene must never fail an agent call — this mirrors the fallback already applied per skill.DEFAULT_ORPHAN_GRACE(6 h), and every retained directory is touched on each pass.On the design-level suggestion in the issue — namespacing
.skills-cacheperIsolationScopewas considered and not taken, for three reasons: it does not help when the same user reaches two replicas over a shared volume;.skills-cacheis inDEFAULT_WORKSPACE_PROJECTION_ROOTS, so a per-user tree would project other users' skills into a sandbox; and it duplicates identical content per user.mtimeis the only liveness signal every replica sharing the volume can observe, so the grace window covers cross-process races too. The window has to outlast the longest call that could still shell out to a staged script; leaving a stale directory costs a few KB, deleting a live one breaks someone's call, so the default errs long. AMarketplaceStager(Path, Duration)overload makes it configurable, andinvalidateAll()remains available for an immediate purge.Public API is additive only: one constructor overload and one constant.
How this was tested
Two new test classes, both of which fail against the unpatched stager (verified by swapping the old implementation back in and re-running — 6 of the 7 relevant cases go red, the
MarketplaceStagerExecBitTestcases stay green):MarketplaceStagerOrphanGcTest(4 cases)Duration.ZEROgrace to maximise pressure)SharedWorkspaceSkillStagingE2ETest(3 cases, real middleware → stager → disk path)HarnessAgents sharing one workspace root, 50 concurrent rounds each<files-root>it advertises is actually on diskFull suite:
mvn test— 89/89 modules SUCCESS, 5945 tests, 0 failures.mvn spotless:applyclean.Checklist
mvn spotless:applymvn test)