fix(state): quarantine a state.db whose page 0 is not SQLite, not just a zeroed one
Startup only quarantined a 0-byte / all-NUL state.db. A file whose first page
was clobbered with record bytes (#102198) went straight to sqlite3.connect,
which raised "file is not a database" and deleted the -wal sidecar — the one
piece of evidence that could have been recovered.
`has_invalid_sqlite_header_preopen` generalises the zeroed probe (zeroed is a
subset of "no SQLite header"; same live-connection contract, never raises).
`quarantine_invalid_state_db` moves the file AND its -wal/-shm aside as
`state.db.<zeroed|notadb>-<ts>-<pid>.bak` before anything opens it; a fresh
DB is created as before.
Re-authored on current main (the quarantine helpers moved to
hermes_state_dbfile.py in d15c61b5dc); one regression test proves the
notadb case is quarantined with its sidecars and the new DB passes
integrity_check.
Refs #102198 (the write-after-SIGTERM that clobbers page 0 is not addressed
here; this preserves the evidence instead of destroying it).
This commit is contained in:
@@ -47,7 +47,8 @@ from hermes_state_schema import SessionSchemaMixin
|
||||
import hermes_state_holders as _state_holders
|
||||
from hermes_state_dbfile import (
|
||||
_canonical_sqlite_path, _connect_tracked_db, _read_sqlite_application_id, _stat_sqlite_sidecar_identity,
|
||||
_watched_sqlite_sidecar_paths, is_zeroed_state_db, quarantine_cross_process_lock, quarantine_zeroed_state_db,
|
||||
_watched_sqlite_sidecar_paths, has_invalid_sqlite_header_preopen, is_zeroed_state_db, quarantine_cross_process_lock,
|
||||
quarantine_invalid_state_db,
|
||||
refuse_deleted_wal_generation,
|
||||
)
|
||||
from hermes_state_messages import SessionMessagesMixin
|
||||
@@ -495,17 +496,17 @@ class SessionDB(
|
||||
try:
|
||||
# Serialize zero-byte check, quarantine, connect and schema commit so concurrent
|
||||
# openers don't race the absent-path -> schema-commit window.
|
||||
if not self.db_path.exists() or is_zeroed_state_db(self.db_path):
|
||||
if not self.db_path.exists() or has_invalid_sqlite_header_preopen(self.db_path):
|
||||
with quarantine_cross_process_lock(self.db_path) as lock_acquired:
|
||||
if not lock_acquired:
|
||||
logger.warning(
|
||||
"startup quarantine lock for %s not acquired within 5s; proceeding",
|
||||
self.db_path,
|
||||
)
|
||||
self._handle_quarantine_if_zeroed(already_locked=lock_acquired)
|
||||
self._handle_quarantine_if_invalid(already_locked=lock_acquired)
|
||||
self._connect_and_init_with_lock_patience()
|
||||
else:
|
||||
self._handle_quarantine_if_zeroed(already_locked=False)
|
||||
self._handle_quarantine_if_invalid(already_locked=False)
|
||||
self._connect_and_init_with_lock_patience()
|
||||
except sqlite3.DatabaseError as exc:
|
||||
# A malformed schema fails on the very first statement (before _init_schema), so the
|
||||
@@ -565,18 +566,18 @@ class SessionDB(
|
||||
conn.row_factory = sqlite3.Row
|
||||
return conn
|
||||
|
||||
def _handle_quarantine_if_zeroed(self, already_locked: bool = False) -> None:
|
||||
def _handle_quarantine_if_invalid(self, already_locked: bool = False) -> None:
|
||||
"""Quarantine a zero-byte/headerless state.db so a fresh one can open; if quarantine failed,
|
||||
raise the clear message instead of opening the zeroed file."""
|
||||
if not (self.db_path.exists() and is_zeroed_state_db(self.db_path)):
|
||||
if not (self.db_path.exists() and has_invalid_sqlite_header_preopen(self.db_path)):
|
||||
return
|
||||
try:
|
||||
zsize = self.db_path.stat().st_size
|
||||
except OSError:
|
||||
zsize = -1
|
||||
qpath = quarantine_zeroed_state_db(self.db_path, already_locked=already_locked)
|
||||
qpath = quarantine_invalid_state_db(self.db_path, already_locked=already_locked)
|
||||
msg = (
|
||||
f"state.db looks ZEROED ({zsize} bytes, no SQLite header). "
|
||||
f"state.db has no SQLite header ({zsize} bytes). "
|
||||
f"Preserved at {qpath or '(quarantine failed — file left in place)'}. "
|
||||
f"Restore from {self.db_path.parent / 'state-snapshots'} via `hermes snapshot list` / "
|
||||
f"`hermes snapshot restore <id>` if available. "
|
||||
@@ -584,7 +585,7 @@ class SessionDB(
|
||||
)
|
||||
logger.error(msg)
|
||||
_set_last_init_error(msg)
|
||||
if qpath is None and self.db_path.exists() and is_zeroed_state_db(self.db_path):
|
||||
if qpath is None and self.db_path.exists() and has_invalid_sqlite_header_preopen(self.db_path):
|
||||
raise sqlite3.DatabaseError(msg)
|
||||
|
||||
def _open_writer_conn(self) -> sqlite3.Connection:
|
||||
|
||||
@@ -194,6 +194,23 @@ def is_zeroed_state_db(path: Path, *, probe_bytes: int = 100, force: bool = Fals
|
||||
return head is not None and not head.startswith(b"SQLite format 3") and all(b == 0 for b in head)
|
||||
|
||||
|
||||
def has_invalid_sqlite_header_preopen(path: Path, *, probe_bytes: int = 100, force: bool = False) -> bool:
|
||||
"""Pre-open byte probe: a pre-existing state.db whose first page is not SQLite (0-byte, NUL, or
|
||||
clobbered page 0 as in #102198). Same live-connection contract as :func:`is_zeroed_state_db`;
|
||||
``force=True`` only for offline files. Never raises."""
|
||||
try:
|
||||
if not path.is_file():
|
||||
return False
|
||||
path.stat()
|
||||
from hermes_cli.sqlite_safe_read import has_live_connection, read_header_bytes_preopen
|
||||
if not force and has_live_connection(path):
|
||||
return False
|
||||
head = read_header_bytes_preopen(path, length=max(16, probe_bytes), force=force)
|
||||
except Exception:
|
||||
return False
|
||||
return head is not None and not head.startswith(b"SQLite format 3")
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def quarantine_cross_process_lock(path: Path, timeout: float = 5.0):
|
||||
"""Acquire the cross-process lock for path.quarantine.lock."""
|
||||
@@ -235,23 +252,24 @@ def quarantine_cross_process_lock(path: Path, timeout: float = 5.0):
|
||||
handle.close()
|
||||
|
||||
|
||||
def quarantine_zeroed_state_db(path: Path, *, already_locked: bool = False) -> Optional[Path]:
|
||||
"""Move a zeroed state.db aside (preserve bytes) and return quarantine path. A cross-process
|
||||
lock stops two concurrent startups racing: the second re-checks under the lock and finds the
|
||||
file gone (or fresh) instead of clobbering the quarantine."""
|
||||
def quarantine_invalid_state_db(path: Path, *, already_locked: bool = False) -> Optional[Path]:
|
||||
"""Move a non-SQLite state.db (zeroed or clobbered page 0) aside with its -wal/-shm sidecars,
|
||||
preserving bytes; return the quarantine path. A cross-process lock stops two concurrent
|
||||
startups racing: the second re-checks under the lock and finds the file gone (or fresh)."""
|
||||
def _do_quarantine():
|
||||
if not path.exists():
|
||||
logger.info("quarantine_zeroed_state_db: %s already moved by another process", path)
|
||||
logger.info("quarantine_invalid_state_db: %s already moved by another process", path)
|
||||
return None
|
||||
if not is_zeroed_state_db(path):
|
||||
logger.info("quarantine_zeroed_state_db: %s is no longer zeroed (another "
|
||||
if not has_invalid_sqlite_header_preopen(path):
|
||||
logger.info("quarantine_invalid_state_db: %s is no longer invalid (another "
|
||||
"process quarantined it and a fresh DB was created)", path)
|
||||
return None
|
||||
kind = "zeroed" if is_zeroed_state_db(path) else "notadb"
|
||||
try:
|
||||
ts = time.strftime("%Y%m%d-%H%M%S")
|
||||
except Exception:
|
||||
ts = "unknown"
|
||||
stem = f"{path.name}.zeroed-{ts}-{os.getpid()}"
|
||||
stem = f"{path.name}.{kind}-{ts}-{os.getpid()}"
|
||||
dest = path.with_name(f"{stem}.bak")
|
||||
n = 0
|
||||
while dest.exists():
|
||||
@@ -260,7 +278,7 @@ def quarantine_zeroed_state_db(path: Path, *, already_locked: bool = False) -> O
|
||||
try:
|
||||
path.rename(dest)
|
||||
except OSError as exc:
|
||||
logger.error("Failed to quarantine zeroed %s: %s", path, exc)
|
||||
logger.error("Failed to quarantine invalid %s: %s", path, exc)
|
||||
return None
|
||||
for suffix in ("-wal", "-shm"):
|
||||
side = Path(str(path) + suffix)
|
||||
@@ -274,7 +292,7 @@ def quarantine_zeroed_state_db(path: Path, *, already_locked: bool = False) -> O
|
||||
with quarantine_cross_process_lock(path) as acquired:
|
||||
if not acquired:
|
||||
logger.error("quarantine lock for %s not acquired within 5s — refusing to "
|
||||
"quarantine without the cross-process lock. The zeroed file "
|
||||
"quarantine without the cross-process lock. The invalid file "
|
||||
"is left in place. If sessions fail to load, restore from "
|
||||
"state-snapshots via `hermes snapshot list` / `hermes snapshot restore <id>`.",
|
||||
path)
|
||||
|
||||
@@ -14,7 +14,7 @@ def test_is_zeroed_state_db_and_quarantine(tmp_path):
|
||||
db.write_bytes(bytes(1024))
|
||||
assert hs.is_zeroed_state_db(db) is True
|
||||
|
||||
q = hs.quarantine_zeroed_state_db(db)
|
||||
q = hs.quarantine_invalid_state_db(db)
|
||||
assert q is not None
|
||||
assert q.exists()
|
||||
assert not db.exists()
|
||||
@@ -61,6 +61,41 @@ def test_sessiondb_opens_fresh_after_zeroed_quarantine(tmp_path, monkeypatch):
|
||||
sdb.close()
|
||||
|
||||
|
||||
def test_sessiondb_quarantines_page_zero_clobber_before_open(tmp_path, monkeypatch):
|
||||
"""#102198: preserve a non-SQLite page 0 instead of opening degraded."""
|
||||
import sqlite3
|
||||
|
||||
import hermes_state as hs
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
db = tmp_path / "state.db"
|
||||
clobbered = (b"bg_032237_0e4ce7A" * 256)[:4096]
|
||||
db.write_bytes(clobbered)
|
||||
(tmp_path / "state.db-wal").write_bytes(b"wal evidence")
|
||||
(tmp_path / "state.db-shm").write_bytes(b"shm evidence")
|
||||
|
||||
assert hs.has_invalid_sqlite_header_preopen(db) is True
|
||||
assert hs.is_zeroed_state_db(db) is False
|
||||
|
||||
sdb = hs.SessionDB(db_path=db)
|
||||
try:
|
||||
backups = list(tmp_path.glob("state.db.notadb-*.bak"))
|
||||
assert len(backups) == 1
|
||||
assert backups[0].read_bytes() == clobbered
|
||||
assert Path(str(backups[0]) + "-wal").read_bytes() == b"wal evidence"
|
||||
assert Path(str(backups[0]) + "-shm").read_bytes() == b"shm evidence"
|
||||
assert db.read_bytes().startswith(b"SQLite format 3\x00")
|
||||
sdb.create_session(session_id="after-quarantine", source="test")
|
||||
finally:
|
||||
sdb.close()
|
||||
|
||||
conn = sqlite3.connect(str(db))
|
||||
try:
|
||||
assert conn.execute("PRAGMA integrity_check").fetchone()[0] == "ok"
|
||||
finally:
|
||||
conn.close()
|
||||
|
||||
|
||||
def test_is_zeroed_state_db_zero_byte_quarantine(tmp_path, monkeypatch):
|
||||
"""#97568: a 0-byte file must be detected as zeroed and quarantined."""
|
||||
import hermes_state as hs
|
||||
@@ -92,7 +127,6 @@ def test_is_zeroed_state_db_zero_byte_quarantine(tmp_path, monkeypatch):
|
||||
sdb.close()
|
||||
|
||||
|
||||
|
||||
def test_concurrent_quarantine_no_clobber(tmp_path):
|
||||
"""#68805: two concurrent startups must not race on quarantine.
|
||||
|
||||
@@ -181,19 +215,23 @@ def test_quarantine_fails_closed_when_lock_held(tmp_path):
|
||||
try:
|
||||
if platform.system() == "Windows":
|
||||
import msvcrt
|
||||
|
||||
handle.seek(0)
|
||||
msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1)
|
||||
else:
|
||||
import fcntl
|
||||
|
||||
fcntl.flock(handle.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
|
||||
lock_held.set()
|
||||
release_lock.wait(timeout=15)
|
||||
if platform.system() == "Windows":
|
||||
import msvcrt
|
||||
|
||||
handle.seek(0)
|
||||
msvcrt.locking(handle.fileno(), msvcrt.LK_UNLCK, 1)
|
||||
else:
|
||||
import fcntl
|
||||
|
||||
fcntl.flock(handle.fileno(), fcntl.LOCK_UN)
|
||||
except OSError:
|
||||
lock_held.clear()
|
||||
@@ -207,11 +245,11 @@ def test_quarantine_fails_closed_when_lock_held(tmp_path):
|
||||
# Reduce the quarantine lock timeout to keep the test fast. We patch
|
||||
# the deadline by calling quarantine directly — it uses a 5s timeout,
|
||||
# but we only need to verify it returns None without moving the file.
|
||||
result = hs.quarantine_zeroed_state_db(db)
|
||||
result = hs.quarantine_invalid_state_db(db)
|
||||
|
||||
# Must fail closed: return None without moving the zeroed file
|
||||
assert result is None, (
|
||||
f"quarantine_zeroed_state_db returned {result} — expected None "
|
||||
f"quarantine_invalid_state_db returned {result} — expected None "
|
||||
f"(fail-closed when lock is held)"
|
||||
)
|
||||
assert db.exists(), "Zeroed state.db was moved despite lock being held"
|
||||
@@ -298,4 +336,3 @@ def test_live_connection_0_byte_not_quarantined_in_process(tmp_path, monkeypatch
|
||||
conn.commit()
|
||||
finally:
|
||||
conn.close()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user