diff --git a/bot_bottle/backend/base.py b/bot_bottle/backend/base.py index d9b4ad30..c2ea5df1 100644 --- a/bot_bottle/backend/base.py +++ b/bot_bottle/backend/base.py @@ -21,7 +21,7 @@ from abc import ABC, abstractmethod from contextlib import AbstractContextManager, contextmanager from dataclasses import dataclass from pathlib import Path -from typing import Generator, Generic, Sequence, TypeVar +from typing import TYPE_CHECKING, Generator, Generic, Sequence, TypeVar from ..agent_provider import AgentProvisionPlan, get_provider from ..egress import EgressPlan @@ -35,6 +35,9 @@ from ..workspace import WorkspacePlan, workspace_plan from .print_util import print_multi, visible_agent_env_names from .util import host_skill_dir +if TYPE_CHECKING: + from .gateway_attach import GatewayAttachResources + class BackendStatus(enum.IntEnum): """Return codes for BottleBackend.status(). READY == 0 so callsites @@ -270,6 +273,11 @@ class Bottle(ABC): PlanT = TypeVar("PlanT", bound=BottlePlan) CleanupT = TypeVar("CleanupT", bound=BottleCleanupPlan) +# The backend-specific handle for one running bottle the reconcile attaches to +# the gateway (a run dir for firecracker, a container name for docker/macOS). +# Registry-style callers that don't care about the handle bind it to `Any` +# (`BottleBackend[Any, Any, Any]`). +AttachTargetT = TypeVar("AttachTargetT") class InfraLaunchError(RuntimeError): @@ -290,7 +298,7 @@ class BottleImages: sidecar: str | Path = "" -class BottleBackend(ABC, Generic[PlanT, CleanupT]): +class BottleBackend(ABC, Generic[PlanT, CleanupT, AttachTargetT]): """Abstract base for selectable bottle backends. Concrete subclasses (e.g. DockerBottleBackend) own their own prepare/launch impls. Parameterized over the backend's concrete plan + cleanup-plan types @@ -518,26 +526,30 @@ class BottleBackend(ABC, Generic[PlanT, CleanupT]): compose ls`; firecracker cross-references its running gateway containers against per-bottle metadata.""" - @abstractmethod def attach_bottled_agents_to_gateway(self) -> None: - """Reconcile every already-running bottle against the freshly-(re)booted - gateway (PRD 0081). The host calls this on the gateway bring-up path — - the cold-boot branch only, never on an adopt of a healthy current - gateway — so a gateway rebuild/restart doesn't silently break running - bottles' gateway-dependent state. + """Reconcile every running bottle against the freshly-(re)booted gateway + on the cold-boot bring-up path (PRD 0081). The shared flow + fail-hard + policy live in `gateway_attach`; backends override only the three + primitives below (ADR 0006).""" + from .gateway_attach import reconcile_running_bottles + reconcile_running_bottles(self) - Reconciling running bottles is a backend responsibility: only the - backend can enumerate its agents and reach them (firecracker over SSH, - docker/macOS over `exec`/`cp`), so every backend must implement it — - cross-backend by construction. Each per-bottle step is best-effort: one - unreachable or malformed bottle is logged and skipped so it never blocks - the others or the gateway coming up. + @abstractmethod + def _gateway_attach_resources(self) -> "GatewayAttachResources": + """Gather what every bottle needs to (re)attach to the current gateway + (the CA now). Raises if the gateway isn't reachable (aborts reconcile).""" - Currently reconciles the **CA**: it replaces each agent's trusted CA - with the current gateway CA (unconditional — there is one gateway, so no - fingerprint match is needed), which rotates the CA for free on every - bring-up. Git-gate re-provision and egress-token restore fold into this - same reconcile on the same trigger (see #516).""" + @abstractmethod + def _running_bottles(self) -> Sequence[AttachTargetT]: + """The live bottles as backend handles for `_attach_bottle_to_gateway`. + Raises if the live set can't be determined (fail hard, no partial set).""" + + @abstractmethod + def _attach_bottle_to_gateway( + self, bottle: AttachTargetT, resources: "GatewayAttachResources", + ) -> None: + """(Re)attach one bottle to the current gateway (SSH / `exec`+`cp`). + Raises `InfraLaunchError` naming the bottle on failure.""" @classmethod @abstractmethod diff --git a/bot_bottle/backend/docker/backend.py b/bot_bottle/backend/docker/backend.py index c4f821ba..83e61815 100644 --- a/bot_bottle/backend/docker/backend.py +++ b/bot_bottle/backend/docker/backend.py @@ -33,6 +33,7 @@ from ...git_gate import GitGatePlan from ...supervisor.plan import SupervisePlan from ...manifest import Manifest from .. import ActiveAgent, BottleBackend, BottleImages, BottleSpec +from ..gateway_attach import GatewayAttachResources from . import cleanup as _cleanup from . import enumerate as _enumerate from . import launch as _launch @@ -40,7 +41,7 @@ from . import resolve_plan as _resolve_plan from .bottle import DockerBottle from .bottle_cleanup_plan import DockerBottleCleanupPlan from .bottle_plan import DockerBottlePlan -class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanupPlan"]): +class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanupPlan", str]): """Docker backend implementation. Selected by BOT_BOTTLE_BACKEND when set to `docker`; retained as a legacy/example backend.""" @@ -141,6 +142,17 @@ class DockerBottleBackend(BottleBackend["DockerBottlePlan", "DockerBottleCleanup def enumerate_active(self) -> Sequence[ActiveAgent]: return _enumerate.enumerate_active() - def attach_bottled_agents_to_gateway(self) -> None: - from .infra_launch import attach_bottled_agents_to_gateway - attach_bottled_agents_to_gateway() + def _gateway_attach_resources(self) -> GatewayAttachResources: + from .infra import DockerInfraService + return GatewayAttachResources( + ca_pem=DockerInfraService().gateway().ca_cert_pem()) + + def _running_bottles(self) -> Sequence[str]: + from .infra_launch import running_agent_containers + return running_agent_containers() + + def _attach_bottle_to_gateway( + self, bottle: str, resources: GatewayAttachResources, + ) -> None: + from .infra_launch import push_ca_to_container + push_ca_to_container(bottle, resources.ca_pem) diff --git a/bot_bottle/backend/docker/infra_launch.py b/bot_bottle/backend/docker/infra_launch.py index 144a8cbd..5a0ef5c0 100644 --- a/bot_bottle/backend/docker/infra_launch.py +++ b/bot_bottle/backend/docker/infra_launch.py @@ -78,11 +78,32 @@ def _network_container_ips(network: str) -> list[str]: return ips -def _push_ca_to_container(container: str, ca_pem: str) -> None: - """Replace one running agent container's trusted gateway CA with `ca_pem` - and rebuild its trust store via `docker cp` + `docker exec`. Unconditional - install — there is one gateway, so no fingerprint match is needed. Raises - `InfraLaunchError` on failure.""" +def running_agent_containers( + network: str = GATEWAY_NETWORK, gateway_name: str = INFRA_NAME, +) -> list[str]: + """Every running agent container on the gateway `network` (the gateway + itself excluded) — the bottles the bring-up reconcile attaches to the fresh + gateway (PRD 0081). Raises `OSError` if `docker` can't be run (fail hard: a + reconcile that can't list its bottles must not look like "no bottles").""" + proc = run_docker([ + "docker", "network", "inspect", "--format", + "{{range .Containers}}{{.Name}}\n{{end}}", network, + ]) + return [ + name for name in (line.strip() for line in proc.stdout.splitlines()) + if name and name != gateway_name + ] + + +def push_ca_to_container(container: str, ca_pem: str) -> None: + """(Re)attach one running agent container to the current gateway: replace + its trusted gateway CA with `ca_pem` and rebuild its trust store via + `docker cp` + `docker exec` (PRD 0081). Unconditional install — there is one + gateway, so no fingerprint match is needed. + + Fail hard: raises `InfraLaunchError` (naming the container) on any failure — + the base reconcile aggregates and surfaces it rather than leaving the agent + silently unable to reach the gateway.""" mkdir = run_docker( ["docker", "exec", container, "mkdir", "-p", "/usr/local/share/ca-certificates"] @@ -110,45 +131,6 @@ def _push_ca_to_container(container: str, ca_pem: str) -> None: ) -def attach_bottled_agents_to_gateway( - ca_pem: str | None = None, - *, - network: str = GATEWAY_NETWORK, - gateway_name: str = INFRA_NAME, -) -> None: - """Reconcile every running agent container against the freshly-(re)created - gateway (PRD 0081): replace each agent's trusted CA with the current gateway - CA, so a gateway rebuild doesn't silently break their TLS egress (#510). - - Per-container steps are best-effort: one failure is logged and skipped so it - never blocks the others or the gateway coming up.""" - if ca_pem is None: - ca_pem = DockerInfraService().gateway().ca_cert_pem() - try: - proc = run_docker([ - "docker", "network", "inspect", "--format", - "{{range .Containers}}{{.Name}}\n{{end}}", network, - ]) - except OSError as exc: - log.info(f"CA reconcile skipped: {exc}") - return - reconciled = 0 - for line in proc.stdout.splitlines(): - name = line.strip() - if not name or name == gateway_name: - continue - try: - _push_ca_to_container(name, ca_pem) - reconciled += 1 - except (OSError, InfraLaunchError) as exc: - log.info(f"CA reconcile skipped for {name}: {exc}") - if reconciled: - log.info( - "reconciled gateway CA into running docker bottle(s)", - context={"count": reconciled}, - ) - - def _reprovision_running_bottles( orchestrator_url: str, network: str = GATEWAY_NETWORK, @@ -255,7 +237,8 @@ def deprovision_consolidated( __all__ = [ "LaunchContext", "launch_consolidated", - "attach_bottled_agents_to_gateway", + "running_agent_containers", + "push_ca_to_container", "deprovision_consolidated", "InfraLaunchError", ] diff --git a/bot_bottle/backend/firecracker/backend.py b/bot_bottle/backend/firecracker/backend.py index 3200891a..edb927e2 100644 --- a/bot_bottle/backend/firecracker/backend.py +++ b/bot_bottle/backend/firecracker/backend.py @@ -20,6 +20,7 @@ from ...git_gate import GitGatePlan from ...manifest import Manifest from ...supervisor.plan import SupervisePlan from .. import ActiveAgent, BottleBackend, BottleImages, BottleSpec +from ..gateway_attach import GatewayAttachResources from . import cleanup as _cleanup from . import enumerate as _enumerate from . import launch as _launch @@ -31,7 +32,7 @@ from .bottle_plan import FirecrackerBottlePlan class FirecrackerBottleBackend( - BottleBackend["FirecrackerBottlePlan", "FirecrackerBottleCleanupPlan"] + BottleBackend["FirecrackerBottlePlan", "FirecrackerBottleCleanupPlan", "Path"] ): name = "firecracker" @@ -119,9 +120,18 @@ class FirecrackerBottleBackend( def enumerate_active(self) -> Sequence[ActiveAgent]: return _enumerate.enumerate_active() - def attach_bottled_agents_to_gateway(self) -> None: - from .infra_launch import attach_bottled_agents_to_gateway - attach_bottled_agents_to_gateway() + def _gateway_attach_resources(self) -> GatewayAttachResources: + from .gateway import FirecrackerGateway + return GatewayAttachResources(ca_pem=FirecrackerGateway().ca_cert_pem()) + + def _running_bottles(self) -> Sequence[Path]: + return _cleanup.live_run_dirs() + + def _attach_bottle_to_gateway( + self, bottle: Path, resources: GatewayAttachResources, + ) -> None: + from .infra_launch import attach_ca_to_agent_vm + attach_ca_to_agent_vm(bottle, resources.ca_pem) def supervise_mcp_url(self, plan: FirecrackerBottlePlan) -> str: return plan.agent_supervise_url diff --git a/bot_bottle/backend/firecracker/infra_launch.py b/bot_bottle/backend/firecracker/infra_launch.py index c2af3391..a55a7733 100644 --- a/bot_bottle/backend/firecracker/infra_launch.py +++ b/bot_bottle/backend/firecracker/infra_launch.py @@ -32,7 +32,6 @@ from dataclasses import dataclass from pathlib import Path from ...egress import EgressPlan -from ...gateway import GatewayError from ...git_gate import GitGatePlan from ...log import info from ...orchestrator.client import OrchestratorClient, OrchestratorClientError @@ -88,56 +87,42 @@ def persist_env_var_secret(private_key: Path, guest_ip: str, secret: str) -> Non ) -def _push_gateway_ca_to_agent(private_key: Path, guest_ip: str, ca_pem: str) -> None: - """Replace one running agent VM's trusted gateway CA with `ca_pem` and - rebuild its trust store over SSH. Unconditional install — there is one +def attach_ca_to_agent_vm(run_dir: Path, ca_pem: str) -> None: + """(Re)attach one running agent VM (identified by its `run_dir`) to the + current gateway: replace its trusted gateway CA with `ca_pem` and rebuild + its trust store over SSH (PRD 0081). Unconditional install — there is one gateway, so no fingerprint match is needed. Bare-pipe input keeps the cert - off argv. Raises `InfraLaunchError` on failure.""" + off argv. + + Fail hard: raises `InfraLaunchError` (naming the bottle) if the run dir is + malformed or the push fails — the base reconcile aggregates and surfaces it + rather than leaving the agent silently unable to reach the gateway.""" + guest_ip = _guest_ip_from_config(run_dir / "config.json") + private_key = run_dir / "bottle_id_ed25519" + if not guest_ip or not private_key.is_file(): + raise InfraLaunchError( + f"{run_dir.name}: cannot resolve guest IP / SSH key to attach it " + f"to the gateway" + ) install = ( "umask 022; mkdir -p /usr/local/share/ca-certificates && " f"cat > {AGENT_CA_PATH} && chmod 644 {AGENT_CA_PATH} && " "update-ca-certificates" ) - proc = subprocess.run( - util.ssh_base_argv(private_key, guest_ip) + [install], - input=ca_pem, capture_output=True, text=True, check=False, - ) + try: + proc = subprocess.run( + util.ssh_base_argv(private_key, guest_ip) + [install], + input=ca_pem, capture_output=True, text=True, check=False, + ) + except OSError as exc: + raise InfraLaunchError(f"{run_dir.name} ({guest_ip}): CA push failed: {exc}") from exc if proc.returncode != 0: raise InfraLaunchError( - f"CA push to {guest_ip} failed: " + f"{run_dir.name} ({guest_ip}): CA push failed: " f"{proc.stderr.strip() or ''}" ) -def attach_bottled_agents_to_gateway() -> None: - """Reconcile every running agent VM against the freshly-booted gateway - (PRD 0081). Replaces each agent's trusted CA with the current gateway CA — - the gateway rootfs is ephemeral, so a cold boot mints a fresh CA and this is - what distributes it (a gateway rebuild doubles as a free CA rotation, #510). - - Per-bottle steps are best-effort: one unreachable or malformed VM is logged - and skipped so it never blocks the others or the gateway coming up.""" - gateway = FirecrackerGateway() - try: - ca_pem = gateway.ca_cert_pem() - except GatewayError as exc: - info(f"bring-up reconcile skipped: gateway CA unavailable: {exc}") - return - reconciled = 0 - for run_dir in cleanup.live_run_dirs(): - guest_ip = _guest_ip_from_config(run_dir / "config.json") - private_key = run_dir / "bottle_id_ed25519" - if not guest_ip or not private_key.is_file(): - continue - try: - _push_gateway_ca_to_agent(private_key, guest_ip, ca_pem) - reconciled += 1 - except (OSError, InfraLaunchError) as exc: - info(f"CA reconcile skipped for {guest_ip}: {exc}") - if reconciled: - info(f"reconciled gateway CA into {reconciled} running Firecracker bottle(s)") - - def _reprovision_running_bottles(client: OrchestratorClient) -> None: """Read keys from live agent VMs and restore the restarted gateway.""" try: @@ -210,7 +195,7 @@ def deprovision_consolidated( __all__ = [ "LaunchContext", "launch_consolidated", - "attach_bottled_agents_to_gateway", + "attach_ca_to_agent_vm", "deprovision_consolidated", "InfraLaunchError", "OrchestratorStartError", diff --git a/bot_bottle/backend/gateway_attach.py b/bot_bottle/backend/gateway_attach.py new file mode 100644 index 00000000..1f805671 --- /dev/null +++ b/bot_bottle/backend/gateway_attach.py @@ -0,0 +1,54 @@ +"""The shared gateway-attach reconcile flow (PRD 0081). + +`BottleBackend.attach_bottled_agents_to_gateway()` delegates here so every +backend reconciles running bottles against a freshly-booted gateway *the same +way* — the control flow + fail-hard policy live in one place and backends +override only the primitives (see ADR 0006). Kept out of `backend/base.py` so +the backend contract module stays lean (the base.py size guardrail). +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any + +from .base import InfraLaunchError + +if TYPE_CHECKING: + from .base import BottleBackend + + +@dataclass(frozen=True) +class GatewayAttachResources: + """Everything a running bottle needs to (re)attach to the current gateway on + a cold boot (PRD 0081). Gathered once per reconcile and handed to every + bottle. The CA now; egress secrets and git-gate state join as the reconcile + grows (#516).""" + + ca_pem: str + + +def reconcile_running_bottles(backend: "BottleBackend[Any, Any, Any]") -> None: + """Reconcile every already-running bottle against the freshly-(re)booted + gateway: gather the attach resources once, then attach every live bottle. + + Fail hard, never skip: a bottle that silently can't reach the fresh gateway + (its egress just starts failing TLS) is worse than a loud bring-up failure, + so any attach failure aborts bring-up. Every bottle is attempted first and + the failures are raised together, so one bring-up surfaces the full blast + radius rather than one bottle at a time.""" + resources = backend._gateway_attach_resources() + failures: list[str] = [] + for bottle in backend._running_bottles(): + try: + backend._attach_bottle_to_gateway(bottle, resources) + except InfraLaunchError as exc: + failures.append(str(exc)) + if failures: + raise InfraLaunchError( + f"could not attach {len(failures)} running bottle(s) to the " + f"freshly-booted gateway:\n " + "\n ".join(failures) + ) + + +__all__ = ["GatewayAttachResources", "reconcile_running_bottles"] diff --git a/bot_bottle/backend/macos_container/backend.py b/bot_bottle/backend/macos_container/backend.py index 53df3c8d..850a5345 100644 --- a/bot_bottle/backend/macos_container/backend.py +++ b/bot_bottle/backend/macos_container/backend.py @@ -14,6 +14,7 @@ from ...git_gate import GitGatePlan from ...supervisor.plan import SupervisePlan from ...manifest import Manifest from .. import ActiveAgent, BottleBackend, BottleImages, BottleSpec +from ..gateway_attach import GatewayAttachResources from . import cleanup as _cleanup from . import enumerate as _enumerate from . import launch as _launch @@ -25,7 +26,7 @@ from .bottle_plan import MacosContainerBottlePlan class MacosContainerBottleBackend( - BottleBackend["MacosContainerBottlePlan", "MacosContainerBottleCleanupPlan"] + BottleBackend["MacosContainerBottlePlan", "MacosContainerBottleCleanupPlan", str] ): """Apple Container backend. Selected by `BOT_BOTTLE_BACKEND=macos-container` or @@ -117,9 +118,19 @@ class MacosContainerBottleBackend( def enumerate_active(self) -> Sequence[ActiveAgent]: return _enumerate.enumerate_active() - def attach_bottled_agents_to_gateway(self) -> None: - from .infra_launch import attach_bottled_agents_to_gateway - attach_bottled_agents_to_gateway() + def _gateway_attach_resources(self) -> GatewayAttachResources: + from .infra import MacosInfraService + return GatewayAttachResources(ca_pem=MacosInfraService().ca_cert_pem()) + + def _running_bottles(self) -> Sequence[str]: + from .infra_launch import running_agent_containers + return running_agent_containers() + + def _attach_bottle_to_gateway( + self, bottle: str, resources: GatewayAttachResources, + ) -> None: + from .infra_launch import push_ca_to_container + push_ca_to_container(bottle, resources.ca_pem) def supervise_mcp_url(self, plan: MacosContainerBottlePlan) -> str: return plan.agent_supervise_url diff --git a/bot_bottle/backend/macos_container/infra_launch.py b/bot_bottle/backend/macos_container/infra_launch.py index 5ca3a13d..76890bfe 100644 --- a/bot_bottle/backend/macos_container/infra_launch.py +++ b/bot_bottle/backend/macos_container/infra_launch.py @@ -97,11 +97,23 @@ def ensure_gateway( return endpoint -def _push_ca_to_container(name: str, ca_pem: str) -> None: - """Replace one running agent container's trusted gateway CA with `ca_pem` - and rebuild its trust store via `container cp` + `container exec`. - Unconditional install — there is one gateway, so no fingerprint match is - needed. Raises `InfraLaunchError` on failure.""" +def running_agent_containers() -> list[str]: + """Every running agent container's name — the bottles the bring-up reconcile + attaches to the fresh gateway (PRD 0081). Raises `EnumerationError` if the + live set can't be determined (fail hard rather than reconcile a partial + set).""" + return [f"{CONTAINER_NAME_PREFIX}{agent.slug}" for agent in enumerate_active()] + + +def push_ca_to_container(name: str, ca_pem: str) -> None: + """(Re)attach one running agent container to the current gateway: replace + its trusted gateway CA with `ca_pem` and rebuild its trust store via + `container cp` + `container exec` (PRD 0081). Unconditional install — there + is one gateway, so no fingerprint match is needed. + + Fail hard: raises `InfraLaunchError` (naming the container) on any failure — + the base reconcile aggregates and surfaces it rather than leaving the agent + silently unable to reach the gateway.""" mkdir = container_mod.run_container_argv( ["container", "exec", name, "mkdir", "-p", "/usr/local/share/ca-certificates"] @@ -131,32 +143,6 @@ def _push_ca_to_container(name: str, ca_pem: str) -> None: ) -def attach_bottled_agents_to_gateway(ca_pem: str | None = None) -> None: - """Reconcile every running agent container against the freshly-(re)created - gateway (PRD 0081): replace each agent's trusted CA with the current gateway - CA, so a gateway rebuild doesn't silently break their TLS egress (#510). - - Per-container steps are best-effort: one failure is logged and skipped so it - never blocks the others or the gateway coming up.""" - if ca_pem is None: - ca_pem = MacosInfraService().ca_cert_pem() - try: - agents = list(enumerate_active()) - except (EnumerationError, OSError) as exc: - info(f"CA reconcile skipped: {exc}") - return - reconciled = 0 - for agent in agents: - name = f"{CONTAINER_NAME_PREFIX}{agent.slug}" - try: - _push_ca_to_container(name, ca_pem) - reconciled += 1 - except (OSError, InfraLaunchError) as exc: - info(f"CA reconcile skipped for {name}: {exc}") - if reconciled: - info(f"reconciled gateway CA into {reconciled} running macOS bottle(s)") - - def _reprovision_running_bottles(endpoint: GatewayEndpoint) -> None: """Recover keys from live Apple containers and restore gateway tokens.""" try: @@ -259,7 +245,8 @@ __all__ = [ "GatewayEndpoint", "LaunchContext", "ensure_gateway", - "attach_bottled_agents_to_gateway", + "running_agent_containers", + "push_ca_to_container", "live_source_ips", "register_agent", "deprovision_consolidated", diff --git a/bot_bottle/backend/selection.py b/bot_bottle/backend/selection.py index 3b50d982..2a9908a5 100644 --- a/bot_bottle/backend/selection.py +++ b/bot_bottle/backend/selection.py @@ -26,10 +26,10 @@ from .base import ActiveAgent, BackendStatus, BottleBackend # Tests may replace _backends with a {name: fake} dict via patch.object; # _get_backends() returns the current module-level value as-is when it # is not None, so test fakes take effect without triggering real imports. -_backends: dict[str, BottleBackend[Any, Any]] | None = None +_backends: dict[str, BottleBackend[Any, Any, Any]] | None = None -def _get_backends() -> dict[str, BottleBackend[Any, Any]]: +def _get_backends() -> dict[str, BottleBackend[Any, Any, Any]]: """Return the registry of all backend instances, loading lazily on first call.""" global _backends # pylint: disable=global-statement if _backends is None: @@ -48,7 +48,7 @@ def get_bottle_backend( name: str | None = None, *, prompt: bool = True, -) -> BottleBackend[Any, Any]: +) -> BottleBackend[Any, Any, Any]: """Resolve the bottle backend. `name` precedence: diff --git a/docs/decisions/0006-shared-behavior-in-base-backend.md b/docs/decisions/0006-shared-behavior-in-base-backend.md new file mode 100644 index 00000000..4817153a --- /dev/null +++ b/docs/decisions/0006-shared-behavior-in-base-backend.md @@ -0,0 +1,65 @@ +# ADR 0006: Shared behaviour lives in the base backend + +- **Status:** Accepted +- **Date:** 2026-07-27 +- **Deciders:** didericis + +## Context + +`BottleBackend` has three concrete implementations (Firecracker, docker, +macOS-container). Cross-backend operations can be expressed two ways: + +1. **Abstract method per backend** — the base declares `@abstractmethod foo()` + and each backend writes its own `foo()` end to end. +2. **Template method** — the base owns the *control flow* of `foo()` as a + concrete method and each backend overrides small **primitives** it calls. + +Form 1 gives each backend total freedom, which is exactly the problem: the three +implementations drift. The bring-up reconcile (PRD 0081) first shipped as form 1 +and immediately diverged — each backend re-implemented "gather CA → list running +bottles → push to each", and one backend's copy silently skipped failures while +another's aborted, an inconsistency caught only in review (#519). The behaviour +that must be identical (the loop, the error policy, the ordering) was duplicated +in the place least likely to stay in sync. + +## Decision + +Prefer the template-method form for cross-backend behaviour: **encode shared +control flow and policy in the base backend; backends override primitives, not +control flow.** Lean toward *less* freedom in the concrete backends and *more* +consistency in the base. + +Concretely, a cross-backend operation on `BottleBackend` should be a concrete +method that: + +- owns the sequencing, iteration, and error policy (e.g. fail-hard vs. + best-effort, fail-fast vs. attempt-all-then-aggregate); and +- delegates only the irreducibly backend-specific steps to `@abstractmethod` + primitives with narrow, well-typed signatures. + +The first application is `attach_bottled_agents_to_gateway()` (PRD 0081): the +base gathers resources once, attempts every running bottle, and raises an +aggregate `InfraLaunchError` if any failed; backends supply only +`_gateway_attach_resources()`, `_running_bottles()`, and +`_attach_bottle_to_gateway()`. + +A backend-specific `@abstractmethod` with no shared control flow (e.g. +`_launch_impl`, `enumerate_active`) stays as form 1 — this ADR is about +operations whose *shape* should be uniform, not about banning abstract methods. + +## Consequences + +- The error policy, ordering, and loop for a cross-backend operation have one + source of truth; a new backend cannot forget them or diverge. +- New backends implement smaller, more focused primitives instead of re-deriving + a whole flow. +- The base carries more logic and needs generic-enough primitive signatures + (e.g. a backend-specific attach-target type parameter on `BottleBackend`). +- Genuinely backend-specific one-offs still use a plain abstract method; the + guidance is a default, not an absolute. + +## Links + +- PRD 0081 (`docs/prds/0081-reprovision-gateway-state-on-bringup.md`). +- Review that prompted this: PR #519. +- `bot_bottle/backend/base.py` (`BottleBackend.attach_bottled_agents_to_gateway`). diff --git a/docs/prds/0083-reprovision-gateway-state-on-bringup.md b/docs/prds/0083-reprovision-gateway-state-on-bringup.md index 7124221b..7007c507 100644 --- a/docs/prds/0083-reprovision-gateway-state-on-bringup.md +++ b/docs/prds/0083-reprovision-gateway-state-on-bringup.md @@ -47,8 +47,13 @@ destroy (the original #450 failure mode). different mechanism per service. - The CA **rotates** on every gateway bring-up (no long-lived CA), distributed to running agents by the same reconcile. -- Per-bottle failures are tolerated: one unreachable or malformed bottle does not - block the others or the gateway coming up. +- Reconcile failures are **loud, never hidden**: if a running bottle can't be + re-attached to the fresh gateway, the bring-up fails and names the bottle + rather than leaving it silently unable to reach the gateway (a bottle whose + egress just starts failing TLS is worse than a loud failure). Every bottle is + attempted first and the failures are raised together, so one bring-up surfaces + the full blast radius. (Reversed from an earlier "tolerate per-bottle + failures" draft after #519 review.) - **All backends** (Firecracker, docker, macOS) reconcile through the *same* contract — an abstract method on the backend base class, so a new backend cannot forget to implement it and none drifts onto a bespoke mechanism. @@ -61,15 +66,27 @@ destroy (the original #450 failure mode). ## Design -**The contract — `attach_bottled_agents_to_gateway()` on the backend ABC.** -Reconciling running bottles against the current gateway is a backend -responsibility (only the backend can enumerate its agents and reach them — -firecracker over SSH, docker/macOS over `exec`/`cp`), so it is an -`@abc.abstractmethod` on `BottleBackend` (`backend/base.py`). Every backend -implements it; the host calls it whenever the gateway is (re)brought up. This is -what makes the fix cross-backend by construction rather than a per-backend -follow-up. It reprovisions **all** registered bottles' gateway-dependent state: -CA, git-gate, and egress tokens. +**The contract — `attach_bottled_agents_to_gateway()`, a concrete flow on the +backend base.** Reconciling running bottles against the current gateway is a +backend responsibility (only the backend can enumerate its agents and reach them +— firecracker over SSH, docker/macOS over `exec`/`cp`). To keep every backend +reconciling identically, the *control flow* lives in a concrete +`attach_bottled_agents_to_gateway()` on `BottleBackend` (`backend/base.py`) and +each backend overrides only three primitives: + +1. `_gateway_attach_resources()` — gather what a bottle needs to attach (the + gateway CA now; egress secrets + git-gate state as this grows). +2. `_running_bottles()` — enumerate the live bottles as backend handles. +3. `_attach_bottle_to_gateway(bottle, resources)` — push the resources into one + bottle, raising `InfraLaunchError` (naming it) on failure. + +The base gathers resources once, attempts every bottle, and — fail hard, never +skip — raises an aggregate `InfraLaunchError` if any failed, so a backend can't +drift onto a bespoke loop or a silent skip. (Encoding shared behaviour in the +base backend, with subclasses overriding primitives rather than control flow, is +[ADR 0006](../decisions/0006-shared-behavior-in-base-backend.md).) The host +calls it whenever the gateway is (re)brought up. It reprovisions **all** +registered bottles' gateway-dependent state: CA, git-gate, and egress tokens. **Trigger — the gateway bring-up path.** The host calls `attach_bottled_agents_to_gateway()` only when the gateway was actually @@ -98,7 +115,9 @@ and its CA is available: + per-repo creds under `/git/`. - **egress token:** read the agent's `ENV_VAR_SECRET` and feed `reprovision_bottles`, restoring the orchestrator's in-memory tokens. -4. Every per-bottle step is wrapped so one failure is logged and skipped. +4. If any per-bottle step fails, the reconcile attempts the rest and then raises + an aggregate naming every bottle that failed — bring-up fails loudly rather + than leaving a bottle silently unable to reach the gateway. **Retire the persistence prototype.** No CA data volume, no git-gate data volume (the abandoned PRs #511 / #513). The launch-time `_reprovision_running_bottles` diff --git a/tests/unit/test_architecture_guardrails.py b/tests/unit/test_architecture_guardrails.py index c4a0ca83..109d830a 100644 --- a/tests/unit/test_architecture_guardrails.py +++ b/tests/unit/test_architecture_guardrails.py @@ -83,8 +83,13 @@ class TestRuntimeModuleSizes(unittest.TestCase): self.assertEqual([], violations) def test_backend_contract_does_not_absorb_preparation_logic(self) -> None: + # base.py holds the backend *contract*; implementation lives elsewhere. + # The cap is a tripwire against absorbing preparation/impl logic, not a + # ban on new contract surface — bumped 580->600 for the PRD 0081 + # gateway-attach contract (delegator + 3 abstract primitives; the flow + # itself lives in backend/gateway_attach.py, not here). caps = { - ROOT / "bot_bottle" / "backend" / "base.py": 580, + ROOT / "bot_bottle" / "backend" / "base.py": 600, ROOT / "bot_bottle" / "backend" / "preparation.py": 160, } oversized = [ diff --git a/tests/unit/test_backend_secret_reprovision.py b/tests/unit/test_backend_secret_reprovision.py index 03a17623..36592faa 100644 --- a/tests/unit/test_backend_secret_reprovision.py +++ b/tests/unit/test_backend_secret_reprovision.py @@ -11,6 +11,8 @@ from types import SimpleNamespace from unittest.mock import Mock, patch from bot_bottle.orchestrator.reprovision import reprovision_bottles +from bot_bottle.backend.base import BottleBackend, InfraLaunchError +from bot_bottle.backend.gateway_attach import GatewayAttachResources from bot_bottle.backend.firecracker import infra_launch as fc from bot_bottle.backend.macos_container import infra_launch as mac from bot_bottle.backend.docker import infra_launch as docker @@ -159,9 +161,55 @@ class TestFirecrackerReprovision(unittest.TestCase): restore.assert_called_once_with(client, {}) -class TestFirecrackerCaReconcile(unittest.TestCase): - """PRD 0081: attach_bottled_agents_to_gateway pushes the current gateway CA - into every running agent VM over SSH.""" +class TestAttachFlow(unittest.TestCase): + """PRD 0081 / #519 review: the base backend owns the reconcile flow and + fails hard — gather resources, attempt every running bottle, then raise an + aggregate if any failed (never silently skip).""" + + def _fake(self, **primitives: object) -> SimpleNamespace: + base: dict[str, object] = { + "_gateway_attach_resources": Mock( + return_value=GatewayAttachResources(ca_pem="PEM")), + "_running_bottles": Mock(return_value=["a", "b", "c"]), + "_attach_bottle_to_gateway": Mock(), + } + base.update(primitives) + return SimpleNamespace(**base) + + def test_attaches_every_running_bottle_once(self) -> None: + fake = self._fake() + BottleBackend.attach_bottled_agents_to_gateway(fake) # type: ignore[arg-type] + self.assertEqual(3, fake._attach_bottle_to_gateway.call_count) + # Each bottle got the single gathered resource set. + res = fake._gateway_attach_resources.return_value + for call in fake._attach_bottle_to_gateway.call_args_list: + self.assertIs(res, call.args[1]) + + def test_attempts_all_then_raises_aggregate_on_failure(self) -> None: + def attach(bottle: str, _resources: object) -> None: + if bottle in ("b", "c"): + raise InfraLaunchError(f"{bottle} unreachable") + fake = self._fake(_attach_bottle_to_gateway=Mock(side_effect=attach)) + with self.assertRaises(InfraLaunchError) as ctx: + BottleBackend.attach_bottled_agents_to_gateway(fake) # type: ignore[arg-type] + msg = str(ctx.exception) + # Every bottle was attempted (not fail-fast), and both failures surface. + self.assertEqual(3, fake._attach_bottle_to_gateway.call_count) + self.assertIn("b unreachable", msg) + self.assertIn("c unreachable", msg) + self.assertIn("2", msg) + + def test_resource_gathering_failure_fails_hard_before_enumerating(self) -> None: + fake = self._fake( + _gateway_attach_resources=Mock(side_effect=RuntimeError("no CA yet"))) + with self.assertRaises(RuntimeError): + BottleBackend.attach_bottled_agents_to_gateway(fake) # type: ignore[arg-type] + fake._running_bottles.assert_not_called() + + +class TestFirecrackerAttachPrimitive(unittest.TestCase): + """PRD 0081: attach_ca_to_agent_vm pushes the current gateway CA into one + running agent VM over SSH, failing hard.""" def _run_dir(self, root: Path, ip: str = "10.243.0.3") -> Path: run_dir = root / "demo" @@ -172,112 +220,90 @@ class TestFirecrackerCaReconcile(unittest.TestCase): })) return run_dir - def test_pushes_current_ca_over_stdin_and_rebuilds_trust_store(self) -> None: + def test_pushes_ca_over_stdin_and_rebuilds_trust_store(self) -> None: with tempfile.TemporaryDirectory() as tmp: run_dir = self._run_dir(Path(tmp)) - gw = Mock() - gw.ca_cert_pem.return_value = "PEM-DATA" - with patch.object(fc, "FirecrackerGateway", return_value=gw), \ - patch.object(fc.cleanup, "live_run_dirs", return_value=(run_dir,)), \ - patch.object(fc.util, "ssh_base_argv", return_value=["ssh", "guest"]), \ - patch.object(fc.subprocess, "run", return_value=_proc()) as run, \ - patch.object(fc, "info"): - fc.attach_bottled_agents_to_gateway() + with patch.object(fc.util, "ssh_base_argv", return_value=["ssh", "guest"]), \ + patch.object(fc.subprocess, "run", return_value=_proc()) as run: + fc.attach_ca_to_agent_vm(run_dir, "PEM-DATA") # The cert goes over stdin (off argv), and the trust store is rebuilt. self.assertEqual("PEM-DATA", run.call_args.kwargs["input"]) script = run.call_args.args[0][-1] self.assertIn(fc.AGENT_CA_PATH, script) self.assertIn("update-ca-certificates", script) - def test_unreachable_vm_is_skipped_not_fatal(self) -> None: + def test_malformed_run_dir_fails_hard(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + empty = Path(tmp) / "no-config" + empty.mkdir() + with self.assertRaises(fc.InfraLaunchError): + fc.attach_ca_to_agent_vm(empty, "PEM") + + def test_push_failure_fails_hard(self) -> None: with tempfile.TemporaryDirectory() as tmp: run_dir = self._run_dir(Path(tmp)) - gw = Mock() - gw.ca_cert_pem.return_value = "PEM" - with patch.object(fc, "FirecrackerGateway", return_value=gw), \ - patch.object(fc.cleanup, "live_run_dirs", return_value=(run_dir,)), \ - patch.object(fc.util, "ssh_base_argv", return_value=["ssh", "guest"]), \ - patch.object(fc.subprocess, "run", return_value=_proc(1, stderr="down")), \ - patch.object(fc, "info") as info: - fc.attach_bottled_agents_to_gateway() # does not raise - self.assertTrue(any("skipped" in c.args[0] for c in info.call_args_list)) - - def test_missing_gateway_ca_skips_the_whole_reconcile(self) -> None: - gw = Mock() - gw.ca_cert_pem.side_effect = fc.GatewayError("no CA yet") - with patch.object(fc, "FirecrackerGateway", return_value=gw), \ - patch.object(fc.cleanup, "live_run_dirs") as live, \ - patch.object(fc, "info"): - fc.attach_bottled_agents_to_gateway() - live.assert_not_called() # bailed before enumerating agents + with patch.object(fc.util, "ssh_base_argv", return_value=["ssh", "guest"]), \ + patch.object(fc.subprocess, "run", return_value=_proc(1, stderr="down")): + with self.assertRaisesRegex(fc.InfraLaunchError, "down"): + fc.attach_ca_to_agent_vm(run_dir, "PEM") -class TestDockerCaReconcile(unittest.TestCase): - def test_pushes_ca_to_each_agent_container_excluding_gateway(self) -> None: +class TestDockerAttachPrimitives(unittest.TestCase): + def test_running_agent_containers_excludes_gateway(self) -> None: # network inspect lists the gateway (excluded) + one agent + a blank line. inspect = _proc(stdout="bot-bottle-orch-gateway\nbot-bottle-a\n\n") + with patch.object(docker, "run_docker", return_value=inspect): + names = docker.running_agent_containers( + network="net", gateway_name="bot-bottle-orch-gateway") + self.assertEqual(["bot-bottle-a"], names) + + def test_running_agent_containers_fails_hard_on_docker_error(self) -> None: + with patch.object(docker, "run_docker", side_effect=FileNotFoundError("docker")): + with self.assertRaises(FileNotFoundError): + docker.running_agent_containers() + + def test_push_ca_cp_then_rebuilds_trust_store(self) -> None: with patch.object( - docker, "run_docker", - side_effect=[inspect, _proc(), _proc(), _proc()], - ) as run, patch.object(docker.log, "info"): - docker.attach_bottled_agents_to_gateway( - "PEM", gateway_name="bot-bottle-orch-gateway") + docker, "run_docker", side_effect=[_proc(), _proc(), _proc()], + ) as run: + docker.push_ca_to_container("bot-bottle-a", "PEM") argvs = [c.args[0] for c in run.call_args_list] - # First call enumerates; the agent then gets mkdir + cp + exec — never - # the gateway container. - self.assertNotIn( - "bot-bottle-orch-gateway", - " ".join(tok for a in argvs[1:] for tok in a), - ) cp = next(a for a in argvs if a[:2] == ["docker", "cp"]) self.assertEqual(f"bot-bottle-a:{docker.AGENT_CA_PATH}", cp[-1]) - exec_argv = next(a for a in argvs if "update-ca-certificates" in a[-1]) - self.assertIn(docker.AGENT_CA_PATH, exec_argv[-1]) + self.assertTrue(any("update-ca-certificates" in a[-1] for a in argvs)) - def test_one_container_failure_does_not_block_others(self) -> None: - inspect = _proc(stdout="a\nb\n") - # a: mkdir fails (skip); b: mkdir/cp/exec all succeed. - with patch.object( - docker, "run_docker", - side_effect=[inspect, _proc(1, stderr="gone"), _proc(), _proc(), _proc()], - ), patch.object(docker.log, "info") as info: - docker.attach_bottled_agents_to_gateway("PEM", gateway_name="gw") - self.assertTrue( - any("skipped for a" in str(c.args[0]) for c in info.call_args_list) - ) - - def test_fetches_ca_from_gateway_when_not_supplied(self) -> None: - svc = Mock() - svc.gateway.return_value.ca_cert_pem.return_value = "FETCHED" - with patch.object(docker, "DockerInfraService", return_value=svc), \ - patch.object(docker, "run_docker", return_value=_proc(stdout="")), \ - patch.object(docker.log, "info"): - docker.attach_bottled_agents_to_gateway(network="net", gateway_name="gw") - svc.gateway.return_value.ca_cert_pem.assert_called_once() + def test_push_ca_fails_hard(self) -> None: + with patch.object(docker, "run_docker", return_value=_proc(1, stderr="gone")): + with self.assertRaisesRegex(docker.InfraLaunchError, "gone"): + docker.push_ca_to_container("bot-bottle-a", "PEM") -class TestMacosCaReconcile(unittest.TestCase): - def test_pushes_ca_to_each_running_agent_container(self) -> None: - agent = SimpleNamespace(slug="demo") - with patch.object(mac, "enumerate_active", return_value=[agent]), \ - patch.object(mac.container_mod, "run_container_argv", - return_value=_proc()) as run, \ - patch.object(mac, "info"): - mac.attach_bottled_agents_to_gateway("PEM") - argvs = [c.args[0] for c in run.call_args_list] - name = f"{mac.CONTAINER_NAME_PREFIX}demo" - self.assertTrue(any(name in a for a in argvs)) - self.assertTrue( - any("update-ca-certificates" in a[-1] for a in argvs if a[-2:-1] == ["-c"]) - ) - self.assertTrue(any(mac.AGENT_CA_PATH in " ".join(a) for a in argvs)) - - def test_enumeration_failure_is_best_effort(self) -> None: +class TestMacosAttachPrimitives(unittest.TestCase): + def test_running_agent_containers_from_enumeration(self) -> None: with patch.object(mac, "enumerate_active", - side_effect=mac.EnumerationError("failed")), \ - patch.object(mac, "info") as info: - mac.attach_bottled_agents_to_gateway("PEM") # does not raise - self.assertIn("skipped", info.call_args.args[0]) + return_value=[SimpleNamespace(slug="demo")]): + names = mac.running_agent_containers() + self.assertEqual([f"{mac.CONTAINER_NAME_PREFIX}demo"], names) + + def test_running_agent_containers_fails_hard(self) -> None: + with patch.object(mac, "enumerate_active", + side_effect=mac.EnumerationError("failed")): + with self.assertRaises(mac.EnumerationError): + mac.running_agent_containers() + + def test_push_ca_cp_then_rebuilds_trust_store(self) -> None: + with patch.object(mac.container_mod, "run_container_argv", + return_value=_proc()) as run: + mac.push_ca_to_container(f"{mac.CONTAINER_NAME_PREFIX}demo", "PEM") + argvs = [c.args[0] for c in run.call_args_list] + self.assertTrue(any(mac.AGENT_CA_PATH in " ".join(a) for a in argvs)) + self.assertTrue(any("update-ca-certificates" in a[-1] for a in argvs)) + + def test_push_ca_fails_hard(self) -> None: + with patch.object(mac.container_mod, "run_container_argv", + return_value=_proc(1, stderr="no")): + with self.assertRaisesRegex(mac.InfraLaunchError, "no"): + mac.push_ca_to_container("x", "PEM") if __name__ == "__main__":