fix(db): close SQLite FDs on context exit and fallback gracefully if FTS5 is missing - #67389
Closed
CryptoByz wants to merge 1 commit into
Closed
fix(db): close SQLite FDs on context exit and fallback gracefully if FTS5 is missing#67389CryptoByz wants to merge 1 commit into
CryptoByz wants to merge 1 commit into
Conversation
Collaborator
Contributor
Author
|
Closing as superseded. While researching the root cause, I found that upstream already landed the same fix independently via multiple commits that are now in
The Thanks for the parallel work on this — glad the approach was validated by independent discovery. |
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.
Fixes #33580
Fixes #13029
What changed and why
Under long-lived daemon environments (such as the gateway dispatcher or web dashboard), processes eventually hit the kernel FD limit and crash with
[Errno 24] Too many open files. This happens becausekanban_db.connect()returned a plainsqlite3.Connectionobject, which has an__exit__context manager method that only manages commits/rollbacks and fails to close the underlying file descriptor. Additionally, on specific platforms or custom Python builds lacking the SQLitefts5module ortrigramtokenizer (commonly Python 3.11 on macOS), session database initialization throws an unhandledsqlite3.OperationalErrorduring migration or table creation, preventing the entire application from starting up instead of degrading search features gracefully.In stage one of the root cause, using standard connection contexts like
with kb.connect() as conn:leaks file descriptors tokanban.dband its WAL sidecar files upon exit. In stage two, the custom workaroundconnect_closing()was only applied to a subset of call sites while over 150 test suites and CLI routines continued to use the unsafeconnect()context directly. For the FTS5 issue, the schema initialization and data migrations unconditionally executedCREATE VIRTUAL TABLE USING fts5statements. Without catching and isolating these errors, a lack of the compiled extension causes immediate startup crashes rather than falling back to normal table structures and SQLLIKE-based queries.Fix
hermes_cli/kanban_db.py
_ConnContext(sqlite3.Connection), a custom subclass overriding__exit__to guarantee connection closure viaself.close()on context exit regardless of transaction outcome.connect()to passfactory=_ConnContexttosqlite3.connect(), making all returned connections safe by default without altering existingwith connect()call sites.hermes_state.py
SessionDB.__init__by ensuring any error during initialization (e.g. database locking or setup failure) immediately closesself._connbefore raising, avoiding unclosed connection leaks._init_schemainside try-except blocks using new static helpers_is_fts5_unavailable_error()and_is_trigram_unavailable_error().current_version < 10andcurrent_version < 11migration blocks from crashing when executing virtual table recreation on systems without FTS5 or the trigram tokenizer, allowing normal tables and indices to be upgraded cleanly.tests/hermes_cli/test_kanban_db.py
test_connect_closes_fd_on_context_manager_exitto verify file descriptor cleanup.test_connect_context_manager_closes_on_exceptionto ensure exception paths close connections.test_connect_returns_conn_context_typeto assert the correct subclass is returned.wal_blocking_connectmock to dynamically subclass the passed factory class to prevent multiple values for keyword argumentfactoryexceptions during testing.What this does NOT change
This does not change the core CRUD SQL logic or database constraints. FTS5 full-text search behavior remains unchanged on environments where FTS5 is compiled and active. The
connect_closing()utility is preserved for backwards compatibility.How to test
Run the pytest suite to verify all existing and new tests pass:
Platforms tested
Linux.