fix(sqlite): close connection before untracking on failure (#75629) - #75709
Closed
JoaoMarcos44 wants to merge 1 commit into
Closed
fix(sqlite): close connection before untracking on failure (#75629)#75709JoaoMarcos44 wants to merge 1 commit into
JoaoMarcos44 wants to merge 1 commit into
Conversation
…rch#75629) _TrackingMixin.close() removed the registry entry before calling the real close(), so a failed close() (e.g. cross-thread use under check_same_thread=True) left the fd open while has_live_connection() already reported false -- reopening the window for read_header_bytes_preopen() to byte-probe a live database and cancel its POSIX advisory locks. Close first, untrack only on success, so a failed close() stays tracked and the byte-probe guard fails closed.
Collaborator
Duplicate of #75699: both current diffs move SQLite untracking after a successful close and cover the same failed cross-thread-close registry invariant. |
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
_TrackingMixin.close()inhermes_cli/sqlite_safe_read.pyremoved aconnection's entry from the live-connection registry before calling the
real
close(). When the real close failed — e.g. cross-thread use underSQLite's default
check_same_thread=True— the registry entry was alreadygone, so
has_live_connection()reportedFalsefor a file descriptor thatwas still open. That let
read_header_bytes_preopen()byte-probe a livedatabase, which on POSIX cancels every advisory lock this process holds on
that file (including a running
VACUUM's exclusive lock), reopening theexact corruption path this module exists to prevent
(
"database disk image is malformed").Fixes #75629.
Root cause
Fix
Close first, untrack only on success — a failed
close()now failsclosed, leaving the registry (and therefore the byte-probe guard) honest:
Behavior on a successful close is unchanged. On a failed close, the
exception still propagates to the caller exactly as before — the only
change is that the registry entry survives, so
has_live_connection()andread_header_bytes_preopen()keep telling the truth.Regression test
tests/test_sqlite_lock_safe_inspection.py::test_close_failure_keeps_connection_tracked:connect_tracked()on a worker thread thatthen blocks (stays alive).
close()on it — SQLite raisesProgrammingError(cross-thread, defaultcheck_same_thread=True).has_live_connection()is stillTrueandread_header_bytes_preopen()still returnsNone(refused).The same assertion fails on
mainbefore this change: step 3 seeshas_live_connection() == Falsewhile the descriptor is still open.Test plan
pytest tests/test_sqlite_lock_safe_inspection.py— 7/7 passed(6 pre-existing + 1 new regression test)
tests/hermes_cli/test_session_recovery.py,tests/hermes_cli/test_kanban_db.py— no new failures. Two failurespresent in
test_kanban_db.py(
test_rate_limit_exit_requeues_without_counting_failure,test_worktree_workspace_explicit_target_materializes_linked_worktree)are pre-existing on
main(confirmed viagit stash+ rerun beforethis diff), unrelated to
sqlite_safe_read.py.Scope
Only two files touched:
hermes_cli/sqlite_safe_read.py(5-line reorder +one docstring clarification) and the test file above.
hermes_state.pyisuntouched.
Not fixed here — related, complementary, tracked separately
This PR closes the registry's lie; it does not address why a close can
fail in the first place, or reap connections nobody ever retries closing:
SessionDB's per-thread read connections open with thedefault
check_same_thread=True, which is the concrete trigger for thefailure this PR guards against on that path. That PR opens readers with
check_same_thread=Falseinstead, so the close succeeds and the guardnever has to fire there.
worker threads that finished long ago, none of them reaped until final
shutdown (
RLIMIT_NOFILEpressure, not a registry lie).All three are independent fixes for independent bugs; this one stands on
its own and should merge regardless of the other two.
Infographic
(Rendered locally and hosted on an isolated, never-merged asset branch —
per
.gitignore's PR-infographic policy, the binary never enters thisPR's history or
main.)