From 0b36c3eb4863071771418b801fc9000ca7e0ecca Mon Sep 17 00:00:00 2001 From: claude Date: Mon, 20 Jul 2026 19:36:41 +0000 Subject: [PATCH] fix: make orchestrator teardown timeout configurable (closes #435) Resolves via ENV VAR -> orchestrator DB config -> default (30 s, up from 5 s): BOT_BOTTLE_ORCHESTRATOR_TEARDOWN_TIMEOUT_SECONDS teardown_timeout_seconds key in new orchestrator_config table (bot-bottle.db) New OrchestratorConfigStore (same DbStore/TableMigrations pattern as the registry) stores the DB-level setting. resolve_teardown_timeout() implements the priority chain and is called at stack.callback registration time in all three backends (macos_container, docker, firecracker). --- .../backend/docker/consolidated_launch.py | 12 +- bot_bottle/backend/docker/launch.py | 5 +- .../firecracker/consolidated_launch.py | 10 +- bot_bottle/backend/firecracker/launch.py | 2 + .../macos_container/consolidated_launch.py | 10 +- bot_bottle/backend/macos_container/launch.py | 2 + bot_bottle/orchestrator/config_store.py | 112 ++++++++++++++++++ 7 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 bot_bottle/orchestrator/config_store.py diff --git a/bot_bottle/backend/docker/consolidated_launch.py b/bot_bottle/backend/docker/consolidated_launch.py index 2d7c230..383f99d 100644 --- a/bot_bottle/backend/docker/consolidated_launch.py +++ b/bot_bottle/backend/docker/consolidated_launch.py @@ -142,11 +142,19 @@ def launch_consolidated( def teardown_consolidated( - bottle_id: str, *, orchestrator_url: str, gateway_name: str = GATEWAY_NAME, + bottle_id: str, + *, + orchestrator_url: str, + gateway_name: str = GATEWAY_NAME, + timeout: float | None = None, ) -> None: """Deregister the bottle and remove its git-gate state from the gateway. Both steps are idempotent so this is safe from a cleanup trap.""" - OrchestratorClient(orchestrator_url).teardown_bottle(bottle_id) + from ...orchestrator.config_store import DEFAULT_TEARDOWN_TIMEOUT_SECONDS + OrchestratorClient( + orchestrator_url, + timeout=timeout if timeout is not None else DEFAULT_TEARDOWN_TIMEOUT_SECONDS, + ).teardown_bottle(bottle_id) deprovision_git_gate(DockerGatewayTransport(gateway_name), bottle_id) diff --git a/bot_bottle/backend/docker/launch.py b/bot_bottle/backend/docker/launch.py index 432b142..8537056 100644 --- a/bot_bottle/backend/docker/launch.py +++ b/bot_bottle/backend/docker/launch.py @@ -62,6 +62,7 @@ from .compose import ( write_compose_file, ) from .consolidated_compose import consolidated_agent_compose +from ...orchestrator.config_store import resolve_teardown_timeout from .consolidated_launch import launch_consolidated, teardown_consolidated from ...orchestrator.gateway import DockerGateway @@ -137,7 +138,9 @@ def launch( plan.egress_plan, git_gate_plan, image_ref=plan.image, tokens=token_values, ) stack.callback( - teardown_consolidated, ctx.bottle_id, orchestrator_url=ctx.orchestrator_url, + teardown_consolidated, ctx.bottle_id, + orchestrator_url=ctx.orchestrator_url, + timeout=resolve_teardown_timeout(), ) # Step 4: install the SHARED gateway CA into the agent (replaces the diff --git a/bot_bottle/backend/firecracker/consolidated_launch.py b/bot_bottle/backend/firecracker/consolidated_launch.py index 899676b..f9c9db4 100644 --- a/bot_bottle/backend/firecracker/consolidated_launch.py +++ b/bot_bottle/backend/firecracker/consolidated_launch.py @@ -91,12 +91,18 @@ def launch_consolidated( ) -def teardown_consolidated(bottle_id: str, *, orchestrator_url: str) -> None: +def teardown_consolidated( + bottle_id: str, *, orchestrator_url: str, timeout: float | None = None, +) -> None: """Deregister the bottle and remove its git-gate state from the gateway VM. Both steps are idempotent so this is safe from a cleanup trap. Does NOT stop the infra VM — it's a persistent per-host singleton shared by every bottle.""" - OrchestratorClient(orchestrator_url).teardown_bottle(bottle_id) + from ...orchestrator.config_store import DEFAULT_TEARDOWN_TIMEOUT_SECONDS + OrchestratorClient( + orchestrator_url, + timeout=timeout if timeout is not None else DEFAULT_TEARDOWN_TIMEOUT_SECONDS, + ).teardown_bottle(bottle_id) deprovision_git_gate(infra_vm.gateway_transport(), bottle_id) diff --git a/bot_bottle/backend/firecracker/launch.py b/bot_bottle/backend/firecracker/launch.py index 26d3779..daed1fc 100644 --- a/bot_bottle/backend/firecracker/launch.py +++ b/bot_bottle/backend/firecracker/launch.py @@ -52,6 +52,7 @@ from ..util import AGENT_CA_BUNDLE, AGENT_CA_PATH from . import firecracker_vm, image_builder, isolation_probe, netpool, util from .bottle import FirecrackerBottle from .bottle_plan import FirecrackerBottlePlan +from ...orchestrator.config_store import resolve_teardown_timeout from .consolidated_launch import ( launch_consolidated, teardown_consolidated, @@ -121,6 +122,7 @@ def launch( stack.callback( teardown_consolidated, ctx.bottle_id, orchestrator_url=ctx.orchestrator_url, + timeout=resolve_teardown_timeout(), ) # Step 5: install the SHARED gateway CA (replaces the per-bottle CA). diff --git a/bot_bottle/backend/macos_container/consolidated_launch.py b/bot_bottle/backend/macos_container/consolidated_launch.py index fcb693e..be8ca8f 100644 --- a/bot_bottle/backend/macos_container/consolidated_launch.py +++ b/bot_bottle/backend/macos_container/consolidated_launch.py @@ -163,11 +163,17 @@ def register_agent( ) -def teardown_consolidated(bottle_id: str, *, orchestrator_url: str) -> None: +def teardown_consolidated( + bottle_id: str, *, orchestrator_url: str, timeout: float | None = None, +) -> None: """Deregister the bottle and remove its git-gate state from the gateway. Both steps are idempotent so this is safe from a cleanup trap. Does NOT stop the gateway — it's a persistent per-host singleton.""" - OrchestratorClient(orchestrator_url).teardown_bottle(bottle_id) + from ...orchestrator.config_store import DEFAULT_TEARDOWN_TIMEOUT_SECONDS + OrchestratorClient( + orchestrator_url, + timeout=timeout if timeout is not None else DEFAULT_TEARDOWN_TIMEOUT_SECONDS, + ).teardown_bottle(bottle_id) deprovision_git_gate(AppleGatewayTransport(), bottle_id) diff --git a/bot_bottle/backend/macos_container/launch.py b/bot_bottle/backend/macos_container/launch.py index be8a348..0d66d5c 100644 --- a/bot_bottle/backend/macos_container/launch.py +++ b/bot_bottle/backend/macos_container/launch.py @@ -65,6 +65,7 @@ from .gateway_hosts import ( set_gateway_host, ) from .bottle_plan import MacosContainerBottlePlan +from ...orchestrator.config_store import resolve_teardown_timeout from .consolidated_launch import ( GatewayEndpoint, ensure_gateway, @@ -153,6 +154,7 @@ def launch( stack.callback( teardown_consolidated, ctx.bottle_id, orchestrator_url=ctx.orchestrator_url, + timeout=resolve_teardown_timeout(), ) info( f"agent {plan.container_name} registered " diff --git a/bot_bottle/orchestrator/config_store.py b/bot_bottle/orchestrator/config_store.py new file mode 100644 index 0000000..28b15c3 --- /dev/null +++ b/bot_bottle/orchestrator/config_store.py @@ -0,0 +1,112 @@ +"""Per-host orchestrator configuration store (key/value settings in bot-bottle.db). + +Co-tenants the shared `bot-bottle.db` via the `DbStore` framework. Settings +are readable by the host launch path directly (no HTTP round-trip to the +orchestrator), so they take effect even before the orchestrator is reachable. +""" + +from __future__ import annotations + +import os +import sqlite3 +from pathlib import Path + +from ..db_store import DbStore +from ..migrations import TableMigrations +from ..paths import host_db_path + +TEARDOWN_TIMEOUT_KEY = "teardown_timeout_seconds" +TEARDOWN_TIMEOUT_ENV = "BOT_BOTTLE_ORCHESTRATOR_TEARDOWN_TIMEOUT_SECONDS" +DEFAULT_TEARDOWN_TIMEOUT_SECONDS = 30.0 + +_MIGRATIONS = TableMigrations( + "orchestrator_config", + [ + """ + CREATE TABLE IF NOT EXISTS orchestrator_config ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + ) + """, + ], +) + + +class OrchestratorConfigStore(DbStore): + """Key/value store for orchestrator settings in the shared host DB.""" + + def __init__(self, db_path: Path | None = None) -> None: + super().__init__(db_path or host_db_path(), _MIGRATIONS) + + def _connect(self) -> sqlite3.Connection: + conn = super()._connect() + conn.execute("PRAGMA busy_timeout=5000") + return conn + + def get(self, key: str) -> str | None: + """Return the value for `key`, or None if absent.""" + try: + with self._connection() as conn: + row = conn.execute( + "SELECT value FROM orchestrator_config WHERE key = ?", (key,) + ).fetchone() + except sqlite3.OperationalError: + return None + return row["value"] if row else None + + def set(self, key: str, value: str) -> None: + """Upsert a setting.""" + with self._connection() as conn: + conn.execute( + "INSERT OR REPLACE INTO orchestrator_config (key, value) VALUES (?, ?)", + (key, value), + ) + self._chmod() + + def delete(self, key: str) -> bool: + """Remove a setting. Returns True if the key existed.""" + with self._connection() as conn: + cur = conn.execute( + "DELETE FROM orchestrator_config WHERE key = ?", (key,) + ) + return cur.rowcount > 0 + + +def resolve_teardown_timeout(db_path: Path | None = None) -> float: + """Return the teardown timeout to use, in priority order: + + 1. ``BOT_BOTTLE_ORCHESTRATOR_TEARDOWN_TIMEOUT_SECONDS`` env var + 2. ``teardown_timeout_seconds`` key in the orchestrator config DB + 3. ``DEFAULT_TEARDOWN_TIMEOUT_SECONDS`` (30 s) + """ + raw = os.environ.get(TEARDOWN_TIMEOUT_ENV, "").strip() + if raw: + try: + value = float(raw) + if value > 0: + return value + except ValueError: + pass + + store = OrchestratorConfigStore(db_path) + if not store.is_migrated(): + store.migrate() + raw_db = store.get(TEARDOWN_TIMEOUT_KEY) + if raw_db is not None: + try: + value = float(raw_db) + if value > 0: + return value + except ValueError: + pass + + return DEFAULT_TEARDOWN_TIMEOUT_SECONDS + + +__all__ = [ + "OrchestratorConfigStore", + "resolve_teardown_timeout", + "TEARDOWN_TIMEOUT_KEY", + "TEARDOWN_TIMEOUT_ENV", + "DEFAULT_TEARDOWN_TIMEOUT_SECONDS", +]