Skip to content

fix(kanban): set journal_mode=WAL once per process, not per connection - #35869

Closed
Macgrady4Ever wants to merge 2 commits into
NousResearch:mainfrom
Macgrady4Ever:fix/kanban-wal-once
Closed

fix(kanban): set journal_mode=WAL once per process, not per connection#35869
Macgrady4Ever wants to merge 2 commits into
NousResearch:mainfrom
Macgrady4Ever:fix/kanban-wal-once

Conversation

@Macgrady4Ever

Copy link
Copy Markdown

Bug: Per-connection PRAGMA journal_mode=WAL causes WAL/SHM desync and DB corruption

Root cause

kanban_db.connect() calls apply_wal_with_fallback() on every connection, which issues PRAGMA journal_mode=WAL each time. This is problematic for two reasons:

1. Exclusive lock on every connect.
SQLite takes an exclusive lock when PRAGMA journal_mode is evaluated, even when the mode is already WAL and no change is needed. With many concurrent connections — dispatcher workers plus a dashboard WebSocket that reconnects every few seconds — this creates a steady stream of exclusive-lock acquisitions under load.

2. DELETE fallback risk on steady-state connections.
apply_wal_with_fallback() contains a code path that falls back to DELETE journal mode when WAL appears unsupported (NFS/SMB/FUSE heuristic). If this fallback fires on a steady-state connection — one opening a DB that is already in WAL mode with live -wal/-shm sidecar files — it flips the persisted journal mode out from under other active connections. Combined with the sidecar files still being present on disk, this desyncs WAL coordination across all open handles and silently corrupts the database.

Incident (2026-05-27)

A real corruption event was triggered by the following sequence:

  1. A bulk-insert operation spawned 8+ gateway workers, each holding a kanban.db connection.
  2. The kanban dashboard event-stream WebSocket was simultaneously reconnecting every few seconds, opening a new connect() call each time.
  3. Each reconnect re-issued PRAGMA journal_mode=WAL via apply_wal_with_fallback.
  4. A transient condition triggered the DELETE fallback on one of those reconnects, flipping the live WAL database to DELETE mode while all 8 worker processes still held WAL handles.
  5. The -wal/-shm sidecar files became orphaned, WAL coordination broke down, and the database was corrupted.

Reproducer (conceptual)

import kanban_db as kb, threading, time

db = kb.kanban_db_path()
kb.connect(db_path=db).close()          # initial connect: WAL set, _WAL_DONE NOT cached

workers = [kb.connect(db_path=db) for _ in range(8)]   # workers hold connections

def dashboard_ws():
    for _ in range(20):
        c = kb.connect(db_path=db)      # re-issues PRAGMA journal_mode=WAL each time
        time.sleep(2)
        c.close()

threading.Thread(target=dashboard_ws, daemon=True).start()
# Under load: transient DELETE fallback fires on one reconnect
# → mode flips under 8 live WAL handles → -wal/-shm desynced → corruption

Fix

Add _WAL_DONE: set[str] and _ensure_wal_once():

_WAL_DONE: set[str] = set()

def _ensure_wal_once(conn, resolved, *, db_label):
    if resolved in _WAL_DONE:
        return                                 # fast path: no PRAGMA, no lock
    row = conn.execute("PRAGMA journal_mode").fetchone()
    if (row[0] if row else "").lower() != "wal":
        from hermes_state import apply_wal_with_fallback
        apply_wal_with_fallback(conn, db_label=db_label)
    _WAL_DONE.add(resolved)
  • First connection per process per path: read current journal_mode. If already WAL (persisted from a prior run) mark done immediately — no call to apply_wal_with_fallback. If not WAL (fresh or reset DB) call the fallback helper as before.
  • All subsequent connections: _WAL_DONE hit → immediate return. No PRAGMA, no exclusive lock, no fallback risk.
  • init_db() and remove_board() discard the path from _WAL_DONE so a rebuilt or re-created DB re-enters WAL correctly.

Relationship to _cross_process_init_lock

The _cross_process_init_lock added in recent upstream commits serialises first-connect WAL/schema setup across processes and addresses a separate multi-process init race. It does not eliminate repeated apply_wal_with_fallback calls on steady-state connections, because:

  • The file lock is acquired on every connect(), not just the first.
  • apply_wal_with_fallback sits outside the if needs_init: guard and runs unconditionally.

This patch is fully compatible and complementary: _ensure_wal_once is called inside _INIT_LOCK (the threading lock), so _WAL_DONE is updated atomically with respect to other threads in the same process.

Changes

hermes_cli/kanban_db.py

  • Add _WAL_DONE: set[str] module-level cache
  • Add _ensure_wal_once(conn, resolved, *, db_label) called from connect()
  • Replace direct apply_wal_with_fallback call in connect() with _ensure_wal_once
  • remove_board(): discard path from _WAL_DONE alongside _INITIALIZED_PATHS
  • init_db(): discard path from _WAL_DONE alongside _INITIALIZED_PATHS

