fix(cli): stop doctor's journal-mode probe from cancelling live SQLite locks - #87921
Open
briandevans wants to merge 5 commits into
Open
fix(cli): stop doctor's journal-mode probe from cancelling live SQLite locks#87921briandevans wants to merge 5 commits into
briandevans wants to merge 5 commits into
Conversation
_read_journal_mode opened each Hermes database with a bare open(db_path, "rb") to read header byte 18. The read itself is harmless; the close() is not. Per sqlite.org/howtocorrupt.html, close() on *any* descriptor for a file cancels every POSIX advisory lock this process holds on it — so the close at the end of that with-block drops the locks a live connection is holding, including the EXCLUSIVE lock a VACUUM holds while it rewrites the whole file. Another process is then free to write into a file its writer still believes it owns, which is the documented route to "database disk image is malformed". This is reachable. run_doctor is not only a standalone CLI process: the dashboard console registers "doctor" (console_engine.py:570) and calls run_doctor directly, in-process (console_engine.py:1297), on the web server's console thread pool — in a process that holds live SessionDB connections (web_server.py:11673, :11689). Typing "doctor" there raw-opened and closed state.db, projects.db, response_store.db, cron/executions.db and every board's kanban.db while those connections were live. The HTTP route at /api/ops/doctor deliberately spawns a subprocess instead; the console path did not. hermes_cli.sqlite_safe_read exists to prevent exactly this, and its read_header_bytes_preopen is documented as "the ONLY sanctioned byte-level read of a database file". It performs the registry check and the open/read/close together under the connection-lifecycle lock, so it refuses once any connection to the path is live. The audit that converted the other byte-probes (hermes_state.py:2750, backup.py:436, kanban_db.py:1861) landed in 95fb477 on 2026-07-25; _read_journal_mode was added in 6583297 on 2026-08-06 and reintroduced the pattern, so this is a regression against an invariant the tree already states, not a refactor preference. The helper is a plain byte read, so the docstring's stated property is preserved: no SQLite engine open, and no -wal/-shm sidecars are created. Only the acquisition of `header` changes; the empty / not-a-database / unrecognized-format-version branches are untouched.
read_header_bytes_preopen returns None for every failure, so routing the probe through it flattened "[Errno 2] No such file or directory: …" and "[Errno 13] Permission denied: …" into one opaque "file could not be read". doctor exists to name the problem, so that detail is worth keeping: _report_database_journal_modes prints the string verbatim, and on a vulnerable SQLite it is the only clue the user gets about why WAL exposure could not be ruled out. _unreadable_reason recovers it from metadata only. stat() reports the missing file, the dangling symlink and the unsearchable parent directory; os.access(..., R_OK) reports the unreadable file that stat() can still see. Neither call takes a file descriptor, so neither can cancel the POSIX advisory locks the previous commit was about — the invariant holds.
Locks the invariant the probe now honours: while this process holds a registered connection to a database, _read_journal_mode reports it as unreadable instead of taking a descriptor whose close() would cancel that connection's POSIX advisory locks. Against the previous implementation the four regression cases fail with `assert 'wal' is None` — it read the header straight out of a live database — and pass once the read is routed through read_header_bytes_preopen. Coverage is both the registry API (track_connection) and connect_tracked, the path SessionDB actually takes, plus the _report_database_journal_modes output so the degraded row is asserted end to end. Two cases deliberately hold in both directions and are guards rather than probes: - an untracked sqlite3.connect holding BEGIN EXCLUSIVE must NOT block the read. Only connections this process registered can be cancelled by a close() we make; another process's locks are irrelevant. Without this, a later "just refuse whenever the file looks busy" change would silently turn every doctor row into "could not be read". - the refusal creates no new -wal/-shm sidecars, which is the property the function's docstring promises and the reason it byte-probes rather than opening a connection in the first place.
…rows read_header_bytes_preopen answers None for a live connection, a missing file and an unreadable file alike, so the error string doctor prints is now chosen rather than inherited from the OSError. These cases pin that choice: the missing file keeps its errno text, and the chmod-000 file is still reported as a permission problem rather than collapsing into the generic message — the behaviour the raw open() gave before. test_reason_does_not_open_the_file is the load-bearing one. It patches builtins.open to raise and asserts _unreadable_reason still answers, which fixes the constraint that makes the helper safe to call on a database path at all: stat() and access() read metadata and take no file descriptor, so no close() of ours can cancel the file's advisory locks. A future edit that reached for open() here to get a better message would reintroduce the original bug on the error path, and this test fails loudly if it does. The root check is written as hasattr(os, "geteuid") and os.geteuid() == 0 rather than the bare call the surrounding tests use. skipif conditions are evaluated at collection time and os.geteuid is POSIX-only, so the bare form raises AttributeError and takes the whole module down on Windows. The pre-existing occurrences are left alone — NousResearch#81926 and NousResearch#84073 are already open against exactly those lines, and this only avoids adding a third instance of the same defect.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a lock-safety bug in hermes doctor when invoked in-process (e.g., via the dashboard console) by avoiding raw open()/close() of database files that may have live SQLite connections in the same process. This aligns doctor with the existing sqlite_safe_read invariants designed to prevent POSIX advisory lock cancellation.
Changes:
- Update
hermes_cli/doctor.py::_read_journal_modeto useread_header_bytes_preopen(...)(refuses when a tracked connection is live) instead of raw file I/O. - Add
_unreadable_reason(...)to preserve actionable error details without opening the DB file. - Add/expand tests to cover refusal behavior under live tracked connections and to validate unreadable-path error reporting without opening files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/doctor.py |
Routes journal-mode header probing through sqlite_safe_read to avoid lock-cancelling raw file closes; adds metadata-only unreadable diagnostics. |
tests/hermes_cli/test_doctor_journal_modes.py |
Adds coverage for live-connection refusal behavior and unreadable-reason reporting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+47
to
+55
| @pytest.fixture | ||
| def clean_registry(): | ||
| """Keep the module-level connection registry from leaking across tests.""" | ||
| yield | ||
| import hermes_cli.sqlite_safe_read as mod | ||
|
|
||
| with mod._live_lock: | ||
| mod._live_connections.clear() | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Agreed, and fixed in 833dc9f6c81 (current head of this branch).
clean_registry in tests/hermes_cli/test_doctor_journal_modes.py:42 now clears the registry on entry as well as teardown, and the teardown clear runs under try/finally so a failing test cannot skip it.
The concrete risk was narrower than "isolation" but worse: every assertion in TestLiveConnectionSafety checks that _read_journal_mode refuses. A stale entry left by an earlier leak would make read_header_bytes_preopen refuse for that stale reason, so those tests would still go green while no longer exercising the connection they set up. Clearing on entry is what makes the refusal attributable to the test's own tracked connection.
tests/hermes_cli/test_doctor_journal_modes.py and tests/test_sqlite_lock_safe_inspection.py pass together: 43 passed.
alt-glitch
added
type/bug
…ndent clean_registry cleared the connection registry only on teardown, so it protected the tests that ran after it but not the test holding it. A leak from earlier in the session — a failed test that never reached its close(), or any test that does not take this fixture — would leave a stale entry behind, and read_header_bytes_preopen would then refuse for that stale reason instead of the one under test. The refusal assertions would still pass, but for the wrong reason, which is the failure mode a regression test can least afford. Clearing on entry as well makes the fixture independent of what ran before it, and the teardown clear now runs under try/finally so a failing test cannot skip it.
Contributor
Author
|
CI audit — the one red is infrastructure, not this diff.
The other 11 of 12 slices pass, along with I can't re-run the job from here (not a collaborator, so |
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.
What does this PR do?
hermes_cli/doctor.py::_read_journal_modereads header byte 18 of every Hermes database with a bareopen(db_path, "rb"). The read is harmless; theclose()is not. Per SQLite's own corruption documentation,close()on any descriptor for a file cancels every POSIX advisory lock the process holds on it — so the close at the end of thatwithblock drops the locks a live connection is holding, including the EXCLUSIVE lock aVACUUMholds while it rewrites the whole file. Another process is then free to write into a file its writer still believes it owns: the documented route todatabase disk image is malformed.This PR routes the read through the repo's own sanctioned helper,
read_header_bytes_preopen, which performs the registry check and theopen/read/closetogether under the connection-lifecycle lock and refuses once any connection to the path is live.Scope, stated honestly: in a standalone
hermes doctorCLI process nothing is live and the current code is harmless. The bug is the in-process dashboard console path (see below). I am not claiminghermes doctorcorrupts databases from a terminal.The invariant is the repo's own, not my preference.
hermes_cli/sqlite_safe_read.pyexists solely to prevent this and states it as a numbered rule in its module docstring:and
read_header_bytes_preopenis documented as "the ONLY sanctioned byte-level read of a database file".This is a regression against that audit, by ancestry:
95fb4778561— "fix(state): close the tracking leak and finish the audit of raw DB reads", 2026-07-25_read_journal_modeintroduced65832970868— "feat(doctor): report per-database journal mode…", 2026-08-06git merge-base --is-ancestor 95fb4778561 65832970868The sweep landed 12 days before the new probe, so the pattern was reintroduced after the tree had already ruled it out. The three sites the sweep converted are still converted:
hermes_state.py:2750,hermes_cli/backup.py:436,hermes_cli/kanban_db.py:1861._read_journal_modewas the only survivor — I grepped everySQLite format 3header probe in the tree to confirm there is no second one.Why it is reachable
run_doctoris not only a CLI entrypoint. Every leg verified againstmain:doctor.py:174—_read_journal_mode(path)runs over every Hermes database.console_engine.py:570registers thedoctorconsole command;console_engine.py:1297-1299doesfrom hermes_cli.doctor import run_doctorand calls it directly, in-process.hermes_cli/web_server.py:16152-16154builds thatHermesConsoleEnginein the dashboard process, dispatched on the console thread pool (:15923).SessionDB(..., read_only=False)atweb_server.py:11673,read_only=Trueat:11689, plus:344,:11686,:11709.SessionDBconnects via_connect_tracked_db→connect_tracked, so these are registered in the very registry the helper consults.So typing
doctorin the dashboard console raw-open()/close()sstate.db,projects.db,response_store.db,cron/executions.dband every board'skanban.dbon a worker thread while those connections are live and the server is serving other requests.The contrast that settles intent: the HTTP route
/api/ops/doctor(web_server.py:13591) deliberately spawns a subprocess via_spawn_hermes_action. That path already avoids running doctor in the connection-holding process; the console path does not.The docstring's stated property is preserved
The function's docstring promises no SQLite engine open and therefore no
-wal/-shmsidecars.read_header_bytes_preopenis itself a plain byte read, so that holds — this is deliberately not fixed by opening a read-only SQLite connection, which would create exactly the sidecars the original author guarded against. A test asserts no new sidecars appear.Related Issue
No filed issue — found by auditing
mainagainstsqlite_safe_read.py's stated rules.Type of Change
Changes Made
hermes_cli/doctor.py—_read_journal_modeacquires the header viaread_header_bytes_preopen(db_path, length=20)instead ofopen(db_path, "rb"). Only the acquisition changes; the empty / not-a-database / unrecognized-format-version branches are untouched.hermes_cli/doctor.py— new_unreadable_reasonhelper. The pre-open reader collapses everyOSErrorintoNone, which would have flattened[Errno 2] No such file or directoryand[Errno 13] Permission deniedinto one opaque string; doctor prints that string verbatim and on a vulnerable SQLite it is the user's only clue why WAL exposure could not be ruled out.stat()andos.access(..., R_OK)recover it from metadata alone — neither takes a file descriptor, so neither can cancel the advisory locks this PR is about.tests/hermes_cli/test_doctor_journal_modes.py— 9 cases acrossTestLiveConnectionSafetyandTestUnreadableReason.Split into 4 atomic commits (two fixes, two test commits); each is independently green.
On
has_live_connection: it is consulted only afterread_header_bytes_preopenhas already returnedNone, purely to choose an error string — never to gate I/O. This is deliberately not the check-then-read patternoffline_file_accesswarns about, because the helper itself remains the guard and performs its check atomically under the lock.How to Test
Regression direction, verified both ways:
doctor.pychange (tests kept, production hunk reverted tomain): 6 failed, 30 passed. The core case fails withAssertionError: assert 'wal' is None— the old probe read the header straight out of a live database.tests/hermes_cli/test_doctor_journal_modes.py,test_doctor.py,test_doctor_dedicated_provider_skip.py,test_doctor_live.py, and the owner module'stests/test_sqlite_lock_safe_inspection.py— 122 passed.Three of the nine new cases hold in both directions by design and are guards rather than probes:
sqlite3.connectholdingBEGIN EXCLUSIVEmust not block the read — only connections this process registered can be cancelled by aclose()we make. Without this, a later "refuse whenever the file looks busy" change would silently turn every doctor row intocould not be read.-wal/-shmsidecars.test_reason_does_not_open_the_filepatchesbuiltins.opento raise and asserts_unreadable_reasonstill answers, so a future edit that reaches foropen()to get a better message fails loudly instead of reintroducing the bug on the error path.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — I ran the doctor suites and thesqlite_safe_readowner suite (122 passed), not the full treeDocumentation & Housekeeping
docs/, docstrings) — the_read_journal_modedocstring now records why the read is routed through the helperCross-platform note. The advisory-lock behaviour is POSIX; on Windows the raw read is not a lock hazard, so this is a no-op there rather than a regression. My new root check is written as
hasattr(os, "geteuid") and os.geteuid() == 0rather than the bareos.geteuid()the surrounding tests use, becauseskipifconditions are evaluated at collection time andos.geteuidis POSIX-only. I deliberately left the pre-existing bare occurrences alone — #81926 and #84073 are already open against exactly those lines, and I did not want to duplicate either; this change only avoids adding a third instance.Related / Positioning
hermes_cli/doctor.pyis a crowded file: I enumerated 43 distinct open PRs touching it in the last 7 days (24 updated on 2026-08-16, 19 on 08-10..08-15) and fetched and grepped each one's diff for_read_journal_mode,open(db_path,read_header_bytes_preopen,_SQLITE_HEADER_MAGICand_report_database_journal_modes. All 43 are clear of this fix site. The nearest is #86678, whose closest hunk is an import at line 18 — this change adds no module-level import, so there is no overlap. This PR is confined to_read_journal_mode(doctor.py:119-155) and is disjoint from all of them.Pre-empting one specific reading: this is not a duplicate of my own #76003, which also touches
hermes_cli/doctor.py.doctor.pyhunks are@@ -703and@@ -1499; this fix site is:119-155. Disjoint by ~580 lines, different functions.gh pr diff 76003 | grep -c _read_journal_mode→ 0. It never mentions the symbol._read_journal_modedoes not exist on fix(cli): bound the doctor state.db health probe with a cancellable SQLite deadline (supersedes #72527) #76003's branch at all — that branch's merge-base predates65832970868, which introduced the function. A textual conflict is not merely unlikely, it is impossible.They are independently reviewable and can land in either order.