fix: dispose database engine on shutdown#8650
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces database engine connection pool disposal during the application shutdown process in core_lifecycle.py. The reviewer suggested adding defensive checks to verify the existence of self.db and its engine attribute before calling dispose(), which prevents potential AttributeError exceptions in uninitialized or testing environments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider whether
self.db.engine.dispose()is actually awaitable in your setup; in vanilla SQLAlchemy’sAsyncEngineit’s a synchronous method, so wrapping it inawaitcould raise aTypeErrorand you may instead want to call it synchronously or viaasyncio.to_threadif it can block. - It may be safer to guard the dispose call with a null/attribute check (e.g.,
if self.db and getattr(self.db, "engine", None):) to avoid errors in shutdown paths where the database might not have been fully initialized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider whether `self.db.engine.dispose()` is actually awaitable in your setup; in vanilla SQLAlchemy’s `AsyncEngine` it’s a synchronous method, so wrapping it in `await` could raise a `TypeError` and you may instead want to call it synchronously or via `asyncio.to_thread` if it can block.
- It may be safer to guard the dispose call with a null/attribute check (e.g., `if self.db and getattr(self.db, "engine", None):`) to avoid errors in shutdown paths where the database might not have been fully initialized.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
After #7724 was reverted (#8638), the SQLite engines use SQLAlchemy's default connection pool again instead of
NullPool. Each pooledaiosqliteconnection owns a background worker thread, andaiosqlitenever marks these threads as daemon, so they inherit the main thread's non-daemon status.core_lifecycle.stop()terminates every manager but never disposes the main database engine, so the pool's idle connection threads stay alive and blockthreading._shutdown()at interpreter exit. As a result, a single Ctrl+C hangs instead of shutting down cleanly.NullPoolhad been masking this by closing each connection (and its thread) right after use.Modifications / 改动点
astrbot/core/core_lifecycle.py: disposeself.db.engineat the end ofstop()so the connection pool closes idle connections and theiraiosqliteworker threads exit, letting the process terminate. The knowledge base engine is already disposed in its ownclose(); this aligns the main engine.Screenshots or Test Results / 运行截图或测试结果
Before:


After:
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Bug Fixes: