didericis/bot-bottle
1
0
Fork 0
Code Issues 34 Pull Requests 11 Actions Packages Releases Wiki Activity

refactor(backend): base owns the gateway-attach flow, fail hard (0081 review)
lint / lint (push) Successful in 1m1s
Details
test / unit (pull_request) Successful in 1m5s
Details
tracker-policy-pr / check-pr (pull_request) Successful in 10s
Details
test / integration-docker (pull_request) Failing after 1m30s
Details
test / coverage (pull_request) Has been skipped
Details

Browse Source 
Addresses review on #519 (@didericis 6143 + the fail-hard direction over the
codex skip).

Base owns the flow (6143 / ADR 0006). `attach_bottled_agents_to_gateway()` is
now a concrete method on `BottleBackend` that delegates to
`gateway_attach.reconcile_running_bottles`; backends override only three
primitives — `_gateway_attach_resources()`, `_running_bottles()`,
`_attach_bottle_to_gateway()`. The shared control flow + error policy live in
one place so a backend can't drift onto a bespoke loop or a silent skip. Keeps
the reconcile flow + `GatewayAttachResources` out of `backend/base.py` (the
size guardrail) in a dedicated `backend/gateway_attach.py`. New ADR 0006 records
the "shared behaviour in the base backend, subclasses override primitives"
theme.

Fail hard, never skip (the maintainer's direction over the codex review's
skip). Any attach failure now aborts bring-up instead of being logged and
skipped: a bottle that silently can't reach the fresh gateway (its egress just
starts failing TLS) is worse than a loud failure. Every bottle is attempted and
the failures are raised together (aggregate `InfraLaunchError`) so one bring-up
surfaces the full blast radius. Resource gathering (the CA fetch) also fails
hard. PRD 0081 goal + design updated to match (reversed from the earlier
"tolerate per-bottle failures" draft).

Bumps the base.py guardrail cap 580->600 for the new contract surface (the
delegator + 3 abstract primitives); the flow itself lives in gateway_attach.py.

Refs #516, #519.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
didericis (claude)
2026-07-27 01:50:06 +00:00
parent 4f2ccaed29
commit 3c0d2fb66f
13 changed files with 420 additions and 251 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/unit/test_backend_secret_reprovision.py
+113 -87
View File
@@ -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__":