pickle -> sqlite store: D6 async writes, D9 reload race, one-shot pickle migration

2026-07-13 13:04:32 +02:00
parent 6e5abf3d2c
commit f6c3e7d8e5
9 changed files with 331 additions and 47 deletions
@@ -1,6 +1,3 @@
-import os
-import pickle
-import tempfile
 import unittest
 from unittest.mock import Mock, patch

@@ -147,7 +144,6 @@ You always try to say something positive about the current day and the Fjærkroa
    def test_update_history(self) -> None:
        updater = self.bot.airesponder
        updater.history = []
-        updater.history_file = None

        question = {"content": '{"channel": "test_channel", "message": "What is the meaning of life?"}'}
        answer = {"content": '{"channel": "test_channel", "message": "42"}'}
@@ -179,15 +175,7 @@ You always try to say something positive about the current day and the Fjærkroa
        next_answer2 = {"content": '{"channel": "other_channel", "message": "Tripple Z"}'}
        updater.update_history(next_question2, next_answer2, 4)
        self.assertEqual(updater.history, [new_answer, next_answer, next_question2, next_answer2])
-
-        # Test case 5: Check history file save using mock
-        with unittest.mock.patch("builtins.open", unittest.mock.mock_open()) as mock_file:
-            _, temp_path = tempfile.mkstemp()
-            os.remove(temp_path)
-            self.bot.airesponder.history_file = temp_path
-            updater.update_history(question, answer, 2)
-            mock_file.assert_called_with(temp_path, "wb")
-            mock_file().write.assert_called_with(pickle.dumps([question, answer]))
+        # File persistence moved to the SQLite store — covered by PER-01 (SPEC-009)


 if __name__ == "__mait__":
@@ -107,17 +107,13 @@ class TestFunctionality(TestBotBase):
            ' "channel": "some_channel", "direct": false, "historise_question": true}',
        )

-    @patch("builtins.open", new_callable=mock_open)
-    def test_update_history_with_file(self, mock_file):
+    def test_update_history_trims_to_limit(self):
        self.bot.airesponder.update_history({"content": '{"q": "What\'s your name?"}'}, {"content": '{"a": "AI"}'}, 10)
        self.assertEqual(len(self.bot.airesponder.history), 2)
        self.bot.airesponder.update_history({"content": '{"q1": "Q1"}'}, {"content": '{"a1": "A1"}'}, 2)
        self.bot.airesponder.update_history({"content": '{"q2": "Q2"}'}, {"content": '{"a2": "A2"}'}, 2)
        self.assertEqual(len(self.bot.airesponder.history), 2)
-        self.bot.airesponder.history_file = "mock_file.pkl"
-        self.bot.airesponder.update_history({"content": '{"q": "What\'s your favorite color?"}'}, {"content": '{"a": "Blue"}'}, 10)
-        mock_file.assert_called_once_with("mock_file.pkl", "wb")
-        mock_file().write.assert_called_once()
+        # File persistence moved to the SQLite store — covered by PER-01 (SPEC-009)


 if __name__ == "__mait__":
