Skip to content

Commit fe0001a

Browse files
phernandezclaude
andcommitted
fix(core): prevent asyncpg engine-dispose crash on Postgres backend
On the Postgres backend (postgresql+asyncpg), the open-engine -> work -> engine.dispose() cycle could crash with 'IndexError: pop from an empty deque' raised from SQLAlchemy's async dispose machinery as it races stdlib asyncio loop teardown. This made Postgres + file watcher (sync_changes=true) unusable via a container restart loop; SQLite is unaffected. Structural fix: install the uvloop event-loop policy at the CLI and MCP server entrypoints, gated on database_backend == postgres, uvloop being importable, and a non-Windows platform. uvloop's C scheduler has no self._ready.popleft() codepath, so this class of crash cannot fire under it. SQLite users keep the default loop. Defense in depth: shield the engine.dispose() calls in db.shutdown_db and alembic/env.run_async_migrations and suppress asyncio.CancelledError so a cancelled teardown can't re-surface the race. PR #838 only fixed the local/SQLite ASGI preinit path and did not address the asyncpg dispose race. Closes #831 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent 1667cdc commit fe0001a

10 files changed

Lines changed: 249 additions & 4 deletions

File tree

pyproject.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ dependencies = [
5151
"litellm>=1.60.0,<2.0.0",
5252
"logfire>=4.19.0",
5353
"psutil>=5.9.0",
54+
# uvloop's C event loop has no self._ready.popleft() codepath, so the
55+
# asyncpg engine-dispose race ("IndexError: pop from an empty deque") that
56+
# crashes the Postgres backend cannot fire under it. Not available on Windows.
57+
"uvloop>=0.21.0; sys_platform != 'win32'",
5458
]
5559

5660
[project.urls]

src/basic_memory/alembic/env.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import asyncio
44
import os
5+
from contextlib import suppress
56
from logging.config import fileConfig
67

78
# Allow nested event loops (needed for pytest-asyncio and other async contexts)
@@ -115,7 +116,15 @@ async def run_async_migrations(connectable):
115116
"""Run migrations asynchronously with AsyncEngine."""
116117
async with connectable.connect() as connection:
117118
await connection.run_sync(do_run_migrations)
118-
await connectable.dispose()
119+
# Trigger: startup migrations on the asyncpg backend dispose the engine
120+
# while the event loop may be tearing down around them.
121+
# Why: that race surfaces "IndexError: pop from an empty deque" from
122+
# base_events._run_once (#831/#877); shielding lets dispose finish atomically
123+
# and suppressing CancelledError keeps a cancelled teardown from re-raising it.
124+
# Outcome: the migration engine always disposes cleanly. (uvloop is the
125+
# structural fix for the race; this hardens the teardown path.)
126+
with suppress(asyncio.CancelledError):
127+
await asyncio.shield(connectable.dispose())
119128

120129

121130
def _run_async_migrations_with_asyncio_run(connectable) -> None:

