Repository navigation
fix(audit): SQLite WAL mode for concurrent agents (#101) #119
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
Changes from 1 commit
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 | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||
| """Shared SQLite connection helper. | ||||||
|
|
||||||
| Every persistent store in the package opens its own SQLite database. By | ||||||
| default ``sqlite3.connect`` uses the rollback-journal mode, which takes an | ||||||
| exclusive lock for the duration of a write and lets only a single writer | ||||||
| touch the file at a time. When several agents (or several processes on the | ||||||
| same machine) share a memory store, that serialisation surfaces as | ||||||
| ``SQLITE_BUSY`` errors. | ||||||
|
|
||||||
| ``connect`` centralises the fix: it enables write-ahead logging (WAL), which | ||||||
| lets readers proceed concurrently with a writer, and sets a busy timeout so a | ||||||
| contended writer waits and retries rather than failing immediately. Both are | ||||||
| plain PRAGMAs with no extra dependencies; standalone behaviour is unchanged | ||||||
| apart from the journal mode. | ||||||
| """ | ||||||
|
|
||||||
| from __future__ import annotations | ||||||
|
|
||||||
| import sqlite3 | ||||||
| from pathlib import Path | ||||||
| from typing import Union | ||||||
|
|
||||||
| # Allow a contended connection to block-and-retry for this many milliseconds | ||||||
| # before raising ``sqlite3.OperationalError: database is locked``. | ||||||
| BUSY_TIMEOUT_MS = 5000 | ||||||
|
|
||||||
|
|
||||||
| def connect(db_path: Union[str, Path]) -> sqlite3.Connection: | ||||||
| """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 | ||||||
| returned connection as before. | ||||||
| """ | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: WAL mode PRAGMA result not checked
result = conn.execute("PRAGMA journal_mode=WAL").fetchone()
if result[0] != "wal":
logger.warning(f"WAL mode not enabled, got: {result[0]}") |
||||||
| return conn | ||||||
|
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. 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:
Suggested change
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ | |
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| from . import __version__ | ||
| from . import __version__, _db | ||
| from .backend import MemoryBackend | ||
|
|
||
| SETTINGS_SCHEMA = """ | ||
|
|
@@ -82,7 +82,7 @@ def __init__( | |
| async def init(self) -> None: | ||
| """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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: If if self._conn is not None:
self._conn.close()This is a pre-existing issue but worth fixing since you're touching this code. |
||
| self._conn.row_factory = sqlite3.Row | ||
| self._conn.executescript(SETTINGS_SCHEMA + AGENT_CONFIG_SCHEMA) | ||
| self._conn.commit() | ||
|
|
@@ -260,7 +260,7 @@ def _ensure_conn(self) -> None: | |
| """Ensure the settings DB connection is open (auto-init if needed).""" | ||
| if self._conn is None: | ||
| 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) | ||
| self._conn.row_factory = sqlite3.Row | ||
| self._conn.executescript(SETTINGS_SCHEMA + AGENT_CONFIG_SCHEMA) | ||
| self._conn.commit() | ||
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.
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:
Then update the PRAGMA line to use the parameter.