fix(audit): SQLite WAL mode for concurrent agents (#101) - #119
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| """Open a SQLite connection in WAL mode with a busy timeout. | ||
|
|
||
| Drop-in replacement for ``sqlite3.connect(db_path)``. Callers that need a | ||
| ``row_factory`` or other connection attributes should set them on the |
There was a problem hiding this comment.
SUGGESTION: Hardcoded busy timeout (5000ms) should be configurable
Consider making this a parameter with a default value so callers can adjust based on their contention expectations:
| ``row_factory`` or other connection attributes should set them on the | |
| BUSY_TIMEOUT_MS = 5000 | |
| def connect(db_path: Union[str, Path], busy_timeout_ms: int = BUSY_TIMEOUT_MS) -> sqlite3.Connection: |
Then update the PRAGMA line to use the parameter.
| conn = sqlite3.connect(db_path) | ||
| # WAL is harmless (silently stays "memory") on ``:memory:`` databases. | ||
| conn.execute("PRAGMA journal_mode=WAL") | ||
| conn.execute(f"PRAGMA busy_timeout={BUSY_TIMEOUT_MS}") |
There was a problem hiding this comment.
WARNING: WAL mode PRAGMA result not checked
PRAGMA journal_mode=WAL returns the actual journal mode. On some filesystems (network drives, some FUSE mounts) WAL cannot be enabled and it silently falls back to DELETE mode. Check the result to ensure WAL was actually enabled:
result = conn.execute("PRAGMA journal_mode=WAL").fetchone()
if result[0] != "wal":
logger.warning(f"WAL mode not enabled, got: {result[0]}")| # WAL is harmless (silently stays "memory") on ``:memory:`` databases. | ||
| conn.execute("PRAGMA journal_mode=WAL") | ||
| conn.execute(f"PRAGMA busy_timeout={BUSY_TIMEOUT_MS}") | ||
| return conn |
There was a problem hiding this comment.
SUGGESTION: Use parameter binding for PRAGMA busy_timeout
While the value is a constant here, using parameter binding is safer practice and avoids any injection concerns if this becomes configurable:
| return conn | |
| conn.execute("PRAGMA busy_timeout=?", (busy_timeout_ms,)) |
| """Initialise settings database.""" | ||
| Path(self._settings_db_path).parent.mkdir(parents=True, exist_ok=True) | ||
| self._conn = sqlite3.connect(self._settings_db_path) | ||
| self._conn = _db.connect(self._settings_db_path) |
There was a problem hiding this comment.
WARNING: _ensure_conn creates a new connection without closing the old one
If _ensure_conn is called multiple times (e.g. after a connection error), the previous connection is leaked. Consider closing the existing connection first:
if self._conn is not None:
self._conn.close()This is a pre-existing issue but worth fixing since you're touching this code.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (2 files changed, 9 files unchanged)
Reviewed by nemotron-3-ultra-550b-a55b-20260604:free · 144,216 tokens |
1 participant
What
Routes every
sqlite3.connect(...)in the package through a new shared helper,taosmd/_db.py:connect(), which enables WAL journal mode and a 5s busy timeout on each connection.Why
By default
sqlite3.connectopens databases in rollback-journal mode. That mode takes an exclusive lock for the duration of a write and permits only one writer at a time, so when multiple agents (or multiple processes on the same machine) share a memory store, contention surfaces asSQLITE_BUSY/ "database is locked" errors.PRAGMA journal_mode=WAL) lets readers proceed concurrently with a writer instead of blocking, which is exactly the multi-machine / concurrent-agent access pattern taOSmd targets.PRAGMA busy_timeout=5000) makes a contended writer block-and-retry for up to 5s rather than failing immediately.Both are plain PRAGMAs: no new dependencies, no benchmark impact. Standalone single-process behaviour is unchanged apart from the journal mode (and WAL silently stays
memoryon:memory:databases, so it is harmless there).Call sites routed
archive.py,vector_memory.py,knowledge_graph.py,access_tracker.py,browsing_history.py,crystallize.py,pending_decisions.py,reflect.py,session_catalog.py,taosmd_backend.py(x2). Existingrow_factory/ other per-connection attributes are preserved (set on the returned connection as before).Tests
Full suite green:
209 passed. Verified WAL + busy_timeout are actually applied on a real file DB and that:memory:stays in memory mode.