From 3674d7780bef940de3dab09b7f0d2aa4037a1dab Mon Sep 17 00:00:00 2001 From: claude Date: Mon, 20 Jul 2026 19:36:41 +0000 Subject: [PATCH 1/4] 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 5b7efc1..0b712fb 100644 --- a/bot_bottle/backend/macos_container/consolidated_launch.py +++ b/bot_bottle/backend/macos_container/consolidated_launch.py @@ -124,11 +124,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 97bef37..e712338 100644 --- a/bot_bottle/backend/macos_container/launch.py +++ b/bot_bottle/backend/macos_container/launch.py @@ -60,6 +60,7 @@ from ..util import AGENT_CA_BUNDLE, AGENT_CA_PATH from . import util as container_mod from .bottle import MacosContainerBottle from .bottle_plan import MacosContainerBottlePlan +from ...orchestrator.config_store import resolve_teardown_timeout from .consolidated_launch import ( GatewayEndpoint, ensure_gateway, @@ -140,6 +141,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", +] -- 2.52.0 From 31000dcbe3648f3ce281d6b967336c6e14ffc195 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 21 Jul 2026 04:09:20 +0000 Subject: [PATCH 2/4] fix: resolve teardown timeout before registration to prevent orphaned state If resolve_teardown_timeout() raised after launch_consolidated()/register_agent() returned, the teardown callback was never registered, leaving the bottle registered with no cleanup path. Resolve the timeout into a local variable before the registration call so any failure aborts before the bottle exists. Add tests covering OrchestratorConfigStore, resolve_teardown_timeout priority ordering (env > db > default), and the source-order invariant that the resolver runs before registration in all three backends. --- bot_bottle/backend/docker/launch.py | 3 +- bot_bottle/backend/firecracker/launch.py | 3 +- bot_bottle/backend/macos_container/launch.py | 3 +- tests/unit/test_orchestrator_config_store.py | 149 +++++++++++++++++++ 4 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_orchestrator_config_store.py diff --git a/bot_bottle/backend/docker/launch.py b/bot_bottle/backend/docker/launch.py index 8537056..f4a3043 100644 --- a/bot_bottle/backend/docker/launch.py +++ b/bot_bottle/backend/docker/launch.py @@ -134,13 +134,14 @@ def launch( token_values = egress_resolve_token_values( plan.egress_plan.token_env_map, effective_env, ) + teardown_timeout = resolve_teardown_timeout() ctx = launch_consolidated( 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, - timeout=resolve_teardown_timeout(), + timeout=teardown_timeout, ) # Step 4: install the SHARED gateway CA into the agent (replaces the diff --git a/bot_bottle/backend/firecracker/launch.py b/bot_bottle/backend/firecracker/launch.py index daed1fc..a5efc00 100644 --- a/bot_bottle/backend/firecracker/launch.py +++ b/bot_bottle/backend/firecracker/launch.py @@ -113,6 +113,7 @@ def launch( token_values = egress_resolve_token_values( plan.egress_plan.token_env_map, effective_env, ) + teardown_timeout = resolve_teardown_timeout() ctx = launch_consolidated( plan.egress_plan, git_gate_plan, guest_ip=slot.guest_ip, @@ -122,7 +123,7 @@ def launch( stack.callback( teardown_consolidated, ctx.bottle_id, orchestrator_url=ctx.orchestrator_url, - timeout=resolve_teardown_timeout(), + timeout=teardown_timeout, ) # Step 5: install the SHARED gateway CA (replaces the per-bottle CA). diff --git a/bot_bottle/backend/macos_container/launch.py b/bot_bottle/backend/macos_container/launch.py index e712338..5bfec1f 100644 --- a/bot_bottle/backend/macos_container/launch.py +++ b/bot_bottle/backend/macos_container/launch.py @@ -130,6 +130,7 @@ def launch( token_values = egress_resolve_token_values( plan.egress_plan.token_env_map, effective_env, ) + teardown_timeout = resolve_teardown_timeout() ctx = register_agent( plan.egress_plan, plan.git_gate_plan, @@ -141,7 +142,7 @@ def launch( stack.callback( teardown_consolidated, ctx.bottle_id, orchestrator_url=ctx.orchestrator_url, - timeout=resolve_teardown_timeout(), + timeout=teardown_timeout, ) info( f"agent {plan.container_name} registered " diff --git a/tests/unit/test_orchestrator_config_store.py b/tests/unit/test_orchestrator_config_store.py new file mode 100644 index 0000000..d56043d --- /dev/null +++ b/tests/unit/test_orchestrator_config_store.py @@ -0,0 +1,149 @@ +"""Unit: OrchestratorConfigStore and resolve_teardown_timeout. + +Also verifies the lifecycle ordering invariant: resolve_teardown_timeout() +must be called before launch_consolidated() / register_agent() so that a +resolver failure cannot leave an orphaned registration with no teardown +callback. +""" + +from __future__ import annotations + +import inspect +import os +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +from bot_bottle.orchestrator.config_store import ( + DEFAULT_TEARDOWN_TIMEOUT_SECONDS, + TEARDOWN_TIMEOUT_ENV, + TEARDOWN_TIMEOUT_KEY, + OrchestratorConfigStore, + resolve_teardown_timeout, +) + + +class TestOrchestratorConfigStore(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.db = Path(self._tmp.name) / "test.db" + self.store = OrchestratorConfigStore(self.db) + self.store.migrate() + + def tearDown(self) -> None: + self._tmp.cleanup() + + def test_get_returns_none_for_absent_key(self) -> None: + self.assertIsNone(self.store.get("missing")) + + def test_set_and_get_roundtrip(self) -> None: + self.store.set("foo", "bar") + self.assertEqual("bar", self.store.get("foo")) + + def test_set_upserts_existing_key(self) -> None: + self.store.set("k", "v1") + self.store.set("k", "v2") + self.assertEqual("v2", self.store.get("k")) + + def test_delete_removes_key(self) -> None: + self.store.set("k", "v") + deleted = self.store.delete("k") + self.assertTrue(deleted) + self.assertIsNone(self.store.get("k")) + + def test_delete_absent_returns_false(self) -> None: + self.assertFalse(self.store.delete("nope")) + + def test_is_migrated_true_after_migrate(self) -> None: + self.assertTrue(self.store.is_migrated()) + + def test_is_migrated_false_before_migrate(self) -> None: + store = OrchestratorConfigStore(Path(self._tmp.name) / "new.db") + self.assertFalse(store.is_migrated()) + + +class TestResolveTeardownTimeout(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.db = Path(self._tmp.name) / "cfg.db" + + def tearDown(self) -> None: + self._tmp.cleanup() + os.environ.pop(TEARDOWN_TIMEOUT_ENV, None) + + def test_returns_default_when_nothing_configured(self) -> None: + self.assertEqual( + DEFAULT_TEARDOWN_TIMEOUT_SECONDS, + resolve_teardown_timeout(self.db), + ) + + def test_env_var_overrides_default(self) -> None: + os.environ[TEARDOWN_TIMEOUT_ENV] = "99" + self.assertEqual(99.0, resolve_teardown_timeout(self.db)) + + def test_env_var_overrides_db_value(self) -> None: + store = OrchestratorConfigStore(self.db) + store.migrate() + store.set(TEARDOWN_TIMEOUT_KEY, "55") + os.environ[TEARDOWN_TIMEOUT_ENV] = "77" + self.assertEqual(77.0, resolve_teardown_timeout(self.db)) + + def test_db_value_overrides_default(self) -> None: + store = OrchestratorConfigStore(self.db) + store.migrate() + store.set(TEARDOWN_TIMEOUT_KEY, "42") + self.assertEqual(42.0, resolve_teardown_timeout(self.db)) + + def test_invalid_env_var_falls_through_to_default(self) -> None: + os.environ[TEARDOWN_TIMEOUT_ENV] = "not-a-number" + self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, resolve_teardown_timeout(self.db)) + + def test_non_positive_env_var_falls_through_to_default(self) -> None: + os.environ[TEARDOWN_TIMEOUT_ENV] = "0" + self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, resolve_teardown_timeout(self.db)) + + def test_invalid_db_value_falls_through_to_default(self) -> None: + store = OrchestratorConfigStore(self.db) + store.migrate() + store.set(TEARDOWN_TIMEOUT_KEY, "bad") + self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, resolve_teardown_timeout(self.db)) + + def test_migrates_db_on_first_call(self) -> None: + # DB does not exist yet; resolve_teardown_timeout must not raise. + result = resolve_teardown_timeout(self.db) + self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, result) + self.assertTrue(OrchestratorConfigStore(self.db).is_migrated()) + + +class TestTeardownTimeoutResolvedBeforeRegistration(unittest.TestCase): + """Ordering invariant: if resolve_teardown_timeout() raises, the bottle + must not yet be registered — no orphaned state can result.""" + + def _src(self, module) -> str: + return inspect.getsource(module) + + def test_docker_resolves_timeout_before_launch_consolidated(self) -> None: + from bot_bottle.backend.docker import launch + src = self._src(launch) + resolve_at = src.index("teardown_timeout = resolve_teardown_timeout()") + launch_at = src.index("ctx = launch_consolidated(") + self.assertLess(resolve_at, launch_at) + + def test_firecracker_resolves_timeout_before_launch_consolidated(self) -> None: + from bot_bottle.backend.firecracker import launch + src = self._src(launch) + resolve_at = src.index("teardown_timeout = resolve_teardown_timeout()") + launch_at = src.index("ctx = launch_consolidated(") + self.assertLess(resolve_at, launch_at) + + def test_macos_resolves_timeout_before_register_agent(self) -> None: + from bot_bottle.backend.macos_container import launch + src = self._src(launch) + resolve_at = src.index("teardown_timeout = resolve_teardown_timeout()") + register_at = src.index("ctx = register_agent(") + self.assertLess(resolve_at, register_at) + + +if __name__ == "__main__": + unittest.main() -- 2.52.0 From 3057043142080df2d62d1e0494421620ce53b070 Mon Sep 17 00:00:00 2001 From: codex Date: Tue, 21 Jul 2026 04:17:16 +0000 Subject: [PATCH 3/4] test(orchestrator): satisfy config store type checking --- tests/unit/test_orchestrator_config_store.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_orchestrator_config_store.py b/tests/unit/test_orchestrator_config_store.py index d56043d..00b5a89 100644 --- a/tests/unit/test_orchestrator_config_store.py +++ b/tests/unit/test_orchestrator_config_store.py @@ -13,7 +13,7 @@ import os import tempfile import unittest from pathlib import Path -from unittest.mock import patch +from types import ModuleType from bot_bottle.orchestrator.config_store import ( DEFAULT_TEARDOWN_TIMEOUT_SECONDS, @@ -120,7 +120,7 @@ class TestTeardownTimeoutResolvedBeforeRegistration(unittest.TestCase): """Ordering invariant: if resolve_teardown_timeout() raises, the bottle must not yet be registered — no orphaned state can result.""" - def _src(self, module) -> str: + def _src(self, module: ModuleType) -> str: return inspect.getsource(module) def test_docker_resolves_timeout_before_launch_consolidated(self) -> None: -- 2.52.0 From 256a4b262d935052bd06055cfae2e22332669e5c Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 21 Jul 2026 04:26:00 +0000 Subject: [PATCH 4/4] refactor(config_store): replace generic key/value table with explicit schema Switch orchestrator_config from a generic key/text store to a typed single-row table with a dedicated teardown_timeout_seconds REAL column. The CHECK (id = 1) constraint enforces at most one config row. Remove TEARDOWN_TIMEOUT_KEY constant; update OrchestratorConfigStore to expose get/set/delete_teardown_timeout_seconds() instead of the generic get/set/delete(key). Resolving the DB value in resolve_teardown_timeout() no longer needs a float() conversion since the column is already REAL. --- bot_bottle/orchestrator/config_store.py | 47 +++++++++----------- tests/unit/test_orchestrator_config_store.py | 36 +++++++-------- 2 files changed, 38 insertions(+), 45 deletions(-) diff --git a/bot_bottle/orchestrator/config_store.py b/bot_bottle/orchestrator/config_store.py index 28b15c3..2cd6f85 100644 --- a/bot_bottle/orchestrator/config_store.py +++ b/bot_bottle/orchestrator/config_store.py @@ -1,4 +1,4 @@ -"""Per-host orchestrator configuration store (key/value settings in bot-bottle.db). +"""Per-host orchestrator configuration store (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 @@ -15,7 +15,6 @@ 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 @@ -24,8 +23,8 @@ _MIGRATIONS = TableMigrations( [ """ CREATE TABLE IF NOT EXISTS orchestrator_config ( - key TEXT PRIMARY KEY, - value TEXT NOT NULL + id INTEGER PRIMARY KEY CHECK (id = 1), + teardown_timeout_seconds REAL ) """, ], @@ -33,7 +32,7 @@ _MIGRATIONS = TableMigrations( class OrchestratorConfigStore(DbStore): - """Key/value store for orchestrator settings in the shared host DB.""" + """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) @@ -43,31 +42,33 @@ class OrchestratorConfigStore(DbStore): conn.execute("PRAGMA busy_timeout=5000") return conn - def get(self, key: str) -> str | None: - """Return the value for `key`, or None if absent.""" + def get_teardown_timeout_seconds(self) -> float | None: + """Return the configured teardown timeout, or None if not set.""" try: with self._connection() as conn: row = conn.execute( - "SELECT value FROM orchestrator_config WHERE key = ?", (key,) + "SELECT teardown_timeout_seconds FROM orchestrator_config WHERE id = 1" ).fetchone() except sqlite3.OperationalError: return None - return row["value"] if row else None + return row["teardown_timeout_seconds"] if row else None - def set(self, key: str, value: str) -> None: - """Upsert a setting.""" + def set_teardown_timeout_seconds(self, value: float) -> None: + """Persist the teardown timeout.""" with self._connection() as conn: conn.execute( - "INSERT OR REPLACE INTO orchestrator_config (key, value) VALUES (?, ?)", - (key, value), + "INSERT OR REPLACE INTO orchestrator_config" + " (id, teardown_timeout_seconds) VALUES (1, ?)", + (value,), ) self._chmod() - def delete(self, key: str) -> bool: - """Remove a setting. Returns True if the key existed.""" + def delete_teardown_timeout_seconds(self) -> bool: + """Clear the stored teardown timeout. Returns True if a value existed.""" with self._connection() as conn: cur = conn.execute( - "DELETE FROM orchestrator_config WHERE key = ?", (key,) + "UPDATE orchestrator_config SET teardown_timeout_seconds = NULL" + " WHERE id = 1 AND teardown_timeout_seconds IS NOT NULL" ) return cur.rowcount > 0 @@ -76,7 +77,7 @@ 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 + 2. ``teardown_timeout_seconds`` in the orchestrator config DB 3. ``DEFAULT_TEARDOWN_TIMEOUT_SECONDS`` (30 s) """ raw = os.environ.get(TEARDOWN_TIMEOUT_ENV, "").strip() @@ -91,14 +92,9 @@ def resolve_teardown_timeout(db_path: Path | None = None) -> float: 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 + db_value = store.get_teardown_timeout_seconds() + if db_value is not None and db_value > 0: + return db_value return DEFAULT_TEARDOWN_TIMEOUT_SECONDS @@ -106,7 +102,6 @@ def resolve_teardown_timeout(db_path: Path | None = None) -> float: __all__ = [ "OrchestratorConfigStore", "resolve_teardown_timeout", - "TEARDOWN_TIMEOUT_KEY", "TEARDOWN_TIMEOUT_ENV", "DEFAULT_TEARDOWN_TIMEOUT_SECONDS", ] diff --git a/tests/unit/test_orchestrator_config_store.py b/tests/unit/test_orchestrator_config_store.py index 00b5a89..ff43987 100644 --- a/tests/unit/test_orchestrator_config_store.py +++ b/tests/unit/test_orchestrator_config_store.py @@ -18,7 +18,6 @@ from types import ModuleType from bot_bottle.orchestrator.config_store import ( DEFAULT_TEARDOWN_TIMEOUT_SECONDS, TEARDOWN_TIMEOUT_ENV, - TEARDOWN_TIMEOUT_KEY, OrchestratorConfigStore, resolve_teardown_timeout, ) @@ -34,26 +33,26 @@ class TestOrchestratorConfigStore(unittest.TestCase): def tearDown(self) -> None: self._tmp.cleanup() - def test_get_returns_none_for_absent_key(self) -> None: - self.assertIsNone(self.store.get("missing")) + def test_get_returns_none_when_not_set(self) -> None: + self.assertIsNone(self.store.get_teardown_timeout_seconds()) def test_set_and_get_roundtrip(self) -> None: - self.store.set("foo", "bar") - self.assertEqual("bar", self.store.get("foo")) + self.store.set_teardown_timeout_seconds(42.5) + self.assertEqual(42.5, self.store.get_teardown_timeout_seconds()) - def test_set_upserts_existing_key(self) -> None: - self.store.set("k", "v1") - self.store.set("k", "v2") - self.assertEqual("v2", self.store.get("k")) + def test_set_overwrites_existing_value(self) -> None: + self.store.set_teardown_timeout_seconds(10.0) + self.store.set_teardown_timeout_seconds(20.0) + self.assertEqual(20.0, self.store.get_teardown_timeout_seconds()) - def test_delete_removes_key(self) -> None: - self.store.set("k", "v") - deleted = self.store.delete("k") + def test_delete_clears_value_and_returns_true(self) -> None: + self.store.set_teardown_timeout_seconds(30.0) + deleted = self.store.delete_teardown_timeout_seconds() self.assertTrue(deleted) - self.assertIsNone(self.store.get("k")) + self.assertIsNone(self.store.get_teardown_timeout_seconds()) def test_delete_absent_returns_false(self) -> None: - self.assertFalse(self.store.delete("nope")) + self.assertFalse(self.store.delete_teardown_timeout_seconds()) def test_is_migrated_true_after_migrate(self) -> None: self.assertTrue(self.store.is_migrated()) @@ -85,14 +84,14 @@ class TestResolveTeardownTimeout(unittest.TestCase): def test_env_var_overrides_db_value(self) -> None: store = OrchestratorConfigStore(self.db) store.migrate() - store.set(TEARDOWN_TIMEOUT_KEY, "55") + store.set_teardown_timeout_seconds(55.0) os.environ[TEARDOWN_TIMEOUT_ENV] = "77" self.assertEqual(77.0, resolve_teardown_timeout(self.db)) def test_db_value_overrides_default(self) -> None: store = OrchestratorConfigStore(self.db) store.migrate() - store.set(TEARDOWN_TIMEOUT_KEY, "42") + store.set_teardown_timeout_seconds(42.0) self.assertEqual(42.0, resolve_teardown_timeout(self.db)) def test_invalid_env_var_falls_through_to_default(self) -> None: @@ -103,14 +102,13 @@ class TestResolveTeardownTimeout(unittest.TestCase): os.environ[TEARDOWN_TIMEOUT_ENV] = "0" self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, resolve_teardown_timeout(self.db)) - def test_invalid_db_value_falls_through_to_default(self) -> None: + def test_non_positive_db_value_falls_through_to_default(self) -> None: store = OrchestratorConfigStore(self.db) store.migrate() - store.set(TEARDOWN_TIMEOUT_KEY, "bad") + store.set_teardown_timeout_seconds(0.0) self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, resolve_teardown_timeout(self.db)) def test_migrates_db_on_first_call(self) -> None: - # DB does not exist yet; resolve_teardown_timeout must not raise. result = resolve_teardown_timeout(self.db) self.assertEqual(DEFAULT_TEARDOWN_TIMEOUT_SECONDS, result) self.assertTrue(OrchestratorConfigStore(self.db).is_migrated()) -- 2.52.0