diff --git a/testing/chaos/sim/docker_exec.py b/testing/chaos/sim/docker_exec.py index fcd328c3..063f4f69 100644 --- a/testing/chaos/sim/docker_exec.py +++ b/testing/chaos/sim/docker_exec.py @@ -133,3 +133,54 @@ def is_container_running(container: str) -> bool: return result.returncode == 0 and result.stdout.strip() == "true" except subprocess.TimeoutExpired: return False + + +def existing_containers(names: list[str], timeout: int = 30) -> list[str] | None: + """Return which of `names` docker still knows about, running or not. + + Deliberately scoped to the names the caller passes in. Enumerating by + compose project label would also sweep up whatever a concurrent run or + a different project owns, and under the parallel CI matrix that turns a + leak check into a flake generator. + + Returns `None` when docker could not be asked -- a query that failed has + observed nothing, and reporting "no survivors" for it would recreate the + silence this check exists to break. + """ + try: + result = subprocess.run( + ["docker", "ps", "-a", "--format", "{{.Names}}"], + capture_output=True, + text=True, + timeout=timeout, + ) + except subprocess.TimeoutExpired: + log.error("docker ps timed out after %ds", timeout) + return None + if result.returncode != 0: + log.error("docker ps exited %d\nstderr: %s", result.returncode, _tail(result.stderr)) + return None + present = set(result.stdout.split()) + return [name for name in names if name in present] + + +def force_remove(names: list[str], timeout: int = 60) -> None: + """Remove the named containers outright, whatever state they are in. + + Best effort and never raises: this runs on a teardown path that has + already reported a leak, and the report is the part that must survive. + """ + if not names: + return + try: + result = subprocess.run( + ["docker", "rm", "-f"] + names, + capture_output=True, + text=True, + timeout=timeout, + ) + except subprocess.TimeoutExpired: + log.error("docker rm -f timed out after %ds", timeout) + return + if result.returncode != 0: + log.error("docker rm -f exited %d\nstderr: %s", result.returncode, _tail(result.stderr)) diff --git a/testing/chaos/sim/nodes.py b/testing/chaos/sim/nodes.py index bff2d6a1..db59e69a 100644 --- a/testing/chaos/sim/nodes.py +++ b/testing/chaos/sim/nodes.py @@ -104,17 +104,34 @@ class NodeManager: self._start_node(nid) def _stop_node(self, node_id: str, duration: float): - """Stop a container.""" + """Stop a container, and record it as down only if it stopped. + + A `docker stop` that exits non-zero -- the container already gone, + the daemon refusing -- once left the node marked down anyway. Every + figure the scenario reasons with is derived from that mark: + `down_count` and so the `max_down_nodes` cap, `_would_disconnect` + and so the `protect_connectivity` guard, and the shared + `down_nodes` set that netem, links and traffic all skip. Returning + before the mutation keeps the model equal to the mesh and lets the + next churn tick retry the node. See ISSUE-2026-0081. + """ container = self.topology.container_name(node_id) docker_exec_quiet(container, "kill 1", timeout=5) # SIGTERM to PID 1 # Use docker stop with a short grace period import subprocess - subprocess.run( + result = subprocess.run( ["docker", "stop", "-t", "2", container], capture_output=True, + text=True, timeout=15, ) + if result.returncode != 0: + log.warning( + "Failed to stop %s: %s; leaving %s marked up", + container, result.stderr.strip(), node_id, + ) + return now = time.time() state = self.node_states[node_id] diff --git a/testing/chaos/sim/runner.py b/testing/chaos/sim/runner.py index 8f661cae..ad41f6fc 100644 --- a/testing/chaos/sim/runner.py +++ b/testing/chaos/sim/runner.py @@ -25,7 +25,7 @@ from .assertions import ( from .compose import generate_compose from .config_gen import write_configs from .control import snapshot_all_congestion, snapshot_all_mmp, snapshot_all_trees -from .docker_exec import docker_compose +from .docker_exec import docker_compose, existing_containers, force_remove from .link_swap import LinkSwapManager from .links import LinkManager from .logs import AnalysisResult, analyze_logs, collect_logs, write_sim_metadata @@ -569,6 +569,85 @@ class SimRunner: except Exception: log.exception("Could not write status file") + def _own_containers(self) -> list[str]: + """Name every container this run asked compose to create. + + Empty before the topology exists, which is the only window in which + a teardown can run with nothing of its own on the host. + """ + if not self.topology: + return [] + return [self.topology.container_name(nid) for nid in self.topology.nodes] + + def _stop_mesh(self) -> None: + """Take the containers down, then check that they actually went. + + `docker compose down` exits 0 while leaving containers behind when it + races an `up -d` that failed part way through: compose enumerates the + project before the stragglers have registered, finds nothing to + remove, and says so successfully. Nothing downstream looks at what + survived, so the leak is invisible at the one moment it is cheap to + see. See ISSUE-2026-0079. + """ + log.info("Stopping containers...") + docker_compose(self.compose_file, ["down"], check=False) + self._check_teardown() + + def _check_teardown(self) -> None: + """Report, then clear, any of this run's containers that outlived `down`. + + Scoped to names this run generated. A check that reasoned about the + compose project label, or about container names in general, would + answer for whatever a concurrent scenario happens to own, and the CI + matrix runs plenty of those at once. + """ + wanted = self._own_containers() + if not wanted: + return + + survivors = existing_containers(wanted) + if survivors is None: + # Detection failed, which is not the same as detecting nothing. + log.error("Could not ask docker what survived `down`; leak undetectable") + return + if not survivors: + return + log.warning( + "`compose down` exited 0 but left %d of this run's %d containers: %s", + len(survivors), len(wanted), ", ".join(survivors), + ) + + # Remedy, kept apart from the detection above on purpose: deleting + # everything from here down leaves the report intact. + force_remove(survivors) + leaked = existing_containers(survivors) + if not leaked: + return + # Either docker could not be asked a second time, or the containers + # are still there after a forced removal. Neither is the transient + # race, and neither clears itself, so this is the run's verdict. + detail = "unknown" if leaked is None else ", ".join(leaked) + log.error("Containers survived forced removal, leaked to the host: %s", detail) + self._write_leak(leaked if leaked is not None else wanted) + self.aborted = True + + def _write_leak(self, names: list[str]) -> None: + """Record the leaked names beside the run's other artifacts. + + Never raises, for the same reason `_write_status` does not: this runs + on a teardown path that has already gone wrong. Written alongside + status.txt rather than into it: the status is the simulation's own + outcome, and a leak on the way out does not retract it. + """ + try: + os.makedirs(self.output_dir, exist_ok=True) + path = os.path.join(self.output_dir, "leaked-containers.txt") + with open(path, "w") as f: + for name in names: + f.write(name + "\n") + except Exception: + log.exception("Could not write leaked-containers file") + def _release_network(self) -> None: """Give this run's claimed /24 back. @@ -593,8 +672,7 @@ class SimRunner: if self.compose_file: # `up -d` can fail part way through, so this run may own # containers or a network even with no mesh to speak of. - log.info("Stopping containers...") - docker_compose(self.compose_file, ["down"], check=False) + self._stop_mesh() self._release_network() return None @@ -755,12 +833,7 @@ class SimRunner: # Status first: it is the one artifact that must exist whatever # else happens, and stopping containers can still time out. self._write_status(status) - log.info("Stopping containers...") - docker_compose( - self.compose_file, - ["down"], - check=False, - ) + self._stop_mesh() self._release_network() return result