[SparshM8]

Summary

Addresses race conditions, redundant downloads, and teardown interference in setup_skills and EnvironmentWorker when multiple sessions share a working directory (#1871).

Key Changes

1. Idempotent Downloads: Checks for an on-disk .version file in {skills_root}/<dirname>/. If the directory exists and matches the resolved version.id, download and extraction are skipped.
2. Atomic & Cross-Process Locking: Wraps both setup and cleanup in _skill_lock, combining in-process _ASYNC_LOCKS with cross-process OS file locking (fcntl on POSIX, msvcrt on Windows). Concurrent sessions starting simultaneously synchronize rather than racing into duplicate downloads or interleaving teardowns.
3. On-Disk Session Leases: Tracks active session references in <skill_dir>/.leases/<context_id>. _cleanup_skills unlinks the exiting session's lease and only removes the directory if no other active sessions remain, preventing in-flight sessions from having their files deleted.
4. keep_skills Opt-Out Flag: Added keep_skills: bool = False to AgentToolContext, EnvironmentWorker, and client.beta.environments.work.worker(). When enabled, cleanup is a no-op, allowing the workdir to function as a persistent worker-level cache across sessions.
5. Comprehensive Tests: Added unit tests covering idempotency, concurrent first setups, session lease preservation, setup/cleanup race interleaving, cross-process OS file locks, and EnvironmentWorker propagation.

Fixes #1871

[johanferguth]

Thanks for picking this up, the redundant download is a real cost and the .version check removes it nicely. I tried the branch locally (7c4c24a, SDK 1.0.0) and wanted to share a few observations in case they help.

What works: two sequential setups on the same workdir now download once instead of twice.

What I think is still open:

1. With the default keep_skills=False, the first session to exit still removes the skill directory the other session is using: the skipped directory is added to the second context's _skill_dirs, and the context that downloaded it removes it on exit. In my test the second session's SKILL.md was gone after the first one exited, in either exit order. The description says "the skills remain intact", which I could only reproduce with keep_skills=True.
2. keep_skills doesn't seem reachable from EnvironmentWorker: _worker.py builds AgentToolContext(workdir=..., max_file_bytes=..., client=..., session=...) without it, so the scenario in #1871 can't opt in.
3. The .version check and write aren't under a lock, so two sessions starting together on an empty workdir still both rmtree and extract. A version change while another session uses the skill also still replaces its files.
4. The branch is based on 1.0.0. On current main (2.6.0), _resolve_skill_version and version_id are gone (version.id is used instead), so it will need a rebase. A test next to tests/lib/tools/test_skills.py would make the behaviour easier to keep.

Since the concurrency in #1871 happens across worker processes sharing a workdir, an in-memory fix may not be enough; a file lock with on-disk session leases might be the more robust route. Happy to help test either way. The repro script I used is below (no network, fakes for the version lookup and the download).

Repro script

"""Check PR #1872 (anthropic-sdk-python): does a shared skill dir survive the first session's exit?

No network, no API key: the version lookup and the archive download are faked; the real
download_session_skills() and AgentToolContext._cleanup_skills() from the PR branch are used.
"""
import asyncio
import json
import tempfile
from pathlib import Path
from types import SimpleNamespace

from anthropic.lib.tools import _skills
from anthropic.lib.tools.agent_toolset import AgentToolContext

DOWNLOADS: list[str] = []


async def fake_resolve(client, skill_id, version):
    return "skillver_1"


async def fake_download(client, skill_id, version_id, dest):
    DOWNLOADS.append(f"{skill_id}@{version_id}")
    await asyncio.sleep(0.05)  # leave room for a concurrent session
    dest.mkdir(parents=True, exist_ok=True)
    (dest / "SKILL.md").write_text("# skill\n")


async def fake_retrieve(version_id, *, skill_id):
    return SimpleNamespace(name="my-skill")


_skills._resolve_skill_version = fake_resolve
_skills._download_and_extract = fake_download
CLIENT = SimpleNamespace(beta=SimpleNamespace(skills=SimpleNamespace(versions=SimpleNamespace(retrieve=fake_retrieve))))
SESSION = SimpleNamespace(agent=SimpleNamespace(skills=[SimpleNamespace(skill_id="skill_01", version="latest")]))


async def setup(workdir):
    return await _skills.download_session_skills(CLIENT, workdir=workdir, session=SESSION)


async def cleanup(dirs, keep_skills):
    # Real _cleanup_skills, called on a stand-in that only carries the two attributes it reads.
    await AgentToolContext._cleanup_skills(SimpleNamespace(keep_skills=keep_skills, _skill_dirs=list(dirs)))


async def sequential(keep_skills):
    DOWNLOADS.clear()
    with tempfile.TemporaryDirectory() as wd:
        a = await setup(wd)  # session A enters
        b = await setup(wd)  # session B enters while A runs
        skill_md = Path(wd) / "skills" / "my-skill" / "SKILL.md"
        await cleanup(a, keep_skills)  # session A exits first, B still running
        return {
            "keep_skills": keep_skills,
            "downloads": len(DOWNLOADS),
            "b_owns_same_dir_as_a": b == a,
            "b_skill_exists_after_a_exit": skill_md.exists(),
        }


async def concurrent_first_setup():
    DOWNLOADS.clear()
    with tempfile.TemporaryDirectory() as wd:
        await asyncio.gather(setup(wd), setup(wd))  # A and B start at the same time, empty workdir
        return {"downloads": len(DOWNLOADS)}


async def main():
    print(json.dumps({
        "default_keep_skills_False": await sequential(False),
        "opt_in_keep_skills_True": await sequential(True),
        "concurrent_first_setup": await concurrent_first_setup(),
    }, indent=2))


asyncio.run(main())

Output on 7c4c24a (3 identical runs):

default keep_skills=False: downloads=1, second session's skill exists after first exits: False
keep_skills=True:          downloads=1, second session's skill exists after first exits: True
concurrent first setup:    downloads=2

Drafted with Claude Code, verified locally.

[SparshM8]

Hi @johanferguth,

Thank you for the thorough feedback and for providing the reproduction script!

I’ll check the issues you pointed out—especially around the cleanup behavior with keep_skills=False, concurrency handling across worker processes, and the rebase on latest main.

Once I make the necessary corrections, I’ll push an updated commit to the branch. Thanks again!

[chrikrah]

@SparshM8 a heads-up for the rework you mentioned: the merge in 5048a8c took main's side of _skills.py, so the diff is now only the keep_skills flag and download_session_skills downloads again on every setup. Flagging it in case that was not intended.

$ git diff 4c7b598 5048a8c --stat     # 4c7b598 = main, the merge base
 src/anthropic/lib/tools/agent_toolset.py | 6 ++++++
 1 file changed, 6 insertions(+)

# probe1872.py: download_session_skills twice on one workdir; retrieve and _download_and_extract faked, no network
$ python -I probe1872.py
# 7c4c24a, your first commit
downloads: 1
# 5048a8c, head
downloads: 2

Could the next push bring the _skills.py change back on top of main, with the suggested test asserting a single download? That test would have caught this merge.

[chrikrah]

@SparshM8 1851e00 downloads each skill once again, and the three new tests fail on main's _skills.py. One race is still open, so I'd hold the merge for it.

blocking: _cleanup_skills reads .leases and calls rmtree without _skill_lock. A context that exits while another one sets up on the same workdir can delete the directory that setup just returned as ready:

# probe1872_race.py: ctx1 and ctx2 share a workdir; ctx1._cleanup_skills() runs while ctx2.setup_skills() starts 0.1 s later; rmtree slowed by 0.3 s to widen the window
$ uv run python -I probe1872_race.py      # 1851e00, Python 3.10, anthropic imported from the export
ctx2 setup returned ['skills/pdf'] | SKILL.md exists: False | downloads: 1
# same, with the loop body of _cleanup_skills inside `async with _skill_lock(skill_dir):`
ctx2 setup returned ['skills/pdf'] | SKILL.md exists: True | downloads: 2
$ uv run pytest tests/lib/tools/test_skills.py tests/lib/environments/test_worker.py -q -n 0
64 passed            # head, and the same count with the lock added
# _skills.py from 4c7b598, head tests
3 failed, 21 passed  # idempotent, concurrent_first_setup, cleanup_respects_leases_and_keep_skills

Taking the lock in _cleanup_skills is the fix I'd make, plus a test for this interleaving. Can you add both in the next push?

[johanferguth]

Hi @SparshM8, thank you for the quick follow-up and for the work on this. 1851e00 addresses the four points from my earlier review: keep_skills is now reachable from EnvironmentWorker, the _skills.py change is back on top of main, tests were added, and the per-context leases cover the case where the first session exits while the second is still running. I re-ran my repro, this time with real AgentToolContext objects, and it confirms it (output below). That's a clear improvement.

While reading the diff, I noticed a few things you may want to look at. I may well be missing context, so please take them as suggestions:

1. Cleanup vs. setup. _cleanup_skills checks .leases and calls rmtree outside _skill_lock, while setup registers its lease under it. If B's setup lands between A's lease check and A's rmtree, B skips the download and then loses its directory, even within a single process (the scenario in setup_skills re-downloads and rmtrees skills on every session, racing when workers share a workdir #1871). The repro shows it when rmtree is artificially slowed down to widen the window, so it says nothing about how often it happens, only that the window exists. Would it make sense to take _skill_lock(skill_dir) around unlink, check and rmtree? The lock file lives in the parent directory, so it should survive the rmtree. Edit: I see @chrikrah already flagged this as blocking in their review, with a tested fix; I missed it before posting. +1, my repro agrees.
2. Version change. If the version differs, the code still goes through rmtree and a new download without looking at .leases. This may be fine as a known limitation, in which case a short note in the docstring could help. A versioned directory would be another option.
3. Tests. The new tests run in a single event loop, so _ASYNC_LOCKS serializes the callers before they reach the file lock. Would a test that bypasses it, or a subprocess-based one, be feasible?
4. Fallback in _cleanup_skills. The remaining[0].unlink() branch seems reachable only without _context_id, and it would remove another context's lease. Is it needed?
5. Description. A small one: when you have a moment, it might be nice to mention the file lock and the leases in the PR description, since it currently describes the first version. No rush at all.

One correction on my side: in my first review I described #1871 as a multi-process case, but the issue describes one worker process serving several sessions. My first repro also used a stand-in object without _context_id, so on this commit it would have gone through the fallback from point 4; that is why I switched to real AgentToolContext objects.

Repro script and output (1851e00, no network: version lookup and archive download faked)

"""PR #1872 repro with real AgentToolContext objects. Network faked: version lookup + archive download."""
import sys, asyncio, json, tempfile, shutil, time
sys.path.insert(0, sys.argv[1] + "/src")
from pathlib import Path
from types import SimpleNamespace
from anthropic.lib.tools import _skills
from anthropic.lib.tools.agent_toolset import AgentToolContext

DL = []
async def fake_dl(client, skill_id, version_id, dest):
    DL.append(version_id); await asyncio.sleep(0.05)
    dest.mkdir(parents=True, exist_ok=True); (dest / "SKILL.md").write_text("# s\n")
_skills._download_and_extract = fake_dl

async def retrieve(v, *, skill_id): return SimpleNamespace(name="my-skill", id="skillver_1")
CLIENT = SimpleNamespace(beta=SimpleNamespace(skills=SimpleNamespace(versions=SimpleNamespace(retrieve=retrieve))))
def sess(i): return SimpleNamespace(id=i, agent=SimpleNamespace(skills=[SimpleNamespace(skill_id="skill_01", version="latest")]))
def ctx(wd, i, keep=False): return AgentToolContext(workdir=wd, client=CLIENT, session=sess(i), keep_skills=keep)

async def sequential(keep):
    DL.clear()
    with tempfile.TemporaryDirectory() as wd:
        a, b = ctx(wd, "a", keep), ctx(wd, "b", keep)
        await a.setup_skills(); await b.setup_skills()
        md = Path(wd, "skills", "my-skill", "SKILL.md")
        await a._cleanup_skills()
        after_a = md.exists()
        await b._cleanup_skills()
        return {"keep_skills": keep, "downloads": len(DL), "b_skill_exists_after_a_exit": after_a,
                "dir_exists_after_both_exit": Path(wd, "skills", "my-skill").exists()}

async def concurrent_first():
    DL.clear()
    with tempfile.TemporaryDirectory() as wd:
        await asyncio.gather(ctx(wd, "a").setup_skills(), ctx(wd, "b").setup_skills())
        return {"downloads": len(DL)}

async def cleanup_vs_setup_race():
    DL.clear()
    real = shutil.rmtree
    def slow(*a, **k): time.sleep(0.3); return real(*a, **k)
    with tempfile.TemporaryDirectory() as wd:
        a, b = ctx(wd, "a"), ctx(wd, "b")
        await a.setup_skills()
        md = Path(wd, "skills", "my-skill", "SKILL.md")
        shutil.rmtree = slow
        try:
            async def b_setup():
                await asyncio.sleep(0.1); await b.setup_skills()
            await asyncio.gather(a._cleanup_skills(), b_setup())
        finally:
            shutil.rmtree = real
        return {"downloads": len(DL), "b_got_dir": bool(b._skill_dirs), "b_skill_exists_after": md.exists()}

async def main():
    print(json.dumps({"seq_keep_False": await sequential(False), "seq_keep_True": await sequential(True),
        "concurrent_first_setup": await concurrent_first(),
        "cleanup_vs_setup_race (rmtree slowed 0.3s to widen window)": await cleanup_vs_setup_race()}, indent=2))
asyncio.run(main())

Output (3 identical runs, Python 3.12):

seq keep_skills=False: downloads=1, B's skill exists after A exits: True, dir removed after both exit: True
seq keep_skills=True:  downloads=1, B's skill exists after A exits: True, dir kept after both exit: True
concurrent first setup: downloads=1
cleanup vs. setup race (rmtree slowed 0.3 s): B got the dir: True, B's skill exists afterwards: False

Thanks again, and happy to help test the next push.

Drafted with Claude Code, verified locally.

[SparshM8]

Hi @chrikrah, @johanferguth,

Thank you both for the keen observations and for catching that setup/cleanup interleaving race window!

I've pushed commit d9bea09e, addressing all points:

1. Locked Cleanup (_cleanup_skills):

   • Wrapped the lease unlink, check, and rmtree inside async with _skill_lock(skill_dir):.
   • Verified with the interleaving test: when Context 1's cleanup runs with a delayed rmtree while Context 2 starts setup, Context 2 now waits on _skill_lock, resulting in SKILL.md exists: True and a safe re-download (downloads: 2).

2. Removed Fallback Unlink:

   • Dropped the remaining[0].unlink() fallback from _cleanup_skills so a context never unlinks another context's lease.

3. Subprocess File Lock Test:

   • Added test_cross_process_file_lock in tests/lib/tools/test_skills.py verifying mutual exclusion across independent OS processes.

4. Interleaving Race Test:

   • Added test_cleanup_and_setup_interleaving_race in tests/lib/tools/test_skills.py using monkeypatched delayed rmtree to lock down the scenario.

5. Docstring Note & PR Description:

   • Added a note to download_session_skills docstring regarding version changes under concurrency.
   • Updated the PR description with the full overview of file locking and session leases.

All tests in test_skills.py and test_worker.py pass cleanly. Ready for your review!

[johanferguth]

Thanks @SparshM8, I re-ran my repro against d9bea09 (3 runs): the cleanup/setup interleaving is fixed — B keeps its skill, at the cost of a second download — and the other cases are unchanged (one download, leases respected, keep_skills=True keeps the directory). tests/lib/tools/test_skills.py passes (26 tests), and the new interleaving test fails on 1851e00, so it does catch the regression.

One tiny cosmetic thing: in the PR description, .version / version.id shows as ersion.id and fcntl as cntl (probably \v and \f eaten by the shell). Nothing else from my side, thanks for the quick turnaround!

[SparshM8]

Hi @johanferguth,

Thank you for running the repro and verifying the fix!

I've fixed the typo/escape character issue in the PR description so .version, version.id, and fcntl render properly now.

Thanks again for the great review and collaboration throughout this PR!