From efd8d2fb01735e5f7a48779e8397f57fc5c591ec Mon Sep 17 00:00:00 2001 From: oskar Date: Tue, 4 Aug 2026 14:55:26 +0200 Subject: [PATCH] =?UTF-8?q?fix(node-agent):=20R1/R2/R3=20=E2=80=94=20stop?= =?UTF-8?q?=20deleting=20deliberately=20stopped=20containers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1 (critical): _prune_stopped_containers no longer calls containers.prune(). The unfiltered API call removes EVERY non-running container, ignoring RestartPolicy and compose labels — that is what deleted ollama@solaria 19 s after an operator `docker stop`. No filter argument can fix it (`until` filters on creation time, not stop time). Replaced with explicit enumeration over containers.list(all=True, filters={"status": "exited"}), skipping anything with restart policy unless-stopped/always/on-failure or a com.docker.compose.project label. A stopped managed service is recorded operator intent and now belongs to the module's NEVER TOUCHED list; only one-off leftovers are removed. Dangling image and build cache prune are unchanged. R2 (high): _sd_card_rate_ok → _cleanup_rate_ok, applied to every cleanup- eligible node type. ai_node and standard pruned every 60 s (1440/day, "0 MB reclaimed" in practically every cycle) with no rate limit at all; they now share sd_card's CLEANUP_INTERVAL_SECS (24 h) and mark the cleanup timestamp. R3 (medium): removals are named in the log — WARNING with the container names when the list is non-empty, INFO otherwise. The old line reported megabytes only, which is why ollama's deletion looked identical to every no-op cycle. Tests: services/node-agent/tests/test_safe_cleanup.py (22 cases) — the ollama regression (exited + unless-stopped survives, prune() never called), all three restart policies, compose-label protection, disposable-leftover removal, sweep continues past a failed remove, WARNING-level naming, and the rate limit for ai_node/standard/sd_card plus lte_node still doing nothing. Refs docs/incidents/2026-07-30-ollama-solaria-vanish.md §7 R1-R3. Co-Authored-By: Claude Opus 5 (1M context) --- services/node-agent/src/node_agent.py | 84 ++++++-- .../node-agent/tests/test_safe_cleanup.py | 201 ++++++++++++++++++ 2 files changed, 273 insertions(+), 12 deletions(-) create mode 100644 services/node-agent/tests/test_safe_cleanup.py diff --git a/services/node-agent/src/node_agent.py b/services/node-agent/src/node_agent.py index 78db505..ed37bbc 100644 --- a/services/node-agent/src/node_agent.py +++ b/services/node-agent/src/node_agent.py @@ -13,10 +13,10 @@ Runs as a Docker container on every managed node. Each cycle it: 6. Optionally rsyncs events (including action_result) to VPS so the control-plane observer/executor can process them. -Cleanup policy (matches health-monitor.sh): +Cleanup policy (matches health-monitor.sh). Every type below is additionally +rate-limited to one cleanup run per CLEANUP_INTERVAL_SECS (24 h): lte_node (chelsty-infra, chelsty-ha) : NO cleanup, NO image operations - sd_card (piha, saturn) : dangling images + stopped containers, - max once per 24 h + sd_card (piha, saturn) : dangling images + stopped containers ai_node (solaria) : dangling + containers + build cache, NEVER docker image prune -a standard (vps) : dangling + containers + build cache + @@ -27,6 +27,9 @@ NEVER TOUCHED on any node: /opt/homelab/config/ All hand-crafted and repo-seeded configuration /opt/homelab/state/ Heartbeat files, observer checkpoint actions/pending|approved|running Live work queue + Stopped containers with a restart policy or a compose project label — a + stopped service is recorded operator intent, not garbage. Container cleanup + enumerates explicitly and never calls the unfiltered containers.prune(). """ import json @@ -131,7 +134,8 @@ MEM_CRIT_PCT = 95 # that is actually stuck flapping. Configurable via env for tuning per fleet. CRASH_LOOP_RESTART_THRESHOLD = int(os.getenv("CRASH_LOOP_RESTART_THRESHOLD", "3")) -# SD-card nodes: enforce 24-hour gap between Docker cleanup runs +# Minimum gap between Docker cleanup runs, on EVERY cleanup-eligible node type +# (was sd_card-only until the 2026-07-30 unfiltered-prune incident) CLEANUP_INTERVAL_SECS = 86_400 LAST_CLEANUP_FILE = STATE_DIR / "last-docker-cleanup" @@ -591,8 +595,14 @@ class NodeAgent: # Safe Docker cleanup # ------------------------------------------------------------------ - def _sd_card_rate_ok(self) -> bool: - """Return True only if 24 hours have elapsed since last cleanup.""" + def _cleanup_rate_ok(self) -> bool: + """Return True only if CLEANUP_INTERVAL_SECS has elapsed since last cleanup. + + Applies to EVERY cleanup-eligible node type, not just sd_card. Before + R2 this guard covered sd_card alone, so ai_node and standard pruned + every 60 s — 1440 chances a day to delete a deliberately stopped + container, for `0 MB reclaimed` in practically every cycle. + """ if LAST_CLEANUP_FILE.exists(): try: last_ts = int(LAST_CLEANUP_FILE.read_text().strip()) @@ -623,14 +633,58 @@ class NodeAgent: logger.error(f"Image prune failed: {exc}") def _prune_stopped_containers(self): + """Remove exited containers that are unambiguously disposable. + + NEVER uses containers.prune(): the Docker API removes *every* + non-running container, ignoring RestartPolicy and compose labels. That + is what deleted `ollama` on SOLARIA 19 s after an operator stopped it + (docs/incidents/2026-07-30-ollama-solaria-vanish.md). No filter can fix + it either — `until` filters on creation time, not stop time, so it + never protects a long-lived service. + + A container carrying `restart: unless-stopped|always|on-failure` or a + compose project label is recorded operator intent, exactly like the + paths in this module's NEVER TOUCHED list. Only one-off leftovers + (restart policy `no`, no compose project) are removed. + """ if not self.docker_client: return try: - result = self.docker_client.containers.prune() - reclaimed = result.get("SpaceReclaimed", 0) // (1024 * 1024) - logger.info(f"Pruned stopped containers ({reclaimed} MB reclaimed)") + containers = self.docker_client.containers.list( + all=True, filters={"status": "exited"} + ) except Exception as exc: - logger.error(f"Container prune failed: {exc}") + logger.error(f"Container list for prune failed: {exc}") + return + + removed, kept = [], 0 + for c in containers: + try: + policy = ( + (c.attrs.get("HostConfig") or {}) + .get("RestartPolicy", {}) + .get("Name", "") + ) + if policy in ("unless-stopped", "always", "on-failure"): + kept += 1 # operator intent — do not touch + continue + if (c.labels or {}).get("com.docker.compose.project"): + kept += 1 # compose-managed — do not touch + continue + c.remove() + removed.append(c.name) + except Exception as exc: + logger.error(f"Failed to remove stopped container {c.name}: {exc}") + + # R3: name what was deleted. The old log line reported megabytes only, + # which is why ollama's removal looked identical to every no-op cycle. + if removed: + logger.warning( + f"Removed {len(removed)} disposable stopped container(s): " + f"{', '.join(removed)} (kept {kept} managed)" + ) + else: + logger.info(f"No disposable stopped containers (kept {kept} managed)") def _prune_build_cache(self): if not self.docker_client: @@ -651,9 +705,14 @@ class NodeAgent: logger.debug("Skipping Docker cleanup: LTE node") return + # Rate limit applies to every cleanup-eligible node type (R2). Cleanup + # is housekeeping, not a health function: once per CLEANUP_INTERVAL_SECS + # reclaims the same space as once per minute, at a fraction of the I/O + # and of the exposure to accidental deletion. + if not self._cleanup_rate_ok(): + return + if self.node_type == "sd_card": - if not self._sd_card_rate_ok(): - return self._prune_dangling_images() self._prune_stopped_containers() # No builder prune: minimise write cycles on SD card @@ -665,6 +724,7 @@ class NodeAgent: self._prune_dangling_images() self._prune_stopped_containers() self._prune_build_cache() + self._mark_cleanup_done() # ------------------------------------------------------------------ # VPS-specific: control-plane filesystem rotation diff --git a/services/node-agent/tests/test_safe_cleanup.py b/services/node-agent/tests/test_safe_cleanup.py new file mode 100644 index 0000000..b136058 --- /dev/null +++ b/services/node-agent/tests/test_safe_cleanup.py @@ -0,0 +1,201 @@ +"""Tests for NodeAgent Docker cleanup — regression cover for the 2026-07-30 +unfiltered-prune incident (docs/incidents/2026-07-30-ollama-solaria-vanish.md). + +`containers.prune()` removes EVERY non-running container, ignoring restart +policy and compose labels; that is what deleted `ollama` on SOLARIA 19 s after +an operator stopped it. These tests pin down the replacement: + + R1 — explicit enumeration; a stopped container with `restart: unless-stopped` + or a compose project label is never removed, and prune() is never called. + R2 — the cleanup rate limit applies to ai_node / standard, not sd_card only. + R3 — removals are named in a WARNING log line. +""" +from __future__ import annotations + +import time +from unittest.mock import MagicMock + +import pytest + +import node_agent + + +# --------------------------------------------------------------------------- +# Fake Docker container helper +# --------------------------------------------------------------------------- + +def make_stopped(name, *, restart_policy="no", compose_project=None): + c = MagicMock() + c.name = name + c.status = "exited" + c.attrs = {"HostConfig": {"RestartPolicy": {"Name": restart_policy}}} + c.labels = {"com.docker.compose.project": compose_project} if compose_project else {} + return c + + +def with_containers(agent, containers): + client = MagicMock() + client.containers.list.return_value = containers + agent.docker_client = client + return client + + +# --------------------------------------------------------------------------- +# R1 — the incident itself +# --------------------------------------------------------------------------- + +def test_unless_stopped_container_survives(agent): + """THE regression: ollama@solaria — exited, restart=unless-stopped, must live.""" + ollama = make_stopped("ollama", restart_policy="unless-stopped") + client = with_containers(agent, [ollama]) + + agent._prune_stopped_containers() + + ollama.remove.assert_not_called() + client.containers.prune.assert_not_called() + + +@pytest.mark.parametrize("policy", ["unless-stopped", "always", "on-failure"]) +def test_every_restart_policy_is_operator_intent(agent, policy): + c = make_stopped("svc", restart_policy=policy) + with_containers(agent, [c]) + agent._prune_stopped_containers() + c.remove.assert_not_called() + + +def test_compose_managed_container_survives(agent): + """No restart policy, but compose owns it → still off limits.""" + c = make_stopped("outline-redis-1", restart_policy="no", compose_project="outline") + with_containers(agent, [c]) + agent._prune_stopped_containers() + c.remove.assert_not_called() + + +def test_disposable_leftover_is_removed(agent): + """restart=no and no compose project → a one-off leftover, safe to remove.""" + c = make_stopped("nervous-shell-42", restart_policy="no") + with_containers(agent, [c]) + agent._prune_stopped_containers() + c.remove.assert_called_once() + + +def test_only_exited_containers_are_enumerated(agent): + """Running containers must never even enter the candidate list.""" + client = with_containers(agent, []) + agent._prune_stopped_containers() + _args, kwargs = client.containers.list.call_args + assert kwargs["all"] is True + assert kwargs["filters"] == {"status": "exited"} + + +def test_mixed_set_removes_only_the_disposable_one(agent): + keep_policy = make_stopped("ollama", restart_policy="unless-stopped") + keep_compose = make_stopped("umami-db", compose_project="umami") + drop = make_stopped("tmp-build", restart_policy="no") + with_containers(agent, [keep_policy, keep_compose, drop]) + + agent._prune_stopped_containers() + + keep_policy.remove.assert_not_called() + keep_compose.remove.assert_not_called() + drop.remove.assert_called_once() + + +def test_remove_failure_does_not_abort_the_sweep(agent): + boom = make_stopped("boom", restart_policy="no") + boom.remove.side_effect = RuntimeError("device or resource busy") + later = make_stopped("later", restart_policy="no") + with_containers(agent, [boom, later]) + + agent._prune_stopped_containers() + + later.remove.assert_called_once() + + +def test_no_docker_client_is_noop(agent): + agent.docker_client = None + agent._prune_stopped_containers() # must not raise + + +# --------------------------------------------------------------------------- +# R3 — say what was deleted +# --------------------------------------------------------------------------- + +def test_removal_is_logged_at_warning_with_names(agent, caplog): + with_containers(agent, [make_stopped("tmp-build", restart_policy="no")]) + + with caplog.at_level("INFO", logger="node-agent"): + agent._prune_stopped_containers() + + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + assert "tmp-build" in warnings[0].message + + +def test_nothing_removed_stays_at_info(agent, caplog): + with_containers(agent, [make_stopped("ollama", restart_policy="unless-stopped")]) + + with caplog.at_level("INFO", logger="node-agent"): + agent._prune_stopped_containers() + + assert [r for r in caplog.records if r.levelname == "WARNING"] == [] + + +# --------------------------------------------------------------------------- +# R2 — rate limit covers every node type +# --------------------------------------------------------------------------- + +@pytest.fixture +def cleanup_calls(agent, monkeypatch): + """Record which prune helpers run, without touching Docker.""" + calls = [] + for helper in ("_prune_dangling_images", "_prune_stopped_containers", + "_prune_build_cache"): + monkeypatch.setattr(agent, helper, lambda h=helper: calls.append(h)) + return calls + + +@pytest.fixture +def fresh_cleanup_marker(tmp_path, monkeypatch): + monkeypatch.setattr(node_agent, "LAST_CLEANUP_FILE", tmp_path / "last-docker-cleanup") + return node_agent.LAST_CLEANUP_FILE + + +@pytest.mark.parametrize("node_type", ["ai_node", "standard", "sd_card"]) +def test_recent_cleanup_blocks_next_run(agent, cleanup_calls, fresh_cleanup_marker, + node_type): + """Was true for sd_card only; after R2 it holds for ai_node and standard too.""" + agent.node_type = node_type + fresh_cleanup_marker.write_text(str(int(time.time()))) + + agent.run_safe_cleanup() + + assert cleanup_calls == [] + + +@pytest.mark.parametrize("node_type,expected", [ + ("ai_node", ["_prune_dangling_images", "_prune_stopped_containers", + "_prune_build_cache"]), + ("standard", ["_prune_dangling_images", "_prune_stopped_containers", + "_prune_build_cache"]), + ("sd_card", ["_prune_dangling_images", "_prune_stopped_containers"]), +]) +def test_stale_marker_allows_cleanup_and_is_refreshed(agent, cleanup_calls, + fresh_cleanup_marker, + node_type, expected): + agent.node_type = node_type + stale = int(time.time()) - node_agent.CLEANUP_INTERVAL_SECS - 1 + fresh_cleanup_marker.write_text(str(stale)) + + agent.run_safe_cleanup() + + assert cleanup_calls == expected + # Marker refreshed for every type, otherwise the guard never engages. + assert int(fresh_cleanup_marker.read_text()) > stale + + +def test_lte_node_still_does_nothing(agent, cleanup_calls, fresh_cleanup_marker): + agent.node_type = "lte_node" + agent.run_safe_cleanup() + assert cleanup_calls == [] + assert not fresh_cleanup_marker.exists()