From 5784706256db6aebbcdf8680a0fccfe7887f5ede Mon Sep 17 00:00:00 2001 From: Cipher208 <269750686+Cipher208@users.noreply.github.com> Date: Wed, 7 Oct 2026 16:29:07 +0200 Subject: [PATCH] fix(shared): let ArchivedMemories create its own table, and stop the test leaking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_archived_memories` failed with `no such table: archived_memories` when run alone, and passed when another test had migrated the shared session database first. A test that needs a neighbour to pass is testing the neighbour, not the class. Two separate defects, one on each side. **The test wrote to the real memory directory.** It built `ArchivedMemories()` with no connection manager, so it used the global one — and before `hermetic_global_db` existed (90c9149, 2026-08-24) that was the live data dir. The evidence is still there: `~/.mcp-ariel-memory/memory.db` holds 32 rows with `user_id='test_sh'`, all of them from 2026-08-24, the day the fixture landed. It now takes its own manager on `tmp_path`, like its neighbour `test_connection.py` already did. `test_dream_buffer` and `test_embedding_cache` are given the same treatment: neither ever checked a global property, and the leak date names `test_sh` as the marker for both. **The class assumed its table already existed.** `archive()` and `get_archived()` went straight to `SELECT`, while the table is created by `_init_db()` — and in production by the Alembic migration. On a migrated database it worked, which is what hid this. `DreamBuffer` solved the identical problem already, with an idempotent `ensure()` guarded by `_ready` and called from each method that touches the table; `ArchivedMemories` now does the same, with the same shape and for the same reason. The schema in `_init_db` matches the migration's definition exactly, so no new migration is needed. Four production call sites construct `ArchivedMemories` and never run migrations — `core/episodic.py`, `lifecycle/forgetting.py`, `rag/conflict.py` and `mcp_server/tools/primitives/forget.py` — so this was reachable outside tests, not just inside them. Two others (`features/compression.py`, `forget.py`) called `_init_db()` by hand before `archive()`; those calls stay, they are now simply redundant rather than load-bearing. Tests: each of the six in the file passes alone (was: one failed alone); one new test covers the class on an already-migrated database. Full suite: 2087 passed. The real memory directory was verified untouched — still 32 rows and the same mtime. --- shared/archived_memories.py | 28 +++++++++++++++++ tests/test_shared/test_shared.py | 53 ++++++++++++++++++++++++++++---- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/shared/archived_memories.py b/shared/archived_memories.py index 3c099ba4..69b82550 100644 --- a/shared/archived_memories.py +++ b/shared/archived_memories.py @@ -10,6 +10,30 @@ class ArchivedMemories: def __init__(self, cm: AsyncConnectionManager | None = None): self._cm = cm or connection_manager + self._ready = False + + async def ensure(self) -> None: + """Create the table if it is not there yet, once per instance. + + Same shape as `DreamBuffer.ensure`, and for the same reason: this class + is constructed in several places that never run migrations, so a method + that touches the table has to be able to bring it into existence. + + Why it was needed: `archive()` and `get_archived()` used to assume the + table existed. On a database that had run the migration it did, which + hid the problem — `tests/test_shared/test_shared.py::test_archived_memories` + passed only when another test had migrated the shared session database + first. Run alone, or with `-p no:randomly`, it failed with + `no such table: archived_memories`. A test that needs a neighbour to + pass is not testing the class. + """ + if self._ready: + return + conn = await self._cm.get(DB_NAME) + cols = [r[1] for r in await (await conn.execute("PRAGMA table_info(archived_memories)")).fetchall()] + if not cols: + await self._init_db() + self._ready = True async def _init_db(self) -> None: await self._cm.execute_script( @@ -36,6 +60,7 @@ async def archive( original_id: int | None = None, reason: str = "manual", ) -> int: + await self.ensure() conn = await self._cm.get(DB_NAME) cursor = await conn.execute( "INSERT INTO archived_memories (user_id, original_id, content, memory_type, importance, archive_reason) VALUES (?, ?, ?, ?, ?, ?)", @@ -46,6 +71,7 @@ async def archive( return int(last_id) if last_id is not None else 0 async def get_archived(self, user_id: str = "default", limit: int = 50) -> list[dict[str, Any]]: + await self.ensure() conn = await self._cm.get(DB_NAME) cursor = await conn.execute( "SELECT * FROM archived_memories WHERE user_id=? ORDER BY archived_at DESC LIMIT ?", @@ -64,11 +90,13 @@ async def get_archived(self, user_id: str = "default", limit: int = 50) -> list[ ] async def count(self, user_id: str = "default") -> int: + await self.ensure() conn = await self._cm.get(DB_NAME) row = await (await conn.execute("SELECT COUNT(*) FROM archived_memories WHERE user_id=?", (user_id,))).fetchone() return int(row[0]) if row else 0 async def restore(self, archived_id: int) -> dict[str, Any] | None: + await self.ensure() conn = await self._cm.get(DB_NAME) row = await (await conn.execute("SELECT * FROM archived_memories WHERE id=?", (archived_id,))).fetchone() if row: diff --git a/tests/test_shared/test_shared.py b/tests/test_shared/test_shared.py index e688cdbe..2ec55928 100644 --- a/tests/test_shared/test_shared.py +++ b/tests/test_shared/test_shared.py @@ -7,37 +7,78 @@ sys.path.insert(0, str(Path(__file__).parent.parent.parent)) -def test_dream_buffer(): +def test_dream_buffer(tmp_path): + from shared.connection import AsyncConnectionManager from shared.dream_buffer import DreamBuffer async def t(): - db = DreamBuffer() + cm = AsyncConnectionManager(base_dir=str(tmp_path)) + db = DreamBuffer(cm=cm) await db.add("test_sh", "s1", "msg", 0.5) assert await db.count("test_sh") >= 1 await db.cleanup_old(max_age_hours=0, max_count=0) + await cm.close_all() asyncio.run(t()) -def test_archived_memories(): +def test_archived_memories(tmp_path): + """Свой manager на tmp_path, а не глобальный. + + Тест строил `ArchivedMemories()` без `cm`, то есть на глобальном + `connection_manager`, и тем самым писал прямо в рабочий каталог памяти: + в `~/.mcp-ariel-memory/memory.db` осталось 32 строки `user_id='test_sh'` + от 24.08.2026 — до того, как conftest получил фикстуру `hermetic_global_db` + (коммит 90c9149). После неё тест стал ещё и непроходимым в одиночку: + таблицу `archived_memories` создавала только миграция, а без соседа, + который её прогонял, вызов падал на `no such table`. + + Хозяин базы теперь свой, и он же проверяет настоящее свойство класса: + таблица появляется сама (см. `ArchivedMemories.ensure`). + """ from shared.archived_memories import ArchivedMemories + from shared.connection import AsyncConnectionManager async def t(): - am = ArchivedMemories() + cm = AsyncConnectionManager(base_dir=str(tmp_path)) + am = ArchivedMemories(cm=cm) await am.archive("test_sh", "Old memory", importance=0.2, reason="test") archived = await am.get_archived("test_sh") assert len(archived) >= 1 + await cm.close_all() + + asyncio.run(t()) + + +def test_archived_memories_works_on_a_migrated_database(tmp_path): + """На мигрированной базе класс работает и таблицу не ломает.""" + from shared.archived_memories import ArchivedMemories + from shared.connection import AsyncConnectionManager + from shared.migrations import MigrationManager + + async def t(): + cm = AsyncConnectionManager(base_dir=str(tmp_path)) + await MigrationManager(cm=cm).migrate() + am = ArchivedMemories(cm=cm) + assert await am.count("test_sh") == 0 + await am.archive("test_sh", "Migrated db row", reason="test") + assert await am.count("test_sh") == 1 + await cm.close_all() asyncio.run(t()) -def test_embedding_cache(): +def test_embedding_cache(tmp_path): + """Тот же принцип: свой manager, чтобы файл не трогал общий каталог вовсе.""" + from shared.connection import AsyncConnectionManager from shared.embeddings import EmbeddingCache async def t(): - ec = EmbeddingCache() + cm = AsyncConnectionManager(base_dir=str(tmp_path)) + ec = EmbeddingCache(cm=cm) emb = await ec.embed_single("test") assert len(emb) == 384 + await cm.close_all() asyncio.run(t())