Skip to content

fix(db): StaticPool for in-memory SQLite + module-cache reset between tests#223

Merged
gizmax merged 1 commit into
mainfrom
fix/test-isolation-staticpool
May 16, 2026
Merged

fix(db): StaticPool for in-memory SQLite + module-cache reset between tests#223
gizmax merged 1 commit into
mainfrom
fix/test-isolation-staticpool

Conversation

@gizmax
Copy link
Copy Markdown
Owner

@gizmax gizmax commented May 16, 2026

Summary

Architectural prerequisite for issue #218 (test isolation). Forces `poolclass=StaticPool` for in-memory SQLite URLs in the engine factory + adds a function-scoped autouse fixture that clears module-global dicts in routes.py and the rate-limiter window store between tests.

Why

The test suite uses `sqlite+aiosqlite://` (no path) for an in-memory database. Without StaticPool, every `async_session()` call may open a fresh aiosqlite connection, and each fresh in-memory connection sees its OWN empty database. A row written by session A is invisible to session B, which surfaces in CI as intermittent 'no such table: runs' / 'no rows returned' failures - the symptom diagnosed at length in `.autoresearch/test-isolation-results.tsv`.

This also explains why previous cleanup-fixture attempts in conftest.py made things strictly worse (65 failures -> 400+ errors): the fixture's row-wipe ran on a different in-memory DB than the test code subsequently read.

What changes

src/sandcastle/models/db.py

  • New `_is_in_memory_sqlite(url)` helper recognising 3 URL shapes
  • `_build_engine_kwargs()` adds `poolclass=StaticPool` when URL is in-memory
  • File-backed SQLite and PostgreSQL paths unchanged

tests/conftest.py

  • Function-scoped autouse fixture clears `routes._hub_cache`, `_batch_store`, `_stats_cache`, rate-limiter window store before every test
  • Shared `_run_async` helper for session-level fixtures

What this does NOT do

Does not close #218 by itself. Local pytest delta with this change:

  • Passed: 15,009 (unchanged)
  • Failed: 70 (unchanged)

Zero new failures, but the 65-test pollution baseline remains. This is the architectural prerequisite.

Follow-ups in #218

  1. Per-test SAVEPOINT isolation via session.begin_nested()
  2. Move module-global caches behind FastAPI Depends()
  3. Move execution_limiter similarly

After those three land, the baseline collapses to zero.

…tween 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.
@gizmax
Copy link
Copy Markdown
Owner Author

gizmax commented May 16, 2026

CI confirmation

Job Result
Dashboard build + tests ✓ PASS
Python tests matches main baseline

Python: 15,009 passed / 70 failed / 1 error - identical to main. Zero regression, zero improvement, as predicted in the PR description.

The architectural change is correct (StaticPool is the right pool class for in-memory SQLite) but it does not reduce the 65-test pollution baseline on its own. The remaining work in #218 needs to land alongside this for the count to drop.

Merge options

  1. Merge now - foundation work, clearly correct, makes the per-test SAVEPOINT refactor in Test isolation: 65 tests fail in full suite, pass in isolation #218 actually possible.
  2. Hold - bundle with the SAVEPOINT work into one PR so the failure-count delta is visible at merge time.

Recommend option 1 - the file diff is tiny (97 lines), the change is obviously right, and follow-on PRs land cleaner once this foundation is in.

@gizmax gizmax merged commit 815fa5a into main May 16, 2026
1 of 2 checks passed
@gizmax gizmax deleted the fix/test-isolation-staticpool branch May 16, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test isolation: 65 tests fail in full suite, pass in isolation

1 participant