[F116] hold the read-clone lock across the dep-probe local clone

dry_upgrade_changes_lockfile called ensure_read_clone (which syncs the
read clone under the _meta-conventions lock then releases it) and ran
'git clone --local --no-hardlinks <read_clone>' OUTSIDE the lock. A
concurrent ensure_read_clone -> _sync_read_clone (fetch + hard-reset to
origin's default branch) could mutate the read clone's working tree /
object db mid-clone, racing the clone and producing an inconsistent or
failing probe.

Split _probe_lockfile_change into _clone_local_into (the local clone,
run under the read-clone lock) + _probe_lockfile_on_clone (the upgrade +
git status, run without the lock on the now-independent copy). The probe
acquires _ensure_lock_for(slug, '_meta-conventions') — the same lock
ensure_read_clone syncs under — and holds it only for the clone step; the
upgrade operates on the full --no-hardlinks copy and never touches the
read clone, so the lock is released before it to avoid blocking
conventions reads for the upgrade duration.

The tiny gap between ensure_read_clone releasing the lock and the probe
re-acquiring it is safe: any concurrent _sync_read_clone completes under
the lock before the probe acquires, so the clone reads a stable state.
This commit is contained in:
Renn F
2026-06-28 23:03:43 +02:00
parent 47bb5403b1
commit 1a773e45d7
2 changed files with 95 additions and 11 deletions
+36 -10
View File
@@ -1293,6 +1293,15 @@ class WorkspaceService:
or pushed. Returns False (don't originate) on a null command or any or pushed. Returns False (don't originate) on a null command or any
probe/command error — fail-safe — and logs loudly. The throwaway is probe/command error — fail-safe — and logs loudly. The throwaway is
always removed. 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() command = str(getattr(project, "dep_update_command", None) or "").strip()
if not command: if not command:
@@ -1312,8 +1321,15 @@ class WorkspaceService:
) )
tmp = Path(tempfile.mkdtemp(prefix="dep-probe-")) tmp = Path(tempfile.mkdtemp(prefix="dep-probe-"))
try: 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( 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: except Exception as exc:
logger.warning( logger.warning(
@@ -1326,17 +1342,14 @@ class WorkspaceService:
shutil.rmtree(tmp, ignore_errors=True) shutil.rmtree(tmp, ignore_errors=True)
@staticmethod @staticmethod
def _probe_lockfile_change( def _clone_local_into(read_clone: Path, clone_dir: Path, timeout: float) -> None:
read_clone: Path, tmp: Path, command: str, lock_paths: list[str] """Local clone of the read clone into ``clone_dir`` (run in a thread).
) -> bool:
"""Sync core of the dep-update probe (run in a thread). True if dirty.
Isolated local clone (``--no-hardlinks``) so the read clone is never ``--no-hardlinks`` forces a full object copy so the clone is an
touched; runs the upgrade with no shell (``shlex.split``); a non-zero independent repo that can be mutated (the upgrade) without touching the
upgrade yields False (fail-safe, don't originate on a broken probe). 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( subprocess.run(
[ [
"git", "git",
@@ -1351,6 +1364,19 @@ class WorkspaceService:
timeout=timeout, timeout=timeout,
check=True, 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( upgrade = subprocess.run(
shlex.split(command), shlex.split(command),
cwd=str(clone_dir), cwd=str(clone_dir),
@@ -7,12 +7,14 @@ or committing/pushing. Fail-safe: a null/failing command returns False.
from __future__ import annotations from __future__ import annotations
import asyncio
import subprocess import subprocess
import time
from typing import TYPE_CHECKING, Any from typing import TYPE_CHECKING, Any
from unittest.mock import AsyncMock, MagicMock from unittest.mock import AsyncMock, MagicMock
import pytest import pytest
from roboco.services.workspace import WorkspaceService from roboco.services.workspace import WorkspaceService, _ensure_lock_for
if TYPE_CHECKING: if TYPE_CHECKING:
from pathlib import Path 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')\"" cmd = "python3 -c \"open('uv.lock','a').write('x')\""
project = _project(cmd, paths=["pnpm-lock.yaml"]) project = _project(cmd, paths=["pnpm-lock.yaml"])
assert await svc.dry_upgrade_changes_lockfile(project) is False 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)"