`setup_skills` re-downloads and `rmtree`s skills on every session, racing when workers share a workdir

[ogiogi93]

Summary

Self-hosted workers that serve multiple sessions concurrently from one workdir (max_concurrent_sessions > 1) hit two problems:

1. download_session_skills unconditionally rmtrees and re-extracts each skill directory even when the resolved version_id is unchanged, so every new session re-downloads the same archives.
2. AgentToolContext._cleanup_skills removes those directories when any one session ends.

Together these mean a session in flight can have its skill files deleted or replaced underneath it by another session starting or finishing. The window is small but real, and the redundant downloads are pure overhead on a long-running worker.

Version

anthropic==1.0.0, Python 3.12.

Repro

1. Run one EnvironmentWorker process serving two sessions concurrently with the same workdir (both agents attach the same skill).
2. Session B starts while session A is running: the skill directory A is using is rmtree'd and re-extracted.
3. Session A ends while B is running: _cleanup_skills removes the directory B is using.

Workaround in use

We monkey-patch AgentToolContext.setup_skills with a process-wide cache (a skill_id -> version_id map behind an asyncio.Lock, downloading only when the resolved version differs) and neutralise the teardown by leaving _skill_dirs empty. This works but pins us to internals (_skills._resolve_skill_version, _skills._download_and_extract).

Request

Either of these would let us drop the patch:

• Skip the download when the resolved version_id matches what is already on disk (an idempotent setup_skills), and
• an opt-out for the teardown rmtree — e.g. AgentToolContext(keep_skills=True) — so a shared skills tree can be treated as a process-wide cache owned by the worker rather than by an individual session.

Happy to open a PR if you have a preference on the shape.

[hipvlady]

The two requests refer to the same issue: setup_skills alters a common path when a session starts and _cleanup_skills releases it on the first session exit, which means that the skills tree has a lifetime that is scoped to the process but lifecycle hooks that are scoped to the session. This arrangement is safe provided that sessions do not overlap, since the assumption that max_concurrent_sessions > 1 contradicts this.

Another shape is worth looking at, as you did ask regarding the PR. Instead of using skill_id , key the extraction path by (skill_id, version_id). Since a new version is extracted into a new directory, no modifications are made in place. setup_skills becomes idempotent by design rather than through an equality check, and the teardown race is eliminated because the directory of no active session is ever the one that is the target of an rmtree. Cleanup then becomes a question of garbage collection, involving either refcounting or sweeping the unused version directories at the time the worker shuts down, rather than carrying out a teardown for each session.

As for the workaround, asyncio.Lock serializes coroutines within a single interpreter, which deals with the repro case as described. It doesn't apply to two worker processes that are both accessing the same workdir. Each process maintains its own lock, neither of them sees the other's extract as it's taking place, and the same situation where a rmtree is carried out under a live reader reoccurs. If that path is ever served by more than one worker, then the exclusion must be decided by the filesystem rather than by the event loop. With content-addressed paths you achieve the required result without needing a lock at all, because there is no longer any shared mutable path that can cause a race condition.