From 223bb376713c8098845a9d3d86d51e5442444536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A0=D0=BE=D0=B1=D0=BE=D1=82?= Date: Sat, 26 Sep 2026 15:48:12 +0200 Subject: [PATCH 1/2] test(integration): background work a test starts is cancelled before the next (B-21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On SQLite every session shares one physical connection (StaticPool), and work started with spawn_tracked or an in-process task_queue fallback is never awaited by the request that started it. A leftover task that rolls back on that connection can undo the next test's flushed-but-uncommitted insert; that test then commits nothing, finds the object in its identity map, and its UPDATE matches zero rows — the StaleDataError test_learnings_api hit on 2026-09-23 and 2026-09-26, passing alone and on rerun. db_session's teardown now cancels and awaits all such tasks before the rollback and the truncate. test_background_work_does_not_outlive_its_test leaves a task running in one test and asserts the next does not inherit it — red without the drain. Integration suite 702/702 on SQLite. Co-Authored-By: Claude Opus 5.5 (1M context) --- backend/tests/integration/conftest.py | 32 +++++++++++++++++ ...ckground_work_does_not_outlive_its_test.py | 35 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 backend/tests/integration/test_background_work_does_not_outlive_its_test.py diff --git a/backend/tests/integration/conftest.py b/backend/tests/integration/conftest.py index 8f3c24f1..5ec2e90c 100644 --- a/backend/tests/integration/conftest.py +++ b/backend/tests/integration/conftest.py @@ -173,11 +173,43 @@ async def _truncate_all(engine) -> None: await conn.execute(text(f"TRUNCATE {quoted} RESTART IDENTITY CASCADE")) +async def _drain_background_work(timeout: float = 5.0) -> int: + """Cancel and await whatever a test left running in the background (B-21). + + Route handlers start work with `spawn_tracked`, and in-process `task_queue` + fallbacks with `asyncio.create_task`. Neither is awaited by the request, so without + this they outlive the test that started them — and on SQLite every session shares + ONE physical connection (`StaticPool`, above). A leftover task that rolls back on + that connection can undo the NEXT test's flushed-but-uncommitted insert; that test + then commits nothing, finds the object in its identity map without a query, and its + `UPDATE` matches zero rows. `test_learnings_api` failed exactly that way + (`StaleDataError: expected to update 1 row(s); 0 were matched`) on 2026-09-23 and + 2026-09-26, passing alone and on rerun. Returns how many tasks were cancelled. + """ + import asyncio + + from app.core import task_queue + from app.core.background import _BACKGROUND_TASKS + + pending = [ + t for t in (*_BACKGROUND_TASKS, *task_queue._fallback_tasks.values()) if not t.done() + ] + for task in pending: + task.cancel() + if pending: + await asyncio.wait(pending, timeout=timeout) + task_queue._fallback_tasks.clear() + return len(pending) + + @pytest_asyncio.fixture() async def db_session(engine) -> AsyncGenerator[AsyncSession, None]: factory = async_sessionmaker(engine, class_=AsyncSession, expire_on_commit=False) async with factory() as session: yield session + # Before the rollback and the truncate: a task still running would otherwise + # execute against the next test's rows on the shared connection (B-21). + await _drain_background_work() await session.rollback() await _truncate_all(engine) diff --git a/backend/tests/integration/test_background_work_does_not_outlive_its_test.py b/backend/tests/integration/test_background_work_does_not_outlive_its_test.py new file mode 100644 index 00000000..8b5b4e7f --- /dev/null +++ b/backend/tests/integration/test_background_work_does_not_outlive_its_test.py @@ -0,0 +1,35 @@ +"""B-21: background work a test starts is gone before the next test begins. + +The two tests run in file order. The first deliberately leaves a task running; the +second asserts the `db_session` teardown cancelled it. Without the drain in +`tests/integration/conftest.py` the second fails. +""" + +from __future__ import annotations + +import asyncio + +import pytest + +from app.core.background import _BACKGROUND_TASKS, spawn_tracked + +_LEFT_BEHIND: list[asyncio.Task] = [] + + +@pytest.mark.asyncio +async def test_a_test_that_leaves_work_running(db_session): + started = asyncio.Event() + + async def lingering(): + started.set() + await asyncio.sleep(3600) + + _LEFT_BEHIND.append(spawn_tracked(lingering(), name="b21-lingering")) + await started.wait() + + +@pytest.mark.asyncio +async def test_the_next_test_does_not_inherit_it(db_session): + assert _LEFT_BEHIND, "the first test did not run first" + assert _LEFT_BEHIND[0].done(), "a task from the previous test is still running" + assert not [t for t in _BACKGROUND_TASKS if not t.done()] From 7276e285e382c797608b68a2cc2a7314444f698d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=A0=D0=BE=D0=B1=D0=BE=D1=82?= Date: Sat, 26 Sep 2026 17:48:11 +0200 Subject: [PATCH 2/2] fix(test-harness): wait for a test's background work, never cancel it (B-21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first drain cancelled leftover tasks, and a task cancelled mid-statement makes SQLAlchemy invalidate the connection — under StaticPool the replacement is a new, EMPTY :memory: database, so every later test failed with "no such table: users" (263 failed, 965 errors on CI). Now the teardown waits up to 30 s and leaves anything still running alone, naming it in a warning. `_assert_schema_survived` fails at the cause if the shared database ever loses its tables again, instead of a cascade. The guard test uses a task that finishes on its own and asserts the next test sees it finished and not cancelled (red without the drain). Co-Authored-By: Claude Opus 5.5 (1M context) --- backend/tests/integration/conftest.py | 63 ++++++++++++++----- ...ckground_work_does_not_outlive_its_test.py | 30 ++++----- 2 files changed, 63 insertions(+), 30 deletions(-) diff --git a/backend/tests/integration/conftest.py b/backend/tests/integration/conftest.py index 5ec2e90c..0d65fe22 100644 --- a/backend/tests/integration/conftest.py +++ b/backend/tests/integration/conftest.py @@ -173,20 +173,28 @@ async def _truncate_all(engine) -> None: await conn.execute(text(f"TRUNCATE {quoted} RESTART IDENTITY CASCADE")) -async def _drain_background_work(timeout: float = 5.0) -> int: - """Cancel and await whatever a test left running in the background (B-21). +async def _drain_background_work(timeout: float = 30.0) -> int: + """Wait for whatever a test left running in the background (B-21). Never cancels. Route handlers start work with `spawn_tracked`, and in-process `task_queue` - fallbacks with `asyncio.create_task`. Neither is awaited by the request, so without - this they outlive the test that started them — and on SQLite every session shares - ONE physical connection (`StaticPool`, above). A leftover task that rolls back on - that connection can undo the NEXT test's flushed-but-uncommitted insert; that test - then commits nothing, finds the object in its identity map without a query, and its - `UPDATE` matches zero rows. `test_learnings_api` failed exactly that way - (`StaleDataError: expected to update 1 row(s); 0 were matched`) on 2026-09-23 and - 2026-09-26, passing alone and on rerun. Returns how many tasks were cancelled. + fallbacks with `asyncio.create_task`. Neither is awaited by the request, so they + outlived the test that started them — and on SQLite every session shares ONE + physical connection (`StaticPool`, above). A leftover task that rolls back on that + connection can undo the NEXT test's flushed-but-uncommitted insert; that test then + commits nothing, finds the object in its identity map without a query, and its + `UPDATE` matches zero rows — the `StaleDataError` `test_learnings_api` hit on + 2026-09-23 and 2026-09-26, passing alone and on rerun. + + **Waiting, not cancelling, and that was learned the hard way.** The first version + cancelled: a task cancelled in the middle of a statement makes SQLAlchemy invalidate + the connection, and under `StaticPool` the replacement is a NEW, EMPTY `:memory:` + database, so every later test failed with "no such table: users" (#439's first CI + run: 263 failed, 965 errors). Waiting lets each task finish its statement and its + transaction. A task still running after `timeout` is left alone and named in a + warning rather than cut mid-query. Returns how many were still running. """ import asyncio + import logging from app.core import task_queue from app.core.background import _BACKGROUND_TASKS @@ -194,12 +202,34 @@ async def _drain_background_work(timeout: float = 5.0) -> int: pending = [ t for t in (*_BACKGROUND_TASKS, *task_queue._fallback_tasks.values()) if not t.done() ] - for task in pending: - task.cancel() - if pending: - await asyncio.wait(pending, timeout=timeout) - task_queue._fallback_tasks.clear() - return len(pending) + if not pending: + return 0 + _done, still = await asyncio.wait(pending, timeout=timeout) + if still: + logging.getLogger(__name__).warning( + "%d background task(s) still running after %.0fs, left alone: %s", + len(still), + timeout, + sorted(t.get_name() for t in still), + ) + return len(still) + + +async def _assert_schema_survived(engine) -> None: + """Fail at the cause, not a thousand tests later: the shared SQLite DB kept its tables.""" + if not _ON_SQLITE: + return + from sqlalchemy import inspect as _inspect + + async with engine.connect() as conn: + present = await conn.run_sync(lambda c: set(_inspect(c).get_table_names())) + missing = _CREATED_TABLES - present + if missing: + raise RuntimeError( + "the shared in-memory database lost its schema during this test's teardown " + f"({len(missing)} tables missing, e.g. {sorted(missing)[:3]}) — the StaticPool " + "connection was replaced, usually by a statement cancelled mid-flight" + ) @pytest_asyncio.fixture() @@ -211,6 +241,7 @@ async def db_session(engine) -> AsyncGenerator[AsyncSession, None]: # execute against the next test's rows on the shared connection (B-21). await _drain_background_work() await session.rollback() + await _assert_schema_survived(engine) await _truncate_all(engine) diff --git a/backend/tests/integration/test_background_work_does_not_outlive_its_test.py b/backend/tests/integration/test_background_work_does_not_outlive_its_test.py index 8b5b4e7f..5d8bd476 100644 --- a/backend/tests/integration/test_background_work_does_not_outlive_its_test.py +++ b/backend/tests/integration/test_background_work_does_not_outlive_its_test.py @@ -1,8 +1,10 @@ -"""B-21: background work a test starts is gone before the next test begins. +"""B-21: background work a test starts has finished before the next test begins. -The two tests run in file order. The first deliberately leaves a task running; the -second asserts the `db_session` teardown cancelled it. Without the drain in -`tests/integration/conftest.py` the second fails. +The two tests run in file order. The first starts a task that takes a moment and does +not await it; the second asserts the `db_session` teardown waited for it — and did not +cancel it, because cancelling mid-statement destroys the shared SQLite database +(see `_drain_background_work`). Without the drain the task is still sleeping when the +second test starts. """ from __future__ import annotations @@ -13,23 +15,23 @@ from app.core.background import _BACKGROUND_TASKS, spawn_tracked -_LEFT_BEHIND: list[asyncio.Task] = [] +_STARTED: list[asyncio.Task] = [] +_FINISHED: list[bool] = [] @pytest.mark.asyncio async def test_a_test_that_leaves_work_running(db_session): - started = asyncio.Event() + async def a_moment_of_work(): + await asyncio.sleep(0.5) + _FINISHED.append(True) - async def lingering(): - started.set() - await asyncio.sleep(3600) - - _LEFT_BEHIND.append(spawn_tracked(lingering(), name="b21-lingering")) - await started.wait() + _STARTED.append(spawn_tracked(a_moment_of_work(), name="b21-a-moment")) + assert not _FINISHED @pytest.mark.asyncio async def test_the_next_test_does_not_inherit_it(db_session): - assert _LEFT_BEHIND, "the first test did not run first" - assert _LEFT_BEHIND[0].done(), "a task from the previous test is still running" + assert _STARTED, "the first test did not run first" + assert _FINISHED == [True], "the previous test's work was still running" + assert not _STARTED[0].cancelled(), "the drain waits; it must not cancel" assert not [t for t in _BACKGROUND_TASKS if not t.done()]