fix(tests): per-request session via QueuePool + WAL (closes #105)

Mark commented

2026-09-11 16:16:50 +00:00

Owner

Re-submission of #121 (which had a non-mergeable state because the base was behind origin/main). Branch is now based on abdf87d (origin/main); diff is identical to #121 minus the obsolete base.

What

Replace the shared-session override in tests/conftest.py with a per-request session strategy backed by QueuePool + WAL SQLite. Concurrent FastAPI requests now get independent SQLAlchemy Sessions with their own DBAPI connections — the actual shape needed under concurrency (SQLAlchemy Session and DBAPI connections are not thread-safe to share).

Why

tests/conftest.py:84-92 yielded a single Session to every concurrent FastAPI request. Sessions are not thread-safe, so concurrent requests corrupted each other's identity map and transaction state, producing non-deterministic PendingRollbackError (500) and spurious 403 Insufficient case access responses.

The issue body says 13 of 14 concurrency tests fail on pristine main. Since the original report, #96/#101 landed, so the current baseline is 12 of 14. This PR makes all 14 pass.

Changes

backend/tests/fixtures/database.py
- Engine switched from StaticPool + in-memory to QueuePool + file-backed SQLite with journal_mode=WAL. WAL allows concurrent readers and writers across multiple connections without database table is locked errors.
- Per-test isolation now via an autouse clean_db fixture that truncates all tables before each test (FK enforcement is disabled during the truncate). The old "wrap each test in a transaction that gets rolled back at teardown" pattern doesn't compose with per-request connections.
- db_session rebinds to the engine (no longer to a per-test connection), so committed data is visible to per-request sessions through the shared SQLite database.
- New db_session_factory yields a fresh Session per FastAPI request, with an after_commit hook that removes corresponding ORM objects from db_session.identity_map after an API delete. The hook preserves the original shared-session behavior expected by tests like test_delete_case_*_succeeds (which assert db_session.get(...) is None after an API delete).
- Removed the now-unused db_connection fixture.
backend/tests/conftest.py
- override_get_session now consumes db_session_factory and yields a fresh per-request Session.
backend/tests/integration/test_integration_concurrent.py
- Added _grant_case_access helper that grants a user case-level membership via CaseAccessService.ensure_membership.
- Updated each concurrency test that has two users operate on the same case to grant the second user explicit editor/viewer membership — required after #68 (case-level access control).
- test_user_cannot_modify_other_users_case: flipped the assertion from 200 to 403 to match the test's stated intent (name says "cannot" but it asserted 200 — pre-#68 stale).

Verification

tests/integration/test_integration_concurrent.py: 14/14 pass (10/10 runs, stable).
tests/integration/test_api_cases.py: 47/47 pass (3/3 runs, stable).
Full integration suite: 252 pass, 37 fail (vs pristine main 239 pass, 50 fail — net +13). All 37 remaining failures are pre-existing (verified by diff against pristine main).
New failures introduced by this change: 0.
ruff check and ruff format --check: clean on changed files.

Risk

Low. The fixture refactor is internal to the test harness — no production code touched. Pre-existing failures in unrelated test modules (e.g. test_modules_twitter.py asserting requires_config == [] on a tuple, test_api_errors.py/test_integration_resilience.py module-flow assertions) are out of scope for #105 and remain failing.

Closes #105 (replaces #121).

Re-submission of #121 (which had a non-mergeable state because the base was behind origin/main). Branch is now based on abdf87d (origin/main); diff is identical to #121 minus the obsolete base. ## What Replace the shared-session override in `tests/conftest.py` with a per-request session strategy backed by QueuePool + WAL SQLite. Concurrent FastAPI requests now get independent SQLAlchemy Sessions with their own DBAPI connections — the actual shape needed under concurrency (SQLAlchemy Session and DBAPI connections are not thread-safe to share). ## Why `tests/conftest.py:84-92` yielded a single Session to every concurrent FastAPI request. Sessions are not thread-safe, so concurrent requests corrupted each other's identity map and transaction state, producing non-deterministic `PendingRollbackError` (500) and spurious 403 `Insufficient case access` responses. The issue body says 13 of 14 concurrency tests fail on pristine main. Since the original report, #96/#101 landed, so the current baseline is 12 of 14. This PR makes all 14 pass. ## Changes - `backend/tests/fixtures/database.py` - Engine switched from `StaticPool` + in-memory to `QueuePool` + file-backed SQLite with `journal_mode=WAL`. WAL allows concurrent readers and writers across multiple connections without `database table is locked` errors. - Per-test isolation now via an autouse `clean_db` fixture that truncates all tables before each test (FK enforcement is disabled during the truncate). The old "wrap each test in a transaction that gets rolled back at teardown" pattern doesn't compose with per-request connections. - `db_session` rebinds to the engine (no longer to a per-test connection), so committed data is visible to per-request sessions through the shared SQLite database. - New `db_session_factory` yields a fresh Session per FastAPI request, with an `after_commit` hook that removes corresponding ORM objects from `db_session.identity_map` after an API delete. The hook preserves the original shared-session behavior expected by tests like `test_delete_case_*_succeeds` (which assert `db_session.get(...) is None` after an API delete). - Removed the now-unused `db_connection` fixture. - `backend/tests/conftest.py` - `override_get_session` now consumes `db_session_factory` and yields a fresh per-request Session. - `backend/tests/integration/test_integration_concurrent.py` - Added `_grant_case_access` helper that grants a user case-level membership via `CaseAccessService.ensure_membership`. - Updated each concurrency test that has two users operate on the same case to grant the second user explicit `editor`/`viewer` membership — required after #68 (case-level access control). - `test_user_cannot_modify_other_users_case`: flipped the assertion from 200 to 403 to match the test's stated intent (name says "cannot" but it asserted 200 — pre-#68 stale). ## Verification - `tests/integration/test_integration_concurrent.py`: 14/14 pass (10/10 runs, stable). - `tests/integration/test_api_cases.py`: 47/47 pass (3/3 runs, stable). - Full integration suite: 252 pass, 37 fail (vs pristine main 239 pass, 50 fail — net +13). All 37 remaining failures are pre-existing (verified by diff against pristine main). - New failures introduced by this change: 0. - `ruff check` and `ruff format --check`: clean on changed files. ## Risk Low. The fixture refactor is internal to the test harness — no production code touched. Pre-existing failures in unrelated test modules (e.g. `test_modules_twitter.py` asserting `requires_config == []` on a tuple, `test_api_errors.py`/`test_integration_resilience.py` module-flow assertions) are out of scope for #105 and remain failing. Closes #105 (replaces #121).

Mark added 1 commit

2026-09-11 16:16:51 +00:00

fix(tests): per-request session via QueuePool + WAL (closes #105 )

CI / Backend Lint & Format (pull_request) Failing after 4s

Details

CI / Golden Fixture Validation (pull_request) Has been skipped

Details

CI / Backend Tests (pull_request) Has been skipped

Details

CI / Backend Security Tests (pull_request) Has been skipped

Details

Performance Regression Tests / Performance Regression Detection (pull_request) Failing after 6s

Details

Stress Tests / Stress & Pipeline Tests (pull_request) Failing after 6s

Details

Stress Tests / Full Data Pipeline Validation (pull_request) Has been skipped

Details

CI / Frontend Lint (pull_request) Failing after 51s

Details

CI / Frontend Tests (pull_request) Has been skipped

Details

CI / Dependency Security Scan (pull_request) Has been skipped

Details

Performance Regression Tests / Compare with Baseline (pull_request) Has been skipped

Details

CI / Frontend E2E Tests (pull_request) Has been skipped

Details

CI / All Checks Passed (pull_request) Failing after 48s

Details

cc3b564a16

The shared-session override in tests/conftest.py:84-92 yielded a single
SQLAlchemy Session to every concurrent FastAPI request. Sessions are not
thread-safe, so concurrent requests corrupted each other's identity map and
transaction state, producing non-deterministic PendingRollbackError (500)
and spurious 403 'Insufficient case access' responses.

14 of the 14 concurrency tests in tests/integration/test_integration_concurrent.py
were affected (the issue body says '13 of 14 fail on pristine main'; since
#96/#101 landed, 12/14 fail — the fix below addresses all of them).

Fix:
- Engine: switched from StaticPool+in-memory to QueuePool+file-backed SQLite
  with WAL journal mode. Each per-request session now gets its own connection
  from the pool, which is the safe shape under concurrency (SQLAlchemy
  Session and DBAPI connections are not thread-safe to share).
- Per-test isolation: the old 'wrap each test in a transaction that gets
  rolled back at teardown' pattern doesn't compose with per-request
  connections, so it is replaced by an autouse clean_db fixture that
  truncates all tables before each test (FK enforcement is disabled
  during the truncate).
- db_session: rebinds to the engine (no longer to a per-test connection),
  so committed data is visible to per-request sessions through the shared
  SQLite database.
- db_session_factory: yields a fresh Session per FastAPI request, with an
  after_commit hook that removes corresponding ORM objects from
  db_session.identity_map after an API delete. The hook preserves the
  original shared-session behavior expected by tests like
  test_delete_case_*_succeeds (which assert db_session.get(...) is None
  after an API delete).

Test updates (tests/integration/test_integration_concurrent.py): several
concurrency tests had assertions written under the pre-#68 access model
where any analyst could edit any case. They now grant explicit
CaseMembership to collaborators via _grant_case_access, matching the
case-level access control (#68). test_user_cannot_modify_other_users_case
was renamed-stale (name says 'cannot' but asserted 200); assertion flipped
to 403 to match the test's stated intent.

Verification:
- test_integration_concurrent.py: 14/14 pass (10/10 runs, stable)
- test_api_cases.py: 47/47 pass (3/3 runs, stable)
- Full integration suite: 252 pass, 37 fail (vs pristine main 239 pass, 50
  fail — net +13). All 37 remaining failures are pre-existing (verified
  by diff against pristine main).
- New failures introduced by this change: 0.