Skip to content

Commit a962f2c

Browse files
phernandezclaude
andcommitted
fix(core): shield remaining asyncpg dispose seams + observe nest_asyncio skip
Address claude-review feedback and the Windows SQLite Unit CI failure on PR #902. - db.py engine_session_factory finally: shield created_engine.dispose() with suppress(CancelledError) to match the other dispose seams (review item 2). - db.py run_migrations finally: shield temp_engine.dispose() the same way; it is on the same asyncpg startup path as the crash (review item 1). - alembic/env.py: log the nest_asyncio ImportError/ValueError at DEBUG instead of silently passing, so the now-expected uvloop ValueError on every Postgres startup is observable without being noisy (review item 3). - tests/cli/test_cli_exit.py: loosen test_bm_version_exits_cleanly subprocess timeout 10s -> 20s to match its sibling --help test. The new uvloop code never runs on the 'bm --version' path (eager version_callback exits before app_callback's body, and db.py is not even imported), so the Windows-only failure is uv-run startup contention under full-suite load, not a regression. Verified: ruff, ty, the affected unit tests, and the Postgres dispose-cycle integration test (testcontainers) all pass locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent fe0001a commit a962f2c

3 files changed

Lines changed: 31 additions & 8 deletions

File tree

src/basic_memory/alembic/env.py

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
from contextlib import suppress
66
from logging.config import fileConfig
77

8+
from loguru import logger
9+
810
# Allow nested event loops (needed for pytest-asyncio and other async contexts)
911
# Note: nest_asyncio doesn't work with uvloop or Python 3.14+, so we handle those cases separately
1012
import sys
@@ -14,9 +16,15 @@
1416
import nest_asyncio
1517

1618
nest_asyncio.apply()
17-
except (ImportError, ValueError):
18-
# nest_asyncio not available or can't patch this loop type (e.g., uvloop)
19-
pass
19+
except (ImportError, ValueError) as exc:
20+
# Trigger: nest_asyncio is absent (ImportError) or refuses to patch the
21+
# running loop (ValueError, e.g. the uvloop policy installed for the
22+
# Postgres backend - #831/#877).
23+
# Why: the uvloop ValueError is now an *expected* path on every Postgres
24+
# startup, so swallowing it silently hides a routine branch.
25+
# Outcome: log at DEBUG (observable, not noisy) and fall through to the
26+
# thread-based migration fallback.
27+
logger.debug(f"nest_asyncio not applied ({exc!r}); using thread-based migration fallback")
2028
# For Python 3.14+, we rely on the thread-based fallback in run_migrations_online()
2129

2230
from sqlalchemy import engine_from_config, pool

src/basic_memory/db.py

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,14 @@ async def engine_session_factory(
410410

411411
yield created_engine, created_session_maker
412412
finally:
413-
await created_engine.dispose()
413+
# Trigger: context-manager teardown can run while the surrounding task is
414+
# being cancelled (e.g. a test aborting mid-fixture).
415+
# Why: on the asyncpg backend a cancellation landing mid-dispose surfaces
416+
# the "IndexError: pop from an empty deque" race (#831/#877); shield the
417+
# dispose and suppress CancelledError to match the other dispose seams.
418+
# Outcome: the per-context engine always disposes cleanly under cancellation.
419+
with suppress(asyncio.CancelledError):
420+
await asyncio.shield(created_engine.dispose())
414421

415422
# Only clear module-level globals if they still point to this context's
416423
# engine/session. This avoids clobbering newer globals from other callers.
@@ -479,7 +486,11 @@ async def run_migrations(
479486
# Trigger: run_migrations() created a temporary engine while module-level
480487
# session maker was not initialized.
481488
# Why: temporary aiosqlite worker threads can outlive CLI command execution
482-
# and block process shutdown if the engine is not disposed.
483-
# Outcome: always dispose temporary engines after migration work completes.
489+
# and block process shutdown if the engine is not disposed. On the asyncpg
490+
# backend a cancellation landing mid-dispose surfaces the same "IndexError:
491+
# pop from an empty deque" race as the other dispose seams (#831/#877), so
492+
# shield the dispose and suppress CancelledError to match them.
493+
# Outcome: always dispose temporary engines cleanly, even under cancellation.
484494
if temp_engine is not None:
485-
await temp_engine.dispose()
495+
with suppress(asyncio.CancelledError):
496+
await asyncio.shield(temp_engine.dispose())

tests/cli/test_cli_exit.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,11 @@ def test_bm_version_exits_cleanly():
1515
["uv", "run", "bm", "--version"],
1616
capture_output=True,
1717
text=True,
18-
timeout=10,
18+
# `uv run` startup under full-suite load (especially on Windows runners)
19+
# can exceed a tight 10s budget even though --version short-circuits
20+
# before any heavy work. This test guards against hangs, not a startup
21+
# performance budget, so match the looser --help timeout below.
22+
timeout=20,
1923
cwd=Path(__file__).parent.parent.parent, # Project root
2024
)
2125
assert result.returncode == 0

0 commit comments

Comments
 (0)