fix(state): quarantine a corrupt/replaced handle out of rebuild_fts() too
optimize_fts() and vacuum() already refuse to run against a quarantined
handle (_db_corrupt / _db_replaced / _db_wal_generation_lost): both would
rewrite index/file pages in place, turning contained, diagnosable
corruption into an amplified one. rebuild_fts() never got the same guard,
despite being the more destructive of the two ("discards and recreates the
index data entirely", per its own docstring, vs. optimize_fts's segment
merge).
It's also independently reachable outside _execute_write's own quarantine
check: gateway/session_transcript.py's _rebuild_fts_once() calls
db.rebuild_fts() directly from the FTS-corruption transcript-retry path,
with no quarantine check of its own (only a WAL split-brain / foreign-holder
check, a different concern). A quarantined handle hitting that retry path
would run a full FTS rebuild — and commit it — on a corrupt, replaced, or
split-WAL-generation file.
Add the same self._raise_if_db_corrupt()/self._raise_if_db_replaced() pair
optimize_fts() already has, at the top of rebuild_fts(), before it enters
the cross-process rebuild admission.
This commit is contained in:
@@ -1216,7 +1216,8 @@ class SessionSearchMixin:
|
||||
|
||||
Uses the FTS5 ``'rebuild'`` command, which rewrites the internal b-tree segments from the content
|
||||
rows. Unlike ``optimize_fts`` (which merges existing segments), ``rebuild`` discards and recreates
|
||||
the index data entirely. See #50502.
|
||||
the index data entirely — the more destructive of the two, so it is quarantined the same way. See
|
||||
#50502.
|
||||
A full structural rebuild must never run concurrently in two processes sharing one state.db — that
|
||||
interleaving has structurally corrupted the database in production (PR #93200) — so this admits
|
||||
through the cross-process ``fts_rebuild_admission`` authority and FAILS CLOSED: if another process
|
||||
@@ -1225,6 +1226,8 @@ class SessionSearchMixin:
|
||||
path, which retries in-process from the gateway housekeeping tick (``retry_deferred_fts_recovery``)
|
||||
and at next startup.
|
||||
"""
|
||||
self._raise_if_db_corrupt()
|
||||
self._raise_if_db_replaced()
|
||||
rebuilt = 0
|
||||
with fts_rebuild_admission(self.db_path) as admitted:
|
||||
if not admitted:
|
||||
|
||||
@@ -307,3 +307,26 @@ class TestVacuumAndMaintenanceRespectQuarantine:
|
||||
_clear_flag(db, flag_name)
|
||||
db._conn = real_conn
|
||||
db.close()
|
||||
|
||||
@pytest.mark.parametrize("flag_name,expected_exc", _QUARANTINE_FLAGS)
|
||||
def test_rebuild_fts_refuses_when_quarantined(self, tmp_path, flag_name, expected_exc):
|
||||
"""rebuild_fts() is reachable outside _execute_write's own quarantine check — the gateway's
|
||||
FTS-corruption transcript-retry path (gateway/session_transcript.py::_rebuild_fts_once)
|
||||
calls it directly. Unlike optimize_fts ("merges existing segments"), rebuild_fts "discards
|
||||
and recreates the index data entirely" — strictly more destructive — so it must refuse at
|
||||
least as eagerly."""
|
||||
db = SessionDB(db_path=tmp_path / "state.db")
|
||||
real_conn = db._conn
|
||||
try:
|
||||
db.create_session(session_id="s1", source="cli", model="test")
|
||||
db.append_message("s1", role="user", content="hello world")
|
||||
recorder = _RecordingConn(real_conn)
|
||||
db._conn = recorder
|
||||
_force_flag(db, flag_name)
|
||||
with pytest.raises(expected_exc):
|
||||
db.rebuild_fts()
|
||||
assert recorder.recorded == []
|
||||
finally:
|
||||
_clear_flag(db, flag_name)
|
||||
db._conn = real_conn
|
||||
db.close()
|
||||
|
||||
Reference in New Issue
Block a user