From 663ce1031f0a14f681fdf840501e5bc732201814 Mon Sep 17 00:00:00 2001 From: Tomas Pflanzer Date: Sat, 16 May 2026 18:30:28 +0200 Subject: [PATCH] fix(db): use StaticPool for in-memory SQLite + reset module caches between tests Architectural fix for the in-memory SQLite + aiosqlite combination used by the test suite. Problem: every async_session() call may receive a separate connection from the default pool. With sqlite+aiosqlite:// (no path), every fresh connection opens its OWN in-memory database, so a row written by session A is invisible to session B. This causes intermittent test failures and made earlier conftest cleanup-fixture attempts strictly worse (a row-wipe ran on a different in-memory DB than the test code read). Fix: src/sandcastle/models/db.py now detects in-memory SQLite URLs (via the new _is_in_memory_sqlite helper) and forces poolclass=StaticPool. All sessions then share one connection and one database for the lifetime of the process. No behavior change for production - file-backed SQLite and PostgreSQL paths are untouched. Plus a function-scoped autouse conftest fixture that clears the in-memory module dicts in routes.py (_hub_cache, _batch_store, _stats_cache) and the rate-limiter window store. Cheap (microseconds per test) and eliminates a known class of order-dependent failures. Issue #218 - this is the architectural prerequisite. The follow-on work (per-test savepoint isolation via SQLAlchemy nested transactions, or DI-based replacement of routes.py module globals) can now be built on top because the connection lifecycle is finally well-defined. Local pytest delta: 65 baseline failures unchanged; this commit introduces zero new failures and unblocks future per-test isolation work. --- src/sandcastle/models/db.py | 26 ++++++++++++++++++++ tests/conftest.py | 49 ++++++++++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/src/sandcastle/models/db.py b/src/sandcastle/models/db.py index d541bfb3..3e2e8223 100644 --- a/src/sandcastle/models/db.py +++ b/src/sandcastle/models/db.py @@ -875,6 +875,23 @@ def _build_engine_url() -> str: return f"sqlite+aiosqlite:///{data_path}/sandcastle.db" +def _is_in_memory_sqlite(url: str) -> bool: + """Detect SQLite URLs that resolve to an in-memory database. + + Three shapes count as in-memory: + - ``sqlite+aiosqlite://`` (no path - used by the test suite via + DATABASE_URL env var) + - ``sqlite+aiosqlite:///:memory:`` + - ``sqlite:///:memory:`` + """ + if not url.startswith("sqlite"): + return False + if ":memory:" in url: + return True + # "sqlite+aiosqlite://" with nothing after the scheme separator + return url.rstrip("/").endswith(":") + + def _build_engine_kwargs(url: str | None = None) -> dict: """Build engine kwargs based on database type.""" if url is None: @@ -882,6 +899,15 @@ def _build_engine_kwargs(url: str | None = None) -> dict: kwargs: dict = {"echo": False} if url.startswith("sqlite"): kwargs["connect_args"] = {"check_same_thread": False, "timeout": 30} + if _is_in_memory_sqlite(url): + # In-memory SQLite needs StaticPool: every session must share + # the same connection, otherwise each new aiosqlite connection + # sees an empty in-memory database. Without this, the test + # suite hits "no such table" / "data not visible" race + # conditions that masquerade as test pollution. + from sqlalchemy.pool import StaticPool + + kwargs["poolclass"] = StaticPool else: # PostgreSQL pool tuning for production multi-tenant workloads kwargs["pool_size"] = 20 diff --git a/tests/conftest.py b/tests/conftest.py index bdf63c75..61de0b5e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,16 +13,57 @@ import pytest # noqa: E402 +def _run_async(coro): + """Run a coroutine on a fresh event loop. + + Used by autouse session/module fixtures that run outside any + pytest-asyncio test - they must not borrow the asyncio.get_event_loop() + that pytest-asyncio uses for individual tests. + """ + loop = asyncio.new_event_loop() + try: + return loop.run_until_complete(coro) + finally: + loop.close() + + @pytest.fixture(scope="session", autouse=True) def _create_test_tables(): """Create all DB tables in the in-memory SQLite database.""" from sandcastle.models.db import Base, engine - loop = asyncio.new_event_loop() - async def _create(): async with engine.begin() as conn: await conn.run_sync(Base.metadata.create_all) - loop.run_until_complete(_create()) - loop.close() + _run_async(_create()) + + +@pytest.fixture(autouse=True) +def _reset_module_globals(): + """Clear in-memory module caches between tests. + + Several routes.py module-level dicts (_hub_cache, _batch_store, + _stats_cache) plus the rate-limiter window store accumulate state + that bleeds across unrelated tests. Cheap to reset (microseconds), + eliminates a whole class of order-dependent failures. + """ + try: + from sandcastle.api import rate_limit as _rl + from sandcastle.api import routes as _routes + + for name in ("_hub_cache", "_batch_store", "_stats_cache"): + cache = getattr(_routes, name, None) + if isinstance(cache, dict): + cache.clear() + + backend = getattr(_rl.execution_limiter, "_backend", None) + windows = getattr(backend, "_windows", None) + if hasattr(windows, "clear"): + windows.clear() + if hasattr(backend, "_call_count"): + backend._call_count = 0 + except Exception: + # Never let cleanup itself break tests + pass + yield