Jasmine Naderi
6048696ed0
Reviewer egilewski identified two recovery-loss paths: Path A — quarantine race (hermes_state.py): SessionDB checks state.db before quarantine_zeroed_state_db() without a shared cross-process lock, and Path.rename() may replace an existing destination. Two writable startups can therefore move the first instance's newly created database over the .zeroed-*.bak, erase the original damaged-file evidence, and replace the live database with another empty one. Fix: add a cross-process lock (msvcrt on Windows, fcntl on POSIX) around quarantine_zeroed_state_db() with a 5s bounded timeout. Under the lock: re-check is_zeroed_state_db (another process may have already quarantined it and created a fresh DB), use a PID-suffixed unique destination, and non-clobbering rename with counter fallback. Path B — size-cap pruning gap (hermes_cli/backup.py): _too_large() runs before failed-database tracking. With keep=1 and a size cap, an oversized state.db is omitted while failed_dbs stays empty, so automatic pruning deletes the older complete snapshot that may contain the only recoverable database. Fix: track oversized DB files in a new oversized_skipped list (both in the directory walk and top-level file loop). The manifest now records oversized_skipped. Pruning is suppressed when failed_dbs or oversized_skipped is non-empty, preserving the older complete snapshot as recovery source. Tests: - test_concurrent_quarantine_no_clobber: two threads racing on the same zeroed state.db — verifies quarantine backup survives with original bytes and live DB is valid. - test_oversized_db_suppresses_pruning: keep=1 + oversized state.db verifies the older complete snapshot is not pruned. All 33 tests pass (3 zeroed_state_db + 25 TestQuickSnapshot + 5 quarantine_forensic_logging).
104 lines
3.2 KiB
Python
104 lines
3.2 KiB
Python
"""#68474 hardening: zeroed state.db detection + quarantine."""
|
|
|
|
from __future__ import annotations
|
|
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
|
|
def test_is_zeroed_state_db_and_quarantine(tmp_path):
|
|
import hermes_state as hs
|
|
|
|
db = tmp_path / "state.db"
|
|
db.write_bytes(bytes(1024))
|
|
assert hs.is_zeroed_state_db(db) is True
|
|
|
|
q = hs.quarantine_zeroed_state_db(db)
|
|
assert q is not None
|
|
assert q.exists()
|
|
assert not db.exists()
|
|
assert q.read_bytes() == bytes(1024)
|
|
|
|
|
|
def test_sessiondb_opens_fresh_after_zeroed_quarantine(tmp_path, monkeypatch):
|
|
import hermes_state as hs
|
|
|
|
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
|
db = tmp_path / "state.db"
|
|
db.write_bytes(bytes(4096))
|
|
|
|
sdb = hs.SessionDB(db_path=db)
|
|
try:
|
|
# Fresh DB should open and accept schema
|
|
assert db.exists()
|
|
assert not hs.is_zeroed_state_db(db)
|
|
# Quarantine retained
|
|
backups = list(tmp_path.glob("state.db.zeroed-*.bak"))
|
|
assert len(backups) == 1
|
|
assert backups[0].stat().st_size == 4096
|
|
finally:
|
|
sdb.close()
|
|
|
|
|
|
def test_concurrent_quarantine_no_clobber(tmp_path):
|
|
"""#68805: two concurrent startups must not race on quarantine.
|
|
|
|
Without the cross-process lock, the second process could move its
|
|
newly-created empty DB over the first process's quarantine backup,
|
|
erasing the original damaged-file evidence. With the lock, the
|
|
second process re-checks under the lock, finds the file no longer
|
|
zeroed (or gone), and returns without clobbering.
|
|
"""
|
|
import hermes_state as hs
|
|
import threading
|
|
import sqlite3
|
|
|
|
db = tmp_path / "state.db"
|
|
db.write_bytes(bytes(4096)) # zeroed (all-NUL) 4 KB file
|
|
|
|
results: list = [None, None]
|
|
errors: list = [None, None]
|
|
|
|
def worker(idx):
|
|
try:
|
|
# Each worker opens its own SessionDB on the same path.
|
|
# The first one quarantines the zeroed file and creates a
|
|
# fresh DB. The second one should find a valid DB (or no
|
|
# file) under the lock and NOT clobber the quarantine.
|
|
sdb = hs.SessionDB(db_path=db)
|
|
try:
|
|
results[idx] = "ok"
|
|
finally:
|
|
sdb.close()
|
|
except Exception as exc:
|
|
errors[idx] = exc
|
|
|
|
t1 = threading.Thread(target=worker, args=(0,))
|
|
t2 = threading.Thread(target=worker, args=(1,))
|
|
t1.start()
|
|
t2.start()
|
|
t1.join(timeout=10)
|
|
t2.join(timeout=10)
|
|
|
|
# Both workers should complete without error
|
|
assert errors[0] is None, f"Worker 0 raised: {errors[0]}"
|
|
assert errors[1] is None, f"Worker 1 raised: {errors[1]}"
|
|
|
|
# The quarantine backup must survive — exactly one .bak file with
|
|
# the original 4096 zeroed bytes.
|
|
backups = list(tmp_path.glob("state.db.zeroed-*.bak"))
|
|
assert len(backups) >= 1, "At least one quarantine backup must exist"
|
|
for bak in backups:
|
|
assert bak.stat().st_size == 4096, (
|
|
f"Quarantine backup {bak} was clobbered: "
|
|
f"expected 4096 bytes, got {bak.stat().st_size}"
|
|
)
|
|
|
|
# The live state.db must be a valid (non-zeroed) SQLite database
|
|
assert db.exists()
|
|
assert not hs.is_zeroed_state_db(db)
|
|
conn = sqlite3.connect(str(db))
|
|
conn.execute("SELECT 1")
|
|
conn.close()
|