diff --git a/scripts/release.py b/scripts/release.py index bba7f93ffbb3..be4f66029c21 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -45,6 +45,7 @@ # Auto-extracted from noreply emails + manual overrides AUTHOR_MAP = { + "290881485+mrparker0980@users.noreply.github.com": "mrparker0980", "yusufalweshdemir@gmail.com": "Dusk1e", "804436395@qq.com": "LaPhilosophie", "266365592+bmoore210@users.noreply.github.com": "bmoore210", diff --git a/tests/tools/test_docker_environment.py b/tests/tools/test_docker_environment.py index 04935d81dfc5..9c80bca5645d 100644 --- a/tests/tools/test_docker_environment.py +++ b/tests/tools/test_docker_environment.py @@ -1833,3 +1833,134 @@ def _fail_recreate(self): result = env.execute("badcmd") assert result.get("returncode") == 127 assert "command not found" in result.get("output", "") + + +def _patch_inspect(monkeypatch, running): + """Make ``docker inspect -f '{{.State.Running}}'`` report *running*. + + *running* may be True/False (container exists) to emit the corresponding + ``true``/``false`` line with rc 0, or the string ``"missing"`` to emulate + a removed container (rc 1, "No such object"). Other docker subcommands fall + back to the standard mock so container creation during recovery still works. + """ + base_calls = _mock_subprocess_run(monkeypatch) + real_run = docker_env.subprocess.run + + def _run(cmd, **kwargs): + if isinstance(cmd, list) and len(cmd) >= 2 and cmd[1] == "inspect": + if running == "missing": + return subprocess.CompletedProcess( + cmd, 1, stdout="", stderr="Error: No such object: x" + ) + return subprocess.CompletedProcess( + cmd, 0, stdout="true\n" if running else "false\n", stderr="" + ) + return real_run(cmd, **kwargs) + + monkeypatch.setattr(docker_env.subprocess, "run", _run) + return base_calls + + +def test_execute_does_not_recover_when_container_still_running(monkeypatch): + """Regression: a non-zero command whose *own output* contains a + container-gone phrase (e.g. ``systemctl status`` printing "is not running") + must NOT tear down and re-run the command while the container is alive. + + Before the liveness probe was added, the substring scan alone fired + recovery here, recreating the container and executing the user's command a + second time — duplicating any side effects. + """ + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + _patch_inspect(monkeypatch, running=True) + env = _make_dummy_env( + persistent_filesystem=True, + persist_across_processes=True, + ) + + super_calls = [] + + def _fake_super_execute(self, command, cwd="", **kwargs): + super_calls.append(command) + # Mimics `systemctl status nginx` on a stopped unit: real, non-zero, + # and the text trips the substring filter. + return {"output": "Unit nginx.service is not running", "returncode": 3} + + def _fail_recreate(self): + pytest.fail("recreation must not run while the container is alive") + + monkeypatch.setattr(docker_env.BaseEnvironment, "execute", _fake_super_execute) + monkeypatch.setattr( + docker_env.DockerEnvironment, "_recreate_container", _fail_recreate + ) + + result = env.execute("systemctl status nginx") + + assert super_calls == ["systemctl status nginx"], ( + "the command must run exactly once when the container is still alive" + ) + assert result.get("returncode") == 3 + assert "is not running" in result.get("output", "") + + +def test_execute_recovers_only_after_liveness_probe_confirms_gone(monkeypatch): + """The full recovery path still fires when the substring filter matches + AND ``docker inspect`` confirms the container is genuinely gone. + """ + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + _patch_inspect(monkeypatch, running="missing") + env = _make_dummy_env( + persistent_filesystem=True, + persist_across_processes=True, + ) + + outputs = iter([ + {"output": "Error response from daemon: No such container: hermes-x", "returncode": 1}, + {"output": "ok", "returncode": 0}, + ]) + + def _fake_super_execute(self, command, cwd="", **kwargs): + return next(outputs) + + recreate_calls = [] + + def _fake_recreate(self): + recreate_calls.append(True) + self._container_id = "recovered-container-id" + return True + + monkeypatch.setattr(docker_env.BaseEnvironment, "execute", _fake_super_execute) + monkeypatch.setattr( + docker_env.DockerEnvironment, "_recreate_container", _fake_recreate + ) + + result = env.execute("echo hi") + + assert recreate_calls == [True], "recovery should fire once the probe confirms the container is gone" + assert result.get("returncode") == 0 + assert result.get("output") == "ok" + + +def test_container_confirmed_gone_reports_state(monkeypatch): + """``_container_confirmed_gone`` reflects the real docker inspect result: + running → False, stopped → True, missing → True, and fails safe (False) + when the probe can't reach the daemon. + """ + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + + _patch_inspect(monkeypatch, running=True) + env = _make_dummy_env(persistent_filesystem=True, persist_across_processes=True) + env._container_id = "live" + assert env._container_confirmed_gone() is False + + _patch_inspect(monkeypatch, running=False) + assert env._container_confirmed_gone() is True + + _patch_inspect(monkeypatch, running="missing") + assert env._container_confirmed_gone() is True + + # Daemon unreachable / probe times out → fail safe, never recover blindly. + def _boom(cmd, **kwargs): + raise subprocess.TimeoutExpired(cmd, 10) + + monkeypatch.setattr(docker_env.subprocess, "run", _boom) + assert env._container_confirmed_gone() is False diff --git a/tools/environments/docker.py b/tools/environments/docker.py index 421eb71be80d..adeb815a1571 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -966,9 +966,51 @@ def _run_bash(self, cmd_string: str, *, login: bool = False, ) def _is_container_gone(self, output: str) -> bool: - """Return True if the output indicates the container no longer exists.""" + """Return True if the output *might* indicate the container is gone. + + This is only a cheap first-pass filter: the phrases live in + ``_NO_CONTAINER_PATTERNS`` precisely because Docker emits them when an + ``exec`` target has vanished, but they also occur verbatim in perfectly + ordinary command output (``systemctl status``, ``docker compose ps``, a + script echoing "service X is not running"). A match here is necessary + but NOT sufficient to trigger recovery — :meth:`execute` confirms the + container's real state via :meth:`_container_confirmed_gone` before + tearing anything down. + """ return any(p in output for p in self._NO_CONTAINER_PATTERNS) + def _container_confirmed_gone(self) -> bool: + """Ask Docker directly whether the current container is dead. + + Recovery re-creates the container *and re-runs the user's command*, so + a false positive duplicates every side effect (a second ``git push``, + file write, package install, network POST). We must therefore never + rely on the substring scan in :meth:`_is_container_gone` alone — that + scans command-controlled output. Instead we run ``docker inspect`` and + only report the container gone when Docker itself confirms it is absent + or not running. + + Fail safe: if the probe can't reach the daemon or times out we return + ``False`` so a live container is never torn down on an unverified guess + (the original error is surfaced to the caller unchanged). + """ + if not self._container_id: + return True + try: + probe = subprocess.run( + [self._docker_exe, "inspect", "-f", "{{.State.Running}}", self._container_id], + capture_output=True, text=True, timeout=10, check=False, + ) + except (subprocess.TimeoutExpired, OSError) as e: + logger.debug("container liveness probe failed: %s — skipping recovery", e) + return False + if probe.returncode != 0: + # Non-zero almost always means "Error: No such object" — the + # container is genuinely gone, so recovery is warranted. + return True + # rc 0: stdout is "true" (running) or "false" (exists but stopped). + return probe.stdout.strip().lower() != "true" + def _recreate_container(self) -> bool: """Recreate the container after it was removed out-of-band. @@ -1057,10 +1099,16 @@ def execute(self, command: str, cwd: str = "", **kwargs) -> dict: transparently before retrying once. """ result = super().execute(command, cwd, **kwargs) + # The substring scan and persist flag are cheap gates kept first so we + # don't spawn a ``docker inspect`` after every non-zero command. Only + # once they pass do we positively confirm the container is actually + # gone — sniffing command output alone would re-run side-effecting + # commands whenever their *own* output mentioned "is not running". if ( result.get("returncode", 0) != 0 - and self._is_container_gone(result.get("output", "")) and self._persist_across_processes + and self._is_container_gone(result.get("output", "")) + and self._container_confirmed_gone() ): if self._recreate_container(): result = super().execute(command, cwd, **kwargs)