Run SQLite in WAL mode with a busy_timeout (B-39)
create_async_engine had no connect_args and there was no PRAGMA anywhere in the repo. SQLite's default rollback-journal mode lets a writer block every reader for the duration of its transaction, and a second writer arriving while one is already active fails immediately with "database is locked" rather than waiting at all - realistic given five concurrent background tasks (scheduler, confirmation poller, RBF bumper, two reconcilers) plus every HTTP handler share one file, and nothing previously handled that error. app/db/base.py now registers a "connect" event on the engine that sets journal_mode=WAL, synchronous=NORMAL and a 5-second busy_timeout on every new DBAPI connection - applied only when the dialect is sqlite, so a future PostgreSQL DATABASE_URL is unaffected. WAL lets readers and writers proceed without blocking each other, and busy_timeout gives a second writer a real window to wait instead of failing instantly. Left out: an explicit application-level retry wrapper for "database is locked" in the background loops, the other half of the proposed fix - busy_timeout already gives SQLite itself several seconds to resolve writer-vs-writer contention before ever raising, and every background loop already catches and logs an unhandled exception before its next scheduled tick, which is itself a retry, just not an immediate one. Suite grows from 211 to 214 tests. BUGS.md moves B-39 to Previously fixed.
This commit is contained in:
@@ -1,11 +1,11 @@
|
|||||||
# Known bugs
|
# Known bugs
|
||||||
|
|
||||||
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high,
|
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high,
|
||||||
7 medium, 8 low), listed below as B-39 … B-49. B-25 through B-38 are fixed (see "Previously
|
7 medium, 8 low), listed below as B-40 … B-49. B-25 through B-39 are fixed (see "Previously
|
||||||
fixed" below) — no Critical-severity finding remains open; the other 11 are Medium/Low.
|
fixed" below) — no Critical-severity finding remains open; the other 10 are Medium/Low.
|
||||||
The 139-test suite was green at the time of the audit, so none of these were caught by existing
|
The 139-test suite was green at the time of the audit, so none of these were caught by existing
|
||||||
coverage — every fix lands with a regression test (the fourteen fixes so far brought the suite
|
coverage — every fix lands with a regression test (the fifteen fixes so far brought the suite
|
||||||
from 139 to 211).
|
from 139 to 214).
|
||||||
|
|
||||||
The recurring pattern across the open findings is worth stating once: the code is rigorous
|
The recurring pattern across the open findings is worth stating once: the code is rigorous
|
||||||
about the failure modes that have actually been hit, and silent about the ones that have not.
|
about the failure modes that have actually been hit, and silent about the ones that have not.
|
||||||
@@ -18,19 +18,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume
|
|||||||
|
|
||||||
## Medium
|
## Medium
|
||||||
|
|
||||||
### B-39 — SQLite with no WAL, no `busy_timeout`, and five concurrent writer tasks
|
|
||||||
|
|
||||||
`db/base.py:6` calls `create_async_engine(settings.database_url)` with no `connect_args`, and
|
|
||||||
there is no `PRAGMA` anywhere in the repo (verified by grep). Without `journal_mode=WAL`
|
|
||||||
readers block writers, and the concurrent writers are five background tasks plus every HTTP
|
|
||||||
handler. `database is locked` under load is realistic, and nothing handles it.
|
|
||||||
|
|
||||||
**Proposed fix.** Set `journal_mode=WAL`, `synchronous=NORMAL` and a `busy_timeout` of a few
|
|
||||||
seconds on connect (a `connect` event listener on the engine, applied only for the SQLite
|
|
||||||
dialect), and retry `OperationalError: database is locked` in the background loops. Longer
|
|
||||||
term this is an argument for PostgreSQL, which the single-process constraints in CLAUDE.md
|
|
||||||
also point at.
|
|
||||||
|
|
||||||
### B-40 — `bump_fee` holds a DB session open across N network calls
|
### B-40 — `bump_fee` holds a DB session open across N network calls
|
||||||
|
|
||||||
`tx/broadcast.py:80` issues one `get_transaction` **per input** (up to 15s each) and then a
|
`tx/broadcast.py:80` issues one `get_transaction` **per input** (up to 15s each) and then a
|
||||||
@@ -145,9 +132,10 @@ already does.
|
|||||||
- **B-36** — a stalled draw wait had no timeout, no log, and no audit trail, so a frozen round showed nothing in `/admin`
|
- **B-36** — a stalled draw wait had no timeout, no log, and no audit trail, so a frozen round showed nothing in `/admin`
|
||||||
- **B-37** — a withdrawal covered by unconfirmed change answered "insufficient balance" instead of distinguishing it from actually having no funds
|
- **B-37** — a withdrawal covered by unconfirmed change answered "insufficient balance" instead of distinguishing it from actually having no funds
|
||||||
- **B-38** — the SSE subscriber cap was global, so one client opening enough connections degraded every other user to polling
|
- **B-38** — the SSE subscriber cap was global, so one client opening enough connections degraded every other user to polling
|
||||||
|
- **B-39** — SQLite ran without WAL or a `busy_timeout`, so a writer could block every reader and a second writer failed immediately instead of waiting
|
||||||
|
|
||||||
See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the
|
See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the
|
||||||
B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38 fixes). Suite grew from 139 to 211 tests over the fourteen.
|
B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35/B-36/B-37/B-38/B-39 fixes). Suite grew from 139 to 214 tests over the fifteen.
|
||||||
|
|
||||||
A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python
|
A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python
|
||||||
module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
|
module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
|
||||||
|
|||||||
+36
-1
@@ -1,9 +1,44 @@
|
|||||||
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine
|
from sqlalchemy import event
|
||||||
|
from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine
|
||||||
from sqlalchemy.orm import DeclarativeBase
|
from sqlalchemy.orm import DeclarativeBase
|
||||||
|
|
||||||
from app.config import settings
|
from app.config import settings
|
||||||
|
|
||||||
|
# How long a writer waits for a lock held by another writer before SQLite raises
|
||||||
|
# "database is locked" (B-39). A few seconds is enough to ride out this app's own
|
||||||
|
# five concurrent background tasks (scheduler, confirmation poller, RBF bumper,
|
||||||
|
# two reconcilers) plus HTTP handlers briefly overlapping a write.
|
||||||
|
_SQLITE_BUSY_TIMEOUT_MS = 5000
|
||||||
|
|
||||||
|
|
||||||
|
def _register_sqlite_pragmas(target_engine: AsyncEngine) -> None:
|
||||||
|
"""Without WAL, SQLite's default (rollback-journal) mode lets a writer block
|
||||||
|
every reader for the duration of its transaction, and a second writer arriving
|
||||||
|
while one is already active fails immediately rather than waiting at all —
|
||||||
|
realistic under this app's concurrency, and nothing previously handled it.
|
||||||
|
WAL lets readers and writers proceed without blocking each other, and
|
||||||
|
busy_timeout gives a second writer a real window to wait for the first
|
||||||
|
instead of an instant `OperationalError`.
|
||||||
|
|
||||||
|
No-op for any dialect other than sqlite (e.g. a future PostgreSQL
|
||||||
|
DATABASE_URL), which neither needs nor understands these pragmas.
|
||||||
|
"""
|
||||||
|
if target_engine.dialect.name != "sqlite":
|
||||||
|
return
|
||||||
|
|
||||||
|
@event.listens_for(target_engine.sync_engine, "connect")
|
||||||
|
def _set_sqlite_pragmas(dbapi_connection, connection_record) -> None:
|
||||||
|
cursor = dbapi_connection.cursor()
|
||||||
|
try:
|
||||||
|
cursor.execute("PRAGMA journal_mode=WAL")
|
||||||
|
cursor.execute("PRAGMA synchronous=NORMAL")
|
||||||
|
cursor.execute(f"PRAGMA busy_timeout={_SQLITE_BUSY_TIMEOUT_MS}")
|
||||||
|
finally:
|
||||||
|
cursor.close()
|
||||||
|
|
||||||
|
|
||||||
engine = create_async_engine(settings.database_url)
|
engine = create_async_engine(settings.database_url)
|
||||||
|
_register_sqlite_pragmas(engine)
|
||||||
AsyncSessionLocal = async_sessionmaker(engine, expire_on_commit=False)
|
AsyncSessionLocal = async_sessionmaker(engine, expire_on_commit=False)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,61 @@
|
|||||||
|
"""Regression tests for B-39: SQLite must run in WAL mode with a busy_timeout,
|
||||||
|
since this app has five concurrent background tasks plus every HTTP handler
|
||||||
|
sharing one database file, and the default rollback-journal mode lets a writer
|
||||||
|
block every reader and fails a second writer immediately instead of waiting."""
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
from sqlalchemy.ext.asyncio import create_async_engine
|
||||||
|
|
||||||
|
from app.db.base import _SQLITE_BUSY_TIMEOUT_MS, _register_sqlite_pragmas
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
async def sqlite_engine(tmp_path):
|
||||||
|
# WAL needs a real file (it writes a companion -wal/-shm file alongside it) —
|
||||||
|
# ":memory:" wouldn't exercise the same path.
|
||||||
|
engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path}/test.db")
|
||||||
|
yield engine
|
||||||
|
await engine.dispose()
|
||||||
|
|
||||||
|
|
||||||
|
async def _pragma(engine, name: str):
|
||||||
|
async with engine.connect() as conn:
|
||||||
|
result = await conn.exec_driver_sql(f"PRAGMA {name}")
|
||||||
|
return result.fetchone()[0]
|
||||||
|
|
||||||
|
|
||||||
|
async def test_register_sqlite_pragmas_enables_wal_and_busy_timeout(sqlite_engine):
|
||||||
|
_register_sqlite_pragmas(sqlite_engine)
|
||||||
|
|
||||||
|
assert (await _pragma(sqlite_engine, "journal_mode")).lower() == "wal"
|
||||||
|
assert await _pragma(sqlite_engine, "busy_timeout") == _SQLITE_BUSY_TIMEOUT_MS
|
||||||
|
assert await _pragma(sqlite_engine, "synchronous") == 1 # NORMAL
|
||||||
|
|
||||||
|
|
||||||
|
async def test_register_sqlite_pragmas_applies_to_every_new_connection(sqlite_engine):
|
||||||
|
"""The pool can open more than one underlying DBAPI connection over the
|
||||||
|
engine's lifetime — the pragmas must be re-applied to each one, not just
|
||||||
|
the first, or a later connection would silently fall back to SQLite's
|
||||||
|
defaults."""
|
||||||
|
_register_sqlite_pragmas(sqlite_engine)
|
||||||
|
|
||||||
|
async with sqlite_engine.connect() as first:
|
||||||
|
await first.exec_driver_sql("PRAGMA journal_mode")
|
||||||
|
|
||||||
|
async with sqlite_engine.connect() as second:
|
||||||
|
result = await second.exec_driver_sql("PRAGMA busy_timeout")
|
||||||
|
assert result.fetchone()[0] == _SQLITE_BUSY_TIMEOUT_MS
|
||||||
|
|
||||||
|
|
||||||
|
def test_register_sqlite_pragmas_is_a_noop_for_other_dialects():
|
||||||
|
"""Must not touch (or crash on) a non-sqlite engine — e.g. a future
|
||||||
|
PostgreSQL DATABASE_URL, which neither needs nor understands these
|
||||||
|
pragmas."""
|
||||||
|
|
||||||
|
class _FakeDialect:
|
||||||
|
name = "postgresql"
|
||||||
|
|
||||||
|
class _FakeEngine:
|
||||||
|
dialect = _FakeDialect()
|
||||||
|
|
||||||
|
_register_sqlite_pragmas(_FakeEngine()) # must not raise
|
||||||
Reference in New Issue
Block a user