fix(robomp): harden sandbox cleanup, git-probe, and worktree-add paths

A diff-scoped review of the event-loop-hang fixes surfaced gaps in the new
timeout/error-handling code and its tests. All at/above the medium floor,
each mutation-verified.

- remove_workspace: prune on any nonzero `git worktree remove` (not just a
  present checkout) and RAISE on a failed prune, so a killed remove that
  leaves a dangling pool registration is cleared or retried instead of
  recording success over stale metadata. Gate git ops on the pool being a
  real clone (ensure_clone mkdir's the dir before cloning, so a failed first
  clone leaves a non-git dir where `git worktree prune` would error), and
  only speculatively prune a missing checkout when ws_root still exists.
- _worktree_add: new helper wrapping the three worktree-add sites; on a
  failed add (incl. the new 124 timeout) it removes the partial checkout and
  prunes the pool before re-raising, so the event retry starts clean. Raises
  a failed prune chained from the add error.
- _reset_origin_url: a timed-out (124) `git remote get-url origin` probe is
  indeterminate; raise before fetch instead of silently skipping the rewrite,
  so a legacy credentialed origin cannot persist and be reused.
- tests: assert the subprocess timeout is passed in the _safe_run/_run
  timeout fakes; add a real-`git worktree prune` integration test; make the
  cancel-drain test deterministic (loop-turn pump, no wall-clock sleep) and
  cover the repeated-cancel branch; add regressions for the prune-failure,
  checkout-gone-on-entry, non-git-pool, and repeat-close cleanup paths.

Op: correct
Restores: spec:pool-cleanup-clears-or-retries-dangling-registration
Restores: spec:indeterminate-git-probes-raise-not-silently-proceed

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"

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.