tests/hermes_cli/test_kanban_db.py

  • test_wal_pragma_not_reissued_on_reconnectapply_wal_with_fallback must not fire on second connect() once _WAL_DONE is populated
  • test_wal_done_cleared_by_init_db — simulates a DB file reset (delete + recreate → DELETE mode); asserts init_db() evicts the stale _WAL_DONE entry so WAL is re-applied on the fresh file
  • test_wal_done_cleared_by_remove_boardremove_board() must evict _WAL_DONE so the path re-enters WAL when the board is re-created
  • test_connect_falls_back_to_delete_on_locking_protocol — also clears _WAL_DONE so the NFS-fallback path is still exercised

Test results

208 passed in 16.32s

(205 existing + 3 new)


🤖 Generated with Claude Code

Macgrady4Ever and others added 2 commits May 31, 2026 20:39
…n (v2)

journal_mode=WAL is a persistent, file-level property, so kanban_db.connect()
does not need to re-issue PRAGMA journal_mode=WAL on every connection. Doing so
takes an exclusive lock on every connect (the dashboard event-stream WebSocket
reopens a connection every few seconds), and the DELETE fallback in
apply_wal_with_fallback can flip the persisted mode out from under other live
connections. Combined with sidecar (-wal/-shm) files being removed under a live
DB, that re-toggling desynced WAL coordination across connections and corrupted
the kanban DB (incident 2026-05-27).

Add _WAL_DONE + _ensure_wal_once(): read journal_mode first and only apply WAL
when the DB is not already WAL, cached once per process per path. Per-connection
PRAGMAs (busy_timeout, synchronous, foreign_keys) still run on every connect.
init_db() and remove_board() discard _WAL_DONE so a rebuilt DB re-enters WAL.

Rebased onto upstream main (1fc7bdc). Upstream added _cross_process_init_lock
to serialize first-connect WAL/schema setup across processes but still calls
apply_wal_with_fallback on every connect. This patch adds an additional safety
layer: skip the fallback call entirely once WAL is confirmed for the process.

205 tests passed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add three tests for the WAL-once guard introduced alongside this fix:

* test_wal_pragma_not_reissued_on_reconnect — verifies apply_wal_with_fallback
  is NOT called on the second connect() once _WAL_DONE is populated.
* test_wal_done_cleared_by_init_db — simulates a DB file reset (delete +
  recreate empty → DELETE mode); asserts init_db() evicts the stale _WAL_DONE
  entry so WAL is re-applied on the fresh file.
* test_wal_done_cleared_by_remove_board — verifies remove_board() evicts
  _WAL_DONE so the path can re-enter WAL after the board is removed.

Also update test_connect_falls_back_to_delete_on_locking_protocol to clear
_WAL_DONE alongside _INITIALIZED_PATHS so the NFS-fallback code path is
still exercised.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels May 31, 2026
@Macgrady4Ever

Copy link
Copy Markdown
Author

@alt-glitch — thanks for triaging this. A heads-up on the type-checker picture, in case it factored into review.

This PR introduces zero new ty diagnostics. I verified with the repo's pinned toolchain (ty==0.0.21) on both the merge base and this branch:

  • main (merge base): 4 ty errors in hermes_cli/kanban_db.py
  • this branch: the same 4 — identical (path, rule, message), only shifted down by +41 lines because the WAL fix inserts code above them.

All 4 are pre-existing on main and live in functions this PR doesn't touch — _record_task_failure, _record_spawn_failure, detect_crashed_workers, and task_age. For what it's worth, the repo's own scripts/lint_diff.py keys diagnostics by (path, rule, message) and intentionally omits the line number, so its advisory diff already classifies these as unchanged, not new.

Blocking checks: ruff check . (PLW1514) passes on the changed files; the full kanban suite passes (208 = 205 existing + 3 new).

I've filed #36181 to track the 4 pre-existing ty errors separately, keeping this PR scoped to just the WAL fix per the "PR contains only changes related to this fix" checklist item. Happy to take that one on in a follow-up if it's wanted.

Let me know if you'd like anything adjusted here.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused WAL-safety investigation. This is already implemented on current main via a shared, more direct guard.

  • dc98314fbd4b8690fdeb07d6d73677c357f2a06d (fix(kanban): skip redundant WAL pragma on already-WAL connections) added a read-only PRAGMA journal_mode probe before any PRAGMA journal_mode=WAL set operation.
  • hermes_state.py:384-390 returns immediately when the database is already WAL; hermes_state.py:403-408 also refuses a DELETE fallback when the on-disk mode is WAL.
  • Both kanban connection paths use that helper at hermes_cli/kanban_db.py:1727-1733 and hermes_cli/kanban_db.py:1759-1761.
  • Current regression coverage at tests/test_hermes_state.py:4819-4850 verifies that an already-WAL connection never executes the set-WAL pragma.

Automated hermes-sweeper review.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants