From da88d0523b2d942907835cf8217b967fcc0656c5 Mon Sep 17 00:00:00 2001 From: mrparker0980 <290881485+mrparker0980@users.noreply.github.com> Date: Sun, 7 Jun 2026 18:59:43 +0300 Subject: [PATCH] fix(docker): confirm container is gone before recovery re-runs the command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What does this PR do? DockerEnvironment.execute() has an out-of-band recovery path (added in #39415) that recreates the container and re-runs the command when it detects the container has been removed. The problem: it decided "container gone" purely by substring-scanning the command's own combined stdout/stderr for phrases like "No such container" / "is not running". Those phrases are extremely common in legitimate non-zero output — `systemctl status x`, `service x status`, `docker compose ps`, kubectl, or any script that prints "service X is not running". When that false positive fired, the live container was torn down and the user's command was executed a *second* time. For anything with side effects (git push, file writes, package install, network POST, a DB migration) that means silent double execution and lost in-container state. It is on by default because persist_across_processes defaults to True. The fix keeps the cheap substring scan as a first-pass filter but no longer trusts it on its own. Before recovering, execute() now asks Docker directly via `docker inspect -f '{{.State.Running}}'` and only proceeds when Docker confirms the container is actually missing or not running. If the container is still alive (the common false-positive case) the original result passes through untouched and the command never re-runs. The probe fails safe: if the daemon is unreachable or the inspect times out we do not recover, so a live container is never destroyed on an unverified guess. Why a positive inspect rather than tighter regex anchoring? Anchoring the patterns to Docker's CLI error format would shrink the false-positive surface but not close it — program output is fully attacker/agent-controllable and can reproduce any prefix. Asking the daemon for ground truth is the only check that cannot be spoofed by command output. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) ## Changes Made - `tools/environments/docker.py`: add `_container_confirmed_gone()` which runs `docker inspect -f '{{.State.Running}}'` and reports gone only when Docker says the container is absent (rc != 0) or stopped; gate the recovery branch in `execute()` on it (after the existing returncode / persist / substring checks, so no extra `docker inspect` runs on ordinary failures). Clarify the `_is_container_gone()` docstring that it is only a pre-filter. - `tests/tools/test_docker_environment.py`: regression test proving a non-zero command whose own output says "is not running" does NOT recover while the container is alive; a test that recovery still fires once the probe confirms the container is gone; and a unit test of `_container_confirmed_gone()` across running / stopped / missing / daemon-unreachable states. - `scripts/release.py`: add the contributor's email to `AUTHOR_MAP` (release gate requirement for new authors). ## How to Test 1. `uv run python -m pytest tests/tools/test_docker_environment.py -q` — all pass, including the new `test_execute_does_not_recover_when_container_still_running`. 2. Reproduce the original bug by reverting `tools/environments/docker.py`: the new regression test fails because `_recreate_container` is invoked and the command runs twice. 3. `uv run ruff check tools/environments/docker.py tests/tools/test_docker_environment.py` and `uv run python scripts/check-windows-footguns.py --all` — both clean. ## Checklist ### Code - [x] I've read the Contributing Guide - [x] My commit messages follow Conventional Commits (`fix(scope):`, etc.) - [x] I searched for existing PRs to make sure this isn't a duplicate - [x] My PR contains only changes related to this fix - [x] I've run the tests and all pass - [x] I've added tests for my changes (required for bug fixes) - [x] I've tested on my platform: macOS 15 (Darwin 25.5) ### Documentation & Housekeeping - [x] I've updated relevant documentation (docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture — N/A - [x] I've considered cross-platform impact (Windows, macOS) — inspect probe and timeouts are platform-neutral; no new POSIX-only calls - [x] I've updated tool descriptions/schemas if I changed tool behavior — N/A --- scripts/release.py | 1 + tests/tools/test_docker_environment.py | 131 +++++++++++++++++++++++++ tools/environments/docker.py | 52 +++++++++- 3 files changed, 182 insertions(+), 2 deletions(-) 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)