fix(repair): treat SQLITE_BUSY as contention, not corruption, in sqlite_integrity_errors - #1932
fix(repair): treat SQLITE_BUSY as contention, not corruption, in sqlite_integrity_errors#1932elitedevs wants to merge 11 commits into
Conversation
Release v3.4.0 — promote develop to main
Release v3.4.1 — promote develop to main
Release v3.5.0 — promote develop to main
…ments (MemPalace#1630) L1's generate() scored drawers by importance/emotional_weight/weight, and the docstring promised "prefer high importance, recent filing". But no ingest path (miner, convo_miner, diary, add_drawer) writes any of those fields, so the sort collapsed to insertion order (oldest first) and recency was never consulted. A scoped `wake-up --wing X` therefore surfaced the *oldest* moments: the opposite of useful. Add filed_at (present on every drawer, ISO-8601, lexically chronological) as the secondary sort key. Importance stays primary for the day a scoring pass populates it; filed_at is the effective signal today, making the "recent filing" half of the promise true with data already present. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
…ace#1648) On Windows, Path.read_text() and open(path, 'a') use locale encoding (GBK on Chinese-locale systems) before PEP 686 / Python 3.15. A valid UTF-8 .gitignore with non-ASCII comments crashes _ensure_mempalace_files_gitignored() with UnicodeDecodeError, which aborts 'mempalace init' on Windows for any user whose .gitignore contains non-ASCII text. Force encoding='utf-8' on both read and append, with errors='replace' on read as a defensive fallback for legacy mixed-encoding files. Co-authored-by: ALaDingAhmad <16530935@qq.com> Co-authored-by: Igor Lins e Silva <4753812+igorls@users.noreply.github.com>
…te_integrity_errors PRAGMA quick_check ran with no explicit busy_timeout, so any peer writer holding the write lock longer than Python's 5 s connect default (batch mines, curator passes on a rollback-journal palace) made quick_check fail with 'database is locked'. Callers treat any error as palace corruption — most damagingly the MCP startup integrity gate (MemPalace#1818), which then refuses to serve every client for the duration of an otherwise healthy batch write, typically triggering a client reconnect storm. Set busy_timeout = 15000 (same pattern as the sqlite fast paths in mcp_server) so transient writers are waited out. Regression test holds the exclusive lock for 7 s — over the 5 s default that masked the bug, under the 15 s explicit timeout.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request configures a 15-second busy timeout (PRAGMA busy_timeout = 15000) on the SQLite connection in sqlite_integrity_errors to prevent transient database locks from being misreported as corruption, and adds a corresponding unit test with a 7-second delay. Feedback suggests avoiding this one-off fix in favor of a centralized connection helper to maintain repository-wide consistency across other SQLite connections. Additionally, it is recommended to refactor the unit test to avoid the 7-second delay, either by querying the PRAGMA directly or using a much shorter timeout, to keep the test suite fast and deterministic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # rollback-journal palace), and callers — including the MCP | ||
| # startup integrity gate (#1818) — misreport a healthy palace | ||
| # as corrupt. Wait out transient writers before giving up. | ||
| conn.execute("PRAGMA busy_timeout = 15000") |
There was a problem hiding this comment.
While setting PRAGMA busy_timeout = 15000 resolves the contention issue for sqlite_integrity_errors, there are multiple sibling SQLite connection implementations across the codebase (such as sqlite_drawer_count and extract_via_sqlite in mempalace/repair.py, and several functions in mempalace/backends/chroma.py) that also connect to chroma.sqlite3 without setting a busy timeout.
According to the repository's general rules, we should avoid applying a one-off fix to a single instance when a common issue or pattern is present in multiple sibling implementations. Instead, we should maintain repository-wide consistency by deferring the fix to a dedicated change that addresses all occurrences together (for example, by introducing a centralized connection helper that consistently configures the read-only URI and busy timeout).
References
- When addressing a common issue or pattern (such as unsafe SQLite URI path encoding) that is present in multiple sibling implementations across the codebase, avoid applying a one-off fix to a single instance. Instead, maintain repository-wide consistency by deferring the fix to a dedicated change that addresses all occurrences together.
| locker.commit() | ||
| locker.close() | ||
|
|
||
| timer = threading.Timer(7.0, _release) |
There was a problem hiding this comment.
Using a 7-second timer in a unit test introduces a significant delay, making the test suite slow to run.
Instead of performing a real-time sleep/wait to verify the busy timeout behavior, we can verify that the PRAGMA busy_timeout is correctly set on the connection by querying it directly (e.g., executing PRAGMA busy_timeout and asserting it returns 15000). If a functional test is absolutely necessary, we can use a much shorter duration (e.g., 0.1 or 0.2 seconds) and configure a smaller timeout for the test connection to keep the test fast and deterministic.
Layer1 previously fetched only the first MAX_SCAN=2000 drawers in insertion order and sorted by importance alone, so wake-up permanently showed the oldest high-importance drawers and never surfaced anything filed after the cap. With ~75K drawers the essential story was frozen in spring 2026. - Metadata-only scan of the full corpus (documents fetched only for the top MAX_DRAWERS winners), MAX_SCAN raised to a safety valve - Score = importance + RECENCY_WEIGHT * 0.5^(age_days/14): a fresh default-importance drawer outranks a stale importance-5 one after ~2 half-lives, so L1 tracks the present, not the archive
…es filed_at, not authored_at
At weight 2.0 a fresh default-importance drawer scored 5.00, losing by ~0.05 to 2-month-old weight-5 drawers whose residual recency kept them at 5.03-5.07. Weight 3.0 gives today's work a decisive edge for ~10 days while evergreen weight-5 facts still fill remaining slots. Dedupe: multi-chunk files (e.g. 5 chunks of one SKILL.md) were taking 5 of 15 L1 slots; cap at one drawer per source_file.
…sy-timeout # Conflicts: # mempalace/layers.py
The Layer1 rewrite (994d76f..8b88ef0) reads a paginated metadata-only scan keyed on ids, then fetches documents for just the winners. The shared mock still returned the old single-shot documents+metadatas shape with no ids, so generate() saw an empty palace and 7 tests failed without exercising the new scoring at all. Rework _mock_chromadb_for_layer to emulate the real access pattern and rewrite test_layer1_batch_exception_breaks so the first page fills a whole scan batch — the only way the mid-scan exception path can fire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZbwhCTsJk5aLjmrbRVMTL
|
Thanks for this contribution, and apologies for the slow turnaround.
If you'd rather not pick it back up, no problem at all — just say so and I'll close it out, and thanks either way for taking the time to send it. |
igorls
added
bug
5 participants
Problem
sqlite_integrity_errors()runsPRAGMA quick_checkwith no explicitbusy_timeout. Any peer writer holding the write lock longer than Python's 5 s connect default makes quick_check fail withdatabase is locked, and every caller treats that as palace corruption.The worst-hit caller is the MCP startup integrity gate (#1818): while a long batch write is in flight (curator pass, batch mine — routine on a rollback-journal palace), every new MCP server process reports the palace as corrupt and refuses to serve. Clients retry, each retry spawns another server process that fails the same gate — we hit this in production with 11 accumulated MCP processes and a fully bricked palace during a healthy 15-minute batch write.
mempalace_statusreportedPRAGMA quick_check failed: database is lockedas an integrity failure with 0 drawers visible, while the CLI on the host read all 38k drawers fine.Contention is not corruption.
Fix
Set
PRAGMA busy_timeout = 15000on the quick_check connection — same pattern already used by the sqlite fast paths inmcp_server.py. Transient writers are waited out; a genuinely wedged lock (>15 s) still surfaces, which is correct for the repair preflight (it must not proceed todelete_collection()when it cannot verify).Test
test_sqlite_integrity_errors_waits_out_transient_writer_lockholdsBEGIN EXCLUSIVEfrom a peer connection for 7 s — deliberately over the 5 s implicit default that masked the bug, under the new 15 s timeout. Verified it fails ondevelopwithout the fix and passes with it. Fulltests/test_repair.pysuite passes (91), ruff clean.Note for maintainers
Longer term you may want the integrity gate to distinguish "couldn't verify (BUSY)" from "verified corrupt" — the repair preflight should abort on both, but the MCP startup gate arguably shouldn't brick clients on the former. This PR keeps the contract unchanged and just makes the check contention-tolerant.