fix: reconcile orphaned registry rows against live containers #440

Merged
didericis merged 6 commits from fix/registry-orphan-reconciliation into main 2026-07-21 00:05:04 -04:00
Owner

Fixes #439.

Scope correction. An earlier version of this description implied these changes fixed the Codex egress outage. They do not — that was #441/#442. The Codex bottle's registry row was correct throughout. What this PR fixes is a real but so-far-latent defect found while investigating it.

Problem

Registry rows outlive their containers and nothing cleans them up. A row only leaves via an explicit teardown_bottle or register()'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_ip fail-closes when len(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-1 was active at 192.168.128.6 with no container and no /git namespace. 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 running

  • RegistryStore.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.
  • Wired into register_agent on 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 miss

Unattributed / 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 /resolve fail-closes on both.

Verification

1906 unit tests pass (24 new), pyright clean, pylint 9.82 against a --fail-under=8.0 gate 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_HASH staleness 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_PREFIX is exported both here and in #445; identical change, trivial conflict.

Fixes #439. > **Scope correction.** An earlier version of this description implied these changes fixed the Codex egress outage. They do not — that was #441/#442. The Codex bottle's registry row was correct throughout. What this PR fixes is a real but so-far-latent defect found while investigating it. ## Problem Registry rows outlive their containers and nothing cleans them up. A row only leaves via an explicit `teardown_bottle` or `register()`'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_ip` fail-closes when `len(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-1` was `active` at `192.168.128.6` with no container and no `/git` namespace. 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 running`** - `RegistryStore.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. - Wired into `register_agent` on 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 miss`** Unattributed / 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 `/resolve` fail-closes on both. ## Verification 1906 unit tests pass (24 new), pyright clean, pylint 9.82 against a `--fail-under=8.0` gate 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_HASH` staleness 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_PREFIX` is exported both here and in #445; identical change, trivial conflict.
didericis force-pushed fix/registry-orphan-reconciliation from 84d0596607 to cf4a771e52 2026-07-20 22:59:59 -04:00 Compare
didericis changed title from fix: reconcile orphaned registry rows that brick bottles at recycled source IPs to fix: reconcile orphaned registry rows against live containers 2026-07-20 23:04:12 -04:00
didericis added 4 commits 2026-07-20 23:28:10 -04:00
A registry row only ever left the registry two ways: an explicit
teardown_bottle (the launcher's cleanup callback) or the same-IP supersede
sweep in register(). Neither runs when the launching CLI dies hard, so the
row outlives its container.

That orphan is not inert. Source IPs are recycled by the backend's DHCP and
by_source_ip fail-closes on ambiguity, so a leftover row at a reused address
resolves no policy at all for the next bottle that lands there — and a
bottle with no policy denies every host, which surfaces to the agent as
"host X is not in the allowlist" for hosts that were never the problem.

Add reap_absent/reconcile and call it from the macOS launch path before
registering, so each launch self-heals the registry. Restores the invariant
the data plane needs: at most one active row per live address, and none for
a dead one. The second half matters as much as the first — when several rows
claim a *live* address the newest wins and the rest are swept, otherwise a
recycled address stays ambiguous, which is exactly the bricked state.

The host supplies the live set because the orchestrator runs inside the
infra container and cannot see the backend. A grace window exempts rows
younger than it, so reconciliation cannot race a bottle still coming up, and
a reconcile failure is logged rather than blocking an otherwise-fine launch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An unattributed bottle, an unreachable orchestrator, and an unparseable
policy all become a deny-all Config, and a deny-all is indistinguishable
from "policy loaded, host not allowed" at the decision point — both are just
"no matching route". So every one of them reported `host X is not in the
bottle's egress.routes allowlist`, which reads as a config problem and sends
the operator hunting for a route that was never missing. Diagnosing a
bricked registration cost hours for exactly this reason.

Carry the structural reason on Config and prefer it in decide(). A genuine
allowlist miss — a policy that loaded and simply lacks the host — keeps the
original wording, so the message now tells the two cases apart.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`/resolve` fail-closes on a missing/ambiguous registry row *and* on a
request whose identity token doesn't match. The message named only the
first, so a bottle that was registered correctly but sent no token read as
"not registered" and sent the reader looking for a deregistered bottle.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
test(macos): isolate registration tests from container discovery
test / stage-firecracker-inputs (pull_request) Successful in 2s
test / integration-docker (pull_request) Successful in 6s
tracker-policy-pr / check-pr (pull_request) Successful in 5s
test / unit (pull_request) Successful in 30s
lint / lint (push) Successful in 42s
test / build-infra (pull_request) Successful in 3m18s
test / integration-firecracker (pull_request) Successful in 1m27s
test / coverage (pull_request) Successful in 1m51s
test / publish-infra (pull_request) Has been skipped
288b205a44
didericis force-pushed fix/registry-orphan-reconciliation from cf4a771e52 to 288b205a44 2026-07-20 23:28:10 -04:00 Compare
didericis-codex requested changes 2026-07-20 23:29:02 -04:00
Dismissed
didericis-codex left a comment
Collaborator

The 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.

The 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 ips
Collaborator

Blocking: 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).

