From 910267b8a8cbdf433ffaa344a6528c7c1e8d3050 Mon Sep 17 00:00:00 2001 From: didericis Date: Tue, 14 Jul 2026 04:07:07 -0400 Subject: [PATCH] fix(orchestrator): only recreate the orchestrator on a real source change ensure_running() unconditionally recreated the bot-bottle-orchestrator container on every bottle launch, added so bind-mounted code changes take effect (the process never reloads). But the orchestrator's per-bottle egress auth tokens live only in its process memory (Orchestrator._tokens), by design never persisted to disk. So starting a second bottle recreated the container and silently dropped the FIRST bottle's already-injected token -- its next authed egress call 403s with "env var EGRESS_TOKEN_0 is unset", and it needs a full restart to recover. Now the orchestrator container is labeled with a content hash of its bind-mounted bot_bottle source, mirroring the gateway's existing image-staleness check. ensure_running only recreates on a hash mismatch (a real code change), so a bottle launch that isn't accompanied by a code change leaves a healthy orchestrator -- and every other active bottle's in-memory tokens -- alone. Fixes #381. Co-Authored-By: Claude Sonnet 5 --- bot_bottle/orchestrator/lifecycle.py | 70 ++++++++++++++++++----- tests/unit/test_orchestrator_lifecycle.py | 70 +++++++++++++++++++---- 2 files changed, 116 insertions(+), 24 deletions(-) diff --git a/bot_bottle/orchestrator/lifecycle.py b/bot_bottle/orchestrator/lifecycle.py index 3fa7153..f8b70fa 100644 --- a/bot_bottle/orchestrator/lifecycle.py +++ b/bot_bottle/orchestrator/lifecycle.py @@ -16,6 +16,7 @@ names + the published port). from __future__ import annotations +import hashlib import time import urllib.error import urllib.request @@ -29,6 +30,10 @@ from .gateway import GATEWAY_IMAGE, GATEWAY_NETWORK, DockerGateway DEFAULT_PORT = 8099 ORCHESTRATOR_NAME = "bot-bottle-orchestrator" ORCHESTRATOR_LABEL = "bot-bottle-orchestrator=1" +# Baked onto the container as a label so `ensure_running` can tell whether the +# running process is executing the *current* bind-mounted source — see +# `_source_hash`. +ORCHESTRATOR_SOURCE_HASH_LABEL = "bot-bottle-orchestrator-source-hash" # The repo root is bind-mounted into the control-plane container so # `python -m bot_bottle.orchestrator` resolves the package (the orchestrator @@ -46,6 +51,22 @@ class OrchestratorStartError(RuntimeError): """The orchestrator container did not become healthy within the timeout.""" +def _source_hash(repo_root: Path) -> str: + """Content hash of the orchestrator's bind-mounted Python source (the + `bot_bottle` package the control-plane process imports). This only + changes when the code that would actually run inside the container + changes — `ensure_running` recreates the container on a mismatch and + otherwise leaves a healthy one alone, so a bottle launch that isn't + accompanied by a code change doesn't restart the process and drop every + *other* active bottle's in-memory egress tokens (`Orchestrator._tokens` + in `service.py`, never persisted to disk by design).""" + h = hashlib.sha256() + for path in sorted((repo_root / "bot_bottle").rglob("*.py")): + h.update(str(path.relative_to(repo_root)).encode()) + h.update(path.read_bytes()) + return h.hexdigest() + + class OrchestratorService: """Manages the orchestrator control-plane container + the shared gateway. Callers only need `ensure_running()` + `url`.""" @@ -88,14 +109,17 @@ class OrchestratorService: proc = run_docker(["docker", "ps", "--filter", f"name=^/{name}$", "--format", "{{.Names}}"]) return name in proc.stdout.split() - def _run_orchestrator_container(self) -> None: + def _run_orchestrator_container(self, source_hash: str) -> None: """Start the control-plane container (idempotent: clears a stale - fixed-name container first). Register-only broker → no docker socket.""" + fixed-name container first). Register-only broker → no docker socket. + Labels the container with `source_hash` so a later `ensure_running` + can detect a real code change (see `_source_hash`).""" run_docker(["docker", "rm", "--force", ORCHESTRATOR_NAME]) proc = run_docker([ "docker", "run", "--detach", "--name", ORCHESTRATOR_NAME, "--label", ORCHESTRATOR_LABEL, + "--label", f"{ORCHESTRATOR_SOURCE_HASH_LABEL}={source_hash}", "--network", self.network, # Host CLI reaches the control plane here; bound to loopback so it # is not exposed on the host's external interfaces. @@ -119,26 +143,46 @@ class OrchestratorService: def _gateway(self) -> DockerGateway: return DockerGateway(self.image, network=self.network, orchestrator_url=self.internal_url) + def _orchestrator_source_current(self, current_hash: str) -> bool: + """True iff the running orchestrator container was created from the + *current* bind-mounted source. Mirrors `DockerGateway`'s + image-staleness check, but by content hash rather than image id since + the orchestrator runs bind-mounted source, not a built image.""" + if not self._container_running(ORCHESTRATOR_NAME): + return False + proc = run_docker([ + "docker", "inspect", "--format", + "{{ index .Config.Labels \"" + ORCHESTRATOR_SOURCE_HASH_LABEL + "\" }}", + ORCHESTRATOR_NAME, + ]) + if proc.returncode != 0: + return True # can't compare -> don't churn a working container + return proc.stdout.strip() == current_hash + def ensure_running( self, *, startup_timeout: float = DEFAULT_STARTUP_TIMEOUT_SECONDS, ) -> str: """Ensure the control plane + shared gateway are up; return the host - control-plane URL. Idempotent — a healthy control plane and a running - gateway are left untouched. Raises `OrchestratorStartError` on - timeout.""" + control-plane URL. Idempotent — a healthy control plane running + current code and a running gateway are left untouched. Raises + `OrchestratorStartError` on timeout.""" gateway = self._gateway() gateway.ensure_built() # rebuild the bundle image on a source change gateway.ensure_running() # creates the shared network + (re)starts gateway - # Always (re)create the orchestrator container. It runs the repo's code - # bind-mounted, but the Python process loaded that code at startup and - # won't reload — so reusing a healthy-but-stale container would keep - # running OLD control-plane code (e.g. dropping the tokens field). Cheap - # (~seconds); the registry DB persists and the current launch - # re-registers its own in-memory state. (The dedicated orchestrator - # image follow-up replaces this with image-staleness detection.) + # Recreate the orchestrator container only when its bind-mounted + # source has actually changed since it started — its Python process + # loaded that code at startup and won't reload, so a stale container + # would keep running OLD control-plane code. Recreating on *every* + # launch (the prior behaviour) would drop every other active + # bottle's in-memory egress tokens each time a new bottle starts, + # since the orchestrator process holds them only in memory (#381). + current_hash = _source_hash(self._repo_root) + if self.is_healthy() and self._orchestrator_source_current(current_hash): + return self.url + log.info("starting orchestrator container", context={"name": ORCHESTRATOR_NAME}) - self._run_orchestrator_container() + self._run_orchestrator_container(current_hash) deadline = time.monotonic() + startup_timeout while time.monotonic() < deadline: diff --git a/tests/unit/test_orchestrator_lifecycle.py b/tests/unit/test_orchestrator_lifecycle.py index 4eeccba..30a7f82 100644 --- a/tests/unit/test_orchestrator_lifecycle.py +++ b/tests/unit/test_orchestrator_lifecycle.py @@ -10,8 +10,10 @@ from unittest.mock import MagicMock, Mock, patch from bot_bottle.orchestrator.lifecycle import ( ORCHESTRATOR_NAME, + ORCHESTRATOR_SOURCE_HASH_LABEL, OrchestratorService, OrchestratorStartError, + _source_hash, ) from tests.unit import use_bottle_root @@ -28,6 +30,10 @@ def _health(status: int) -> MagicMock: return m +def _proc(returncode: int = 0, stdout: str = "", stderr: str = "") -> Mock: + return Mock(returncode=returncode, stdout=stdout, stderr=stderr) + + class TestOrchestratorService(unittest.TestCase): def setUp(self) -> None: self._tmp = tempfile.TemporaryDirectory() @@ -46,25 +52,67 @@ class TestOrchestratorService(unittest.TestCase): with patch(_URLOPEN, side_effect=urllib.error.URLError("refused")): self.assertFalse(self.svc.is_healthy()) - def test_ensure_running_always_recreates_orchestrator(self) -> None: - # Even when a control plane is already healthy, the orchestrator is - # recreated so bind-mounted code changes take effect (its process - # won't reload). The gateway is ensured too. - run = Mock(return_value=Mock(returncode=0, stderr="")) + def test_ensure_running_noop_when_healthy_and_source_unchanged(self) -> None: + # A healthy control plane already running the *current* bind-mounted + # source is left alone — recreating it on every launch would drop + # every other active bottle's in-memory egress tokens (#381). + current = _source_hash(self.svc._repo_root) + calls: list[list[str]] = [] + + def fake(argv: list[str]) -> Mock: + calls.append(argv) + if argv[:2] == ["docker", "ps"]: + return _proc(stdout=ORCHESTRATOR_NAME) + if argv[:2] == ["docker", "inspect"]: + return _proc(stdout=current) + return _proc() + with patch(_URLOPEN, return_value=_health(200)), \ - patch(_GATEWAY) as gw_cls, patch(_RUN, run), patch(_SLEEP): + patch(_GATEWAY) as gw_cls, patch(_RUN, side_effect=fake), patch(_SLEEP): self.assertEqual(self.svc.url, self.svc.ensure_running()) gw_cls.return_value.ensure_running.assert_called() # gateway kept up - runs = [c.args[0] for c in run.call_args_list if c.args[0][:2] == ["docker", "run"]] - self.assertEqual(1, len(runs)) # orchestrator recreated + runs = [c for c in calls if c[:2] == ["docker", "run"]] + rms = [c for c in calls if c[:3] == ["docker", "rm", "--force"] and ORCHESTRATOR_NAME in c] + self.assertEqual([], runs) # not recreated + self.assertEqual([], rms) + + def test_ensure_running_recreates_when_source_changed(self) -> None: + # Healthy, but the running container's label doesn't match the + # current source hash (a real code change) — recreate so it takes + # effect, same as the gateway's image-staleness check. + calls: list[list[str]] = [] + + def fake(argv: list[str]) -> Mock: + calls.append(argv) + if argv[:2] == ["docker", "ps"]: + return _proc(stdout=ORCHESTRATOR_NAME) + if argv[:2] == ["docker", "inspect"]: + return _proc(stdout="stale-hash") + return _proc() + + with patch(_URLOPEN, return_value=_health(200)), \ + patch(_GATEWAY), patch(_RUN, side_effect=fake), patch(_SLEEP): + self.assertEqual(self.svc.url, self.svc.ensure_running()) + runs = [c for c in calls if c[:2] == ["docker", "run"]] + self.assertEqual(1, len(runs)) self.assertIn(ORCHESTRATOR_NAME, runs[0]) + # the fresh container is labeled with the current hash, not the stale one + current = _source_hash(self.svc._repo_root) + self.assertIn(f"{ORCHESTRATOR_SOURCE_HASH_LABEL}={current}", runs[0]) def test_ensure_running_starts_orchestrator_container_when_absent(self) -> None: - run = Mock(return_value=Mock(returncode=0, stderr="")) + calls: list[list[str]] = [] + + def fake(argv: list[str]) -> Mock: + calls.append(argv) + if argv[:2] == ["docker", "ps"]: + return _proc(stdout="") # not running + return _proc() + with patch(_URLOPEN, side_effect=[urllib.error.URLError("down"), _health(200)]), \ - patch(_GATEWAY), patch(_RUN, run), patch(_SLEEP): + patch(_GATEWAY), patch(_RUN, side_effect=fake), patch(_SLEEP): self.assertEqual(self.svc.url, self.svc.ensure_running()) - runs = [c.args[0] for c in run.call_args_list if c.args[0][:2] == ["docker", "run"]] + runs = [c for c in calls if c[:2] == ["docker", "run"]] self.assertEqual(1, len(runs)) argv = runs[0] self.assertIn(ORCHESTRATOR_NAME, argv)