-
Notifications
You must be signed in to change notification settings - Fork 7.6k
fix(repair): treat SQLITE_BUSY as contention, not corruption, in sqlite_integrity_errors #1932
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
be95467
bc9c052
2c08bb2
bacb935
5cbc37e
3dad77d
994d76f
ace6284
8b88ef0
e273f84
de63128
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import os | ||
| import sqlite3 | ||
| import threading | ||
| from contextlib import closing | ||
| from unittest.mock import MagicMock, call, patch | ||
|
|
||
|
|
@@ -1358,6 +1359,45 @@ def test_sqlite_integrity_errors_returns_empty_for_healthy_db(tmp_path): | |
| assert repair.sqlite_integrity_errors(str(palace)) == [] | ||
|
|
||
|
|
||
| def test_sqlite_integrity_errors_waits_out_transient_writer_lock(tmp_path): | ||
| """A concurrent writer must read as contention, not corruption. | ||
|
|
||
| Python's sqlite3.connect ships a 5-second default busy timeout, but | ||
| real peer writes (batch mines, curator passes) routinely hold the | ||
| write lock longer than that. Before the explicit busy_timeout fix, | ||
| quick_check then failed with "database is locked" and the MCP startup | ||
| integrity gate (#1818) reported the palace as corrupt — every client | ||
| failed loudly (and typically reconnect-stormed) for the entire | ||
| duration of an otherwise healthy batch write. | ||
|
|
||
| The 7-second hold below is deliberate: over the 5 s default that | ||
| masked the bug, under the 15 s explicit timeout that fixes it. | ||
| """ | ||
| palace = tmp_path / "palace" | ||
| palace.mkdir() | ||
| db_path = palace / "chroma.sqlite3" | ||
|
|
||
| with sqlite3.connect(db_path) as conn: | ||
| conn.execute("CREATE TABLE dummy(id INTEGER PRIMARY KEY)") | ||
| conn.commit() | ||
|
|
||
| locker = sqlite3.connect(db_path, check_same_thread=False) | ||
| locker.execute("BEGIN EXCLUSIVE") | ||
|
|
||
| def _release(): | ||
| locker.commit() | ||
| locker.close() | ||
|
|
||
| timer = threading.Timer(7.0, _release) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| timer.start() | ||
| try: | ||
| errors = repair.sqlite_integrity_errors(str(palace)) | ||
| finally: | ||
| timer.join() | ||
|
|
||
| assert errors == [] | ||
|
|
||
|
|
||
| def test_sqlite_integrity_errors_reports_unreadable_sqlite_file(tmp_path): | ||
| palace = tmp_path / "palace" | ||
| palace.mkdir() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While setting
PRAGMA busy_timeout = 15000resolves the contention issue forsqlite_integrity_errors, there are multiple sibling SQLite connection implementations across the codebase (such assqlite_drawer_countandextract_via_sqliteinmempalace/repair.py, and several functions inmempalace/backends/chroma.py) that also connect tochroma.sqlite3without 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