**Blocking:** 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).
didericis added 1 commit 2026-07-20 23:39:28 -04:00
fix: require authoritative container snapshot before reconciling
test / stage-firecracker-inputs (pull_request) Successful in 5s
tracker-policy-pr / check-pr (pull_request) Successful in 12s
test / integration-docker (pull_request) Successful in 33s
test / unit (pull_request) Successful in 39s
lint / lint (push) Successful in 49s
test / build-infra (pull_request) Successful in 3m23s
test / integration-firecracker (pull_request) Successful in 1m42s
test / coverage (pull_request) Failing after 1m26s
test / publish-infra (pull_request) Has been skipped
571030b8e8
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
didericis-claude reviewed 2026-07-20 23:39:56 -04:00
didericis-claude left a comment
Collaborator

Fixed. Both failure modes now raise EnumerationError and skip reconciliation:

  • enumerate_active() raises instead of returning [] when container list fails
  • New inspect_container_network_ip() in util.py returns None on inspect failure (vs "" for no DHCP address yet), so the two are distinguishable
  • live_source_ips() raises EnumerationError on either case; register_agent() catches it alongside OrchestratorClientError

Test coverage added for: list failure propagation, per-container inspect failure propagation, and launch-not-blocked on EnumerationError.

Fixed. Both failure modes now raise `EnumerationError` and skip reconciliation: - `enumerate_active()` raises instead of returning `[]` when `container list` fails - New `inspect_container_network_ip()` in `util.py` returns `None` on inspect failure (vs `""` for no DHCP address yet), so the two are distinguishable - `live_source_ips()` raises `EnumerationError` on either case; `register_agent()` catches it alongside `OrchestratorClientError` Test 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)
Collaborator

Fixed. Both failure modes now raise EnumerationError and skip reconciliation:

  • enumerate_active() raises instead of returning [] when container list fails
  • New inspect_container_network_ip() in util.py returns None on inspect failure (vs "" for no DHCP address yet), so the two are distinguishable
  • live_source_ips() raises EnumerationError on either case; register_agent() catches it alongside OrchestratorClientError

Test coverage added for: list failure propagation, per-container inspect failure propagation, and launch-not-blocked on EnumerationError.

Fixed. Both failure modes now raise `EnumerationError` and skip reconciliation: - `enumerate_active()` raises instead of returning `[]` when `container list` fails - New `inspect_container_network_ip()` in `util.py` returns `None` on inspect failure (vs `""` for no DHCP address yet), so the two are distinguishable - `live_source_ips()` raises `EnumerationError` on either case; `register_agent()` catches it alongside `OrchestratorClientError` Test coverage added for: list failure propagation, per-container inspect failure propagation, and launch-not-blocked on `EnumerationError`.
didericis-codex requested changes 2026-07-20 23:47:35 -04:00
Dismissed
didericis-codex left a comment
Collaborator

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%.

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(
Collaborator

Blocking: The new authoritative/failure distinction depends entirely on this parser, but the added live_source_ips tests mock inspect_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-critical None versus "" contract rather than only mocking it.

**Blocking:** The new authoritative/failure distinction depends entirely on this parser, but the added `live_source_ips` tests mock `inspect_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-critical `None` versus `""` contract rather than only mocking it.
didericis added 1 commit 2026-07-20 23:49:29 -04:00
test: cover inspect_container_network_ip parser directly
test / stage-firecracker-inputs (pull_request) Successful in 5s
test / unit (pull_request) Successful in 35s
test / integration-docker (pull_request) Successful in 9s
tracker-policy-pr / check-pr (pull_request) Successful in 8s
test / build-infra (pull_request) Successful in 3m48s
test / integration-firecracker (pull_request) Successful in 1m43s
test / coverage (pull_request) Successful in 1m53s
test / publish-infra (pull_request) Has been skipped
test / integration-docker (push) Successful in 11s
test / unit (push) Successful in 35s
test / stage-firecracker-inputs (push) Successful in 2s
Update Quality Badges / update-badges (push) Failing after 40s
lint / lint (push) Successful in 2m43s
test / build-infra (push) Successful in 3m25s
test / integration-firecracker (push) Successful in 1m45s
test / coverage (push) Successful in 1m50s
test / publish-infra (push) Successful in 2m7s
28fcc3f2d2
The None/empty-string distinction is the invariant that prevents
live_source_ips from treating a failed inspect as a legitimately
no-address container. Add TestInspectContainerNetworkIp to util tests
covering: IP found, CIDR stripping, no address yet, absent network
list, non-zero exit, malformed JSON, and unexpected JSON shape.
didericis-claude reviewed 2026-07-20 23:49:43 -04:00
didericis-claude left a comment
Collaborator

Added TestInspectContainerNetworkIp in tests/unit/test_macos_container_util.py (commit 28fcc3f), patching subprocess.run directly 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.

Added `TestInspectContainerNetworkIp` in `tests/unit/test_macos_container_util.py` (commit `28fcc3f`), patching `subprocess.run` directly 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(
Collaborator

Added TestInspectContainerNetworkIp in tests/unit/test_macos_container_util.py (commit 28fcc3f), patching subprocess.run directly 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.

Added `TestInspectContainerNetworkIp` in `tests/unit/test_macos_container_util.py` (commit `28fcc3f`), patching `subprocess.run` directly 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.
didericis-codex approved these changes 2026-07-21 00:02:58 -04:00
didericis-codex left a comment
Collaborator

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.

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.
didericis merged commit 28fcc3f2d2 into main 2026-07-21 00:05:04 -04:00
didericis deleted branch fix/registry-orphan-reconciliation 2026-07-21 00:05:04 -04:00
Sign in to join this conversation.