[F129,F130] harden quality gate _run_one exit status + timeout cleanup

F129: _run_one returned 'proc.returncode or 0', masking a None returncode
(communicate returned without a recorded exit code — process killed
out-of-band) as 0 / success. Treat None as a non-zero failure (fail-closed).

F130: on timeout, _run_one killed the subprocess but never awaited wait()
— communicate() was cancelled so it never closed the stdout/stderr pipes,
leaving a transient zombie + leaked FDs. Await wait() after kill() to reap
the process and close the transports.
This commit is contained in:
Renn F
2026-06-29 00:21:44 +02:00
parent f69d13a1d1
commit 7f5967bc02
2 changed files with 72 additions and 1 deletions
+11 -1
View File
@@ -86,5 +86,15 @@ async def _run_one(workspace: Path, command: str) -> tuple[int, str]:
) )
except TimeoutError: except TimeoutError:
proc.kill() proc.kill()
# Reap the killed process and close the stdout/stderr pipe transports
# (communicate() was cancelled, so it never closed them). Without this
# the process lingers as a transient zombie and the FDs leak.
await proc.wait()
return 124, f"command timed out after {_GATE_TIMEOUT_SECONDS}s" return 124, f"command timed out after {_GATE_TIMEOUT_SECONDS}s"
return proc.returncode or 0, stdout.decode("utf-8", errors="replace") rc = proc.returncode
if rc is None:
# communicate() returned without a recorded exit code (the process
# was terminated out-of-band). Fail closed — an unknown status must
# not pass the gate.
return 1, stdout.decode("utf-8", errors="replace")
return rc, stdout.decode("utf-8", errors="replace")
+61
View File
@@ -4,6 +4,7 @@ i_am_done, blocking a red submit before it reaches QA. Full tests stay on CI.
from __future__ import annotations from __future__ import annotations
import asyncio
from types import SimpleNamespace from types import SimpleNamespace
from typing import TYPE_CHECKING from typing import TYPE_CHECKING
from unittest.mock import AsyncMock, MagicMock from unittest.mock import AsyncMock, MagicMock
@@ -13,6 +14,7 @@ if TYPE_CHECKING:
from uuid import uuid4 from uuid import uuid4
import pytest import pytest
from roboco.services.gateway import quality_gate
from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
from roboco.services.gateway.choreographer._impl import _IAmDoneContext from roboco.services.gateway.choreographer._impl import _IAmDoneContext
from roboco.services.gateway.quality_gate import GateResult, run_quality_commands from roboco.services.gateway.quality_gate import GateResult, run_quality_commands
@@ -67,6 +69,65 @@ def test_gate_result_summary_and_excerpt() -> None:
assert 0 < len(failed.output_excerpt) < len(failed.output) assert 0 < len(failed.output_excerpt) < len(failed.output)
# --- _run_one None-returncode fail-closed -----------------------------------
_TIMEOUT_EXIT_CODE = 124
@pytest.mark.asyncio
async def test_run_one_treats_none_returncode_as_failure(
tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""A None returncode (process terminated without a recorded exit code, e.g.
killed out-of-band during communicate) must NOT be masked as 0 / success.
The gate is fail-closed — an unknown exit status is a failure, not a pass.
"""
fake_proc = MagicMock()
fake_proc.returncode = None # abnormal: communicate returned, no code set
fake_proc.communicate = AsyncMock(return_value=(b"some output", b""))
async def _fake_shell(*_args: object, **_kwargs: object) -> object:
return fake_proc
monkeypatch.setattr(asyncio, "create_subprocess_shell", _fake_shell)
rc, out = await quality_gate._run_one(tmp_path, "anything")
assert rc != 0, "a None returncode masked as 0 lets a red gate pass"
assert "some output" in out
@pytest.mark.asyncio
async def test_run_one_reaps_killed_timeout_process(
tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch
) -> None:
"""On timeout, _run_one must kill AND await wait() to reap the killed
process and close the stdout/stderr pipe transports. kill alone leaves a
transient zombie + leaked FDs (communicate() was cancelled, so it never
closed the pipes)."""
fake_proc = MagicMock()
fake_proc.returncode = -9 # killed by SIGKILL
# communicate() never completes on its own — wait_for cancels it.
async def _communicate() -> tuple[bytes, bytes]:
await asyncio.sleep(30)
return (b"", b"")
fake_proc.communicate = _communicate
fake_proc.kill = MagicMock()
fake_proc.wait = AsyncMock(return_value=-9)
async def _fake_shell(*_args: object, **_kwargs: object) -> object:
return fake_proc
monkeypatch.setattr(asyncio, "create_subprocess_shell", _fake_shell)
monkeypatch.setattr(quality_gate, "_GATE_TIMEOUT_SECONDS", 0.01)
rc, _msg = await quality_gate._run_one(tmp_path, "slow-command")
assert rc == _TIMEOUT_EXIT_CODE
fake_proc.kill.assert_called_once()
fake_proc.wait.assert_awaited_once()
# --- GitService command selection ------------------------------------------- # --- GitService command selection -------------------------------------------