mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[F078] release_executor: deadline every subprocess (git/make/gh/clone)
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.
This commit is contained in:
Renn F
@@ -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)
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user