From 0f307307b2f715cbe005856be731cb6575182ad5 Mon Sep 17 00:00:00 2001 From: YifanXu1999 Date: Fri, 6 Nov 2026 20:08:44 +0800 Subject: [PATCH] fix(cleanup): compare container mounts as a set The docker-container action records `docker inspect` identity at plan time and compares it at apply time and again after recovery capture. Docker returns `Mounts` in no fixed order, so an untouched exited container was refused with "Container identity or execution state changed since plan" (reproduced 2026-11-06 in the PostgreSQL round-2 cleanup, where the plan and a fresh inspect differed only in mount order). identity() now sorts Mounts by type, name, source and destination, and both checks compare through same(), which normalizes the recorded side too, so plans written before this change still apply. A changed image, state, mount set or new process still refuses. Tests: two inspect payloads that differ only in mount order pass the apply check (new and old-format plans); a changed, missing or replaced mount, a changed image or a changed state refuses before any Docker command. The order test fails on the previous code. Full tools/test_cleanup.py: 46 OK. docs/general/cleanup.md describes the identity comparison. Production LOC: 102831 -> 102831 (delta 0) Scope: tools/production_loc.py at the parent and staged tree (core 38395, engine 61777, api 2659, dsh 0); tools-, test- and docs-only change. Reference absent (retired R9c; last counted 65,417 at tree 498dd1917812). Co-Authored-By: Claude Opus 5.5 --- docs/general/cleanup.md | 6 +++- tools/cleanup_docker.py | 26 +++++++++++++---- tools/test_cleanup.py | 62 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/docs/general/cleanup.md b/docs/general/cleanup.md index 851c827539..a4477728c2 100644 --- a/docs/general/cleanup.md +++ b/docs/general/cleanup.md @@ -464,7 +464,11 @@ The script enforces these boundaries: daemon; both plan and apply require it to contain only one `layerfs-daemon` process, with no Exec descendants. Protected containers and binds are refused. The daemon is stopped before recovery capture. A changed identity or newly - active command blocks removal. Recovery images retain changed filesystem layers; + active command blocks removal. The identity is the container's id, creation + time, image, configuration, mounts and execution state, checked at apply and + again after recovery capture. Docker lists mounts in no fixed order, so they + are compared as a set: a reordering alone is not a change, but an added, + removed or altered mount is. Recovery images retain changed filesystem layers; removing a container alone may reclaim little space. For aggressive retirement, an exact action may set `discard_rootfs: true` and list `preserve_paths`, such as `/layerfs-local/config` and `/usr/local/bin/layerfs-daemon`. Those files are diff --git a/tools/cleanup_docker.py b/tools/cleanup_docker.py index 629e470bca..b0b3831a9f 100644 --- a/tools/cleanup_docker.py +++ b/tools/cleanup_docker.py @@ -15,10 +15,26 @@ def inspect(name): return json.loads(run(["docker", "container", "inspect", name]).stdout)[0] +def _mount_key(mount): + fields = tuple(str(mount.get(key) or "") for key in ("Type", "Name", "Source", "Destination")) + return fields + (json.dumps(mount, sort_keys=True),) + + +def normalized(recorded): + # Docker reports Mounts in no fixed order, so compare them as a set. + return recorded | {"Mounts": sorted(recorded.get("Mounts") or [], key=_mount_key)} + + def identity(info): - return {key: info[key] for key in ("Id", "Created", "Image", "Config", "Mounts")} | { - "state": {key: info["State"][key] for key in ("Status", "Running", "StartedAt", "FinishedAt")} - } + return normalized( + {key: info[key] for key in ("Id", "Created", "Image", "Config", "Mounts")} + | {"state": {key: info["State"][key] for key in ("Status", "Running", "StartedAt", "FinishedAt")}} + ) + + +def same(recorded, current): + """Compare identities; also accepts plans recorded before Mounts were sorted.""" + return normalized(recorded) == normalized(current) def guard(info, action, scope): @@ -47,7 +63,7 @@ def guard(info, action, scope): def retire(action, scope, output, pass_id, record_event, repo=None): info = inspect(action["name"]) guard(info, action, scope) - if identity(info) != action["identity"]: + if not same(action["identity"], identity(info)): raise Refused("Container identity or execution state changed since plan") dest = output / ("container-" + info["Id"][:12]) dest.mkdir(mode=0o700) @@ -116,7 +132,7 @@ def retire(action, scope, output, pass_id, record_event, repo=None): record_event(output, state="archived", action=action, recovery=recovery) current = inspect(info["Id"]) guard(current, action, scope) - if identity(current) != stopped: + if not same(stopped, identity(current)): raise Refused("Container changed during recovery capture") record_event(output, state="removing", action=action, recovery=recovery) run(["docker", "rm", info["Id"]]) diff --git a/tools/test_cleanup.py b/tools/test_cleanup.py index 6771dbf5ee..5a9a998c58 100644 --- a/tools/test_cleanup.py +++ b/tools/test_cleanup.py @@ -14,7 +14,7 @@ import cleanup from cleanup_discover import discover -from cleanup_docker import guard as guard_container +from cleanup_docker import guard as guard_container, identity as container_identity, retire, same as same_container from cleanup_retirement import retirement_open_files from cleanup_archive import archive from cleanup_state import Refused, ensure_idle, ensure_volume_idle, git, inventory, same_volume @@ -337,6 +337,66 @@ def test_container_using_pinned_volume_cannot_be_retired(self): with self.assertRaises(Refused): guard_container(info, {}, {"protected_volumes": ["pinned"]}) + def exited_container(self, mounts): + return { + "Id": "exact", + "Name": "/build", + "Created": "created", + "Image": "sha256:image", + "Config": {"Env": ["A=1"]}, + "Mounts": mounts, + "State": {"Status": "exited", "Running": False, "StartedAt": "s", "FinishedAt": "f"}, + } + + def test_container_mount_order_is_not_an_identity_change(self): + cargo = {"Type": "volume", "Name": "cargo", "Source": "/v/cargo", "Destination": "/cargo-home"} + work = {"Type": "bind", "Source": "/src", "Destination": "/work", "RW": False} + target = {"Type": "volume", "Name": "target", "Source": "/v/target", "Destination": "/target"} + planned = self.exited_container([cargo, work, target]) + recorded = container_identity(planned) + # A plan written before Mounts were sorted keeps Docker's order. + legacy = {**recorded, "Mounts": [cargo, work, target]} + current = self.exited_container([target, cargo, work]) + self.assertTrue(same_container(legacy, container_identity(current))) + self.assertEqual(recorded, container_identity(current)) + for identity in (recorded, legacy): + with ( + patch("cleanup_docker.inspect", return_value=current), + patch("cleanup_docker.run", side_effect=RuntimeError("capture reached")) as operation, + ): + # Reaching the first Docker capture command proves the identity check passed. + with self.assertRaisesRegex(RuntimeError, "capture reached"): + retire({"name": "exact", "identity": identity}, {}, self.output, "P", lambda *a, **k: None) + operation.assert_called_once_with(["docker", "diff", "exact"]) + for record in self.output.iterdir(): + for path in record.iterdir(): + path.unlink() + record.rmdir() + + def test_container_mount_set_change_refuses_before_capture(self): + cargo = {"Type": "volume", "Name": "cargo", "Source": "/v/cargo", "Destination": "/cargo-home"} + work = {"Type": "bind", "Source": "/src", "Destination": "/work", "RW": False} + recorded = container_identity(self.exited_container([cargo, work])) + changed = [ + self.exited_container([cargo, {**work, "Source": "/other"}]), + self.exited_container([cargo]), + {**self.exited_container([work, cargo]), "Image": "sha256:other"}, + { + **self.exited_container([work, cargo]), + "State": {**self.exited_container([])["State"], "Status": "created"}, + }, + ] + for current in changed: + self.assertFalse(same_container(recorded, container_identity(current))) + with ( + patch("cleanup_docker.inspect", return_value=current), + patch("cleanup_docker.run") as operation, + ): + with self.assertRaisesRegex(Refused, "identity or execution state changed"): + retire({"name": "exact", "identity": recorded}, {}, self.output, "P", lambda *a, **k: None) + operation.assert_not_called() + self.assertEqual(list(self.output.iterdir()), []) + def test_caches_discovery_keeps_helpers_and_active_profile(self): tree = self.root / "linked" target = tree / "target/linux"