From b7bcc0bbf09c69e43ecfbc2f8fcf67b4eed72488 Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Thu, 2 Jul 2026 09:58:14 +0900 Subject: [PATCH] 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 | 8 ++++- python/robomp/src/tasks.py | 6 ++++ python/robomp/tests/test_sandbox.py | 56 ++++++++++++++++++++++++++--- python/robomp/tests/test_tasks.py | 35 ++++++++++++++++++ 4 files changed, 99 insertions(+), 6 deletions(-) diff --git a/python/robomp/src/sandbox.py b/python/robomp/src/sandbox.py index b048c3a88..b63805407 100644 --- a/python/robomp/src/sandbox.py +++ b/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: diff --git a/python/robomp/src/tasks.py b/python/robomp/src/tasks.py index 08e7b1a64..ae77df876 100644 --- a/python/robomp/src/tasks.py +++ b/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 diff --git a/python/robomp/tests/test_sandbox.py b/python/robomp/tests/test_sandbox.py index 071d63141..402ab4471 100644 --- a/python/robomp/tests/test_sandbox.py +++ b/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, + ) diff --git a/python/robomp/tests/test_tasks.py b/python/robomp/tests/test_tasks.py index ed63ba5c9..0ea004a0f 100644 --- a/python/robomp/tests/test_tasks.py +++ b/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" + )