fix(orchestrator): address review on host control server transport (#468)
prd-number-check / require-numbered-prds (pull_request) Failing after 11s
test / integration-docker (pull_request) Successful in 20s
lint / lint (push) Successful in 59s
test / unit (pull_request) Failing after 52s
test / coverage (pull_request) Has been skipped
tracker-policy-pr / check-pr (pull_request) Failing after 11m18s

Codex review on #496:

- **High — ambiguous delivery no longer orphans a launched bottle.** A
  timeout / dropped response from the host controller is now the ambiguous
  BrokerUnavailableError (distinct from the definite BrokerAuthError /
  BrokerClientError). OrchestratorCore.launch_bottle keeps the registry
  row on the ambiguous case instead of deregistering — deregistering would
  orphan a running container with no record (reconcile reaps rows, never
  containers). The row is left for reconcile to reap iff the bottle is not
  actually live. Definite failures still roll back, so a real failure
  leaves no orphan row.
- **Medium — the privileged endpoint bounds request bodies.** The host
  server rejects an oversized Content-Length with 413 before reading it,
  and sets a per-request socket timeout, so a caller that can merely reach
  the socket (no signed token) can't exhaust memory or a handler thread.

Tests: ambiguous-keep vs definite-rollback in the launch path; the
BrokerUnavailableError/BrokerClientError split in BrokerClient; the 413
body cap + handler error paths (driven in-thread, since daemon request
threads lose coverage) plus a deterministic real-socket check that
declares an oversized Content-Length but sends a sliver (rejection on the
header, no unread-body reset race); and the __main__ entrypoint broker
selection. Diff-coverage 98%; pyright clean; pylint 9.8.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-07-26 08:43:00 +00:00
parent 79dd4926bb
commit 1553a98275
8 changed files with 340 additions and 30 deletions
+31 -5
View File
@@ -11,7 +11,12 @@ from contextlib import closing
from pathlib import Path
from unittest.mock import patch
from bot_bottle.orchestrator.broker import LaunchBroker, LaunchRequest, StubBroker
from bot_bottle.orchestrator.broker import (
BrokerUnavailableError,
LaunchBroker,
LaunchRequest,
StubBroker,
)
from bot_bottle.orchestrator.store.registry_store import RegistryStore
from bot_bottle.orchestrator.service import OrchestratorCore
from bot_bottle.orchestrator.store.secret_store import new_env_var_secret
@@ -25,8 +30,8 @@ from bot_bottle.orchestrator.supervisor import (
class _FailingBroker(LaunchBroker):
"""Verifies the token like any broker, then fails the launch — to
exercise the orchestrator's registry rollback."""
"""Verifies the token like any broker, then fails the launch *definitely*
to exercise the orchestrator's registry rollback."""
def _launch(self, req: LaunchRequest) -> None:
raise RuntimeError("launch failed")
@@ -35,6 +40,18 @@ class _FailingBroker(LaunchBroker):
pass
class _UnavailableBroker(LaunchBroker):
"""Verifies the token, then raises the *ambiguous* BrokerUnavailableError —
the host may already have launched — so the orchestrator must KEEP the
registry row rather than orphan a running container."""
def _launch(self, req: LaunchRequest) -> None:
raise BrokerUnavailableError("delivery dropped after send")
def _teardown(self, req: LaunchRequest) -> None:
pass
class TestOrchestrator(unittest.TestCase):
def setUp(self) -> None:
self._tmp = tempfile.TemporaryDirectory()
@@ -136,11 +153,20 @@ class TestOrchestrator(unittest.TestCase):
self.assertIsNotNone(self.orch.resolve("10.243.0.3", rec.identity_token))
self.assertIsNone(self.orch.resolve("10.243.0.3", "wrong-token"))
def test_launch_rolls_back_registry_on_broker_failure(self) -> None:
def test_launch_rolls_back_registry_on_definite_broker_failure(self) -> None:
orch = OrchestratorCore(self.store, _FailingBroker(self.secret), self.secret)
with self.assertRaises(RuntimeError):
orch.launch_bottle("10.243.0.9")
self.assertEqual([], self.store.all()) # no orphan
self.assertEqual([], self.store.all()) # no orphan row
def test_launch_keeps_registry_on_ambiguous_broker_failure(self) -> None:
# The host may already have launched the bottle before the response was
# lost, so deregistering would orphan a running container with no row.
# The row is kept for reconcile to reap iff the bottle is not live.
orch = OrchestratorCore(self.store, _UnavailableBroker(self.secret), self.secret)
with self.assertRaises(BrokerUnavailableError):
orch.launch_bottle("10.243.0.9")
self.assertEqual(1, len(self.store.all())) # row survives — no orphan container
def test_gateway_status_reports_unconfigured(self) -> None:
# The orchestrator no longer owns a standalone gateway lifecycle; the