From 7f5967bc02251d7bffdc77ff8f81f4fedafaa041 Mon Sep 17 00:00:00 2001
From: Renn F <rennf93@users.noreply.github.com>
Date: Mon, 29 Jun 2026 00:21:44 +0200
Subject: [PATCH] [F129,F130] harden quality gate _run_one exit status +
 timeout cleanup
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 roboco/services/gateway/quality_gate.py | 12 ++++-
 tests/unit/gateway/test_quality_gate.py | 61 +++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/roboco/services/gateway/quality_gate.py b/roboco/services/gateway/quality_gate.py
index aedb5d6c..02d1a38f 100644
--- a/roboco/services/gateway/quality_gate.py
+++ b/roboco/services/gateway/quality_gate.py
@@ -86,5 +86,15 @@ async def _run_one(workspace: Path, command: str) -> tuple[int, str]:
         )
     except TimeoutError:
         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 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")
diff --git a/tests/unit/gateway/test_quality_gate.py b/tests/unit/gateway/test_quality_gate.py
index 5aafbc91..669b6897 100644
--- a/tests/unit/gateway/test_quality_gate.py
+++ b/tests/unit/gateway/test_quality_gate.py
@@ -4,6 +4,7 @@ i_am_done, blocking a red submit before it reaches QA. Full tests stay on CI.
 
 from __future__ import annotations
 
+import asyncio
 from types import SimpleNamespace
 from typing import TYPE_CHECKING
 from unittest.mock import AsyncMock, MagicMock
@@ -13,6 +14,7 @@ if TYPE_CHECKING:
 from uuid import uuid4
 
 import pytest
+from roboco.services.gateway import quality_gate
 from roboco.services.gateway.choreographer import Choreographer, ChoreographerDeps
 from roboco.services.gateway.choreographer._impl import _IAmDoneContext
 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)
 
 
+# --- _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 -------------------------------------------