diff --git a/BUGS.md b/BUGS.md index ee2101d..e738c01 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,11 +1,11 @@ # Known bugs 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 -fixed" below) — no Critical-severity finding remains open; the other 11 are Medium/Low. +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 10 are Medium/Low. 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 -from 139 to 211). +coverage — every fix lands with a regression test (the fifteen fixes so far brought the suite +from 139 to 214). 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. @@ -18,19 +18,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume ## 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 `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-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-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 -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 module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical, diff --git a/app/db/base.py b/app/db/base.py index cac5738..cbe56bc 100644 --- a/app/db/base.py +++ b/app/db/base.py @@ -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 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) +_register_sqlite_pragmas(engine) AsyncSessionLocal = async_sessionmaker(engine, expire_on_commit=False) diff --git a/tests/unit/test_db_base.py b/tests/unit/test_db_base.py new file mode 100644 index 0000000..3a172c8 --- /dev/null +++ b/tests/unit/test_db_base.py @@ -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