diff --git a/hermes_state.py b/hermes_state.py index 4cf28502c2..ba5d8fce6a 100644 --- a/hermes_state.py +++ b/hermes_state.py @@ -933,13 +933,31 @@ class SessionDB( token = uuid.uuid4().hex try: with self._lock: - self._conn.execute( - "INSERT OR IGNORE INTO state_meta (key, value) VALUES (?, ?)", - (_STATE_DB_GENERATION_KEY, token), - ) + # Read before writing. The stamp is minted once per FILE, so + # every open after the first one used to issue an + # INSERT OR IGNORE that could not insert anything — and a + # write statement takes the database write lock even when it + # changes nothing. With a sibling process holding that lock + # the no-op INSERT blocks for the full busy timeout (~1s at + # the timeout=1.0 this connection is opened with) and the + # whole block is then abandoned by the handler below, so the + # stamp was LOST precisely when contention made it slowest. + # state_meta is keyed by TEXT PRIMARY KEY, so the probe is a + # primary-key seek. First opener still wins via + # INSERT OR IGNORE, and racers still converge on the winner's + # token through the re-read. row = self._conn.execute( "SELECT value FROM state_meta WHERE key = ?", (_STATE_DB_GENERATION_KEY,), ).fetchone() + if not (row and row[0]): + self._conn.execute( + "INSERT OR IGNORE INTO state_meta (key, value) VALUES (?, ?)", + (_STATE_DB_GENERATION_KEY, token), + ) + row = self._conn.execute( + "SELECT value FROM state_meta WHERE key = ?", + (_STATE_DB_GENERATION_KEY,), + ).fetchone() if row and row[0]: token = str(row[0]) pragma_row = self._conn.execute("PRAGMA application_id").fetchone() diff --git a/hermes_state_schema.py b/hermes_state_schema.py index 65a4526bac..f9b78e05cd 100644 --- a/hermes_state_schema.py +++ b/hermes_state_schema.py @@ -889,8 +889,13 @@ class SessionSchemaMixin: # Heal NULL ``active`` rows on every startup: older reconciler builds added ``active`` # without NOT NULL DEFAULT 1, so ``WHERE active = 1`` loaders hid whole histories. A # ``current_version < 12`` gate never re-ran for already-v12+ databases. + # Read before writing: an UPDATE takes the write lock even when it matches no rows, so + # the unconditional form blocked every open behind a sibling's write transaction. The + # probe is short-circuited on the modern NOT NULL column and index-served on legacy + # ones. Deliberately not INDEXED BY (raises "no query solution" on NOT NULL columns). with contextlib.suppress(sqlite3.OperationalError): - cursor.execute("UPDATE messages SET active = 1 WHERE active IS NULL") + if cursor.execute("SELECT 1 FROM messages WHERE active IS NULL LIMIT 1").fetchone() is not None: + cursor.execute("UPDATE messages SET active = 1 WHERE active IS NULL") fts5_available = self._sqlite_supports_fts5(cursor) stale_row = cursor.execute("SELECT 1 FROM state_meta WHERE key = ? LIMIT 1", (FTS_STALE_KEY,)).fetchone() @@ -1012,7 +1017,13 @@ class SessionSchemaMixin: and not self._has_fts_trash(cursor) and not self._fts_external_index_empty_with_messages(cursor) ): - self.set_meta("fts_storage_version", str(FTS_STORAGE_VERSION), cursor=cursor) + # Stamp only when it would change something: on a settled DB every condition above + # already holds, and re-writing the same value takes the write lock on every open. + if cursor.execute( + "SELECT 1 FROM state_meta WHERE key = 'fts_storage_version' AND value = ? LIMIT 1", + (str(FTS_STORAGE_VERSION),), + ).fetchone() is None: + self.set_meta("fts_storage_version", str(FTS_STORAGE_VERSION), cursor=cursor) # Advance schema_version — deliberately NOT gated on the FTS opt-in (that would block # every future migration for a user who never optimizes). FTS5 unavailable is the diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index a3013deac8..5987d730fd 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -620,6 +620,77 @@ class TestMessageStorage: + def test_open_does_not_take_the_write_lock_when_nothing_needs_writing(self, tmp_path): + """A read-write open must not block on a sibling's write transaction. + + Opening used to issue two unconditional writes — the generation + stamp's ``INSERT OR IGNORE`` and the NULL-``active`` repair ``UPDATE`` + — and a write statement takes the database write lock even when it + changes nothing. With another process holding that lock, each one + blocked for a full busy timeout, so a plain open cost ~2s on a + database that needed no repair at all. Both are now gated on a read. + """ + db_path = tmp_path / "state.db" + # Open until the file has settled: the first opens legitimately mint + # the generation stamp and the FTS layout marker. From then on a + # healthy DB needs no writes at all to be opened. + SessionDB(db_path=db_path).close() + SessionDB(db_path=db_path).close() + + holder = sqlite3.connect(db_path, timeout=60) + holder.execute("PRAGMA busy_timeout=60000") + holder.execute("BEGIN IMMEDIATE") + holder.execute("UPDATE state_meta SET value = value WHERE key = 'nonexistent'") + try: + started = time.perf_counter() + session_db = SessionDB(db_path=db_path) + elapsed = time.perf_counter() - started + session_db.close() + finally: + holder.rollback() + holder.close() + + # One busy timeout is ~1s (the connection is opened with timeout=1.0); + # on the pre-fix code this took two of them. A generous bound keeps + # this from flaking on a loaded CI box while still failing loudly if a + # write creeps back onto the open path. + assert elapsed < 0.5, f"open blocked on the write lock for {elapsed:.3f}s" + + def test_generation_stamp_survives_write_lock_contention(self, tmp_path): + """The stamp must be readable even when the write lock is held. + + The stamp is minted once per file, so every later open only needs to + READ it. While the whole block was write-first, a held write lock made + it raise, the handler swallowed it, and the process ended up with no + generation token at all — losing the deleted-WAL/replaced-file guard + exactly when contention made it most likely to matter. + """ + db_path = tmp_path / "state.db" + SessionDB(db_path=db_path).close() # settle the FTS layout marker too + first = SessionDB(db_path=db_path) + try: + on_disk = first._conn.execute( + "SELECT value FROM state_meta WHERE key = ?", + (hermes_state._STATE_DB_GENERATION_KEY,), + ).fetchone()[0] + finally: + first.close() + assert on_disk + + holder = sqlite3.connect(db_path, timeout=60) + holder.execute("PRAGMA busy_timeout=60000") + holder.execute("BEGIN IMMEDIATE") + holder.execute("UPDATE state_meta SET value = value WHERE key = 'nonexistent'") + try: + session_db = SessionDB(db_path=db_path) + try: + assert session_db._db_file_generation_token == on_disk + finally: + session_db.close() + finally: + holder.rollback() + holder.close() + def test_startup_heals_null_active_rows(self, tmp_path): """Rows written as active=NULL before the fix are un-hidden on startup.