diff --git a/python/robomp/src/sandbox.py b/python/robomp/src/sandbox.py index 61670dc25..bf7a8b565 100644 --- a/python/robomp/src/sandbox.py +++ b/python/robomp/src/sandbox.py @@ -354,6 +354,30 @@ def _run( return proc +def _worktree_add(add_cmd: list[str], *, pool: Path, repo_dir: Path) -> None: + """Run `git worktree add`, cleaning partial state on failure. + + A worktree-add killed mid-operation (the 120s `_run` timeout surfaces as + GitCommandError 124, or any nonzero git failure) can leave a partial + checkout at `repo_dir` and/or a dangling pool worktree registration. Left + behind, the event retry hits stale metadata and fails again on the same + path. Best-effort remove the checkout and prune the pool, then re-raise so the + retry starts from a clean path. If the prune itself fails (incl. a 124 + timeout), raise that instead — chained from the add error — since a + dangling registration left behind is exactly what poisons the retry. + """ + try: + _run(add_cmd, cwd=pool) + except GitCommandError as add_err: + shutil.rmtree(repo_dir, ignore_errors=True) + pruned = _safe_run(["git", "worktree", "prune"], cwd=pool) + if pruned.returncode != 0: + raise GitCommandError( + ["git", "worktree", "prune"], pruned.returncode, pruned.stdout, pruned.stderr + ) from add_err + raise + + _SHARED_OMP_GID = 2000 @@ -710,10 +734,17 @@ class SandboxManager: def _reset_origin_url(repo_dir: Path, clone_url: str) -> None: """`git remote set-url origin ` if origin exists and differs. - Best-effort: silent no-op on failure (probe `get-url` first so we don't - spam logs on first-time clones where origin isn't configured yet). + Best-effort: silent no-op on a "no origin" failure (probe `get-url` + first so we don't spam logs on first-time clones). A timed-out probe + (124) is indeterminate and raises instead — see below. """ probe = _safe_run(["git", "remote", "get-url", "origin"], cwd=repo_dir) + if probe.returncode == 124: + # A timed-out probe is indeterminate: we cannot tell whether origin + # embeds a legacy credential that must be rewritten before fetch. + # Fail closed so the event retries rather than fetching against a + # possibly-credentialed origin. + raise GitCommandError(["git", "remote", "get-url", "origin"], probe.returncode, probe.stdout, probe.stderr) if probe.returncode != 0: return if probe.stdout.strip() == clone_url: @@ -778,7 +809,11 @@ class SandboxManager: if not repo_exists: if pr_head is not None: self.transport.fetch_pr_head(repo=repo, pool_dir=pool, pr_number=pr_head) - _run(["git", "worktree", "add", "--detach", str(repo_dir), "FETCH_HEAD"], cwd=pool) + _worktree_add( + ["git", "worktree", "add", "--detach", str(repo_dir), "FETCH_HEAD"], + pool=pool, + repo_dir=repo_dir, + ) else: # Make sure the requested start point exists locally (best-effort). # For follow-ups on an existing PR, `existing_branch` is the remote @@ -793,7 +828,11 @@ class SandboxManager: # start point; fail instead so the event retries. raise GitCommandError(probe, check.returncode, check.stdout, check.stderr) if check.returncode == 0: - _run(["git", "worktree", "add", str(repo_dir), branch], cwd=pool) + _worktree_add( + ["git", "worktree", "add", str(repo_dir), branch], + pool=pool, + repo_dir=repo_dir, + ) else: start_point = f"origin/{default_branch}" if existing_branch: @@ -805,17 +844,10 @@ class SandboxManager: raise GitCommandError(remote_probe, remote.returncode, remote.stdout, remote.stderr) if remote.returncode == 0: start_point = f"origin/{existing_branch}" - _run( - [ - "git", - "worktree", - "add", - "-b", - branch, - str(repo_dir), - start_point, - ], - cwd=pool, + _worktree_add( + ["git", "worktree", "add", "-b", branch, str(repo_dir), start_point], + pool=pool, + repo_dir=repo_dir, ) else: slot_git_env = _git_env_for_repo(repo_dir) @@ -956,19 +988,38 @@ class SandboxManager: with self._repo_lock(repo): ws_root = self.workspace_root(repo, number) repo_dir = ws_root / "repo" - if repo_dir.exists(): - pool = self.pool_path(repo) - removed = _safe_run(["git", "worktree", "remove", "--force", str(repo_dir)], cwd=pool) - if removed.returncode != 0: - # A failed `git worktree remove` (nonzero exit, incl. a 124 - # timeout) may have deleted the checkout but left the pool's - # worktree registration dangling, or vice versa. Delete any - # leftover checkout ourselves, then prune the dangling pool - # registration so a later `git worktree add` for the same path - # does not trip on stale metadata. `repo_dir.exists()` is not a - # reliable proxy: a killed remove can clear the checkout first. - shutil.rmtree(repo_dir, ignore_errors=True) - _safe_run(["git", "worktree", "prune"], cwd=pool) + pool = self.pool_path(repo) + # `repo_dir.exists()` is not a reliable proxy for "nothing to clean + # up in the pool": a worktree-add or a prior remove killed mid-flight + # can leave the checkout gone but the pool's worktree registration + # dangling, which fails the next `git worktree add` for this path. + # Only run git in a REAL pool clone: `ensure_clone` mkdir's the pool + # dir BEFORE cloning, so a failed first clone can leave a non-git dir + # here, and `git worktree prune` in it would error. Mirror + # `ensure_clone`'s own `.git`/`HEAD` validity check. + if (pool / ".git").exists() or (pool / "HEAD").exists(): + needs_prune = False + if repo_dir.exists(): + removed = _safe_run(["git", "worktree", "remove", "--force", str(repo_dir)], cwd=pool) + if removed.returncode != 0: + shutil.rmtree(repo_dir, ignore_errors=True) + needs_prune = True + elif ws_root.exists(): + # Checkout gone but the workspace root remains -> a prior op + # was killed mid-flight and may have left a dangling + # registration. A fully-cleaned workspace has no ws_root, so + # a plain repeat close prunes nothing. + needs_prune = True + if needs_prune: + pruned = _safe_run(["git", "worktree", "prune"], cwd=pool) + if pruned.returncode != 0: + # Prune is the step that clears the dangling registration. + # If it fails (incl. a 124 timeout), report it so the + # cleanup event retries instead of recording success with + # stale metadata still blocking the next add. + raise GitCommandError( + ["git", "worktree", "prune"], pruned.returncode, pruned.stdout, pruned.stderr + ) if ws_root.exists(): shutil.rmtree(ws_root, ignore_errors=True) diff --git a/python/robomp/tests/test_sandbox.py b/python/robomp/tests/test_sandbox.py index 0feb03c59..fa002127d 100644 --- a/python/robomp/tests/test_sandbox.py +++ b/python/robomp/tests/test_sandbox.py @@ -1117,6 +1117,8 @@ def test_remove_workspace_prunes_pool_after_failed_worktree_remove(tmp_path: Pat repo_dir = ws_root / "repo" repo_dir.mkdir(parents=True, exist_ok=True) pool = mgr.pool_path("o/r") + pool.mkdir(parents=True, exist_ok=True) + (pool / ".git").mkdir(exist_ok=True) # mark as a real git pool for the cleanup gate calls: list[tuple[list[str], object]] = [] @@ -1158,6 +1160,8 @@ def test_remove_workspace_prunes_when_failed_remove_already_deleted_checkout( repo_dir = ws_root / "repo" repo_dir.mkdir(parents=True, exist_ok=True) pool = mgr.pool_path("o/r") + pool.mkdir(parents=True, exist_ok=True) + (pool / ".git").mkdir(exist_ok=True) # mark as a real git pool for the cleanup gate calls: list[tuple[list[str], object]] = [] @@ -1877,19 +1881,26 @@ def test_remove_workspace_acquires_repo_lock(tmp_path: Path, monkeypatch: pytest def test_safe_run_timeout_returns_124(monkeypatch: pytest.MonkeyPatch) -> None: import robomp.sandbox as s + seen: dict[str, object] = {} + def boom(*a: object, **k: object) -> subprocess.CompletedProcess: + seen["timeout"] = k.get("timeout") cmd = a[0] if a else k.get("args", ["git"]) raise subprocess.TimeoutExpired(cmd=cmd, timeout=1) # type: ignore monkeypatch.setattr("robomp.sandbox.subprocess.run", boom) r = s._safe_run(["git", "status"]) assert r.returncode == 124 + assert seen["timeout"] == s._DEFAULT_SANDBOX_SUBPROCESS_TIMEOUT def test_run_timeout_raises_git_command_error_124(monkeypatch: pytest.MonkeyPatch) -> None: import robomp.sandbox as s + seen: dict[str, object] = {} + def boom(*a: object, **k: object) -> subprocess.CompletedProcess: + seen["timeout"] = k.get("timeout") cmd = a[0] if a else k.get("args", ["git"]) raise subprocess.TimeoutExpired(cmd=cmd, timeout=1) # type: ignore @@ -1897,6 +1908,7 @@ def test_run_timeout_raises_git_command_error_124(monkeypatch: pytest.MonkeyPatc with pytest.raises(s.GitCommandError) as exc: s._run(["git", "status"]) assert exc.value.returncode == 124 + assert seen["timeout"] == s._DEFAULT_SANDBOX_SUBPROCESS_TIMEOUT def test_ensure_workspace_raises_when_local_branch_probe_times_out(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: @@ -2011,3 +2023,286 @@ def test_ensure_workspace_raises_when_symbolic_ref_probe_times_out( existing_branch="feature/x", slot_uid=None, ) + + +def test_remove_workspace_real_prune_clears_dangling_registration( + tmp_path: Path, upstream_repo: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Integration guard: the other prune tests fully mock `_safe_run`, so the + # REAL `git worktree prune` never runs and the stale-metadata risk this + # patch targets is not actually exercised. Here we build a real workspace + # (real pool clone + real worktree registration), fake ONLY the + # `git worktree remove` to "time out" (124), and let the real rmtree+prune + # run. The dangling registration must actually be gone afterward, and a + # fresh add at the same path must succeed — the real integration risk. + import robomp.sandbox as s + + mgr = SandboxManager(tmp_path / "workspaces") + ws = mgr.ensure_workspace( + repo="octo/widget", + number=21, + title="t", + clone_url=str(upstream_repo), + default_branch="main", + author_name="robomp-bot", + author_email="robomp-bot@example.invalid", + ) + pool = mgr.pool_path("octo/widget") + repo_dir = ws.repo_dir + assert repo_dir.exists() + listed = subprocess.run( + ["git", "-C", str(pool), "worktree", "list", "--porcelain"], + capture_output=True, + text=True, + check=True, + ).stdout + assert str(repo_dir) in listed # sanity: registration exists + + real_safe_run = s._safe_run + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + if cmd[:3] == ["git", "worktree", "remove"]: + # A remove that "times out" without unregistering — the source must + # then rmtree the checkout and run the REAL prune to clear metadata. + return subprocess.CompletedProcess(cmd, 124, "", "timed out") + return real_safe_run(cmd, **k) # type: ignore[arg-type] + + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + mgr.remove_workspace(repo="octo/widget", number=21) + + listed_after = subprocess.run( + ["git", "-C", str(pool), "worktree", "list", "--porcelain"], + capture_output=True, + text=True, + check=True, + ).stdout + assert str(repo_dir) not in listed_after, listed_after + # The path is genuinely reusable again — the actual failure the fix prevents. + subprocess.run( + ["git", "-C", str(pool), "worktree", "add", "--detach", str(repo_dir), "HEAD"], + capture_output=True, + text=True, + check=True, + ) + assert repo_dir.exists() + + +def test_worktree_add_cleans_partial_state_on_failure(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # A `git worktree add` killed mid-op (124) or otherwise failing must leave no + # partial checkout and no dangling registration, or the event retry poisons + # itself on the same path. + import robomp.sandbox as s + + pool = tmp_path / "pool" + pool.mkdir() + repo_dir = tmp_path / "ws" / "repo" + repo_dir.mkdir(parents=True) + (repo_dir / "leftover").write_text("partial", encoding="utf-8") + + def boom_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + raise s.GitCommandError(cmd, 124, "", "timed out") + + pruned: list[list[str]] = [] + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + pruned.append(list(cmd)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox._run", boom_run) + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + with pytest.raises(GitCommandError) as exc: + s._worktree_add(["git", "worktree", "add", str(repo_dir), "main"], pool=pool, repo_dir=repo_dir) + + assert exc.value.returncode == 124 # the original add failure is surfaced + assert not repo_dir.exists() # partial checkout removed + assert ["git", "worktree", "prune"] in pruned # pool pruned + + +def test_worktree_add_raises_prune_failure_chained_from_add_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # If the cleanup prune ALSO fails, that failure must be raised (a dangling + # registration left behind is what poisons the retry) — chained from the + # original add error so the root cause is not lost. + import robomp.sandbox as s + + pool = tmp_path / "pool" + pool.mkdir() + repo_dir = tmp_path / "ws" / "repo" + repo_dir.mkdir(parents=True) + + add_err = s.GitCommandError(["git", "worktree", "add"], 1, "", "add failed") + + def boom_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + raise add_err + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess(cmd, 124, "", "prune timed out") + + monkeypatch.setattr("robomp.sandbox._run", boom_run) + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + with pytest.raises(GitCommandError) as exc: + s._worktree_add(["git", "worktree", "add", str(repo_dir), "main"], pool=pool, repo_dir=repo_dir) + + assert exc.value.returncode == 124 # the PRUNE failure (124), not the add (1) + assert exc.value.__cause__ is add_err # chained from the add error + + +def test_ensure_clone_fails_before_fetch_when_origin_probe_times_out( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # An older deploy may have baked `https://user:pass@…` into origin; + # `_reset_origin_url` rewrites it before fetch so the PAT never persists. A + # timed-out `git remote get-url origin` (124) is indeterminate — ensure_clone + # must fail closed BEFORE fetch_pool rather than fetch against a possibly- + # credentialed origin. + mgr = SandboxManager(tmp_path / "workspaces") + pool = mgr.pool_path("octo/widget") + (pool / ".git").mkdir(parents=True) # take the idempotent-refresh path + + fetched = {"called": False} + mgr.transport = SimpleNamespace( + fetch_pool=lambda **k: fetched.__setitem__("called", True), + clone_pool=lambda **k: None, + ) # type: ignore + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + if cmd[:4] == ["git", "remote", "get-url", "origin"]: + return subprocess.CompletedProcess(cmd, 124, "", "timed out") + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + with pytest.raises(GitCommandError): + mgr.ensure_clone(repo="octo/widget", clone_url="https://github.com/octo/widget.git", default_branch="main") + assert fetched["called"] is False, ( + "fetch_pool ran despite an indeterminate origin probe — a legacy credential could persist and be reused" + ) + + +def test_remove_workspace_raises_when_prune_times_out(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + # After a failed `git worktree remove`, `git worktree prune` is the step that + # clears the dangling registration. If prune ITSELF fails (incl. a 124 + # timeout), remove_workspace must raise so the cleanup event retries — not + # record success while stale metadata still blocks the next add. + mgr = SandboxManager(tmp_path) + ws_root = mgr.workspace_root("o/r", 31) + repo_dir = ws_root / "repo" + repo_dir.mkdir(parents=True, exist_ok=True) + pool = mgr.pool_path("o/r") + pool.mkdir(parents=True, exist_ok=True) + (pool / ".git").mkdir(exist_ok=True) # mark as a real git pool for the cleanup gate + + calls: list[list[str]] = [] + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + calls.append(list(cmd)) + if cmd[:3] == ["git", "worktree", "remove"]: + # Distinct code (1, NOT 124) so the assertion below proves the raised + # error came from PRUNE, not from the remove failure. + return subprocess.CompletedProcess(cmd, 1, "", "remove failed") + if cmd[:3] == ["git", "worktree", "prune"]: + return subprocess.CompletedProcess(cmd, 124, "", "prune timed out") + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + with pytest.raises(GitCommandError) as exc: + mgr.remove_workspace(repo="o/r", number=31) + # Prune actually ran, and the surfaced error is prune's (124) — not remove's (1). + assert ["git", "worktree", "prune"] in calls, "prune did not run after a failed remove" + assert exc.value.returncode == 124 + + +def test_remove_workspace_prunes_when_checkout_already_gone_on_entry( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # A prior remove/worktree-add killed mid-flight can leave the checkout gone + # but the pool's worktree registration dangling. On the NEXT remove_workspace + # the checkout is already missing on entry, yet the stale registration must + # still be pruned — the old `if repo_dir.exists()` guard skipped it entirely. + mgr = SandboxManager(tmp_path) + ws_root = mgr.workspace_root("o/r", 33) + repo_dir = ws_root / "repo" + ws_root.mkdir(parents=True, exist_ok=True) # ws_root present, but NO repo_dir + pool = mgr.pool_path("o/r") + pool.mkdir(parents=True, exist_ok=True) + (pool / ".git").mkdir(exist_ok=True) # mark as a real git pool for the cleanup gate + assert not repo_dir.exists() # precondition: checkout already gone + + calls: list[list[str]] = [] + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + calls.append(list(cmd)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + mgr.remove_workspace(repo="o/r", number=33) + + # No `git worktree remove` (nothing to remove), but prune MUST run to clear + # any dangling registration for the missing path. + assert ["git", "worktree", "prune"] in calls, ( + "prune was skipped for a checkout already gone on entry — dangling registration would persist" + ) + assert not any(c[:3] == ["git", "worktree", "remove"] for c in calls) + assert not ws_root.exists() # ws_root still cleaned up + + +def test_remove_workspace_no_git_ops_when_pool_is_not_a_real_clone( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # `ensure_clone` mkdir's the pool dir BEFORE cloning, so a failed first clone + # can leave a non-git dir at pool_path. A later remove_workspace (e.g. on + # reopen) must NOT run `git worktree prune` there — it would error on a + # non-git dir and raise. It must be a clean no-op that still clears ws_root. + mgr = SandboxManager(tmp_path) + ws_root = mgr.workspace_root("o/r", 41) + ws_root.mkdir(parents=True, exist_ok=True) + pool = mgr.pool_path("o/r") + pool.mkdir(parents=True, exist_ok=True) # exists but is NOT a git repo (no .git/HEAD) + + calls: list[list[str]] = [] + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + calls.append(list(cmd)) + return subprocess.CompletedProcess(cmd, 128, "", "not a git repository") + + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + # Must NOT raise despite the pool dir existing. + mgr.remove_workspace(repo="o/r", number=41) + + assert calls == [], "ran git in a non-git pool dir — would error and raise on cleanup" + assert not ws_root.exists() # ws_root still cleaned up + + +def test_remove_workspace_skips_prune_on_repeat_close_after_full_cleanup( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # A fully-cleaned workspace has no ws_root. A duplicate/repeat close (GitHub + # can redeliver a close webhook) must NOT run a speculative `git worktree + # prune` on the shared pool every time — there is no dangling registration + # once the first cleanup succeeded. Guarded by `ws_root.exists()`. + mgr = SandboxManager(tmp_path) + ws_root = mgr.workspace_root("o/r", 43) + repo_dir = ws_root / "repo" + pool = mgr.pool_path("o/r") + pool.mkdir(parents=True, exist_ok=True) + (pool / ".git").mkdir(exist_ok=True) # a REAL git pool (persists across closes) + assert not ws_root.exists() and not repo_dir.exists() # already fully cleaned + + calls: list[list[str]] = [] + + def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]: + calls.append(list(cmd)) + return subprocess.CompletedProcess(cmd, 0, "", "") + + monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run) + + mgr.remove_workspace(repo="o/r", number=43) + + assert calls == [], "repeat close ran a spurious git worktree prune on an already-clean workspace" diff --git a/python/robomp/tests/test_tasks.py b/python/robomp/tests/test_tasks.py index 0ea004a0f..98304823b 100644 --- a/python/robomp/tests/test_tasks.py +++ b/python/robomp/tests/test_tasks.py @@ -102,19 +102,30 @@ async def test_run_workspace_op_drains_thread_before_propagating_cancel(): await asyncio.to_thread(started.wait, 1.0) assert started.is_set() - # Cancel the AWAITING coroutine while the thread is mid-flight. - task.cancel() - # Let the loop deliver the cancellation into the helper's drain loop. - await asyncio.sleep(0.05) + async def pump(turns: int = 20) -> None: + # Deterministically advance the loop without a wall-clock sleep: each + # sleep(0) drains the ready queue, so a DETACHING (pre-fix) helper would + # resolve `task` within these turns. A draining helper keeps it pending + # while the worker thread is still blocked on `proceed`. + for _ in range(turns): + await asyncio.sleep(0) + # Cancel the AWAITING coroutine while the thread is mid-flight, then a SECOND + # time while it is still blocked. The repeated cancel must land on the drain + # loop's re-`await` and be swallowed by its `continue` branch, NOT abandon + # the thread. The whole sequence runs under try/finally so any failed assert + # still releases the worker and cannot leak a blocked thread into later tests. try: - # The thread must NOT have been abandoned: it is still blocked on - # `proceed`, so `finished` is not set and the task has not resolved yet. + task.cancel() + await pump() + assert not task.done(), "helper propagated the first cancel before the thread completed (thread abandoned)" + task.cancel() + await pump() + # The thread is still blocked on `proceed`, so it has not finished and + # the task has not resolved despite two cancels. assert not finished.is_set(), "thread finished before we released it — impossible unless abandoned" - assert not task.done(), "helper propagated cancel before the thread completed (thread abandoned)" + assert not task.done(), "helper abandoned the thread after a repeated cancel" finally: - # Always release the worker, even if an assert above fails, so a failed - # run cannot leave a blocked thread leaking into later tests. proceed.set() # The helper must now let the thread finish, THEN raise CancelledError.