From 571030b8e8de9b43f6f9199bd2ee745a8c34b116 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 21 Jul 2026 03:39:18 +0000 Subject: [PATCH] fix: require authoritative container snapshot before reconciling A failed container list or a partial per-container inspect were previously indistinguishable from a legitimately empty/partial live set, so reconciliation could silently unregister healthy bottles. - enumerate_active() now raises EnumerationError instead of returning [] when `container list` fails - Add inspect_container_network_ip() to util, which returns None on inspect failure (vs "" for "no DHCP address yet"), so live_source_ips can tell the two apart - live_source_ips() raises EnumerationError on either failure mode; register_agent() catches it and skips reconciliation, same as OrchestratorClientError - Update tests: rename test_empty_when_the_cli_fails to test_raises_when_the_cli_fails, update patching to the new function, add coverage for list failure, per-container inspect failure, and the launch-not-blocked path --- .../macos_container/consolidated_launch.py | 23 +++++++---- .../backend/macos_container/enumerate.py | 9 ++++- bot_bottle/backend/macos_container/util.py | 35 +++++++++++++++++ tests/unit/test_macos_consolidated_launch.py | 39 ++++++++++++++++++- tests/unit/test_macos_container_cleanup.py | 6 ++- 5 files changed, 99 insertions(+), 13 deletions(-) diff --git a/bot_bottle/backend/macos_container/consolidated_launch.py b/bot_bottle/backend/macos_container/consolidated_launch.py index 0d7fdfd..fcb693e 100644 --- a/bot_bottle/backend/macos_container/consolidated_launch.py +++ b/bot_bottle/backend/macos_container/consolidated_launch.py @@ -41,7 +41,7 @@ from ...orchestrator.client import OrchestratorClient, OrchestratorClientError from ...orchestrator.registration import registration_inputs from ..docker.gateway_provision import deprovision_git_gate, provision_git_gate from . import util as container_mod -from .enumerate import CONTAINER_NAME_PREFIX, enumerate_active +from .enumerate import CONTAINER_NAME_PREFIX, EnumerationError, enumerate_active from .gateway import GATEWAY_NETWORK from .gateway_provision import AppleGatewayTransport from .infra import MacosInfraService, OrchestratorStartError @@ -98,14 +98,21 @@ def live_source_ips(network: str) -> list[str]: The reconciliation input: the orchestrator lives inside the infra container and cannot enumerate the host's containers, so the host has to tell it which bottles are actually up. Containers that have not been - assigned an address yet contribute nothing (the read is non-fatal) — the - reap's grace window, not this list, is what protects an in-flight - launch.""" + assigned an address yet contribute nothing — the reap's grace window, not + this list, is what protects an in-flight launch. + + Raises `EnumerationError` when the live set cannot be determined + authoritatively: either the container listing fails or any individual + inspect fails. Callers must skip reconciliation in that case to avoid + unregistering healthy bottles.""" ips: list[str] = [] for agent in enumerate_active(): - ip = container_mod.try_container_ipv4_on_network( - f"{CONTAINER_NAME_PREFIX}{agent.slug}", network, - ) + name = f"{CONTAINER_NAME_PREFIX}{agent.slug}" + ip = container_mod.inspect_container_network_ip(name, network) + if ip is None: + raise EnumerationError( + f"container inspect {name!r} failed; live set is not authoritative" + ) if ip: ips.append(ip) return ips @@ -133,7 +140,7 @@ def register_agent( # reconciliation failure must not block an otherwise-fine launch. try: client.reconcile(live_source_ips(endpoint.network)) - except OrchestratorClientError as e: + except (OrchestratorClientError, EnumerationError) as e: info(f"registry reconciliation skipped: {e}") inputs = registration_inputs(egress_plan) reg = client.register_bottle( diff --git a/bot_bottle/backend/macos_container/enumerate.py b/bot_bottle/backend/macos_container/enumerate.py index f8ed062..01ce207 100644 --- a/bot_bottle/backend/macos_container/enumerate.py +++ b/bot_bottle/backend/macos_container/enumerate.py @@ -19,6 +19,10 @@ CONTAINER_NAME_PREFIX = "bot-bottle-" _INFRA_NAMES = frozenset({INFRA_NAME}) +class EnumerationError(RuntimeError): + """container list failed; the resulting live set is not authoritative.""" + + def enumerate_active() -> list[ActiveAgent]: result = subprocess.run( ["container", "list", "--quiet"], @@ -27,7 +31,10 @@ def enumerate_active() -> list[ActiveAgent]: check=False, ) if result.returncode != 0: - return [] + raise EnumerationError( + f"container list failed: " + f"{(result.stderr or '').strip() or ''}" + ) out: list[ActiveAgent] = [] for name in sorted(line.strip() for line in result.stdout.splitlines()): if not name.startswith(CONTAINER_NAME_PREFIX) or name in _INFRA_NAMES: diff --git a/bot_bottle/backend/macos_container/util.py b/bot_bottle/backend/macos_container/util.py index 9c5f3aa..6ea6f2a 100644 --- a/bot_bottle/backend/macos_container/util.py +++ b/bot_bottle/backend/macos_container/util.py @@ -572,6 +572,41 @@ def try_container_ipv4_on_network(name: str, network: str) -> str: return "" +def inspect_container_network_ip(name: str, network: str) -> str | None: + """IP of `name` on `network`, distinguishing inspect failure from "not yet". + + Returns: + - the IP string when the container has one on `network` + - "" when inspect succeeds but no address is assigned yet (in-flight DHCP) + - None when the inspect command itself fails (authoritative list impossible) + """ + result = subprocess.run( + [_CONTAINER, "inspect", name], + capture_output=True, text=True, check=False, + ) + if result.returncode != 0: + return None + try: + data = json.loads(result.stdout or "[]") + except json.JSONDecodeError: + return None + if isinstance(data, list): + data = data[0] if data else {} + if not isinstance(data, dict): + return None + status = data.get("status") + networks = status.get("networks") if isinstance(status, dict) else None + if not isinstance(networks, list): + return "" + for entry in networks: + if not isinstance(entry, dict) or entry.get("network") != network: + continue + raw = entry.get("ipv4Address") + if isinstance(raw, str) and raw: + return raw.split("/", 1)[0] + return "" + + def wait_container_ipv4_on_network( name: str, network: str, *, timeout: float = 15.0, poll: float = 0.25, ) -> str: diff --git a/tests/unit/test_macos_consolidated_launch.py b/tests/unit/test_macos_consolidated_launch.py index e180eb2..5e1e7a6 100644 --- a/tests/unit/test_macos_consolidated_launch.py +++ b/tests/unit/test_macos_consolidated_launch.py @@ -147,7 +147,7 @@ class TestLiveSourceIps(unittest.TestCase): def test_maps_slugs_to_container_addresses(self) -> None: from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \ - patch(f"{_MOD}.container_mod.try_container_ipv4_on_network", + patch(f"{_MOD}.container_mod.inspect_container_network_ip", side_effect=["10.0.0.1", "10.0.0.2"]) as ip: got = live_source_ips("net0") self.assertEqual(["10.0.0.1", "10.0.0.2"], got) @@ -158,10 +158,30 @@ class TestLiveSourceIps(unittest.TestCase): nothing — the reap's grace window, not this list, protects it.""" from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \ - patch(f"{_MOD}.container_mod.try_container_ipv4_on_network", + patch(f"{_MOD}.container_mod.inspect_container_network_ip", side_effect=["", "10.0.0.2"]): self.assertEqual(["10.0.0.2"], live_source_ips("net0")) + def test_container_list_failure_raises(self) -> None: + """If container list fails, the live set is not authoritative and + reconciliation must be skipped.""" + from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips + from bot_bottle.backend.macos_container.enumerate import EnumerationError + with patch(f"{_MOD}.enumerate_active", + side_effect=EnumerationError("container list failed")): + with self.assertRaises(EnumerationError): + live_source_ips("net0") + + def test_per_container_inspect_failure_raises(self) -> None: + """If any individual inspect fails, the live set is not authoritative.""" + from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips + from bot_bottle.backend.macos_container.enumerate import EnumerationError + with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \ + patch(f"{_MOD}.container_mod.inspect_container_network_ip", + side_effect=["10.0.0.1", None]): + with self.assertRaises(EnumerationError): + live_source_ips("net0") + class TestRegisterAgentReconciles(unittest.TestCase): """Registration self-heals the registry first: an orphan row at a recycled @@ -201,3 +221,18 @@ class TestRegisterAgentReconciles(unittest.TestCase): client.reconcile.side_effect = OrchestratorClientError("unreachable") self._register(client) client.register_bottle.assert_called_once() + + def test_enumeration_error_does_not_block_the_launch(self) -> None: + """A partial container listing must not abort the launch — skip + reconciliation and proceed, just as with an unreachable orchestrator.""" + from bot_bottle.backend.macos_container.enumerate import EnumerationError + client = _client() + with patch(f"{_MOD}.OrchestratorClient", return_value=client), \ + patch(f"{_MOD}.provision_git_gate"), \ + patch(f"{_MOD}.live_source_ips", + side_effect=EnumerationError("container list failed")): + register_agent( + _egress_plan(), _git_plan(), + source_ip="10.0.0.7", endpoint=_endpoint(), + ) + client.register_bottle.assert_called_once() diff --git a/tests/unit/test_macos_container_cleanup.py b/tests/unit/test_macos_container_cleanup.py index 990536e..48b6871 100644 --- a/tests/unit/test_macos_container_cleanup.py +++ b/tests/unit/test_macos_container_cleanup.py @@ -66,8 +66,10 @@ class TestMacosContainerEnumerate(unittest.TestCase): agents = self._enumerate("bot-bottle-mac-infra\nbot-bottle-dev-abc\n") self.assertEqual(["dev-abc"], [a.slug for a in agents]) - def test_empty_when_the_cli_fails(self): - self.assertEqual([], self._enumerate("", returncode=1)) + def test_raises_when_the_cli_fails(self): + from bot_bottle.backend.macos_container.enumerate import EnumerationError + with self.assertRaises(EnumerationError): + self._enumerate("", returncode=1) if __name__ == "__main__":