From 01232e8e217ad8e0c4499cb3063a409eb466d1ff Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 24 Jul 2026 19:28:37 -0700 Subject: [PATCH] fix(update): stop `hermes update` stalling for minutes on a large state.db MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The post-update state.db integrity guard called verify_sqlite_integrity() with max_bytes=0, which disables the size ceiling and forces a full PRAGMA integrity_check. That pragma walks every b-tree page in the file, so its cost scales with database size — measured on a real 30 GB state.db: 143.5s with a cold page cache (worse under an update's memory pressure), with zero output on screen. The update looks hung right after "✓ Code updated!" and a CPU sits pegged. Multi-GB session databases are normal for heavy users, so a size-unbounded check is never an acceptable default on the update path. - verify_sqlite_integrity(): max_bytes now defaults to DEFAULT_INTEGRITY_CHECK_MAX_BYTES (2 GiB) instead of 0. max_bytes=0 remains the explicit opt-in for a full scan. - The oversized path no longer degrades to a header-only check: it adds a constant-time structural probe (read-only open + schema_version + sqlite_master read) so the malformed-schema class is still caught, not just the #68474 zeroed-file signature. - Drop the explicit max_bytes=0 at the post-update guard and in copy_db_and_verify() so both inherit the bounded default. Measured on the reporter's real 30 GB state.db: 143.5s cold → 0.001s, still valid=True. Corruption detection verified at multi-GB scale for both classes (zeroed header, malformed schema) — both still fail closed. Tests: default-is-bounded invariant, oversized probe catches malformed schema, max_bytes=0 still forces the full check. --- hermes_cli/backup.py | 70 ++++++++++++++++++++----- hermes_cli/main.py | 1 - tests/hermes_cli/test_state_db_guard.py | 51 +++++++++++++++++- 3 files changed, 106 insertions(+), 16 deletions(-) diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index f9d1bc31d6d1..be6b034ac33d 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -289,29 +289,44 @@ def _safe_copy_db(src: Path, dst: Path) -> bool: _SQLITE_HEADER = b"SQLite format 3\0" +# Default ceiling above which ``PRAGMA integrity_check`` is skipped in favour +# of the (O(1)) header + structural probe. ``integrity_check`` walks every +# b-tree page in the file, so its cost scales with database size: on a 30 GB +# state.db it runs for many minutes of pegged CPU with no output, which reads +# to the user as a hung `hermes update` (#70553 follow-up). Sessions databases +# in the tens of GB are normal for heavy users, so the size-unbounded check is +# never an acceptable default on the update path. +DEFAULT_INTEGRITY_CHECK_MAX_BYTES = 2 << 30 # 2 GiB + def verify_sqlite_integrity( path: Path, *, check_header: bool = True, run_pragma: bool = True, - max_bytes: int = 0, + max_bytes: int = DEFAULT_INTEGRITY_CHECK_MAX_BYTES, ) -> dict: """Verify that a SQLite database at *path* is intact. Checks, in order: 1. File exists and has an expected minimum size. 2. SQLite header magic bytes are present. - 3. A read-only ``PRAGMA integrity_check`` execution passes. + 3. For files at or under ``max_bytes``, a read-only + ``PRAGMA integrity_check``. For larger files, a cheap structural + probe (schema read) instead — see ``max_bytes``. Args: path: Path to the database file. check_header: When true (default), verify the SQLite header magic. run_pragma: When true (default), run ``PRAGMA integrity_check`` via a read-only connection and verify the result is ``"ok"``. - max_bytes: When > 0, the file must be at most this many bytes. - Useful to catch a multi-GB DB before running ``PRAGMA integrity_check`` - on it (which reads the whole file into the pager). + max_bytes: Size ceiling for the full ``PRAGMA integrity_check``. + Files larger than this fall back to the header check plus a + cheap structural probe, because ``integrity_check`` pages + through the ENTIRE file — minutes of silent pegged CPU on a + multi-GB database. Defaults to + :data:`DEFAULT_INTEGRITY_CHECK_MAX_BYTES` (2 GiB); pass ``0`` + to force the full check regardless of size. Returns: A dict with keys: @@ -354,14 +369,38 @@ def verify_sqlite_integrity( return result if oversized: - # Too large to page through PRAGMA integrity_check; the header check - # above (which catches the #68474 zeroed signature) is the gate. - result["valid"] = True - result["message"] = ( - f"size {st.st_size:,} bytes exceeds max_bytes {max_bytes:,}; " - "skipped PRAGMA integrity_check (header check passed)" - ) + # Too large to page through PRAGMA integrity_check (which is O(file + # size) and would peg a CPU for minutes on a multi-GB state.db). + # Fall back to a cheap O(1) structural probe: the header check above + # catches the #68474 zeroed signature, and opening the DB read-only + # plus reading sqlite_master + the page geometry catches the + # malformed-schema and truncated-header-page classes. Both are + # constant-time — they parse the schema, they do not walk the data. run_pragma = False + probe = None + try: + probe = sqlite3.connect(f"file:{path}?mode=ro", uri=True, timeout=1.0) + probe.execute("PRAGMA schema_version").fetchone() + probe.execute("SELECT count(*) FROM sqlite_master").fetchone() + result["valid"] = True + result["message"] = ( + f"size {st.st_size:,} bytes exceeds max_bytes {max_bytes:,}; " + "skipped PRAGMA integrity_check (header + schema probe passed)" + ) + except sqlite3.DatabaseError as exc: + result["valid"] = False + result["message"] = f"schema probe failed: {exc}" + return result + except Exception as exc: + result["valid"] = False + result["message"] = f"schema probe error: {exc}" + return result + finally: + if probe is not None: + try: + probe.close() + except Exception: + pass if run_pragma: conn = None @@ -399,11 +438,14 @@ def copy_db_and_verify(src: Path, dst: Path) -> bool: """Like :func:`_safe_copy_db` but verifies the destination after copy. Returns True only when the copy succeeded AND the destination is valid - SQLite (header + integrity check). + SQLite (header + integrity check). Verification honours the default + size ceiling — a multi-GB destination gets the header + schema probe + rather than a full ``PRAGMA integrity_check`` that would page through + the whole file. """ if not _safe_copy_db(src, dst): return False - integrity = verify_sqlite_integrity(dst, run_pragma=True, max_bytes=0) + integrity = verify_sqlite_integrity(dst, run_pragma=True) if not integrity.get("valid"): try: dst.unlink(missing_ok=True) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index f9640d19f58e..722edd8b4edf 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -11726,7 +11726,6 @@ def _cmd_update_impl(args, gateway_mode: bool): _state_path, check_header=True, run_pragma=True, - max_bytes=0, ) if _state_ok.get("valid"): logger.debug( diff --git a/tests/hermes_cli/test_state_db_guard.py b/tests/hermes_cli/test_state_db_guard.py index 0d38085fa447..48251e6e2b07 100644 --- a/tests/hermes_cli/test_state_db_guard.py +++ b/tests/hermes_cli/test_state_db_guard.py @@ -66,8 +66,9 @@ def test_header_ok_but_garbage_body_fails_pragma(tmp_path): def test_oversized_db_skips_pragma_but_still_checks_header(valid_db): res = verify_sqlite_integrity(valid_db, max_bytes=1) - # Header intact → size-only pass; pragma skipped. + # Header intact + schema probe passes → pass without the full pragma. assert res["valid"] is True + assert "skipped PRAGMA integrity_check" in res["message"] size = valid_db.stat().st_size valid_db.write_bytes(b"\x00" * size) res = verify_sqlite_integrity(valid_db, max_bytes=1) @@ -75,6 +76,54 @@ def test_oversized_db_skips_pragma_but_still_checks_header(valid_db): assert res["valid"] is False +def test_default_max_bytes_bounds_the_pragma_by_size(): + """The default must NOT be size-unbounded. + + ``PRAGMA integrity_check`` walks every page in the file, so an unbounded + default made `hermes update` peg a CPU for minutes on a multi-GB + state.db with no output (read as a hang). Callers that omit max_bytes + must inherit a finite ceiling. + """ + import inspect + + from hermes_cli.backup import DEFAULT_INTEGRITY_CHECK_MAX_BYTES + + default = inspect.signature(verify_sqlite_integrity).parameters["max_bytes"].default + assert default == DEFAULT_INTEGRITY_CHECK_MAX_BYTES + assert default > 0, "size-unbounded integrity_check is never a safe default" + + +def test_oversized_db_probe_catches_malformed_schema(tmp_path): + """Skipping the pragma must not mean skipping corruption detection. + + A file with a valid header whose schema cannot be parsed has to fail + the oversized path via the cheap structural probe. + """ + path = tmp_path / "state.db" + conn = sqlite3.connect(path) + conn.execute("CREATE TABLE t (a INTEGER)") + conn.commit() + conn.close() + + raw = bytearray(path.read_bytes()) + # Corrupt the schema b-tree page (page 2 onward) while leaving the + # 16-byte header magic intact, so only the probe can catch it. + for i in range(100, min(len(raw), 4096)): + raw[i] = 0xFF + path.write_bytes(bytes(raw)) + + res = verify_sqlite_integrity(path, max_bytes=1) + assert res["valid"] is False + assert "probe" in res["message"] + + +def test_max_bytes_zero_forces_full_check(valid_db): + """``max_bytes=0`` remains the explicit opt-in for a full scan.""" + res = verify_sqlite_integrity(valid_db, max_bytes=0) + assert res["valid"] is True + assert "integrity check passed" in res["message"] + + def test_copy_db_and_verify_roundtrip(valid_db, tmp_path): dst = tmp_path / "snapshot" / "state.db" dst.parent.mkdir()