fix(state): a busy write lock no longer loses the message that hit a corrupt search index - #120534
Merged
Merged
Conversation
૮ >ﻌ< ა ci reviewran on f0d730c — fix(state): stop a waiting FTS detach once the file is quara debug infoCI timingsCI timings · View report · View jobWall time 4m53s vs 9m13s (-47.0%). 5 job(s) slower, 6 faster, 1 unchanged.
|
arkheioncorp
left a comment
There was a problem hiding this comment.
APPROVE
Fixes a real problem: a busy write lock on a corrupt FTS index was causing the writer to give up after its 1-second busy timeout, losing the canonical write. Now the detach retry loop waits out the lock holder on the write budget (_WRITE_PATIENCE_S), so the canonical row lands instead of escaping as "database disk image is malformed".
What is solid:
- The new
deadline/patience_sparameters propagate from_execute_write, so the wait budget is the same one the writer already budgeted for its retry loop. Consistent. - The sibling-holding-the-lock test is well-constructed: a thread takes
BEGIN IMMEDIATEafter the corruption check, holds it past the 1s busy timeout, and the writer waits it out. The message lands,_fts_staleis True,_db_corruptis False. Good coverage of the exact scenario. - The
check_then_contendmonkeypatch is careful to only trigger the sibling on the first hit (guardif hit and not holder), avoiding duplicate threads. - The
pytest.skipfor SQLite builds that defer FTS shadow corruption past the insert trigger is the right call — this is a build-dependent behavior and the test should not fail on those builds.
One minor note:
- The test uses
threading.Timer(1.6, release.set)to release the sibling after 1.6s. With the writer's_WRITE_PATIENCE_Sdefault and the sibling holding for "10" seconds (therelease.wait(10)), this seems to rely on the Timer firing before the sibling's wait times out. The 1.6s is well within the writer's patience budget, but if_WRITE_PATIENCE_Swere shorter than 1.6s this test could flake. Confirm the default patience is comfortably above 1.6s, or make the Timer interval a function of the patience setting.
No security issues. The lock-wait is a correctness fix, not an exposure.
…rrupt FTS index When a canonical write trips a corrupt FTS index, SessionDB detaches the derived indexes (breadcrumb + trigger drop) and retries the write. The detach ran one BEGIN IMMEDIATE on the writer connection, whose busy timeout is only 1 s, and gave up on "database is locked" — so the canonical write escaped as "database disk image is malformed". The usual lock holder is a sibling writer (gateway + TUI) detaching the same index, so under load the second writer's turn was lost. The detach now waits out lock contention on the caller's write budget with the same jittered retry as _execute_write (default _WRITE_PATIENCE_S for the search fail-open callers). Repro: a second process takes BEGIN IMMEDIATE the instant the corruption error surfaces and holds it 2.5 s. Base: append raises after 1.02 s (3/3). Fixed: the row lands after the holder releases, FTS detached (3/3). Found by the E2E sqlite torture chamber (fts_corruption_fail_open) at load ~200.
The FTS fail-open detach now waits up to the caller's write budget (20 s / 60 s) for the write lock, so the one-time quarantine check before the loop left a long window: a sibling that quarantined the file meanwhile still got its triggers dropped and the stale breadcrumb committed on the quarantined handle. Re-check the handle flag and the process-wide storage latch at the top of every attempt, via the same _raise_if_db_corrupt(storage=True) that _execute_write runs per attempt. Classify the retryable lock error with is_sqlite_lock_error (result code first) instead of a locked/busy substring match, matching #120488.
teknium1
force-pushed
the
fix/fts-fail-open-lock-patience
branch
from
September 23, 2026 23:47
01d2550 to
f0d730c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A message written while the session search index is corrupt is no longer lost when another Hermes process (gateway + TUI, gateway + CLI) holds the state.db write lock at the moment the index gets detached.
Changes
hermes_state_fts.py::_enter_fts_fail_open: the detach (stale breadcrumb + FTS trigger drop) now waits outdatabase is locked/busywith the same jittered retry as_execute_write, on the caller's write budget. The default is_WRITE_PATIENCE_S, used by the search fail-open callers.hermes_state.py::_execute_writepasses its owndeadline/patience_sinto the detach, so a transcript write keeps its 60 s budget._raise_if_db_corrupt(storage=True), the same check_execute_writeruns per attempt). A sibling that quarantines the file while the detach waits for the lock now stops it: no trigger drop and no stale breadcrumb are committed on a quarantined handle, andStateDbCorruptErrorsurfaces.hermes_state_errors.is_sqlite_lock_error(result code first, text only as fallback), consistent with fix(state): SessionDB open waits out a DELETE-mode lock reported as 'vtable constructor failed' instead of failing #120488, instead of alocked/busysubstring match.session_searchfail-open call sites inhermes_state_search.py(_match_rowsand themessages_ftsMATCH arm of_search_messages_impl) pass no budget, so their detach now inherits the default_WRITE_PATIENCE_S(20 s) of write-lock patience instead of giving up after the 1 s busy timeout. A search that hits a corrupt index while a sibling holds the write lock can therefore block up to 20 s before falling back to canonical LIKE, rather than raising.tests/hermes_state/test_fts_index_fail_open.py: two new invariant tests, plus the existing_enter_fts_fail_openstub updated for the new keyword arguments.Root cause: the detach ran a single
BEGIN IMMEDIATEon the writer connection, whose busy timeout is only 1 s, and gave up ondatabase is locked. The canonical write then escaped asdatabase disk image is malformed. The usual lock holder is a sibling writer detaching the same corrupt index, so under load the second writer lost its turn.Validation
origin/main)BEGIN IMMEDIATEright as the corruption surfaces and holds it 2.5 sDatabaseError: fts5: corrupt structure recordafter 1.02 sfts_corruption_fail_open(#120171) with a 3 s pre-detach window and a 1.5 s detach hold injected via a scratch copy, WAL + DELETE arms, 2 runs eachCould not detach corrupt FTS indexes …: database is locked→DatabaseError('database disk image is malformed'), same as the union-run flaketest_detach_waits_out_a_sibling_holding_the_write_locktest_quarantine_while_detach_waits_commits_nothing(review follow-up)assert 0 > 0: triggers dropped on the quarantined file)tests/hermes_state/(133 files)test_hermes_state.pyhit the 300 s per-file cap under host load, then passed alone (275 passed). Its slowest test also takes ~15 s on base.tests/e2e/core/sqlite/(#120171 branch + this fix, no injection)Live repro: before, a sibling holding the lock for 2.5 s cost the write every time (3/3). After, the write lands every time (3/3), and the torture-chamber episode goes from 4/4 red to 4/4 green under the same injection.
Found while triaging the E2E sqlite torture-chamber flake on #120171. The follow-on
kill9_everything"roles still running" failure was a cascade from that episode leaving its gateway writer alive. Thetests/core-e2ehead already fixes that part: f601e22 stops a failed episode's stragglers in afinally.Infographic