src/basic_memory/cli/app.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,14 @@ def app_callback(
5757
container = CliContainer.create()
5858
set_container(container)
5959

60+
# Trigger: Postgres backend resolved at CLI startup, before any asyncio.run().
61+
# Why: uvloop must own the event-loop policy before the loop is created so the
62+
# asyncpg engine-dispose race (#831/#877) cannot fire. No-op for SQLite.
63+
# Outcome: subsequent asyncio.run() calls in CLI commands use uvloop on Postgres.
64+
from basic_memory.db import maybe_install_uvloop
65+
66+
maybe_install_uvloop(container.config)
67+
6068
# Trigger: first-run init confirmation before command output.
6169
# Why: informational "initialized" message belongs above command results, not in the upsell panel.
6270
# Outcome: one-time plain line printed before the subcommand runs.

src/basic_memory/cli/commands/mcp.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,18 @@ def _run_background_auto_update() -> None:
9898
if transport == "stdio":
9999
threading.Thread(target=_run_background_auto_update, daemon=True).start()
100100

101+
# Trigger: MCP server startup on the Postgres backend, before the transport
102+
# creates its event loop.
103+
# Why: the watcher/lifespan path runs startup migrations + engine.dispose() on
104+
# asyncpg, which races stdlib asyncio teardown and crashes the container loop
105+
# (#831/#877). uvloop's C scheduler structurally avoids that race and must own
106+
# the loop policy before the loop is created. No-op for SQLite.
107+
# Outcome: `basic-memory mcp` on Postgres runs on uvloop. (The CLI callback also
108+
# installs it; this keeps the server startup seam explicit and self-contained.)
109+
from basic_memory.db import maybe_install_uvloop
110+
111+
maybe_install_uvloop(ConfigManager().config)
112+
101113
# Run the MCP server (blocks)
102114
# Lifespan handles: initialization, migrations, file sync, cleanup
103115
logger.info(f"Starting MCP server with {transport.upper()} transport")

src/basic_memory/db.py

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import asyncio
22
import os
33
import sys
4-
from contextlib import asynccontextmanager
4+
from contextlib import asynccontextmanager, suppress
55
from enum import Enum, auto
66
from pathlib import Path
77
from typing import AsyncGenerator, Optional
@@ -39,6 +39,44 @@
3939
if sys.platform == "win32": # pragma: no cover
4040
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
4141

42+
43+
def maybe_install_uvloop(config: BasicMemoryConfig) -> bool:
44+
"""Install the uvloop event-loop policy for the Postgres backend.
45+
46+
Trigger: process entrypoint starting with database_backend == postgres,
47+
uvloop importable, and a non-Windows platform.
48+
Why: asyncpg engine teardown (engine.dispose()) races the stdlib asyncio
49+
loop shutdown and surfaces "IndexError: pop from an empty deque" from
50+
base_events._run_once (see #831/#877). uvloop's C scheduler has no
51+
self._ready.popleft() codepath, so that class of crash cannot fire under it.
52+
Outcome: Postgres deployments run on uvloop; SQLite users keep the default
53+
loop (no behavior change, smaller blast radius). Must run before the event
54+
loop is created, i.e. before asyncio.run().
55+
56+
Returns:
57+
True if the uvloop policy was installed, False otherwise.
58+
"""
59+
# uvloop is not available on Windows; the default loop already differs there.
60+
if sys.platform == "win32": # pragma: no cover
61+
return False
62+
63+
# Limit the change to the backend that actually hits the asyncpg dispose race.
64+
if config.database_backend != DatabaseBackend.POSTGRES:
65+
return False
66+
67+
# Deferred import: uvloop is an optional, platform-gated dependency and the
68+
# default (SQLite) path must not require it to be installed.
69+
try:
70+
import uvloop
71+
except ImportError: # pragma: no cover
72+
logger.warning("uvloop not available - using default event loop for Postgres backend")
73+
return False
74+
75+
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())
76+
logger.info("Installed uvloop event-loop policy for Postgres backend")
77+
return True
78+
79+
4280
# Module level state
4381
_engine: Optional[AsyncEngine] = None
4482
_session_maker: Optional[async_sessionmaker[AsyncSession]] = None
@@ -320,7 +358,15 @@ async def shutdown_db() -> None: # pragma: no cover
320358
global _engine, _session_maker
321359