@@ -0,0 +1,143 @@
+"""Unit coverage for SPEC-009 persistence (PER-01..05) + CFG-04 (D9)."""
+
+import pickle
+import sqlite3
+import stat
+import tempfile
+import threading
+import unittest
+from pathlib import Path
+from unittest.mock import MagicMock
+
+from fjerkroa_bot.ai_responder import AIMessage, AIResponder
+from fjerkroa_bot.persistence import PersistentStore
+
+from .test_bdd_envelope import FakeModelResponder, envelope
+from .test_main import TestBotBase
+
+
+class StoreBase(unittest.TestCase):
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.db_path = Path(self.tmp.name) / "bot.db"
+
+    def tearDown(self):
+        self.tmp.cleanup()
+
+
+class TestHistoryRoundtrip(StoreBase):
+    def test_history_survives_restart(self):
+        """PER-01: history rows come back per channel, in order, from a fresh store."""
+        store = PersistentStore(self.db_path)
+        entries = [{"role": "user", "content": "one"}, {"role": "assistant", "content": "two"}]
+        store.save_history("chat", entries)
+        store.save_history("other", [{"role": "user", "content": "elsewhere"}])
+        reloaded = PersistentStore(self.db_path)
+        self.assertEqual(reloaded.load_history("chat"), entries)
+        self.assertEqual(reloaded.load_history("other"), [{"role": "user", "content": "elsewhere"}])
+
+    def test_save_replaces_previous_state(self):
+        """PER-01: save_history reflects trims — replaced, not appended."""
+        store = PersistentStore(self.db_path)
+        store.save_history("chat", [{"role": "user", "content": "a"}, {"role": "user", "content": "b"}])
+        store.save_history("chat", [{"role": "user", "content": "b"}])
+        self.assertEqual(store.load_history("chat"), [{"role": "user", "content": "b"}])
+
+
+class TestMemoryRoundtrip(StoreBase):
+    def test_memory_survives_restart(self):
+        """PER-02: per-channel memory string persists across store instances."""
+        store = PersistentStore(self.db_path)
+        store.save_memory("chat", "remember this")
+        self.assertEqual(PersistentStore(self.db_path).load_memory("chat"), "remember this")
+        self.assertIsNone(PersistentStore(self.db_path).load_memory("unknown"))
+
+
+class TestPickleMigration(StoreBase):
+    def test_pickles_migrate_once(self):
+        """PER-03: legacy pickles import once, files renamed *.migrated, no re-import."""
+        history_file = Path(self.tmp.name) / "chat.dat"
+        memory_file = Path(self.tmp.name) / "chat.memory"
+        with open(history_file, "wb") as fd:
+            pickle.dump([{"role": "user", "content": "old times"}], fd)
+        with open(memory_file, "wb") as fd:
+            pickle.dump("old memory", fd)
+
+        config = {"system": "s", "history-limit": 5, "history-directory": self.tmp.name}
+        responder = AIResponder(config, "chat")
+        self.assertEqual(responder.history, [{"role": "user", "content": "old times"}])
+        self.assertEqual(responder.memory, "old memory")
+        self.assertFalse(history_file.exists())
+        self.assertFalse(memory_file.exists())
+        self.assertTrue(history_file.with_suffix(".dat.migrated").exists())
+
+        # second start reads from the DB, does not re-import
+        responder2 = AIResponder(config, "chat")
+        self.assertEqual(responder2.history, [{"role": "user", "content": "old times"}])
+
+
+class TestDatabaseHygiene(StoreBase):
+    def test_wal_version_and_permissions(self):
+        """PER-04: WAL mode, user_version = 1, file mode 0600."""
+        PersistentStore(self.db_path)
+        conn = sqlite3.connect(self.db_path)
+        try:
+            self.assertEqual(conn.execute("PRAGMA journal_mode").fetchone()[0], "wal")
+            self.assertEqual(conn.execute("PRAGMA user_version").fetchone()[0], 1)
+        finally:
+            conn.close()
+        mode = stat.S_IMODE(self.db_path.stat().st_mode)
+        self.assertEqual(mode, 0o600)
+
+
+class TestWritesOffEventLoop(unittest.IsolatedAsyncioTestCase):
+    async def test_persistence_runs_in_worker_thread(self):
+        """PER-05: history writes happen off the event loop (D6)."""
+        tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(tmp.cleanup)
+        config = {"system": "s", "history-limit": 5, "history-directory": tmp.name}
+        responder = FakeModelResponder(config, "chat")
+        writing_threads = set()
+        original_save = responder.store.save_history
+
+        def capture_save(channel, history):
+            writing_threads.add(threading.current_thread())
+            return original_save(channel, history)
+
+        responder.store.save_history = capture_save
+        responder.scripted.append(envelope(answer="hei", answer_needed=True))
+        await responder.send(AIMessage("alice", "hei", "chat"))
+        self.assertTrue(writing_threads)
+        self.assertNotIn(threading.main_thread(), writing_threads)
+        # and the data actually landed
+        self.assertTrue(PersistentStore(Path(tmp.name) / "bot.db").load_history("chat"))
+
+
+class TestConfigReloadRace(TestBotBase):
+    async def test_reload_swaps_on_event_loop(self):
+        """CFG-04: watchdog thread schedules the config swap via call_soon_threadsafe (D9)."""
+        new_config = dict(self.config_data)
+        new_config["history-limit"] = 99
+        self.bot.load_config = lambda path: new_config
+        self.bot.loop = MagicMock()
+        event = MagicMock()
+        event.src_path = self.bot.config_file
+        self.bot.on_config_file_modified(event)
+        self.bot.loop.call_soon_threadsafe.assert_called_once()
+        apply_fn = self.bot.loop.call_soon_threadsafe.call_args.args[0]
+        apply_fn()
+        self.assertEqual(self.bot.config["history-limit"], 99)
+        self.assertEqual(self.bot.airesponder.config["history-limit"], 99)
+
+    async def test_reload_applies_directly_without_loop(self):
+        """CFG-04: before the loop runs, the swap applies synchronously."""
+        new_config = dict(self.config_data)
+        new_config["history-limit"] = 42
+        self.bot.load_config = lambda path: new_config
+        loop = MagicMock()
+        loop.call_soon_threadsafe.side_effect = RuntimeError("no running loop")
+        self.bot.loop = loop
+        event = MagicMock()
+        event.src_path = self.bot.config_file
+        self.bot.on_config_file_modified(event)
+        self.assertEqual(self.bot.config["history-limit"], 42)