Skip to content

Commit ddb6822

Browse files
zzstoatzzclaude
andauthored
fix: dispose engine after import-time table creation (#1255) (#1256)
when `ensure_db_tables_exist()` runs on import, it creates an async engine via `asyncio.run()`. the engine's aiosqlite worker thread would remain cached after the event loop closed, potentially preventing python from exiting cleanly. this adds a `dispose_engine` parameter to `create_db_and_tables()` which disposes the engine and removes it from the cache after table creation. `ensure_db_tables_exist()` now uses this to clean up after itself. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent f0bee18 commit ddb6822

3 files changed

Lines changed: 68 additions & 11 deletions

File tree

docs/api-reference/marvin-database.mdx

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,15 +120,16 @@ Custom type for Usage objects that stores them as JSON in the database.
120120

121121
### `create_db_and_tables`
122122
```python
123-
def create_db_and_tables(force: bool = False)
123+
def create_db_and_tables(force: bool = False, dispose_engine: bool = False)
124124
```
125-
Create all database tables synchronously.
126-
127-
This is a synchronous alternative to create_db_and_tables() that can be used
128-
in contexts where asyncio.run() cannot be called.
125+
Create all database tables.
129126

130127
Args:
131128
force: If True, drops all existing tables before creating new ones.
129+
dispose_engine: If True, dispose the engine after creating tables.
130+
This is useful when called from asyncio.run() to ensure the
131+
aiosqlite worker thread is cleaned up and doesn't prevent
132+
Python from exiting.
132133

133134
### `ensure_db_tables_exist`
134135
```python

src/marvin/database.py

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -416,14 +416,17 @@ def _run_migrations(alembic_log_level: str = "WARNING") -> bool:
416416
return False
417417

418418

419-
async def create_db_and_tables(*, force: bool = False) -> None:
420-
"""Create all database tables synchronously.
421-
422-
This is a synchronous alternative to create_db_and_tables() that can be used
423-
in contexts where asyncio.run() cannot be called.
419+
async def create_db_and_tables(
420+
*, force: bool = False, dispose_engine: bool = False
421+
) -> None:
422+
"""Create all database tables.
424423
425424
Args:
426425
force: If True, drops all existing tables before creating new ones.
426+
dispose_engine: If True, dispose the engine after creating tables.
427+
This is useful when called from asyncio.run() to ensure the
428+
aiosqlite worker thread is cleaned up and doesn't prevent
429+
Python from exiting.
427430
"""
428431
engine = get_async_engine()
429432

@@ -435,6 +438,15 @@ async def create_db_and_tables(*, force: bool = False) -> None:
435438
await conn.run_sync(Base.metadata.create_all)
436439
logger.debug("Database tables created.")
437440

441+
if dispose_engine:
442+
await engine.dispose()
443+
# Remove the engine from the cache since it's disposed
444+
try:
445+
loop = asyncio.get_running_loop()
446+
except RuntimeError:
447+
loop = None
448+
_async_engine_cache.pop(loop, None)
449+
438450

439451
def init_database_if_necessary():
440452
"""Initialize the database file if necessary.
@@ -543,7 +555,10 @@ def ensure_db_tables_exist():
543555
# Create tables synchronously using asyncio.run() if we're not in an async context
544556
try:
545557
with _create_tables_lock:
546-
asyncio.run(create_db_and_tables())
558+
# Use dispose_engine=True to clean up the aiosqlite worker thread
559+
# after table creation. Without this, the cached engine's worker
560+
# thread could prevent Python from exiting cleanly.
561+
asyncio.run(create_db_and_tables(dispose_engine=True))
547562
logger.debug("Database tables created successfully.")
548563
except Exception as e:
549564
logger.error(f"Failed to create database tables: {e}")

tests/basic/engine/test_database.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
1+
import asyncio
2+
import threading
3+
14
from sqlalchemy import select
25
from sqlalchemy.orm import selectinload
36

47
from marvin.database import (
58
DBMessage,
69
DBThread,
10+
_async_engine_cache,
711
create_db_and_tables,
812
)
913

@@ -104,3 +108,40 @@ async def test_relationship_operations(session):
104108

105109
result = await session.execute(select(DBThread).where(DBThread.id == "test-thread"))
106110
assert result.scalars().first() is None
111+
112+
113+
def test_create_db_and_tables_with_dispose_cleans_up_engine():
114+
"""Test that create_db_and_tables with dispose_engine=True cleans up resources.
115+
116+
Regression test for issue #1255: process hangs on exit after importing marvin.
117+
118+
When create_db_and_tables is called with dispose_engine=True (as it is from
119+
ensure_db_tables_exist), the engine should be disposed and removed from
120+
the cache. This ensures the aiosqlite worker thread is cleaned up and
121+
doesn't prevent Python from exiting.
122+
"""
123+
124+
def get_non_daemon_threads():
125+
"""Return non-daemon threads excluding the main thread."""
126+
return [
127+
t
128+
for t in threading.enumerate()
129+
if t is not threading.main_thread() and not t.daemon
130+
]
131+
132+
# record initial state
133+
initial_cache_size = len(_async_engine_cache)
134+
initial_threads = len(get_non_daemon_threads())
135+
136+
# run create_db_and_tables with dispose_engine=True
137+
asyncio.run(create_db_and_tables(dispose_engine=True))
138+
139+
# verify engine was removed from cache
140+
assert len(_async_engine_cache) == initial_cache_size
141+
142+
# verify no new non-daemon threads were left behind
143+
current_threads = get_non_daemon_threads()
144+
assert len(current_threads) == initial_threads, (
145+
f"expected {initial_threads} non-daemon threads, "
146+
f"found {len(current_threads)}: {[t.name for t in current_threads]}"
147+
)

0 commit comments

Comments
 (0)