diff --git a/roboco/services/workspace.py b/roboco/services/workspace.py index c6168365..9175c55a 100644 --- a/roboco/services/workspace.py +++ b/roboco/services/workspace.py @@ -1293,6 +1293,15 @@ class WorkspaceService: or pushed. Returns False (don't originate) on a null command or any probe/command error — fail-safe — and logs loudly. The throwaway is always removed. + + The local ``git clone --local`` from the read clone runs UNDER the + project's read-clone lock (the same lock ``ensure_read_clone`` syncs + under) so a concurrent ``ensure_read_clone`` → ``_sync_read_clone`` + (fetch + hard-reset to origin's default branch) cannot mutate the read + clone mid-clone. The lock is released before the upgrade runs: the + upgrade operates on the independent local copy and never touches the + read clone, so holding the lock past the clone would only block + conventions reads for the upgrade duration. """ command = str(getattr(project, "dep_update_command", None) or "").strip() if not command: @@ -1312,8 +1321,15 @@ class WorkspaceService: ) tmp = Path(tempfile.mkdtemp(prefix="dep-probe-")) try: + clone_dir = tmp / "repo" + timeout = settings.workspace_dep_install_timeout_seconds + lock = _ensure_lock_for(slug, "_meta-conventions") + async with lock: + await asyncio.to_thread( + self._clone_local_into, read_clone, clone_dir, timeout + ) return await asyncio.to_thread( - self._probe_lockfile_change, read_clone, tmp, command, lock_paths + self._probe_lockfile_on_clone, clone_dir, command, lock_paths ) except Exception as exc: logger.warning( @@ -1326,17 +1342,14 @@ class WorkspaceService: shutil.rmtree(tmp, ignore_errors=True) @staticmethod - def _probe_lockfile_change( - read_clone: Path, tmp: Path, command: str, lock_paths: list[str] - ) -> bool: - """Sync core of the dep-update probe (run in a thread). True if dirty. + def _clone_local_into(read_clone: Path, clone_dir: Path, timeout: float) -> None: + """Local clone of the read clone into ``clone_dir`` (run in a thread). - Isolated local clone (``--no-hardlinks``) so the read clone is never - touched; runs the upgrade with no shell (``shlex.split``); a non-zero - upgrade yields False (fail-safe, don't originate on a broken probe). + ``--no-hardlinks`` forces a full object copy so the clone is an + independent repo that can be mutated (the upgrade) without touching the + read clone. Caller holds the read-clone lock so ``_sync_read_clone`` + cannot mutate the source mid-clone. """ - clone_dir = tmp / "repo" - timeout = settings.workspace_dep_install_timeout_seconds subprocess.run( [ "git", @@ -1351,6 +1364,19 @@ class WorkspaceService: timeout=timeout, check=True, ) + + @staticmethod + def _probe_lockfile_on_clone( + clone_dir: Path, command: str, lock_paths: list[str] + ) -> bool: + """Run the upgrade on the independent local clone + report dirty (in a thread). + + Runs the upgrade with no shell (``shlex.split``); a non-zero upgrade + yields False (fail-safe, don't originate on a broken probe). The lock is + NOT held here — the clone is a full independent copy and the upgrade + never touches the read clone. + """ + timeout = settings.workspace_dep_install_timeout_seconds upgrade = subprocess.run( shlex.split(command), cwd=str(clone_dir), diff --git a/tests/integration/services/test_dep_update_probe.py b/tests/integration/services/test_dep_update_probe.py index 2ae2007f..657435d9 100644 --- a/tests/integration/services/test_dep_update_probe.py +++ b/tests/integration/services/test_dep_update_probe.py @@ -7,12 +7,14 @@ or committing/pushing. Fail-safe: a null/failing command returns False. from __future__ import annotations +import asyncio import subprocess +import time from typing import TYPE_CHECKING, Any from unittest.mock import AsyncMock, MagicMock import pytest -from roboco.services.workspace import WorkspaceService +from roboco.services.workspace import WorkspaceService, _ensure_lock_for if TYPE_CHECKING: from pathlib import Path @@ -94,3 +96,59 @@ async def test_explicit_dep_update_paths_scope(tmp_path: Path) -> None: cmd = "python3 -c \"open('uv.lock','a').write('x')\"" project = _project(cmd, paths=["pnpm-lock.yaml"]) assert await svc.dry_upgrade_changes_lockfile(project) is False + + +@pytest.mark.asyncio +async def test_probe_holds_read_clone_lock_across_local_clone( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """F116: the dep-update probe must hold the read-clone lock for the + duration of the local ``git clone --local`` from the read clone, so a + concurrent ``ensure_read_clone`` → ``_sync_read_clone`` (fetch + hard-reset + to origin's default branch) cannot mutate the read clone mid-clone. The + lock is released before the upgrade runs on the independent copy (the + upgrade never touches the read clone, so holding the lock past the clone + would needlessly block conventions reads for the upgrade duration).""" + read_clone = _make_read_clone(tmp_path) + svc = _svc(read_clone) + # Unique slug → a fresh lock not shared with any other test. + project = MagicMock( + slug="f116-probe", dep_update_command="python3 -c pass", dep_update_paths=None + ) + lock = _ensure_lock_for("f116-probe", "_meta-conventions") + assert not lock.locked() + + def slow_clone(read_clone: Path, clone_dir: Path, timeout: float) -> None: + # Real local clone so the dir is valid, then hold the lock a while so + # the test coroutine can observe the lock is held mid-clone. + subprocess.run( + [ + "git", + "clone", + "--local", + "--no-hardlinks", + str(read_clone), + str(clone_dir), + ], + capture_output=True, + text=True, + timeout=timeout, + check=True, + ) + time.sleep(0.4) + + def noop_probe(_clone_dir: Path, _command: str, _lock_paths: list[str]) -> bool: + return False + + monkeypatch.setattr(WorkspaceService, "_clone_local_into", staticmethod(slow_clone)) + monkeypatch.setattr( + WorkspaceService, "_probe_lockfile_on_clone", staticmethod(noop_probe) + ) + + probe_task = asyncio.create_task(svc.dry_upgrade_changes_lockfile(project)) + # ensure_read_clone is mocked (instant) → the probe immediately enters the + # lock + clone step. Give it a beat, then assert the lock is held. + await asyncio.sleep(0.1) + assert lock.locked(), "read-clone lock must be held during the local-clone step" + await asyncio.wait_for(probe_task, timeout=3) + assert not lock.locked(), "lock released after the clone step (upgrade needs none)"