fix(robomp): address PR #4184 review nits

- log the worker thread's exception when a workspace op raises during
  caller cancellation, so a persistently failing setup surfaces instead
  of being buried behind CancelledError.
- raise on a timed-out (124) git symbolic-ref probe in the repo-exists
  path, matching the rev-parse probes, instead of silently accepting the
  caller-supplied branch.
- assert the subprocess timeout is passed in the two _chown_workspace
  test fakes so a refactor cannot silently drop the bound.

Op: correct
Restores: spec:indeterminate-git-probes-raise-not-silently-proceed

python/robomp/src/sandbox.py

@@ -819,12 +819,18 @@ class SandboxManager:
                         )
             else:
                 slot_git_env = _git_env_for_repo(repo_dir)
+                symref = ["git", "symbolic-ref", "--quiet", "--short", "HEAD"]
                 current = _safe_run(
-                    ["git", "symbolic-ref", "--quiet", "--short", "HEAD"],
+                    symref,
                     cwd=repo_dir,
                     env=slot_git_env,
                     **slot_git_kwargs,
                 )
+                if current.returncode == 124:
+                    # A timed-out probe is indeterminate, not "detached HEAD".
+                    # Silently keeping the caller-supplied branch could mismatch
+                    # the real checkout; fail so the event retries.
+                    raise GitCommandError(symref, current.returncode, current.stdout, current.stderr)
                 if current.returncode == 0 and current.stdout.strip():
                     branch = current.stdout.strip()
                     if existing_branch is not None and existing_branch != branch:

python/robomp/src/tasks.py

@@ -54,6 +54,12 @@ async def _run_workspace_op(func: Callable[..., _T], /, **kwargs: object) -> _T:
                 continue
             except BaseException:
                 break
+        if not inner.cancelled() and inner.exception() is not None:
+            log.warning(
+                "workspace op %s raised during caller cancellation",
+                getattr(func, "__name__", func),
+                exc_info=inner.exception(),
+            )
         raise
 
 

python/robomp/tests/test_sandbox.py

