From 1f192d785a17fdc7650ad3868a1e106f0aaa8693 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 21 Jul 2026 04:26:00 +0000 Subject: [PATCH] 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())