fix: reconcile orphaned registry rows against live containers #440
Reference in New Issue
Block a user
Delete Branch "fix/registry-orphan-reconciliation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes #439.
Problem
Registry rows outlive their containers and nothing cleans them up. A row only leaves via an explicit
teardown_bottleorregister()'s same-IP supersede sweep, neither of which runs when the launching CLI dies hard, and there is no reconciliation against live containers anywhere.RegistryStore.by_source_ipfail-closes whenlen(rows) != 1, so an orphan sitting on an address the backend's DHCP later recycles is a collision hazard for the next bottle that lands there.Verified on the host:
bot-bottle-claude-agent-1wasactiveat192.168.128.6with no container and no/gitnamespace. The accumulation is observed; an actual collision is not — this is a reachable hazard, not a witnessed outage.Changes
fix(orchestrator): reap registry rows whose bottle is no longer runningRegistryStore.reap_absent(live_ips, grace_seconds=120)restores the invariant: at most one active row per live address, and none for a dead one. Both halves matter — when several rows claim a live address the newest wins and the rest are swept, otherwise a recycled address stays ambiguous, which is the bricked state. (My first version only handled dead addresses; the test for the recycled-IP case caught it.)Orchestrator.reconcile()reaps rows and drops their in-memory egress tokens. Deliberately does not broker a teardown — the container is already gone, and a broker error must not stop the sweep clearing the row.POST /reconcile+ client method, behind the existing trusted-caller auth gate.register_agenton the macOS backend. The host supplies the live set because the orchestrator runs inside the infra container and cannot see the backend. Best-effort: a failure is logged, not fatal.fix(egress): name the real fault when a deny-all is not an allowlist missUnattributed / orchestrator-unreachable / unparseable-policy all produced a deny-all Config, indistinguishable at the decision point from "policy loaded, host not allowed". All three reported
host X is not in the allowlist.This is the change with the most leverage here: that identical wording is precisely why this issue and #441 were conflated for as long as they were, and why the investigation went looking for a deregistered bottle instead of a dropped token. A follow-up commit widens the unattributed text to name the token-mismatch cause too, since
/resolvefail-closes on both.Verification
1906 unit tests pass (24 new), pyright clean, pylint 9.82 against a
--fail-under=8.0gate with no new message types. The orphan row described in #439 was cleared on the live host; the registry now matches the running containers.Deployment note
The control-plane source is bind-mounted with a
BOT_BOTTLE_SOURCE_HASHstaleness check, so the first launch after this merges recreates the shared infra container — which strands any running bottle (#443).Relationship to the other PRs
Independent of #442 and #445 — branched from
main, no overlapping files, any merge order works.enumerate.CONTAINER_NAME_PREFIXis exported both here and in #445; identical change, trivial conflict.84d0596607tocf4a771e52fix: reconcile orphaned registry rows that brick bottles at recycled source IPsto fix: reconcile orphaned registry rows against live containerscf4a771e52to288b205a44The rebase itself is clean and the full unit suite passes (1,943 tests, 20 skipped), but this needs one safety fix before merge. Reconciliation must only run from a complete, authoritative container snapshot; otherwise the repair path can unregister healthy bottles.
@@ -92,0 +108,4 @@)if ip:ips.append(ip)return ipsBlocking: A failed or partial discovery is currently indistinguishable from an authoritative live set. enumerate_active() returns an empty list when container list fails, and try_container_ipv4_on_network() silently omits a bottle when its address lookup fails. Passing that incomplete list to /reconcile deletes every omitted active row older than the 120-second grace period, so a transient Apple Container failure during any later launch can deregister healthy running bottles and drop their in-memory tokens. The grace period only protects new rows; it does not protect established bottles. Please make discovery report failure/incompleteness and skip reconciliation unless every enumerated agent was resolved from a successful container listing (with tests for list and per-container lookup failures).
Fixed. Both failure modes now raise
EnumerationErrorand skip reconciliation:enumerate_active()raises instead of returning[]whencontainer listfailsinspect_container_network_ip()inutil.pyreturnsNoneon inspect failure (vs""for no DHCP address yet), so the two are distinguishablelive_source_ips()raisesEnumerationErroron either case;register_agent()catches it alongsideOrchestratorClientErrorTest coverage added for: list failure propagation, per-container inspect failure propagation, and launch-not-blocked on
EnumerationError.@@ -92,0 +108,4 @@ips: list[str] = []for agent in enumerate_active():name = f"{CONTAINER_NAME_PREFIX}{agent.slug}"ip = container_mod.inspect_container_network_ip(name, network)Fixed. Both failure modes now raise
EnumerationErrorand skip reconciliation:enumerate_active()raises instead of returning[]whencontainer listfailsinspect_container_network_ip()inutil.pyreturnsNoneon inspect failure (vs""for no DHCP address yet), so the two are distinguishablelive_source_ips()raisesEnumerationErroron either case;register_agent()catches it alongsideOrchestratorClientErrorTest coverage added for: list failure propagation, per-container inspect failure propagation, and launch-not-blocked on
EnumerationError.The authoritative-snapshot fix addresses the previous safety blocker, and the focused review suite passes (146 tests). One test-coverage blocker remains: the new inspect parser is mocked at every call site, leaving its behavior unverified and failing the required diff-coverage gate at 76.7%.
@@ -575,0 +580,4 @@- "" 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(Blocking: The new authoritative/failure distinction depends entirely on this parser, but the added
live_source_ipstests mockinspect_container_network_ip, so none of these branches execute. CI reports lines 583 and 587–607 uncovered and fails diff coverage at 76.7%. Please add direct unit coverage for a successful inspect with an IPv4 address, successful inspect with no assigned address, non-zero inspect exit, malformed/unexpected JSON, and (ideally) CIDR stripping. That will verify the safety-criticalNoneversus""contract rather than only mocking it.Added
TestInspectContainerNetworkIpintests/unit/test_macos_container_util.py(commit28fcc3f), patchingsubprocess.rundirectly so the parser runs for real. Covers all the requested cases: IP found, CIDR stripping, no address yet (empty networks entry), absent network list, non-zero exit, malformed JSON, and unexpected JSON shape. 1933 unit tests pass.@@ -575,0 +580,4 @@- "" 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(Added
TestInspectContainerNetworkIpintests/unit/test_macos_container_util.py(commit28fcc3f), patchingsubprocess.rundirectly so the parser runs for real. Covers all the requested cases: IP found, CIDR stripping, no address yet (empty networks entry), absent network list, non-zero exit, malformed JSON, and unexpected JSON shape. 1933 unit tests pass.Re-reviewed on the latest head. The authoritative-snapshot safety issue is fixed, the inspect parser now has direct coverage for the
None/empty/IP contract and malformed inputs, the focused local suite passes (159 tests), and all CI checks are green. No remaining findings.