Skip to content

Commit 2933701

Browse files
jpheinclaude
authored andcommitted
fix(blob-seq-marker): tests + style nit per @igorls MemPalace#1177 review
Three new tests cover the marker contract: - test_fix_blob_seq_ids_writes_marker_after_blob_path — marker is written after a successful BLOB → INTEGER conversion. - test_fix_blob_seq_ids_writes_marker_when_already_integer — marker is written even on a no-op (already-INTEGER) path. The point of the marker is to skip sqlite3.connect() on subsequent calls, not to record that a conversion happened. - test_fix_blob_seq_ids_skips_sqlite_when_marker_present — verifies the load-bearing property via monkeypatched sqlite3.connect: when the marker exists, the function never opens sqlite. This is the bug MemPalace#1090 fix — opening Python's sqlite3 against a live ChromaDB 1.5.x WAL DB corrupts the next PersistentClient call, and we must never do it again post-migration. Also folded in @igorls's style nit: - Path(marker).touch() instead of open(marker, "a").close(). Same effect, reads cleaner. Moved Path import to the top of the module since it didn't yet exist there. 35/35 backend tests pass. The "Production: fresh MCP server + stop hook + mempalace mine subprocess" checkbox in the PR body is checked in the PR reply — the canonical 151K palace has been running this fix since MemPalace#1177 was filed (via Syncthing-replicated source on the disks daemon at v1.7.0); zero PersistentClient corruption since. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent cf8a6a5 commit 2933701

2 files changed

Lines changed: 71 additions & 1 deletion

File tree

mempalace/backends/chroma.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import logging
55
import os
66
import sqlite3
7+
from pathlib import Path
78
from typing import Any, Optional
89

910
import chromadb
@@ -286,7 +287,7 @@ def _fix_blob_seq_ids(palace_path: str) -> None:
286287
# confirmed to be in the INTEGER-seq_id state and future opens can skip the
287288
# sqlite3.connect() entirely.
288289
try:
289-
open(marker, "a").close()
290+
Path(marker).touch()
290291
except OSError:
291292
logger.exception("Could not write migration marker %s", marker)
292293

tests/test_backends.py

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,75 @@ def test_fix_blob_seq_ids_noop_without_database(tmp_path):
382382
_fix_blob_seq_ids(str(tmp_path)) # should not raise
383383

384384

385+
def test_fix_blob_seq_ids_writes_marker_after_blob_path(tmp_path):
386+
"""The .blob_seq_ids_migrated marker is written after a successful BLOB → INTEGER conversion."""
387+
from pathlib import Path
388+
from mempalace.backends.chroma import _BLOB_FIX_MARKER
389+
390+
db_path = tmp_path / "chroma.sqlite3"
391+
conn = sqlite3.connect(str(db_path))
392+
conn.execute("CREATE TABLE embeddings (rowid INTEGER PRIMARY KEY, seq_id)")
393+
conn.execute("INSERT INTO embeddings (seq_id) VALUES (?)", ((42).to_bytes(8, "big"),))
394+
conn.commit()
395+
conn.close()
396+
397+
marker = tmp_path / _BLOB_FIX_MARKER
398+
assert not marker.exists()
399+
400+
_fix_blob_seq_ids(str(tmp_path))
401+
402+
assert marker.is_file(), "marker must be written after a successful migration"
403+
404+
405+
def test_fix_blob_seq_ids_writes_marker_when_already_integer(tmp_path):
406+
"""The marker is written even when the migration is a no-op (already INTEGER).
407+
408+
The point of the marker is to skip the sqlite3 open on subsequent calls,
409+
not to record that a conversion happened. So a clean palace gets the
410+
marker on first run too — next ``_fix_blob_seq_ids`` call short-circuits
411+
before touching the sqlite3 file.
412+
"""
413+
from pathlib import Path
414+
from mempalace.backends.chroma import _BLOB_FIX_MARKER
415+
416+
db_path = tmp_path / "chroma.sqlite3"
417+
conn = sqlite3.connect(str(db_path))
418+
conn.execute("CREATE TABLE embeddings (rowid INTEGER PRIMARY KEY, seq_id INTEGER)")
419+
conn.execute("INSERT INTO embeddings (seq_id) VALUES (42)")
420+
conn.commit()
421+
conn.close()
422+
423+
marker = tmp_path / _BLOB_FIX_MARKER
424+
assert not marker.exists()
425+
426+
_fix_blob_seq_ids(str(tmp_path))
427+
428+
assert marker.is_file(), "marker must be written even when no BLOBs found"
429+
430+
431+
def test_fix_blob_seq_ids_skips_sqlite_when_marker_present(tmp_path):
432+
"""When the marker exists, ``_fix_blob_seq_ids`` does not open sqlite3.
433+
434+
This is the load-bearing property of the marker — opening Python's
435+
sqlite3 against a live ChromaDB 1.5.x WAL DB corrupts the next
436+
PersistentClient call (#1090). Once a palace has been migrated, we
437+
never want to open it again, even read-only.
438+
"""
439+
from pathlib import Path
440+
from unittest.mock import patch
441+
from mempalace.backends.chroma import _BLOB_FIX_MARKER
442+
443+
# Pre-create the marker so the function should short-circuit.
444+
db_path = tmp_path / "chroma.sqlite3"
445+
db_path.write_bytes(b"sentinel") # presence required for the function to proceed
446+
(tmp_path / _BLOB_FIX_MARKER).touch()
447+
448+
with patch("mempalace.backends.chroma.sqlite3.connect") as mock_connect:
449+
_fix_blob_seq_ids(str(tmp_path))
450+
451+
mock_connect.assert_not_called()
452+
453+
385454
# ── quarantine_stale_hnsw ─────────────────────────────────────────────────
386455

387456

0 commit comments

Comments
 (0)