From ef8d682200ffd78cc153f15350036bfda620b6c9 Mon Sep 17 00:00:00 2001 From: John Paul Soliva Date: Thu, 3 Sep 2026 14:11:39 +0900 Subject: [PATCH] perf(state): stop taking the state.db write lock to open a database that needs no writes Every read-write SessionDB open issued three writes that usually change nothing, and a write statement takes the database write lock even when it matches no rows: - _ensure_db_file_generation's INSERT OR IGNORE into state_meta. The stamp is minted once per FILE, so every open after the first inserted nothing. - the NULL-`active` heal, UPDATE messages SET active = 1 WHERE active IS NULL, which matches nothing on a healthy database. - the fts_storage_version stamp, which re-wrote the same value on every open of an already-optimized database. The connection is opened with timeout=1.0, so each blocked write costs a full busy timeout while a sibling process holds the write lock, and the open path's patience loop can ultimately give up and raise. Gate all three on a read. Measured on a real 99 MB state.db (239 sessions, 6470 messages) with a sibling holding the write lock: 2117-2136 ms -> 3.6-7.1 ms. On an already-optimized database the unpatched open does not merely stall, it raises `database is locked`; patched it completes in 3.7-6.8 ms. With a sibling running 200 ms write transactions in a loop (n=20 opens): p50 894.6 -> 9.6 ms, p90 1094.7 -> 13.9 ms. A settled database now issues zero main-database write statements to open. The reads cost nothing measurable: the state_meta probe is a primary-key seek (2.0 us), the messages probe is 1.7 us on the modern NOT NULL column (unsatisfiable constraint, short-circuited) and 0.4-0.6 us at 300k rows on a legacy default-less column via the partial index that already exists for exactly this predicate. An uncontended open is unchanged. Semantics are preserved. INSERT OR IGNORE still resolves the first-opener race inside SQLite and racers still converge on the winner's token via the re-read; the application_id gate and the PASSIVE-only checkpoint are untouched; the heal is still considered on every startup, as #60108 deliberately made it, with only the write now conditional on a read proving there is something to repair. Read-first also fixes a correctness bug. Under contention the generation block was abandoned by its `except sqlite3.Error` handler, so a process ended up with no generation token at all even though the value was already on disk and a plain read would have returned it -- and that token feeds the deleted-WAL and replaced-file guards added by #101221. The heal's `except OperationalError: pass` likewise skipped the repair silently, so the unconditional form did not even deliver the unconditional repair it advertised whenever it mattered most. The probe deliberately does not use INDEXED BY: that hint raises OperationalError("no query solution") against the modern NOT NULL column, and the existing handler would swallow it, disabling the repair forever. --- hermes_state.py | 26 +++++++++++--- hermes_state_schema.py | 15 ++++++-- tests/test_hermes_state.py | 71 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 6 deletions(-) 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.