322360
if _engine:
323-
await _engine.dispose()
361+
# Trigger: teardown can run while the surrounding task is being cancelled
362+
# (e.g. lifespan shutdown, unshielded CLI cleanup).
363+
# Why: a cancellation landing mid-dispose surfaces the asyncpg
364+
# "IndexError: pop from an empty deque" race (#831/#877); shielding lets
365+
# dispose finish atomically, and suppressing CancelledError keeps a
366+
# cancelled shutdown from re-raising the underlying race.
367+
# Outcome: connections always close cleanly even under cancellation.
368+
with suppress(asyncio.CancelledError):
369+
await asyncio.shield(_engine.dispose())
324370
_engine = None
325371
_session_maker = None
326372

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
"""Integration test for the asyncpg engine-dispose race (issue #831 / #877).
2+
3+
On the Postgres backend (``postgresql+asyncpg``), the "open async engine -> work
4+
-> await engine.dispose()" cycle could crash with
5+
``IndexError: pop from an empty deque`` raised from SQLAlchemy's async dispose
6+
machinery as it races event-loop teardown. This made Postgres + file watcher
7+
unusable (container restart loop).
8+
9+
These tests exercise the real create-engine -> query -> dispose cycle against a
10+
live Postgres instance (testcontainers) for many iterations. The race is
11+
timing-dependent, so we loop enough to be meaningful and assert clean
12+
completion. They are skipped on SQLite, which is unaffected.
13+
14+
Run with:
15+
BASIC_MEMORY_TEST_POSTGRES=1 uv run pytest \
16+
test-int/test_postgres_dispose_cycle.py -q
17+
"""
18+
19+
import pytest
20+
from sqlalchemy import text
21+
22+
from basic_memory import db
23+
from basic_memory.config import DatabaseBackend
24+
from basic_memory.db import DatabaseType, _create_postgres_engine
25+
26+
27+
@pytest.mark.asyncio
28+
async def test_create_query_dispose_cycle_is_clean(app_config, db_backend):
29+
"""Repeatedly create -> query -> dispose an asyncpg engine without crashing."""
30+
if db_backend != "postgres":
31+
pytest.skip("Postgres-specific test - asyncpg dispose race does not affect SQLite")
32+
33+
db_url = DatabaseType.get_db_url(app_config.database_path, DatabaseType.POSTGRES, app_config)
34+
35+
# Loop enough iterations to make the timing-dependent dispose race observable.
36+
for _ in range(50):
37+
engine = _create_postgres_engine(db_url, app_config)
38+
async with engine.connect() as conn:
39+
result = await conn.execute(text("SELECT 1"))
40+
assert result.scalar() == 1
41+
# This dispose is the call site that raced loop teardown in #831/#877.
42+
await engine.dispose()
43+
44+
45+
@pytest.mark.asyncio
46+
async def test_shutdown_db_dispose_is_clean(app_config, config_manager, db_backend):
47+
"""Repeated get_or_create_db -> query -> shutdown_db cycles complete cleanly.
48+
49+
Exercises the shielded ``shutdown_db`` teardown path against asyncpg. The
50+
autouse ``cleanup_global_db_after_test`` fixture in conftest restores module
51+
state afterwards.
52+
"""
53+
if db_backend != "postgres":
54+
pytest.skip("Postgres-specific test - asyncpg dispose race does not affect SQLite")
55+
56+
assert app_config.database_backend == DatabaseBackend.POSTGRES
57+
58+
for _ in range(25):
59+
engine, session_maker = await db.get_or_create_db(
60+
app_config.database_path,
61+
db_type=DatabaseType.POSTGRES,
62+
ensure_migrations=False,
63+
config=app_config,
64+
)
65+
async with db.scoped_session(session_maker) as session:
66+
result = await session.execute(text("SELECT 1"))
67+
assert result.scalar() == 1
68+
# Shielded dispose - must not surface the asyncpg deque race.
69+
await db.shutdown_db()

tests/cli/test_cli_telemetry.py

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from __future__ import annotations
44

5+
from types import SimpleNamespace
56
from typing import Any, cast
67

78
import logfire
@@ -29,8 +30,16 @@ def test_app_callback_registers_command_operation(monkeypatch) -> None:
2930
resource = object()
3031

3132
monkeypatch.setattr(cli_app, "init_cli_logging", lambda: None)
32-
monkeypatch.setattr(cli_app.CliContainer, "create", staticmethod(lambda: object()))
33+
# Container needs a `config` attribute because app_callback passes it to the
34+
# uvloop policy installer; the helper itself is stubbed below for this
35+
# telemetry-focused test.
36+
monkeypatch.setattr(
37+
cli_app.CliContainer, "create", staticmethod(lambda: SimpleNamespace(config=object()))
38+
)
3339
monkeypatch.setattr(cli_app, "set_container", lambda container: None)
40+
# app_callback installs the uvloop policy for the Postgres backend; stub the
41+
# helper so this telemetry test does not depend on event-loop policy state.
42+
monkeypatch.setattr("basic_memory.db.maybe_install_uvloop", lambda config: False)
3443
monkeypatch.setattr(cli_app, "maybe_show_init_line", lambda command_name: None)
3544
monkeypatch.setattr(cli_app, "maybe_show_cloud_promo", lambda command_name: None)
3645
monkeypatch.setattr(cli_app, "maybe_run_periodic_auto_update", lambda command_name: None)

