From 68899ffda9d50581ace8188619f70d14f2f9668b Mon Sep 17 00:00:00 2001 From: Renn F Date: Sun, 28 Jun 2026 17:56:03 +0200 Subject: [PATCH] [F078] release_executor: deadline every subprocess (git/make/gh/clone) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hung git/make/gh/clone would block the CEO-gated release loop indefinitely. Wrap each proc.communicate() in asyncio.wait_for via a shared _await_proc helper; on expiry proc.kill() the child and return a non-zero rc (124) so every caller's fail-closed branch fires. Mirrors the quality-gate _run_one kill-on-timeout idiom. Deadlines are generous (30min gate / 10min clone / 5min push+gh) so a legitimate slow op is never wrongly aborted — floor-assertion tests pin the floors to guard exactly that logical regression. Green path returns the real rc unchanged. --- roboco/services/release_executor.py | 49 +++- ...est_release_executor_subprocess_timeout.py | 255 ++++++++++++++++++ 2 files changed, 292 insertions(+), 12 deletions(-) create mode 100644 tests/unit/services/test_release_executor_subprocess_timeout.py diff --git a/roboco/services/release_executor.py b/roboco/services/release_executor.py index bf1d589c..700cc07a 100644 --- a/roboco/services/release_executor.py +++ b/roboco/services/release_executor.py @@ -132,6 +132,33 @@ class ReleaseExecutor: _CI_POLL_INTERVAL_SECONDS = 30 _CI_MAX_POLLS = 80 # ~40 min ceiling +# Subprocess deadlines. A hung git / make / gh would otherwise block the +# CEO-gated release loop indefinitely. Each is generous enough that a +# legitimate, slow operation is never wrongly aborted — only a true hang fails +# closed. Mirrors the quality-gate ``_run_one`` kill-on-timeout idiom. +_GIT_OP_TIMEOUT_SECONDS = 300 # git add/commit/rev-parse/ls-remote/push +_RELEASE_GATE_TIMEOUT_SECONDS = 1800 # make quality — full ruff/mypy/pytest suite +_PUBLISH_TIMEOUT_SECONDS = 300 # gh release create +_CLONE_TIMEOUT_SECONDS = 600 # git clone / rm -rf the release clone + +# The conventional non-zero rc a timed-out subprocess reports so every caller's +# fail-closed branch (rc != 0) fires instead of hanging the release loop. +_TIMEOUT_RC = 124 + + +async def _await_proc( + proc: asyncio.subprocess.Process, timeout: float +) -> tuple[int, str]: + """Communicate with a subprocess under a deadline; on expiry ``kill()`` the + child so a hang fails closed instead of wedging the release loop, and + return a non-zero rc so every caller's fail-closed branch fires.""" + try: + out, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) + except TimeoutError: + proc.kill() + return _TIMEOUT_RC, f"subprocess timed out after {int(timeout)}s" + return proc.returncode or 0, out.decode("utf-8", "replace") + @dataclass(frozen=True) class _ReleaseContext: @@ -164,8 +191,7 @@ class _GitReleaseOps: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, ) - out, _ = await proc.communicate() - return proc.returncode or 0, out.decode("utf-8", "replace") + return await _await_proc(proc, _GIT_OP_TIMEOUT_SECONDS) async def is_already_published(self, version: str) -> bool: rc, out = await self._git("ls-remote", "--tags", "origin", f"v{version}") @@ -212,13 +238,13 @@ class _GitReleaseOps: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, ) - out, _ = await proc.communicate() - if proc.returncode != 0: + rc, out = await _await_proc(proc, _RELEASE_GATE_TIMEOUT_SECONDS) + if rc != 0: logger.warning( - "release gate (make quality) failed", - tail=out.decode("utf-8", "replace")[-2000:], + "release gate (make quality) failed or timed out", + tail=out[-2000:], ) - return proc.returncode == 0 + return rc == 0 async def commit_and_push(self, version: str) -> str: add_rc, add_out = await self._git("add", "-A") @@ -280,9 +306,9 @@ class _GitReleaseOps: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, ) - out, _ = await proc.communicate() - text = out.decode("utf-8", "replace").strip() - if proc.returncode != 0: + rc, out = await _await_proc(proc, _PUBLISH_TIMEOUT_SECONDS) + text = out.strip() + if rc != 0: logger.error("gh release create failed", error=text[:300]) raise RuntimeError(f"gh release create failed: {text[:200]}") url = next( @@ -360,5 +386,4 @@ async def _run(cmd: list[str]) -> tuple[int, str]: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, ) - out, _ = await proc.communicate() - return proc.returncode or 0, out.decode("utf-8", "replace") + return await _await_proc(proc, _CLONE_TIMEOUT_SECONDS) diff --git a/tests/unit/services/test_release_executor_subprocess_timeout.py b/tests/unit/services/test_release_executor_subprocess_timeout.py new file mode 100644 index 00000000..d3a2c028 --- /dev/null +++ b/tests/unit/services/test_release_executor_subprocess_timeout.py @@ -0,0 +1,255 @@ +"""F078 — ``_GitReleaseOps`` subprocesses (git, ``make quality``, ``gh release +create``, the release-clone ``git clone``) had no deadline: a hung child would +block the CEO-gated release loop indefinitely. + +The fix wraps each ``proc.communicate()`` in ``asyncio.wait_for`` and, on +expiry, ``proc.kill()``s the child and returns a non-zero rc (124) so the +caller fails closed — mirroring the quality-gate ``_run_one`` kill-on-timeout +idiom. The deadlines are generous (a full ``make quality`` run, a network +push/clone can legitimately take minutes) so a healthy release is never +wrongly aborted. + +These tests hang the subprocess (a never-resolving ``communicate``) and patch +the timeout constants tiny so a deterministic fail-close is asserted in well +under a second — never relying on real wall-clock timing of the defaults. +""" + +from __future__ import annotations + +import asyncio +from pathlib import Path + +import pytest +from roboco.services.release_executor import ( + _CLONE_TIMEOUT_SECONDS, + _GIT_OP_TIMEOUT_SECONDS, + _PUBLISH_TIMEOUT_SECONDS, + _RELEASE_GATE_TIMEOUT_SECONDS, + _TIMEOUT_RC, + _GitReleaseOps, + _run, +) + +# Floors encoding the logical-regression guard: a deadline below these would +# silently abort a legitimate slow release. Named (not magic) for ruff PLR2004. +_MIN_GATE_TIMEOUT = 1800 # full make quality suite — ruff/mypy/pytest +_MIN_GIT_OP_TIMEOUT = 300 # network push / ls-remote +_MIN_CLONE_TIMEOUT = 600 # full clone on a slow link +_MIN_PUBLISH_TIMEOUT = 120 # gh release create + + +# --------------------------------------------------------------------------- +# Fakes: a hanging subprocess (never-resolving communicate) and a done one. +# --------------------------------------------------------------------------- + + +class _HangingProc: + """Subprocess whose ``communicate()`` never resolves — a hung git/make/gh. + + Records ``kill()`` so a test can assert the child was reaped, not leaked. + """ + + def __init__(self) -> None: + self.killed = False + self._never: asyncio.Future[None] = asyncio.Future() + + async def communicate(self) -> tuple[bytes, bytes]: + await self._never # wait_for cancels this on timeout -> TimeoutError + return (b"", b"") + + def kill(self) -> None: + self.killed = True + + +class _DoneProc: + """Subprocess that completes immediately with a fixed rc + stdout.""" + + def __init__(self, returncode: int, out: bytes) -> None: + self.returncode = returncode + self._out = out + self.killed = False + + async def communicate(self) -> tuple[bytes, bytes]: + return (self._out, b"") + + def kill(self) -> None: + self.killed = True + + +def _exec_returning(proc: object) -> object: + async def _exec(*_args: object, **_kwargs: object) -> object: + return proc + + return _exec + + +def _ops() -> _GitReleaseOps: + """A ``_GitReleaseOps`` built without a DB session — these methods only use + ``self._root`` / ``self._default_branch``, never the session.""" + ops = _GitReleaseOps.__new__(_GitReleaseOps) + ops._slug = "roboco" + ops._default_branch = "master" + ops._root = Path("/tmp/roboco-release-f078") + ops._auth_url = "https://x@github.com/o/roboco" + ops._ci_workflow = None + return ops + + +# --------------------------------------------------------------------------- +# Constant shape — generous floors so a legitimate slow op isn't wrongly killed. +# --------------------------------------------------------------------------- + + +def test_timeouts_are_named_module_constants() -> None: + """Each deadline is a module-level constant (ruff PLR2004), not a magic + number inline.""" + for c in ( + _GIT_OP_TIMEOUT_SECONDS, + _RELEASE_GATE_TIMEOUT_SECONDS, + _PUBLISH_TIMEOUT_SECONDS, + _CLONE_TIMEOUT_SECONDS, + ): + assert isinstance(c, int | float) + assert c > 0 + + +def test_gate_timeout_is_generous_enough_for_a_full_make_quality() -> None: + """``make quality`` runs the full ruff/mypy/pytest suite — a legitimate run + can take many minutes. The deadline must be large enough that a healthy + release is never aborted; 30 min is consistent with the ~40 min CI poll + ceiling. This guards the logical regression: a too-short gate timeout would + silently fail-closed a release that would have succeeded.""" + assert _RELEASE_GATE_TIMEOUT_SECONDS >= _MIN_GATE_TIMEOUT + + +def test_network_op_timeouts_are_generous() -> None: + """Network git ops (push, ls-remote) and a full clone can legitimately take + minutes on a slow link; the deadlines must not wrongly abort them.""" + assert _GIT_OP_TIMEOUT_SECONDS >= _MIN_GIT_OP_TIMEOUT + assert _CLONE_TIMEOUT_SECONDS >= _MIN_CLONE_TIMEOUT + assert _PUBLISH_TIMEOUT_SECONDS >= _MIN_PUBLISH_TIMEOUT + + +# --------------------------------------------------------------------------- +# Hung subprocess → fail-closed within the (tiny, patched) deadline. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_git_op_times_out_and_kills_proc(monkeypatch: pytest.MonkeyPatch) -> None: + """A hung git op returns rc 124 (fail-closed) and kills the child proc — it + must not hang the release loop.""" + monkeypatch.setattr( + "roboco.services.release_executor._GIT_OP_TIMEOUT_SECONDS", 0.05 + ) + proc = _HangingProc() + monkeypatch.setattr( + "roboco.services.release_executor.asyncio.create_subprocess_exec", + _exec_returning(proc), + ) + ops = _ops() + rc, out = await asyncio.wait_for(ops._git("rev-parse", "HEAD"), timeout=2.0) + assert rc == _TIMEOUT_RC + assert "timed out" in out + assert proc.killed + + +@pytest.mark.asyncio +async def test_run_gate_times_out_returns_false( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A hung ``make quality`` fails closed: ``run_gate`` returns False (the + release aborts before commit), and the child is killed — not wedged.""" + monkeypatch.setattr( + "roboco.services.release_executor._RELEASE_GATE_TIMEOUT_SECONDS", 0.05 + ) + proc = _HangingProc() + monkeypatch.setattr( + "roboco.services.release_executor.asyncio.create_subprocess_exec", + _exec_returning(proc), + ) + ops = _ops() + passed = await asyncio.wait_for(ops.run_gate(), timeout=2.0) + assert passed is False + assert proc.killed + + +@pytest.mark.asyncio +async def test_publish_release_times_out_raises( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A hung ``gh release create`` raises RuntimeError (fail-closed — never + reports a bogus published URL) and kills the child.""" + monkeypatch.setattr( + "roboco.services.release_executor._PUBLISH_TIMEOUT_SECONDS", 0.05 + ) + proc = _HangingProc() + monkeypatch.setattr( + "roboco.services.release_executor.asyncio.create_subprocess_exec", + _exec_returning(proc), + ) + ops = _ops() + with pytest.raises(RuntimeError, match="timed out"): + await asyncio.wait_for(ops.publish_release("1.0.0", "notes"), timeout=2.0) + assert proc.killed + + +@pytest.mark.asyncio +async def test_clone_run_times_out_and_kills_proc( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A hung ``git clone`` (the release-clone prep) returns rc 124 and kills + the child — the caller raises, fail-closed, instead of hanging.""" + monkeypatch.setattr("roboco.services.release_executor._CLONE_TIMEOUT_SECONDS", 0.05) + proc = _HangingProc() + monkeypatch.setattr( + "roboco.services.release_executor.asyncio.create_subprocess_exec", + _exec_returning(proc), + ) + rc, out = await asyncio.wait_for( + _run(["git", "clone", "https://x@github.com/o/roboco", "/tmp/x"]), + timeout=2.0, + ) + assert rc == _TIMEOUT_RC + assert "timed out" in out + assert proc.killed + + +# --------------------------------------------------------------------------- +# Regression: the happy path still returns the real rc + decoded output. +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_git_op_green_path_returns_real_rc( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The timeout wrap must not break the happy path: a completing proc + returns its real rc + decoded stdout, and is NOT killed.""" + proc = _DoneProc(0, b"deadbeef\n") + monkeypatch.setattr( + "roboco.services.release_executor.asyncio.create_subprocess_exec", + _exec_returning(proc), + ) + ops = _ops() + rc, out = await asyncio.wait_for(ops._git("rev-parse", "HEAD"), timeout=2.0) + assert rc == 0 + assert out == "deadbeef\n" + assert not proc.killed + + +@pytest.mark.asyncio +async def test_run_gate_green_path_returns_true( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A green ``make quality`` (rc 0) returns True — the timeout wrap doesn't + turn a passing gate into a failure.""" + proc = _DoneProc(0, b"all good\n") + monkeypatch.setattr( + "roboco.services.release_executor.asyncio.create_subprocess_exec", + _exec_returning(proc), + ) + ops = _ops() + passed = await asyncio.wait_for(ops.run_gate(), timeout=2.0) + assert passed is True + assert not proc.killed