@@ -24,6 +24,7 @@ from robomp.git_ops import (
     fetch_ref as git_fetch_ref,
 )
 from robomp.sandbox import (
+    _DEFAULT_SANDBOX_SUBPROCESS_TIMEOUT,
     SandboxManager,
     Workspace,
     _chown_workspace,
@@ -508,10 +509,12 @@ def test_chown_workspace_runs_chown_and_chmod_as_root_on_linux(tmp_path: Path, m
 
     monkeypatch.setattr("robomp.sandbox.platform.system", lambda: "Linux")
     monkeypatch.setattr("robomp.sandbox.os.geteuid", lambda: 0)
-    monkeypatch.setattr(
-        "robomp.sandbox.subprocess.run",
-        lambda cmd, *, check, timeout=None: calls.append((cmd, check)),
-    )
+
+    def fake_run(cmd, *, check, timeout):
+        assert timeout == _DEFAULT_SANDBOX_SUBPROCESS_TIMEOUT
+        calls.append((cmd, check))
+
+    monkeypatch.setattr("robomp.sandbox.subprocess.run", fake_run)
 
     _chown_workspace(tmp_path, 2001)
 
@@ -532,8 +535,9 @@ def test_chown_workspace_makes_workspace_slot_owned(tmp_path: Path, monkeypatch:
     file_path.chmod(0o777)
     owned: dict[Path, tuple[int, int]] = {}
 
-    def fake_run(cmd: list[str], *, check: bool, timeout: float | None = None) -> None:
+    def fake_run(cmd: list[str], *, check: bool, timeout: float) -> None:
         assert check
+        assert timeout == _DEFAULT_SANDBOX_SUBPROCESS_TIMEOUT
         if cmd[:2] == ["chown", "-R"]:
             uid_text, gid_text = cmd[2].split(":", 1)
             root = Path(cmd[3])
@@ -1929,3 +1933,45 @@ def test_ensure_workspace_raises_when_remote_branch_probe_times_out(tmp_path: Pa
             existing_branch="feature/x",
             slot_uid=None,
         )
+
+
+def test_ensure_workspace_raises_when_symbolic_ref_probe_times_out(
+    tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    mgr = SandboxManager(tmp_path)
+    mgr.natives_cache = None
+    mgr.transport = SimpleNamespace(
+        clone_pool=lambda **k: None,
+        fetch_pool=lambda **k: None,
+        fetch_base_ref=lambda **k: None,
+        fetch_pr_head=lambda **k: None,
+    )  # type: ignore
+
+    # Force the repo_exists=True branch: the worktree already has a .git.
+    repo_dir = mgr.workspace_root("o/r", 1) / "repo"
+    (repo_dir / ".git").mkdir(parents=True)
+
+    def fake_safe_run(cmd: list[str], **k: object) -> subprocess.CompletedProcess[str]:
+        if cmd[:2] == ["git", "symbolic-ref"]:
+            return subprocess.CompletedProcess(cmd, 124, "", "timed out")
+        return subprocess.CompletedProcess(cmd, 0, "", "")
+
+    monkeypatch.setattr("robomp.sandbox._safe_run", fake_safe_run)
+    monkeypatch.setattr("robomp.sandbox._run", lambda *a, **k: subprocess.CompletedProcess(["x"], 0, "", ""))
+    monkeypatch.setattr("robomp.sandbox._chown_workspace", lambda *a, **k: None)
+    monkeypatch.setattr("robomp.sandbox._share_git_metadata_with_slots", lambda *a, **k: None)
+    monkeypatch.setattr("robomp.sandbox._provision_runtime_dirs", lambda *a, **k: None)
+    monkeypatch.setattr("robomp.sandbox._git_env_for_repo", lambda *a, **k: {})
+
+    with pytest.raises(GitCommandError):
+        mgr.ensure_workspace(
+            repo="o/r",
+            number=1,
+            title="t",
+            clone_url="https://x/o/r.git",
+            default_branch="main",
+            author_name="n",
+            author_email="e@e",
+            existing_branch="feature/x",
+            slot_uid=None,
+        )

python/robomp/tests/test_tasks.py

@@ -1,4 +1,5 @@
 import asyncio
+import logging
 import threading
 from types import SimpleNamespace
 
@@ -121,3 +122,37 @@ async def test_run_workspace_op_drains_thread_before_propagating_cancel():
         await task
     # Deterministic in the fixed helper: the thread completed before the cancel propagated.
     assert finished.is_set(), "thread did not complete before cancellation propagated"
+
+
+async def test_run_workspace_op_logs_worker_exception_on_concurrent_cancel(caplog):
+    started = threading.Event()
+    proceed = threading.Event()
+    boom = RuntimeError("git exploded")
+
+    def failing_op(**_kwargs):
+        started.set()
+        assert proceed.wait(2.0), "proceed was never set — test bug"
+        raise boom
+
+    task = asyncio.create_task(tasks._run_workspace_op(failing_op))
+    await asyncio.to_thread(started.wait, 1.0)
+    assert started.is_set()
+
+    # Cancel the caller while the worker is still blocked (mid-flight), so the
+    # helper enters its cancel-drain loop and is awaiting the shielded inner.
+    task.cancel()
+    await asyncio.sleep(0.05)
+
+    with caplog.at_level(logging.WARNING, logger="robomp.tasks"):
+        # Release the worker so inner completes WITH an exception while the
+        # helper is draining -> the drain's `await shield(inner)` re-raises boom,
+        # breaks the loop, and the guarded log.warning must fire.
+        proceed.set()
+        with pytest.raises(asyncio.CancelledError):
+            await task
+
+    warnings = [r for r in caplog.records if r.levelno == logging.WARNING]
+    assert warnings, "worker exception during cancel was not logged"
+    assert any(r.exc_info and r.exc_info[1] is boom for r in warnings), (
+        "the worker's exception was not attached to the warning"
+    )