fix(cli): bound the doctor state.db health probe with a cancellable SQLite deadline (supersedes #72527) - #76003
Conversation
…QLite deadline (supersedes NousResearch#72527) `hermes doctor` stalls indefinitely right after printing `state.db exists (N sessions)`. The next statement is `_db_opens_cleanly(state_db_path)`, whose `PRAGMA integrity_check` walks every page of the file — O(file size), with no output and no way out. The repo already documents the pathology in its own test suite (tests/hermes_cli/test_doctor.py, `_isolate_home`: "a multi-minute PRAGMA integrity_check that blew the 300s per-file budget") and already bounds the same scan in hermes_cli/backup.py with a byte ceiling. The probe is now cancellable at the SQLite level. `_db_opens_cleanly` takes a keyword-only `deadline_seconds` (default `None` = the existing unbounded contract, so the five `repair_state_db_schema` verification callers, `hermes sessions repair` and `session_recovery` are untouched). When a deadline is given it installs a progress handler that aborts the running statement once the wall clock runs out; measured, `PRAGMA integrity_check` polls that handler ~170 times on a 41 MB database, so the deadline genuinely interrupts the scan instead of merely stopping the caller from waiting on it. A timeout must never be mistaken for corruption. An aborted statement raises `sqlite3.OperationalError("interrupted")`, which is a `sqlite3.DatabaseError` — so returning it as the probe's "reason" string makes `hermes doctor --fix` call `repair_state_db_schema()`, whose last strategy drops the whole `messages_fts%` schema and VACUUMs. A healthy-but-large state.db would be rewritten for being slow. The deadline is therefore raised as `DBHealthProbeTimeout`, outside the `sqlite3` exception hierarchy, and doctor reports "health check skipped" and takes the repair branch only for a real corruption reason. Around that, doctor runs the probe on `tools.daemon_pool`'s `DaemonThreadPoolExecutor` with an explicit `shutdown(wait=False)` and a small grace over the SQLite deadline, for the residual case SQLite cannot interrupt at all (a `connect()` or page read wedged in an uninterruptible I/O syscall). `with ThreadPoolExecutor(...)` is deliberately avoided: its exit calls `shutdown(wait=True)` and blocks until the worker finishes, and stdlib pool workers are joined unconditionally by `concurrent.futures.thread._python_exit` even after `shutdown(wait=False)`, which would only move the hang to interpreter exit. The `SELECT COUNT(*) FROM sessions` immediately above the probe is inside the same bounded region. Repair paths are deliberately left unbounded: a user-requested repair must be allowed to complete. Fixes NousResearch#72441
There was a problem hiding this comment.
Pull request overview
Bounds the hermes doctor state.db health probe so the diagnostic can’t hang indefinitely on large or slow SQLite databases, while preserving the safety invariant that a timeout is not treated as corruption (and therefore never escalates into destructive repair).
Changes:
- Add a cancellable SQLite-level deadline to
_db_opens_cleanly()via a progress handler, surfaced as a newDBHealthProbeTimeoutexception (not asqlite3.Error). - Wrap doctor’s
state.dbreads in an abandonable daemon-thread boundary to ensure the command returns even if SQLite can’t be interrupted (e.g., wedged I/O). - Add a focused regression test module covering wall-clock bounding and the “timeout never triggers repair” invariant.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_state.py |
Adds progress-handler-based deadlines to the DB health probe and raises a dedicated timeout exception instead of returning a corruption reason. |
hermes_cli/doctor.py |
Applies a bounded/abandonable execution path to the state.db health probe and reports timeouts as “skipped” without entering repair. |
tests/hermes_cli/test_doctor_state_db_deadline.py |
Adds regression tests for bounded execution time and for preventing timeout → repair escalation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if count is not None: | ||
| check_ok(f"{_DHH}/state.db exists ({count} sessions)") | ||
|
|
There was a problem hiding this comment.
Good catch — fixed in d3b6184.
state_db_path.exists() is already true when we get here, so the file's existence is established regardless of whether the probe returned. The boundary-timeout path now emits an explicit row instead of dropping it:
ℹ ~/.hermes/state.db exists (session count not read — the health check timed out before it returned)
check_info rather than check_ok, because the count genuinely was not read and this should not read as a passed check. test_doctor_returns_without_waiting_for_a_blocked_probe now asserts that line is present, so the row cannot silently disappear again.
…es out In the worst-case path — the abandonable boundary fires because SQLite could not be interrupted at all — the session count never comes back and the `state.db exists (N sessions)` row was dropped entirely, even though the file's existence is already established. Report existence with an explicit note that the count was not read.
|
CI audit — the single failure on this branch is a pre-existing baseline on clean
This PR touches
|
Related: #72527 addresses the same |
|
Live reproduction/validation evidence from a large real-world
After comparing the open implementations, #76003 looks like the best consolidation lane rather than opening another competing PR. Its cancellable SQLite progress-handler, daemon fallback for uninterruptible I/O, and explicit rule that timeout is inconclusive—not corruption and never a repair trigger—cover the bug class more generally than the local size threshold. One rebase/non-regression point worth retaining against current This is supporting live evidence, not a claim that I ran the exact #76003 head against current |
|
Thanks for the detailed 8.7 GB live-database validation, and for calling out the retention points in your comment. At current head This is pinned by For clarity, I have not tested the exact live database; the confirmation above is against the current branch implementation and its focused deadline coverage. |
What does this PR do?
Supersedes #72527 (thanks @JonthanaHanh — the diagnosis and the line are right; the boundary is what needs replacing) and closes the hang reported in #72441.
hermes doctorstalls indefinitely right after printingstate.db exists (N sessions). The next statement is_db_opens_cleanly(state_db_path)(hermes_cli/doctor.py), whosePRAGMA integrity_checkwalks every page of the file — O(file size), with no output and no way to interrupt it. The repo already documents this exact pathology in its own test suite:and already bounds the same scan elsewhere:
hermes_cli/backup.py:320-353,404-441capsintegrity_checkwith a byte ceiling "becauseintegrity_checkwalks every page … O(file size)", falling back to a cheap header/structural probe.This is built directly to the two changes requested in review on #72527:
Cross-surface reach — one call site, three surfaces, no extra files touched.
run_doctoris the shared implementation for the terminal command (hermes_cli/main.py:4827-4829), the Hermes Consoledoctorverb (hermes_cli/console_engine.py:1279-1281), and the desktop Command Center maintenance op (hermes_cli/web_server.py:12581-12584,POST /api/ops/doctor). A wedged probe left the Command Center's Doctor action reportingrunning: trueforever.Why the thread timeout in #72527 does not bound the command
Its diff submits the probe to a
with concurrent.futures.ThreadPoolExecutor(max_workers=1) as _ex:block. Leaving that block callsExecutor.shutdown(wait=True), which blocks until the probe finishes — so the 30 sresult()only delays the warning. Measured on this branch, with a probe that blocks for 90 s:hermes doctorwall clockorigin/mainorigin/main+ #72527Switching that to
shutdown(wait=False)on a stdlib pool would not fix it either: stdlib workers are registered inconcurrent.futures.thread._threads_queues, whose atexit hook joins every worker unconditionally. The hang just moves to interpreter exit.tools/daemon_pool.py's module docstring states this and exists precisely for this case; measured here, a stdlib pool with one abandoned 15 s worker takes 15.08 s to exit the interpreter aftershutdown(wait=False), versus 0.04 s forDaemonThreadPoolExecutor.The data-safety invariant (neither the original PR nor the review covers this)
A progress-handler abort raises
sqlite3.OperationalError("interrupted"), andOperationalErroris a subclass ofsqlite3.DatabaseError._db_opens_cleanlyends withexcept sqlite3.DatabaseError as exc: return str(exc), so a naive deadline returns"interrupted"as an unhealthy reason. Indoctor.pya non-Nonereason under--fixcallsrepair_state_db_schema(), whose escalation path drops the wholemessages_fts%schema and VACUUMs. A healthy-but-largestate.dbwould be sent into destructive repair merely for being slow. Verified by building exactly that naive shape locally:repair_state_db_schemawas called once, on a healthy database, printingRepaired state.db FTS write health.The deadline is therefore surfaced as a new
DBHealthProbeTimeout, deliberately outside thesqlite3exception hierarchy so noexcept sqlite3.DatabaseErrorhandler can swallow it as corruption, and doctor reports the check as skipped rather than passed or failed.Related Issue
Fixes #72441
Type of Change
Changes Made
hermes_state.py_db_opens_cleanlygains a keyword-onlydeadline_seconds: Optional[float] = None. The default is the historical unbounded contract, so every existing caller is byte-for-byte unaffected._ProbeDeadlineprogress handler is installed on the probe connection (1000 VM instructions per callback). Measured:PRAGMA integrity_checkfires it 170 times on a 41 MB database and 21 times on a 700 KB one, so the deadline is polled continuously through the scan.DBHealthProbeTimeout(plainException, not asqlite3error), raised only when the handler actually aborted a statement — never returned as a reason string._run_db_health_probe(conn)so the connection prologue (and any deadline on it) is owned by the caller. The statement bodies are unchanged.hermes_cli/doctor.py_STATE_DB_PROBE_DEADLINE_S = 30.0(matching fix(cli): add timeout to state.db health probe in hermes doctor (#72441) #72527's chosen budget) and_STATE_DB_PROBE_ABANDON_GRACE_S = 5.0, module constants so tests can monkeypatch them. No config key and no CLI flag — a diagnostic should not need tuning in order to terminate._run_abandonable(fn, timeout_s):tools.daemon_pool.DaemonThreadPoolExecutor+ explicitshutdown(wait=False)in afinally. Neverwith ThreadPoolExecutor(...). This is the belt-and-braces layer for work SQLite cannot interrupt at all — aconnect()or page read wedged in an uninterruptible I/O syscall, e.g. a hung network mount. The grace means the in-statement cancellation is normally what stops the probe (it closes its connection on the way out) and the thread boundary is the fallback.check_warn+ an entry inissuesstating the database was not modified, and the repair branch is skipped entirely (elif), not entered with aNonereason.SELECT COUNT(*) FROM sessionsimmediately above the probe now runs inside the same bounded region. Exceptions from it still propagate unchanged to theis_malformed_db_errorhandler, so malformed-schema detection and repair are preserved.tests/hermes_cli/test_doctor_state_db_deadline.py(new file, 6 tests).Sibling-site sweep
Root cause: a read-only
state.dbhealth probe runs unbounded inside an interactive diagnostic. Every site of it, and why:hermes_cli/doctor.py_db_opens_cleanly(state_db_path)hermes_cli/doctor.pySELECT COUNT(*) FROM sessions(immediately above)hermes_cli/console_engine.py:1279-1281(doctorconsole verb)run_doctor; file not touchedhermes_cli/web_server.py:12581-12584(POST /api/ops/doctor)hermes doctor; file not touchedhermes_state.py×5 insiderepair_state_db_schemahermes_cli/sessions_cmd.py(hermes sessions repair)hermes_cli/console_engine.py:1492(sessions repairverb)hermes_cli/session_recovery.py:1009hermes_cli/doctor.pypost---fixre-count andPRAGMA wal_checkpoint(PASSIVE)should_fix-only;PASSIVEis non-blocking by definitionhermes_cli/backup.py:404-441hermes_cli/kanban_db.pyhermes kanban repair), explicit repair pathAll excluded call sites pass no
deadline_secondsand so keep the exact behaviour they have today.How to Test
Reproduce the hang and the fix without a multi-gigabyte database, by making the probe block:
origin/main, monkeypatchhermes_state._db_opens_cleanlytotime.sleep(90)and callrun_doctor(Namespace(fix=False, ack=None))against an isolatedHERMES_HOME. It returns after 92.4 s with no warning. Apply fix(cli): add timeout to state.db health probe in hermes doctor (#72441) #72527's diff and it returns after 92.3 s, having printed the timeout warning at 30 s. On this branch it returns after 37.3 s.Results (per file, matching
scripts/run_tests.sh's per-file isolation):tests/hermes_cli/test_doctor_state_db_deadline.pytests/hermes_cli/test_doctor.pytests/test_state_db_malformed_repair.pytests/test_hermes_state.pytests/hermes_cli/test_doctor_command_install.pytests/hermes_cli/test_session_listing.pytests/test_sqlite_wal_reset_gate.pyNew tests:
test_doctor_returns_without_waiting_for_a_blocked_proberun_doctorreturns in well under half thattest_probe_timeout_is_reported_as_skipped_not_as_corruptiontest_probe_timeout_does_not_trigger_repair_under_fix--fixand a timing-out probe,repair_state_db_schemais never calledtest_progress_handler_cancels_a_real_integrity_checkdeadline_seconds=0raisesDBHealthProbeTimeout, the exception is not asqlite3.Error, and the same file returnsNone(healthy) with no deadlinetest_no_deadline_preserves_existing_behaviourNone, damaged still returns the reason string rather than raisingtest_doctor_reports_a_healthy_db_normallystate.db exists (N sessions)with no timeout warningChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the focused + adjacent files listed above per-file instead; a flatpytest tests/run is not reproducible locally (three runs, three different failure counts), whereasscripts/run_tests.shisolates per fileDocumentation & Housekeeping
docs/, docstrings) — docstrings on_db_opens_cleanly,DBHealthProbeTimeout,_ProbeDeadlineand_run_abandonablecli-config.yaml.exampleif I added/changed config keys — N/A, no config key added (deliberately)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Asqlite3.Connection.set_progress_handlerandDaemonThreadPoolExecutorare platform-independent; only tested on macOSContract Protected
Invariant: a health-probe deadline is never a corruption signal and never escalates to repair.
_db_opens_cleanlyis the sole input to doctor's "is this database damaged?" decision, and under--fixa non-Nonereturn callsrepair_state_db_schema(), which backs up and rewrites the file — its last strategy drops the entiremessages_fts%schema and VACUUMs. A cancelled probe knows nothing about the database, so it must be unrepresentable as a reason string.state.dbunderhermes doctor --fix.PRAGMA integrity_checkexceeds the deadline, the progress handler aborts it, SQLite raisesOperationalError("interrupted"). Because that is asqlite3.DatabaseError, the obvious implementation returns"interrupted"as the reason and the user's healthy database is rewritten. Reproduced locally against exactly that shape;repair_state_db_schemafired once on a healthy DB.DBHealthProbeTimeoutderives fromException, not fromsqlite3.Error. No present or futureexcept sqlite3.DatabaseError/except sqlite3.OperationalErrorhandler inside the probe can reclassify it as damage.test_progress_handler_cancels_a_real_integrity_checkassertsnot isinstance(exc, sqlite3.Error)so a later refactor cannot quietly move it under that hierarchy._ProbeDeadline.fired— set only when the handler actually returned non-zero — rather than string-matching"interrupted", so a genuinely interrupted statement arising from any other cause is still reported as a real reason and is not swallowed. It is also checked after the probe returns, so it holds no matter which of the probe's internal handlers caught the aborted statement first.test_no_deadline_preserves_existing_behaviourpins that with no deadline a damaged file still returns its reason string and never raises, sohermes sessions repair,session_recovery, and the five in-repair verification calls keep the exact contract they have today.test_probe_timeout_does_not_trigger_repair_under_fixrunsrun_doctor(fix=True)with a timing-out probe and assertsrepair_state_db_schemais called zero times.Related / Positioning
JonthanaHanh) — same line, same 30 s budget, superseded here: itswith ThreadPoolExecutor(...)boundary callsshutdown(wait=True)on exit and so does not bound the command (92.3 s vs 90 s of blocked probe, measured above), it has no test, and it leaves_write_reason = Noneon timeout, which reports a skipped check as a pass.Kyzcreig) — also edits_db_opens_cleanly, but a genuinely different defect: it reclassifies theexcept sqlite3.OperationalError"no such table" branch to distinguish a fresh mid-init file from a store whose FTS trigger outlived its virtual table. This PR deliberately stays out of that block — the deadline is installed in the connection prologue and checked after the probe returns — so the two are independently mergeable and compose correctly (if that new branch is itself interrupted, the post-probe deadline check still converts the result to a timeout rather than a verdict).