fix: dispose engine after import-time table creation#1256
Merged
Conversation
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>
github-actions
Bot
added
documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1255 where importing marvin causes the process to hang on exit. The fix adds a dispose_engine parameter to create_db_and_tables() to properly clean up the aiosqlite worker thread after table creation.
- Adds
dispose_engineparameter tocreate_db_and_tables()for controlled engine cleanup - Updates
ensure_db_tables_exist()to dispose the engine after import-time table creation - Adds regression test to verify engine disposal and thread cleanup
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/marvin/database.py | Adds dispose_engine parameter to create_db_and_tables() with disposal logic; updates ensure_db_tables_exist() to pass dispose_engine=True |
| tests/basic/engine/test_database.py | Adds regression test test_create_db_and_tables_with_dispose_cleans_up_engine to verify engine disposal and thread cleanup |
| docs/api-reference/marvin-database.mdx | Updates documentation to reflect new dispose_engine parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dispose_engineparameter tocreate_db_and_tables()to clean up the aiosqlite worker threadensure_db_tables_exist()now disposes the engine after table creation to prevent hangingBackground
When marvin is imported,
ensure_db_tables_exist()runsasyncio.run(create_db_and_tables()). This creates an async engine that gets cached. The underlying aiosqlite connection spawns a worker thread that could prevent Python from exiting cleanly if not properly disposed.Changes
dispose_engine: bool = Falseparameter tocreate_db_and_tables()dispose_engine=True, the engine is disposed and removed from the cache after table creationensure_db_tables_exist()to passdispose_engine=Truewhen called from importTest plan
test_create_db_and_tables_with_dispose_cleans_up_engine🤖 Generated with Claude Code