feat(db_store): decouple migration from store construction
Remove auto-migration from DbStore.__init__; add explicit check_migrations() and migrate() methods. The CLI now checks both stores on every startup and prompts the user to confirm before migrating. supervise.prepare() calls .migrate() directly now that __init__ no longer does it implicitly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -7,8 +7,11 @@ from __future__ import annotations
|
|||||||
|
|
||||||
import sys
|
import sys
|
||||||
|
|
||||||
|
from ..audit_store import AuditStore
|
||||||
|
from ..db_store import DbVersionError
|
||||||
from ..log import Die, die, error
|
from ..log import Die, die, error
|
||||||
from ..manifest import ManifestError
|
from ..manifest import ManifestError
|
||||||
|
from ..queue_store import QueueStore
|
||||||
from ._common import PROG
|
from ._common import PROG
|
||||||
from . import list as _list_mod
|
from . import list as _list_mod
|
||||||
from .cleanup import cmd_cleanup
|
from .cleanup import cmd_cleanup
|
||||||
@@ -74,6 +77,21 @@ def main(argv: list[str] | None = None) -> int:
|
|||||||
if handler is None:
|
if handler is None:
|
||||||
usage()
|
usage()
|
||||||
die(f"unknown command: {command}")
|
die(f"unknown command: {command}")
|
||||||
|
queue_store = QueueStore("")
|
||||||
|
audit_store = AuditStore()
|
||||||
|
if not queue_store.check_migrations() or not audit_store.check_migrations():
|
||||||
|
sys.stderr.write("bot-bottle: database schema is out of date\n")
|
||||||
|
sys.stderr.write("Migrate now? [y/N] ")
|
||||||
|
sys.stderr.flush()
|
||||||
|
try:
|
||||||
|
answer = sys.stdin.readline().strip().lower()
|
||||||
|
except EOFError:
|
||||||
|
answer = ""
|
||||||
|
if answer != "y":
|
||||||
|
error("migration required — re-run and confirm to migrate")
|
||||||
|
return 1
|
||||||
|
queue_store.migrate()
|
||||||
|
audit_store.migrate()
|
||||||
try:
|
try:
|
||||||
return handler(rest) or 0
|
return handler(rest) or 0
|
||||||
except ManifestError as e:
|
except ManifestError as e:
|
||||||
|
|||||||
+22
-3
@@ -11,6 +11,10 @@ except ImportError:
|
|||||||
from migrations import TableMigrations # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
|
from migrations import TableMigrations # type: ignore[import-not-found] # pylint: disable=import-error,no-name-in-module
|
||||||
|
|
||||||
|
|
||||||
|
class DbVersionError(Exception):
|
||||||
|
"""Raised when the on-disk schema is behind the current migration list."""
|
||||||
|
|
||||||
|
|
||||||
class DbStore:
|
class DbStore:
|
||||||
"""Base for SQLite-backed stores. Subclasses resolve db_path then call super().__init__."""
|
"""Base for SQLite-backed stores. Subclasses resolve db_path then call super().__init__."""
|
||||||
|
|
||||||
@@ -18,14 +22,29 @@ class DbStore:
|
|||||||
self.db_path = db_path
|
self.db_path = db_path
|
||||||
self._migrations = migrations
|
self._migrations = migrations
|
||||||
self.db_path.parent.mkdir(parents=True, exist_ok=True)
|
self.db_path.parent.mkdir(parents=True, exist_ok=True)
|
||||||
self._init()
|
|
||||||
|
|
||||||
def _connect(self) -> sqlite3.Connection:
|
def _connect(self) -> sqlite3.Connection:
|
||||||
conn = sqlite3.connect(self.db_path)
|
conn = sqlite3.connect(self.db_path)
|
||||||
conn.row_factory = sqlite3.Row
|
conn.row_factory = sqlite3.Row
|
||||||
return conn
|
return conn
|
||||||
|
|
||||||
def _init(self) -> None:
|
def check_migrations(self) -> bool:
|
||||||
|
"""Return True if the DB is fully up-to-date, False if migration is needed."""
|
||||||
|
if not self.db_path.exists():
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
with self._connect() as conn:
|
||||||
|
row = conn.execute(
|
||||||
|
"SELECT version FROM schema_versions WHERE module = ?",
|
||||||
|
(self._migrations.schema_key,),
|
||||||
|
).fetchone()
|
||||||
|
except sqlite3.OperationalError:
|
||||||
|
return False
|
||||||
|
version = row[0] if row else 0
|
||||||
|
return version == len(self._migrations.migrations)
|
||||||
|
|
||||||
|
def migrate(self) -> None:
|
||||||
|
"""Apply any pending migrations and set permissions on the DB file."""
|
||||||
with self._connect() as conn:
|
with self._connect() as conn:
|
||||||
self._migrations.apply(conn)
|
self._migrations.apply(conn)
|
||||||
self._chmod()
|
self._chmod()
|
||||||
@@ -37,4 +56,4 @@ class DbStore:
|
|||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
__all__ = ["DbStore"]
|
__all__ = ["DbStore", "DbVersionError"]
|
||||||
|
|||||||
@@ -357,8 +357,8 @@ class Supervise(ABC):
|
|||||||
must be set by the launch step before .start runs."""
|
must be set by the launch step before .start runs."""
|
||||||
del stage_dir
|
del stage_dir
|
||||||
db_path = host_db_path()
|
db_path = host_db_path()
|
||||||
QueueStore(slug)
|
QueueStore(slug).migrate()
|
||||||
AuditStore(db_path)
|
AuditStore(db_path).migrate()
|
||||||
return SupervisePlan(
|
return SupervisePlan(
|
||||||
slug=slug,
|
slug=slug,
|
||||||
db_path=db_path,
|
db_path=db_path,
|
||||||
|
|||||||
@@ -12,11 +12,17 @@ from unittest.mock import patch
|
|||||||
|
|
||||||
import bot_bottle.cli as climod
|
import bot_bottle.cli as climod
|
||||||
from bot_bottle.cli import main
|
from bot_bottle.cli import main
|
||||||
|
from bot_bottle.db_store import DbStore
|
||||||
from bot_bottle.log import Die
|
from bot_bottle.log import Die
|
||||||
from bot_bottle.manifest import ManifestError
|
from bot_bottle.manifest import ManifestError
|
||||||
|
|
||||||
|
|
||||||
class TestMainDispatch(unittest.TestCase):
|
class TestMainDispatch(unittest.TestCase):
|
||||||
|
def setUp(self) -> None:
|
||||||
|
patcher = patch.object(DbStore, "check_migrations", return_value=True)
|
||||||
|
self._mock_check = patcher.start()
|
||||||
|
self.addCleanup(patcher.stop)
|
||||||
|
|
||||||
def test_no_args_prints_usage_returns_2(self) -> None:
|
def test_no_args_prints_usage_returns_2(self) -> None:
|
||||||
with patch("sys.stderr", io.StringIO()):
|
with patch("sys.stderr", io.StringIO()):
|
||||||
self.assertEqual(2, main([]))
|
self.assertEqual(2, main([]))
|
||||||
|
|||||||
@@ -8,6 +8,8 @@ from datetime import datetime, timezone
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
from bot_bottle import supervise
|
from bot_bottle import supervise
|
||||||
|
from bot_bottle.audit_store import AuditStore
|
||||||
|
from bot_bottle.queue_store import QueueStore
|
||||||
from bot_bottle.supervise import (
|
from bot_bottle.supervise import (
|
||||||
AuditEntry,
|
AuditEntry,
|
||||||
Proposal,
|
Proposal,
|
||||||
@@ -113,6 +115,7 @@ class TestQueueIO(unittest.TestCase):
|
|||||||
self._tmp = tempfile.TemporaryDirectory(prefix="bot-bottle-supervise-test.")
|
self._tmp = tempfile.TemporaryDirectory(prefix="bot-bottle-supervise-test.")
|
||||||
self._home_patch = self._patch_home(Path(self._tmp.name))
|
self._home_patch = self._patch_home(Path(self._tmp.name))
|
||||||
self.slug = "dev"
|
self.slug = "dev"
|
||||||
|
QueueStore("").migrate()
|
||||||
|
|
||||||
def tearDown(self):
|
def tearDown(self):
|
||||||
self._home_patch()
|
self._home_patch()
|
||||||
@@ -222,6 +225,7 @@ class TestAuditLog(unittest.TestCase):
|
|||||||
def setUp(self):
|
def setUp(self):
|
||||||
self._tmp = tempfile.TemporaryDirectory(prefix="bot-bottle-supervise-audit.")
|
self._tmp = tempfile.TemporaryDirectory(prefix="bot-bottle-supervise-audit.")
|
||||||
self._home_patch = self._patch_home(Path(self._tmp.name))
|
self._home_patch = self._patch_home(Path(self._tmp.name))
|
||||||
|
AuditStore().migrate()
|
||||||
|
|
||||||
def tearDown(self):
|
def tearDown(self):
|
||||||
self._home_patch()
|
self._home_patch()
|
||||||
|
|||||||
@@ -12,7 +12,9 @@ from pathlib import Path
|
|||||||
from unittest.mock import patch
|
from unittest.mock import patch
|
||||||
|
|
||||||
from bot_bottle import supervise
|
from bot_bottle import supervise
|
||||||
|
from bot_bottle.audit_store import AuditStore
|
||||||
from bot_bottle.cli import supervise as supervise_cli
|
from bot_bottle.cli import supervise as supervise_cli
|
||||||
|
from bot_bottle.queue_store import QueueStore
|
||||||
from bot_bottle.supervise import (
|
from bot_bottle.supervise import (
|
||||||
Proposal,
|
Proposal,
|
||||||
STATUS_APPROVED,
|
STATUS_APPROVED,
|
||||||
@@ -59,6 +61,8 @@ class _FakeHomeMixin:
|
|||||||
|
|
||||||
supervise.bot_bottle_root = fake_root # type: ignore[assignment]
|
supervise.bot_bottle_root = fake_root # type: ignore[assignment]
|
||||||
self._restore_home = lambda: setattr(supervise, "bot_bottle_root", original)
|
self._restore_home = lambda: setattr(supervise, "bot_bottle_root", original)
|
||||||
|
QueueStore("").migrate()
|
||||||
|
AuditStore().migrate()
|
||||||
|
|
||||||
def _teardown_fake_home(self):
|
def _teardown_fake_home(self):
|
||||||
self._restore_home()
|
self._restore_home()
|
||||||
|
|||||||
@@ -45,19 +45,22 @@ class TestPathHelpers(unittest.TestCase):
|
|||||||
class TestReadMalformed(unittest.TestCase):
|
class TestReadMalformed(unittest.TestCase):
|
||||||
def test_read_proposal_missing_row(self) -> None:
|
def test_read_proposal_missing_row(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
with patch.dict("os.environ", {"HOME": d}), \
|
with patch.dict("os.environ", {"HOME": d}):
|
||||||
self.assertRaises(FileNotFoundError):
|
QueueStore("slug").migrate()
|
||||||
read_proposal("slug", "p")
|
with self.assertRaises(FileNotFoundError):
|
||||||
|
read_proposal("slug", "p")
|
||||||
|
|
||||||
def test_read_response_missing_row(self) -> None:
|
def test_read_response_missing_row(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
with patch.dict("os.environ", {"HOME": d}), \
|
with patch.dict("os.environ", {"HOME": d}):
|
||||||
self.assertRaises(FileNotFoundError):
|
QueueStore("slug").migrate()
|
||||||
read_response("slug", "p")
|
with self.assertRaises(FileNotFoundError):
|
||||||
|
read_response("slug", "p")
|
||||||
|
|
||||||
def test_list_pending_reads_db_only(self) -> None:
|
def test_list_pending_reads_db_only(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
with patch.dict("os.environ", {"HOME": d}):
|
with patch.dict("os.environ", {"HOME": d}):
|
||||||
|
QueueStore("slug").migrate()
|
||||||
supervise.write_proposal(_proposal())
|
supervise.write_proposal(_proposal())
|
||||||
pending = list_pending_proposals("slug")
|
pending = list_pending_proposals("slug")
|
||||||
self.assertEqual(1, len(pending))
|
self.assertEqual(1, len(pending))
|
||||||
@@ -66,6 +69,7 @@ class TestReadMalformed(unittest.TestCase):
|
|||||||
def test_list_pending_skips_when_response_present(self) -> None:
|
def test_list_pending_skips_when_response_present(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
with patch.dict("os.environ", {"HOME": d}):
|
with patch.dict("os.environ", {"HOME": d}):
|
||||||
|
QueueStore("slug").migrate()
|
||||||
p = _proposal()
|
p = _proposal()
|
||||||
supervise.write_proposal(p)
|
supervise.write_proposal(p)
|
||||||
supervise.write_response("slug", supervise.Response(
|
supervise.write_response("slug", supervise.Response(
|
||||||
@@ -79,15 +83,17 @@ class TestReadMalformed(unittest.TestCase):
|
|||||||
class TestWaitForResponse(unittest.TestCase):
|
class TestWaitForResponse(unittest.TestCase):
|
||||||
def test_missing_response_times_out(self) -> None:
|
def test_missing_response_times_out(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
with patch.dict("os.environ", {"HOME": d}), \
|
with patch.dict("os.environ", {"HOME": d}):
|
||||||
self.assertRaises(TimeoutError):
|
QueueStore("slug").migrate()
|
||||||
wait_for_response("slug", "p", deadline=time.monotonic())
|
with self.assertRaises(TimeoutError):
|
||||||
|
wait_for_response("slug", "p", deadline=time.monotonic())
|
||||||
|
|
||||||
def test_empty_db_response_does_not_count(self) -> None:
|
def test_empty_db_response_does_not_count(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
with patch.dict("os.environ", {"HOME": d}), \
|
with patch.dict("os.environ", {"HOME": d}):
|
||||||
self.assertRaises(TimeoutError):
|
QueueStore("slug").migrate()
|
||||||
wait_for_response("slug", "p", deadline=time.monotonic())
|
with self.assertRaises(TimeoutError):
|
||||||
|
wait_for_response("slug", "p", deadline=time.monotonic())
|
||||||
|
|
||||||
|
|
||||||
class TestReadAuditEntries(unittest.TestCase):
|
class TestReadAuditEntries(unittest.TestCase):
|
||||||
@@ -99,6 +105,7 @@ class TestReadAuditEntries(unittest.TestCase):
|
|||||||
def test_reads_entries_from_db(self) -> None:
|
def test_reads_entries_from_db(self) -> None:
|
||||||
with tempfile.TemporaryDirectory() as home, \
|
with tempfile.TemporaryDirectory() as home, \
|
||||||
patch.dict("os.environ", {"HOME": home}):
|
patch.dict("os.environ", {"HOME": home}):
|
||||||
|
AuditStore().migrate()
|
||||||
write_audit_entry(AuditEntry(
|
write_audit_entry(AuditEntry(
|
||||||
timestamp="t",
|
timestamp="t",
|
||||||
bottle_slug="slug",
|
bottle_slug="slug",
|
||||||
@@ -142,6 +149,7 @@ class TestStoreGuardBranches(unittest.TestCase):
|
|||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "q.db"
|
db = Path(d) / "q.db"
|
||||||
store = QueueStore("key", db_path=db)
|
store = QueueStore("key", db_path=db)
|
||||||
|
store.migrate()
|
||||||
self.assertTrue(db.is_file())
|
self.assertTrue(db.is_file())
|
||||||
self.assertEqual(db, store.db_path)
|
self.assertEqual(db, store.db_path)
|
||||||
|
|
||||||
@@ -149,6 +157,7 @@ class TestStoreGuardBranches(unittest.TestCase):
|
|||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "q.db"
|
db = Path(d) / "q.db"
|
||||||
store = QueueStore("key", db_path=db)
|
store = QueueStore("key", db_path=db)
|
||||||
|
store.migrate()
|
||||||
db.unlink()
|
db.unlink()
|
||||||
self.assertEqual([], store.list_pending_proposals())
|
self.assertEqual([], store.list_pending_proposals())
|
||||||
|
|
||||||
@@ -156,6 +165,7 @@ class TestStoreGuardBranches(unittest.TestCase):
|
|||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "q.db"
|
db = Path(d) / "q.db"
|
||||||
store = QueueStore("key", db_path=db)
|
store = QueueStore("key", db_path=db)
|
||||||
|
store.migrate()
|
||||||
db.unlink()
|
db.unlink()
|
||||||
self.assertEqual([], store.list_all_pending_proposals())
|
self.assertEqual([], store.list_all_pending_proposals())
|
||||||
|
|
||||||
@@ -163,27 +173,31 @@ class TestStoreGuardBranches(unittest.TestCase):
|
|||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "q.db"
|
db = Path(d) / "q.db"
|
||||||
store = QueueStore("key", db_path=db)
|
store = QueueStore("key", db_path=db)
|
||||||
|
store.migrate()
|
||||||
db.unlink()
|
db.unlink()
|
||||||
store.archive_proposal("anything") # must not raise
|
store.archive_proposal("anything") # must not raise
|
||||||
|
|
||||||
def test_queue_store_chmod_oserror_is_swallowed(self):
|
def test_queue_store_chmod_oserror_is_swallowed(self):
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "q.db"
|
db = Path(d) / "q.db"
|
||||||
|
store = QueueStore("key", db_path=db)
|
||||||
with patch("pathlib.Path.chmod", side_effect=OSError("ro")):
|
with patch("pathlib.Path.chmod", side_effect=OSError("ro")):
|
||||||
QueueStore("key", db_path=db) # must not raise
|
store.migrate() # must not raise
|
||||||
|
|
||||||
def test_audit_store_missing_db_read_returns_empty(self):
|
def test_audit_store_missing_db_read_returns_empty(self):
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "a.db"
|
db = Path(d) / "a.db"
|
||||||
store = AuditStore(db_path=db)
|
store = AuditStore(db_path=db)
|
||||||
|
store.migrate()
|
||||||
db.unlink()
|
db.unlink()
|
||||||
self.assertEqual([], store.read_audit_entries("egress", "slug"))
|
self.assertEqual([], store.read_audit_entries("egress", "slug"))
|
||||||
|
|
||||||
def test_audit_store_chmod_oserror_is_swallowed(self):
|
def test_audit_store_chmod_oserror_is_swallowed(self):
|
||||||
with tempfile.TemporaryDirectory() as d:
|
with tempfile.TemporaryDirectory() as d:
|
||||||
db = Path(d) / "a.db"
|
db = Path(d) / "a.db"
|
||||||
|
store = AuditStore(db_path=db)
|
||||||
with patch("pathlib.Path.chmod", side_effect=OSError("ro")):
|
with patch("pathlib.Path.chmod", side_effect=OSError("ro")):
|
||||||
AuditStore(db_path=db) # must not raise
|
store.migrate() # must not raise
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
|
|||||||
@@ -17,6 +17,8 @@ from unittest.mock import patch
|
|||||||
# bare name `supervise`.
|
# bare name `supervise`.
|
||||||
sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / "bot_bottle"))
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / "bot_bottle"))
|
||||||
import supervise as _sv # noqa: E402 # type: ignore
|
import supervise as _sv # noqa: E402 # type: ignore
|
||||||
|
import queue_store as _qs # noqa: E402 # type: ignore
|
||||||
|
import audit_store as _as # noqa: E402 # type: ignore
|
||||||
|
|
||||||
from bot_bottle import supervise_server # noqa: E402
|
from bot_bottle import supervise_server # noqa: E402
|
||||||
from bot_bottle.supervise_server import (
|
from bot_bottle.supervise_server import (
|
||||||
@@ -267,6 +269,8 @@ class TestHandleToolsCall(unittest.TestCase):
|
|||||||
self._tmp = tempfile.TemporaryDirectory(prefix="supervise-server-test.")
|
self._tmp = tempfile.TemporaryDirectory(prefix="supervise-server-test.")
|
||||||
self._home_patch = self._patch_home(Path(self._tmp.name))
|
self._home_patch = self._patch_home(Path(self._tmp.name))
|
||||||
self.config = ServerConfig(bottle_slug="dev")
|
self.config = ServerConfig(bottle_slug="dev")
|
||||||
|
_qs.QueueStore("dev").migrate()
|
||||||
|
_as.AuditStore().migrate()
|
||||||
|
|
||||||
def tearDown(self):
|
def tearDown(self):
|
||||||
self._home_patch()
|
self._home_patch()
|
||||||
|
|||||||
Reference in New Issue
Block a user