mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-25 17:18:11 +00:00
fix(update): stop hermes update stalling for minutes on a large state.db
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.
This commit is contained in:
parent
ca35663013
commit
01232e8e21
3 changed files with 106 additions and 16 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue