Skip to content

fix(sqlite): move every long-lived daemon store off WAL to rollback journalling - #968

Open
ziomik wants to merge 3 commits into
bradbrok:mainfrom
ziomik:fix/wal-orphan-rollback-journal
Open

fix(sqlite): move every long-lived daemon store off WAL to rollback journalling#968
ziomik wants to merge 3 commits into
bradbrok:mainfrom
ziomik:fix/wal-orphan-rollback-journal

Conversation

@ziomik

@ziomik ziomik commented Aug 1, 2026

Copy link
Copy Markdown

Why

Production incident on 2026-08-01: four daemon stores (conversations, tasks, agent_comms, research) were found holding …-wal (deleted), with 54 messages, 5 inbox rows and 2 tasks reachable only from the daemon's address space. /proc/<pid>/locks showed the daemon holding no locks at all.

The failure mode is structural, not a one-off:

  1. Every store opens the same database file once per thread (self._thread_local.connection), so one process holds several independent connections to one inode.
  2. POSIX record locks are owned per (process, inode), not per descriptor. The moment any of those connections closes — a worker thread ending, an explicit _reset_connection() — the process drops all of its advisory locks on that file, including the ones the surviving connections still rely on.
  3. The daemon is then invisible to other processes. The next external opener (a backup, an operator running sqlite3, a script) sees an unlocked database, believes it is the sole connection, and on close checkpoints and unlinks the -wal/-shm.
  4. The daemon's surviving connections keep writing into the now-orphaned inode. Those writes are visible only through the daemon; every external reader sees data frozen at the last checkpoint, and the whole tail is lost when the process exits.

Note that #956's _reset_connection() healing closes a connection — i.e. it is itself a trigger for step 2. That pattern is untouched here; under rollback journalling there is no -wal for anyone to unlink, so the trigger no longer has a payload.

What

New module src/pinky_daemon/sqlite_journal.py with configure_rollback_journal(conn, db_label=…):

It is called from the thread-local connection factory of every long-lived daemon store, so each per-thread connection is configured, not just the first:

activity, agent_comms, apps, audit (hooks.AuditStore), conversations, dreams, mesh, message_context, outreach_config, presentations, research, sessions (×2), skills, tasks, triggers, user_profiles, voice_calls — plus the per-call connections in kb_store, librarian_runner, analytics_store and plugin_manager (×2).

This converges the hand-rolled copies already living in conversations_agents.db (#797/#220) on one implementation instead of letting them drift.

dreams.db and skills.db are included: the module docstring notes them as already-remedied, but the thread-local refactor put both back on PRAGMA journal_mode=WAL in their connection factory. This restores them.

Trade-off, deliberately accepted: rollback mode serialises readers against the writer. These are low-throughput control-plane stores; correctness of the write path matters more here than reader concurrency.

Out of scope

Still on WAL, untouched by this PR: pinky_identity (2 sites), pinky_hub, pinky_federation, pinky_memory (2 sites). Worth a follow-up.

Tests

  • New tests/test_sqlite_journal.py: mode conversion, hot-WAL drain, retry/fail-loud behaviour, and a subprocess test pinning that the FTS5 shadow tables in conversations.db survive the checkpoint on a genuinely hot WAL (not just on the reasoning).
  • The existing per-store concurrency hammers now assert truncate instead of wal on each thread-local connection.

🤖 Opened by Engineer

Engineer and others added 3 commits August 1, 2026 11:31
…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 <noreply@anthropic.com>
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant