fix(sqlite): move the non-daemon stores off WAL to rollback journalling (#356) - #977
Open
ziomik wants to merge 4 commits into
Open
fix(sqlite): move the non-daemon stores off WAL to rollback journalling (#356)#977ziomik wants to merge 4 commits into
ziomik wants to merge 4 commits into
Conversation
…nalling Port of fix/wal-orphan-rollback-journal (651f2f8 + 00550a4) onto the deployed tree, which still opens one shared connection per store instead of the upstream thread-local one — the pragma call site differs, the bug and the remedy do not. WAL + POSIX locks owned per (process, inode) let an external opener unlink the -wal: any connection close drops the whole process's locks, the next opener believes it is alone, checkpoints and unlinks, and the daemon keeps writing into an orphaned inode — writes visible only in-process and lost at exit. Observed in production 2026-08-01 on conversations, tasks, agent_comms and research; conversations_tasks.db-wal went orphan again on the restarted daemon (pid 998969) before this deploy.
…rnalling Follow-up to ba1bded, which converted the four stores caught by the 2026-08-01 orphaned-WAL incident (conversations, tasks, agent_comms, research). Every other daemon store had the same exposure: WAL plus several connections to one file inside one process, where the first close drops the whole process's POSIX locks and lets an outside opener checkpoint and unlink the -wal out from under the survivors. Converted via configure_rollback_journal(), which checkpoints any hot WAL first and raises rather than silently staying on WAL: activity, apps, audit (hooks), mesh, message_context, outreach_config, presentations, sessions (both SessionStore and SessionEventStore), triggers, user_profiles, voice_calls ...plus the stores that open a fresh connection per call — kb, librarian state, analytics and the plugin state/context DBs. Those are exposed too: two overlapping short connections in one process still share locks, so whichever closes first disarms the other. Notes: - message_context_store's explicit busy_timeout=5000 is dropped; the helper installs 30s, which is the right way round under rollback journalling (readers serialise against the writer). - analytics_store set the pragma inside its schema executescript; the call now runs on the init connection, before the script. TRUNCATE is not persisted in the header, so its later per-call connections report "delete" — still rollback, which is what matters. - Deliberately NOT ported: upstream's _reset_connection() healing. Closing a descriptor is precisely what drops the process's locks on that inode. - Out of scope, still on WAL: pinky_identity, pinky_hub, pinky_federation, pinky_memory. Tests: extends the existing rollback-mode parametrisation to the twelve long-lived stores, adds cover for the four per-call ones, and adds a source guard so a new daemon store cannot copy-paste its way back onto WAL. test_message_context_store's assertion updated (it pinned "wal"). 307 passed across the affected suites. Co-Authored-By: Claude Opus 5 <[email protected]>
…y hammers The thread-local hammer tests pinned `journal_mode == "wal"` on each connection they opened. Now that every long-lived store configures rollback journalling in its connection factory, they pin "truncate" — the same property, against the mode the stores actually run in.
Completes the sweep started in this branch. The orphaned-WAL failure mode documented in pinky_daemon.sqlite_journal is a property of how a process holds SQLite open, not of which package the store lives in, and these four modules have the same shape as the daemon stores: one long-lived check_same_thread=False connection kept open for the life of the process. Converted: - pinky_federation.state.FederationStateStore - pinky_hub.hub_store.HubStore - pinky_identity.bearer_tokens.BearerTokenStore - pinky_identity.signer_store.EncryptedSignerStore - pinky_memory.store.ReflectionStore — both the constructor and reopen(), which built a second connection and would have silently reverted to WAL They import the daemon's helper rather than growing copies of it; that is what the module was extracted for. pinky_daemon/__init__.py is a docstring and a version string, so this adds no import weight beyond sqlite3 itself. ReflectionStore's two explicit busy_timeout=5000 pragmas are dropped in favour of the helper's 30000, matching the standardisation in bradbrok#942. signer_store's security-notes docstring advertised WAL; it now points at sqlite_journal for why WAL is unsafe here. Tests: tests/pinky_federation/test_state.py pinned journal_mode == "wal"; converted, as 4b9bd7f did for the daemon stores. New tests/test_non_daemon_stores_no_wal.py pins all five stores plus reopen(), asserts no -wal/-shm sidecars appear, and covers in-place migration of a database left in WAL by an older build. Note for reviewers: the on-disk assertion is != "wal" rather than == "truncate". Only WAL is sticky in the database header; TRUNCATE is a per-connection setting, so an independent opener reports the "delete" default. Both are rollback modes with no -wal to unlink, which is the property that matters.
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
1 participant
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.
Closes the SQLite reliability thread opened on 2026-08-01.
b5fad85,87e2a60and77f0662converted the daemon stores; this converts the five that live outsidepinky_daemonand were explicitly left out of scope there.What changes
PRAGMA journal_mode=WAL→configure_rollback_journal()in:pinky_federation.state.FederationStateStorepinky_hub.hub_store.HubStorepinky_identity.bearer_tokens.BearerTokenStorepinky_identity.signer_store.EncryptedSignerStorepinky_memory.store.ReflectionStore— both the constructor andreopen(), which built a second connection and would have silently reverted that file to WALThese import the daemon's helper rather than growing four copies of it.
signer_store's security-notes docstring advertised WAL and now points atsqlite_journalinstead.ReflectionStore's two explicitbusy_timeout=5000pragmas are dropped for the helper's 30000, matching #942.Why this is not the low-priority cleanup it was filed as
Checked what is actually on disk in production right now. Four of the five stores have no database yet —
data/federation/state.db,data/hub.dband bothdata/identity/*.dbare absent; those subsystems aren't in use. The fifth is very much in use:All nine per-agent
memory.dbfiles are on WAL with live-wal/-shmsidecars — aiena, fixer, sentinel, engineer, satoshi, alter-ego, writer, seo-pro, segugio. Orphaned-walunder a running daemon is the exact signature of the 2026-08-01 incident, and this is the same database that needed a hot-patch on 2026-08-02 for a poisoned row. So in practice this PR is apinky_memoryfix that also future-proofs four dormant stores.Deploy note — please read before restarting prod
configure_rollback_journal()checkpoints any hot WAL and then raisesSqliteJournalConfigErrorrather than silently staying on WAL (sqlite_journal.py:85-100). That is the right behaviour, but it means the first restart after this deploy converts nine livememory.dbfiles in place.pinky_memoryruns as a per-agent MCP subprocess. If a subprocess from the old build still holds the WAL lock on a file the new one is converting, the checkpoint fails, the store raises, and that agent comes up with no memory instead of degrading quietly.Mitigation: make sure the old MCP subprocesses are fully gone before the new ones start — a clean stop/start, not an overlapping rolling restart.
Testing
tests/test_non_daemon_stores_no_wal.py(new) — pins all five stores plusreopen(), asserts no-wal/-shmsidecars appear, and covers in-place migration of a database left in WAL by an older buildtests/pinky_federation/test_state.pypinnedjournal_mode == "wal"; converted, as77f0662did for the daemon storestest_non_daemon_stores_no_wal,test_sqlite_journal,pinky_federation/test_state)4620 passed, 2 skippedin 41:36 — run from an isolated worktree, never from the live checkout (these stores default to relativedb_paths and would otherwise touch the production databases)One caveat on that full-suite number, stated plainly: it was produced on the original base, before this branch was rebased onto #968's head to keep it clean of unrelated local commits. On the current base I have only re-run the 71 targeted tests, which pass. The change itself is byte-identical across both bases (
git diffbetween them is empty), but the underlying base is not — it now includes #942 — so CI is the first full-suite run on this exact tree.The on-disk assertion is
!= "wal"rather than== "truncate"on purpose: only WAL is sticky in the database header, so an independent opener reports thedeletedefault. Both are rollback modes with no-walto unlink, which is the property that matters.Review note
pinky_identity,pinky_hub,pinky_federationandpinky_memorynow import frompinky_daemon.sqlite_journal, so the MCP server packages take a dependency on the daemon package.pinky_daemon/__init__.pyis a docstring and a version string, so this adds no import weight beyondsqlite3— but it is a new coupling direction and worth an explicit opinion from a reviewer rather than being buried in a commit message.🤖 Opened by Engineer