tests/cli/test_db_reindex.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from typer.testing import CliRunner
1010

1111
from basic_memory.cli.app import app
12+
from basic_memory.config import DatabaseBackend
1213
import basic_memory.cli.commands.db as db_cmd # noqa: F401
1314

1415

@@ -21,6 +22,8 @@ def _stub_app_config(*, semantic_search_enabled: bool = True) -> SimpleNamespace
2122
semantic_search_enabled=semantic_search_enabled,
2223
database_path=Path("/tmp/basic-memory.db"),
2324
get_project_mode=lambda project_name: None,
25+
# app_callback reads this to decide whether to install the uvloop policy.
26+
database_backend=DatabaseBackend.SQLITE,
2427
)
2528

2629

tests/db/test_uvloop_policy.py

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
"""Unit tests for the uvloop event-loop policy installer.
2+
3+
Covers the structural fix for issue #831 / #877: the asyncpg engine-dispose
4+
race ("IndexError: pop from an empty deque") is avoided by running the Postgres
5+
backend on uvloop, whose C scheduler has no self._ready.popleft() codepath.
6+
7+
These tests verify the gating logic of ``maybe_install_uvloop`` without touching
8+
a real database. The global event-loop policy is saved and restored around each
9+
test so installing uvloop here cannot leak into the rest of the suite.
10+
"""
11+
12+
import asyncio
13+
import sys
14+
15+
import pytest
16+
17+
from basic_memory.config import BasicMemoryConfig, DatabaseBackend
18+
from basic_memory.db import maybe_install_uvloop
19+
20+
21+
@pytest.fixture
22+
def restore_event_loop_policy():
23+
"""Save/restore the global event-loop policy around a test."""
24+
original = asyncio.get_event_loop_policy()
25+
try:
26+
yield
27+
finally:
28+
asyncio.set_event_loop_policy(original)
29+
30+
31+
def _postgres_config() -> BasicMemoryConfig:
32+
return BasicMemoryConfig(
33+
env="test",
34+
database_backend=DatabaseBackend.POSTGRES,
35+
database_url="postgresql+asyncpg://user:pass@localhost/db",
36+
)
37+
38+
39+
def _sqlite_config() -> BasicMemoryConfig:
40+
return BasicMemoryConfig(env="test", database_backend=DatabaseBackend.SQLITE)
41+
42+
43+
@pytest.mark.skipif(sys.platform == "win32", reason="uvloop is not available on Windows")
44+
def test_installs_uvloop_for_postgres_backend(restore_event_loop_policy):
45+
"""uvloop policy is installed when backend is Postgres and uvloop is available."""
46+
import uvloop
47+
48+
installed = maybe_install_uvloop(_postgres_config())
49+
50+
assert installed is True
51+
assert isinstance(asyncio.get_event_loop_policy(), uvloop.EventLoopPolicy)
52+
53+
54+
def test_no_uvloop_for_sqlite_backend(restore_event_loop_policy):
55+
"""SQLite users keep the default loop - the helper is a no-op."""
56+
before = asyncio.get_event_loop_policy()
57+
58+
installed = maybe_install_uvloop(_sqlite_config())
59+
60+
assert installed is False
61+
# Policy must be unchanged for the default (SQLite) path.
62+
assert asyncio.get_event_loop_policy() is before
63+
64+
65+
@pytest.mark.skipif(sys.platform == "win32", reason="uvloop is not available on Windows")
66+
def test_uvloop_unavailable_is_a_safe_noop(restore_event_loop_policy, monkeypatch):
67+
"""When uvloop cannot be imported the helper returns False without raising."""
68+
import builtins
69+
70+
real_import = builtins.__import__
71+
72+
def _fail_uvloop_import(name, *args, **kwargs):
73+
if name == "uvloop":
74+
raise ImportError("simulated missing uvloop")
75+
return real_import(name, *args, **kwargs)
76+
77+
monkeypatch.setattr(builtins, "__import__", _fail_uvloop_import)
78+
79+
before = asyncio.get_event_loop_policy()
80+
installed = maybe_install_uvloop(_postgres_config())
81+
82+
assert installed is False
83+
assert asyncio.get_event_loop_policy() is before

uv.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)