diff --git a/loopx/workflow_skill_install.py b/loopx/workflow_skill_install.py index 7adcf01c4..38ddc3754 100644 --- a/loopx/workflow_skill_install.py +++ b/loopx/workflow_skill_install.py @@ -10,6 +10,7 @@ import tempfile from typing import Any, Iterator, Mapping +from .file_lock import exclusive_file_lock from .skill_install_readback import ( ARK_MANAGED_AGENT_REQUIRED_SKILL_IDS, PACKAGED_HOST_SKILL_IDS, @@ -35,6 +36,9 @@ _PACKAGED_SKILL_SENTINEL = PurePosixPath( "share/loopx/skills/loopx-project/SKILL.md" ) +# `exclusive_file_lock` appends `.lock`, so the sibling it guards keeps the +# lock file at the path installs have always used. +_INSTALL_LOCK_STEM = ".loopx-workflow-skills" def _valid_source_root(path: Path) -> bool: @@ -100,16 +104,12 @@ def default_workflow_skills_dir(env: Mapping[str, str] | None = None) -> Path: @contextmanager def _exclusive_install_lock(skills_dir: Path) -> Iterator[None]: - import fcntl - skills_dir.mkdir(parents=True, exist_ok=True) - lock_path = skills_dir / ".loopx-workflow-skills.lock" - with lock_path.open("a+", encoding="utf-8") as handle: - fcntl.flock(handle.fileno(), fcntl.LOCK_EX) - try: - yield - finally: - fcntl.flock(handle.fileno(), fcntl.LOCK_UN) + with exclusive_file_lock( + skills_dir / _INSTALL_LOCK_STEM, + operation="workflow_skill_install", + ): + yield def _install_one_skill(source: Path, target: Path) -> str: diff --git a/tests/test_workflow_skill_install.py b/tests/test_workflow_skill_install.py index f56bc0fc4..8a4b1d631 100644 --- a/tests/test_workflow_skill_install.py +++ b/tests/test_workflow_skill_install.py @@ -1,8 +1,14 @@ from __future__ import annotations +from contextlib import contextmanager import json from pathlib import Path +from typing import Any, Iterator +import pytest + +from loopx import file_lock +from loopx import workflow_skill_install as install_module from loopx.skill_install_readback import ( ARK_MANAGED_AGENT_REQUIRED_SKILL_IDS, PACKAGED_HOST_SKILL_IDS, @@ -84,6 +90,67 @@ def test_uninstall_preserves_locally_modified_skill(tmp_path: Path) -> None: assert (skills_dir / SKILL_INSTALL_READBACK_FILENAME).is_file() +class _ByteRangeLockBackend: + """Stand-in for ``msvcrt`` so the Windows lock branch runs on any host.""" + + LK_NBLCK = 1 + LK_UNLCK = 0 + + def __init__(self) -> None: + self.modes: list[int] = [] + + def locking(self, file_descriptor: int, mode: int, length: int) -> None: + self.modes.append(mode) + + +def test_install_takes_the_windows_lock_branch_without_fcntl( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + backend = _ByteRangeLockBackend() + monkeypatch.setattr(file_lock, "fcntl", None) + monkeypatch.setattr(file_lock, "msvcrt", backend) + skills_dir = tmp_path / "skills" + + installed = workflow_skill_install(skills_dir=skills_dir, execute=True) + + assert installed["ok"] is True + assert installed["after"]["ready"] is True + assert backend.modes == [backend.LK_NBLCK, backend.LK_UNLCK] + + +def test_install_serializes_on_the_shared_workflow_skill_lock( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + skills_dir = tmp_path / "skills" + held: list[Path] = [] + locked: list[Path] = [] + real_readback = install_module.write_skill_install_readback + + @contextmanager + def recording_lock(path: Path, **kwargs: Any) -> Iterator[Path]: + locked.append(path) + held.append(path) + try: + yield path + finally: + held.pop() + + def recording_readback(**kwargs: Any) -> Any: + assert held, "the install readback was written outside the lock" + return real_readback(**kwargs) + + monkeypatch.setattr(install_module, "exclusive_file_lock", recording_lock) + monkeypatch.setattr(install_module, "write_skill_install_readback", recording_readback) + + installed = workflow_skill_install(skills_dir=skills_dir, execute=True) + + assert installed["ok"] is True + assert locked == [skills_dir / ".loopx-workflow-skills"] + assert held == [] + + def test_inspect_does_not_create_target(tmp_path: Path) -> None: skills_dir = tmp_path / "skills"