-
Notifications
You must be signed in to change notification settings - Fork 154
fix: prevent CLI commands from hanging on exit #462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add tests to verify CLI commands (tool, status) exit cleanly without hanging. The tests reproduce a bug where CLI commands hang indefinitely after displaying output. The root cause is that ensure_initialization() creates database connections via asyncio.run() that are not cleaned up before the event loop closes. These tests will fail until the fix is applied. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Cedric Hurst <cedric@spantree.net>
Fix issue where CLI commands like `basic-memory tool --help` and `basic-memory status` hang indefinitely after displaying output. Root cause: - ensure_initialization() and CLI commands create database connections via asyncio.run() - When asyncio.run() completes, the event loop closes - But global database engine holds async connections preventing exit - Process hangs indefinitely Fix: - ensure_initialization: Wrap initialize_app() with db.shutdown_db() cleanup in finally block before asyncio.run() returns - status command: Add inline cleanup wrapper to close database connections after run_status() completes Test: - Add regression tests that verify CLI commands exit cleanly - Tests use subprocess with timeout to detect hangs - Verifies both `tool` subcommands and `status` command 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Cedric Hurst <cedric@spantree.net>
|
|
This fixes CLI commands (status, project, tools) hanging indefinitely after completing their work. The root causes were: 1. Database connections created during initialization weren't cleaned up before the event loop closed, leaving orphaned connections 2. Multiple calls to asyncio.run() created separate event loops that didn't properly coordinate database connection cleanup Changes: - Make migration f8a9b2c3d4e5 idempotent by checking if columns/indexes exist before adding them (prevents failures on partial migrations) - Add lifespan to MCP server using FastMCP's context manager, replacing the separate thread for file sync - Add db.shutdown_db() cleanup to ensure_initialization() to properly clean up connections before the event loop closes - Skip ensure_initialization for API-using commands (status, project, tools, sync) since they get database connections through deps.py - Add run_with_cleanup() helper for CLI commands that handles proper database cleanup after coroutine completion - Remove ensure_migrations=False parameters (redundant since engine is reused within a single event loop) - Add regression tests in tests/cli/test_cli_exit.py to verify CLI commands exit cleanly within timeout Closes #462 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
Thank you for identifying this issue! We've incorporated the fix as part of a larger database initialization refactor in #471 that includes:
Unfortunately we couldn't merge this PR directly due to CLA requirements on the merge commit, so we re-implemented the core fixes. Your contribution was instrumental in identifying the root cause. Closing in favor of #471. |
This fixes CLI commands (status, project, tools) hanging indefinitely after completing their work. The root causes were: 1. Database connections created during initialization weren't cleaned up before the event loop closed, leaving orphaned connections 2. Multiple calls to asyncio.run() created separate event loops that didn't properly coordinate database connection cleanup Changes: - Make migration f8a9b2c3d4e5 idempotent by checking if columns/indexes exist before adding them (prevents failures on partial migrations) - Add lifespan to MCP server using FastMCP's context manager, replacing the separate thread for file sync - Add db.shutdown_db() cleanup to ensure_initialization() to properly clean up connections before the event loop closes - Skip ensure_initialization for API-using commands (status, project, tools, sync) since they get database connections through deps.py - Add run_with_cleanup() helper for CLI commands that handles proper database cleanup after coroutine completion - Remove ensure_migrations=False parameters (redundant since engine is reused within a single event loop) - Add regression tests in tests/cli/test_cli_exit.py to verify CLI commands exit cleanly within timeout Closes #462 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Summary
Fixes #461
CLI commands (
tool --help,status, etc.) hang indefinitely after displaying output because database connections created duringasyncio.run()are not cleaned up before the event loop closes.Changes
src/basic_memory/services/initialization.py: Wrapinitialize_app()withdb.shutdown_db()cleanup in finally blocksrc/basic_memory/cli/commands/status.py: Add inline cleanup wrapper forrun_status()tests/cli/test_cli_tool_exit.py: Add regression tests using subprocess timeouts to detect hangsTest plan
🤖 Generated with Claude Code