fix(kanban): probe integrity_check in read-only immutable mode to avoid WAL checkpoint race - #60653
fix(kanban): probe integrity_check in read-only immutable mode to avoid WAL checkpoint race#60653zhangtaibo wants to merge 1 commit into
Conversation
…id WAL checkpoint race
`_guard_existing_db_is_healthy` runs `PRAGMA integrity_check` on every
new process's first `connect()`. The probe previously opened the DB in
read/write mode, which under SQLite WAL mode opens the -wal/-shm
sidecars and may trigger a WAL checkpoint.
Under a worker stampede (nightly cron spawning N workers + web-ui
spawning `hermes kanban watch` children), many processes concurrently
run the probe — each triggering a checkpoint — while the gateway is
the active writer. The racing checkpoint vs. writer corrupts indexes
(`wrong # of entries in index idx_events_*`), recurring nightly at
cron-burst moments.
Fix: open the probe via `file:{path}?immutable=1` URI. SQLite skips
WAL/SHM entirely — no sidecar files opened, no locks taken, no
checkpoint triggered. Structural integrity is fully verifiable from
the main DB file alone (the post-checkpoint stable state); WAL
contents are data, not structure, and any deferred WAL-resident
corruption surfaces on the next checkpoint by the gateway (the
single writer) rather than racing it.
Verified via strace: r/w probe opens kanban.db + kanban.db-wal +
kanban.db-shm (3 files, the latter two O_RDWR|O_CREAT); readonly
probe opens only kanban.db (1 file, no write/fsync/fdatasync).
10 concurrent probes complete cleanly with wal/shm untouched;
intentionally corrupted DBs are still detected
(`sqlite refused to open file: database disk image is malformed`).
Complements NousResearch#53819's writer-serialization proposal (different race:
multi-writer contention vs. probe-checkpoint-vs-writer). Targets the
recurring symptom reported in NousResearch#34385.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Two concerns from testing this on a live deployment (WAL, multi-process worker fleet), plus a data point. 1. The probe race this PR targets may not exist: probes are already serialized by
2. Immutable mode takes no read locks and ignores the WAL entirely. But Alternative that keeps the PR's goal (probe never writes the live DB/sidecars) without the torn-read hazard: open Data point on the symptom this PR cites: we hit the recurring-corruption + backup-cascade shape too (251 |
|
Follow-up to my earlier comment — after running the snapshot-probe variant in production for a day, I think both that variant and this PR are patching the symptom at the wrong level. The deeper issue: integrity checking is a global, periodic concern implemented as a per-process startup ritual. Every process that ever touches a board — dispatcher, each spawned worker, every CLI invocation, each dashboard — runs a full-DB
Where the pieces belong:
We're running (2) + the snapshot probe as a local patch now — happy to PR it if maintainers prefer that shape over |
|
Correction to my first comment, from completed forensics on our incident: I claimed the first-connect probes are strictly serialized by The torn-read objection to
Notably: no OOM kills, no process crashes, no gateway restarts in the onset window — but the host was observably stalled (trivial SQLite count queries exceeded 10s) with the gateway service cgroup (daemon + its spawned worker subprocesses and their build toolchains — the daemon itself idles at ~60MB) at 13.5G RSS + 11.8G swap in a 16G WSL2 VM. The initial WAL-page producer is undecidable from our evidence because of a capturable gap worth fixing regardless of probe mode: Concrete suggestions that fall out, orthogonal to probe mode:
|
|
Follow-up: the architectural form of the feedback above is now a standalone proposal — #62009 (maintenance operations owner-only, data reads/writes unchanged). It narrows this PR's question rather than competing with it: with a single owner probing on a schedule, whichever probe mode this PR settles on runs in one process instead of every connecting process. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing the first-connect probe path. The contention premise remains on current main: _cross_process_init_lock deliberately proceeds without its cross-process lock after its bounded timeout (hermes_cli/kanban_db.py:1288-1292, 1387-1394), and connect() then calls the full guard (hermes_cli/kanban_db.py:1738-1745).
Problems
immutable=1is unsafe for this live board. The proposed URI athermes_cli/kanban_db.py:1354tells SQLite to skip locking and change detection; SQLite documents that if such a file changes, reads can return incorrect results orSQLITE_CORRUPT(SQLite URI documentation). Normal connections continue to enable WAL and write the board (hermes_cli/kanban_db.py:1722-1731,1758-1770).- The guard currently intentionally opens read/write so SQLite can recover/checkpoint a healthy WAL or hot journal before declaring corruption (
hermes_cli/kanban_db.py:1623-1635). Replacing it with a probe that intentionally excludes WAL/SHM can miss a WAL-resident integrity failure. - The PR changes only production code; it adds no concurrent-writer/WAL probe regression test.
Suggested changes
- Keep a WAL-consistent probe rather than
immutable=1, and add a real multiprocess writer/probe regression. - Consider the owner-only maintenance direction in #62009 so the bounded-lock fall-through cannot create a fleet of independent probes and quarantines.
Automated hermes-sweeper review.
| immutable mode falls back to locking. | ||
| """ | ||
| busy_timeout_ms = _resolve_busy_timeout_ms() | ||
| uri = f"file:{path}?immutable=1" |
There was a problem hiding this comment.
immutable=1 asserts this file cannot change and disables SQLite locking/change detection, but this guard runs against an actively written WAL board. SQLite documents incorrect results or SQLITE_CORRUPT if an immutable-marked file changes. Please do not use this mode for the live integrity decision; preserve a WAL-consistent view and cover it with a concurrent writer/probe regression.
|
Closing: immutable=1 on a live-WAL database is documented-unsafe (reads can return SQLITE_CORRUPT or stale data). The underlying idea — don't checkpoint from the probe — is right and is handled differently now that #68654 moved checkpointing to the dispatcher tick. A mode=ro (non-immutable) probe PR would be welcome. |
Summary
_guard_existing_db_is_healthyrunsPRAGMA integrity_checkon every new process's firstconnect(). Previously the probe opened the DB in read/write mode, which under SQLite WAL mode opens the-wal/-shmsidecars and can trigger a WAL checkpoint. Under a worker stampede this races the gateway (the active writer) and corrupts indexes.This PR switches the probe to read-only
immutable=1URI mode so SQLite skips WAL/SHM entirely — no sidecars opened, no locks taken, no checkpoint triggered.Root cause
Recurring
wrong # of entries in index idx_events_*corruption, nightly at cron-burst moments (02:00). Pattern observed across ~11 incidents:hermes kanban watchchildren._guard_existing_db_is_healthyon firstconnect()(_INITIALIZED_PATHSis per-process, so the cache does not help across processes).-wal/-shm(O_RDWR|O_CREAT) → may trigger a WAL checkpoint.Fix
Add
_sqlite_connect_readonly(path)that opens viafile:{path}?immutable=1URI. SQLite treats the file as immutable: it does not open-wal/-shm, does not take a WAL lock, does not checkpoint._guard_existing_db_is_healthynow uses the readonly probe. Structural integrity is fully verifiable from the main DB file alone (the post-checkpoint stable state). WAL contents are data, not structure; any deferred WAL-resident corruption surfaces on the next checkpoint by the gateway (the single writer) rather than racing it.Verification (strace)
kanban.db+kanban.db-wal+kanban.db-shm(latter twoO_RDWR|O_CREAT)fsync/fdatasyncon WALkanban.dbonly-wal/-shmnever created.sqlite refused to open file: database disk image is malformed.connect() → query → writeflow unchanged (writes still go through the normal r/w_sqlite_connect).Relationship to existing issues
fcntl.flockon writes) — that targets a different race (multi-writer contention). Both can coexist; this PR is the smaller, more surgical fix for the probe-triggered races we observe nightly.Test plan
python3 -c "import ast; ast.parse(open('hermes_cli/kanban_db.py').read())"— syntax OKwrite/fsync/fdatasyncsyscallsO_RDWR|O_CREATon-wal/-shmKanbanDbCorruptErrorwith backupok/malformedeither way)🤖 Generated with Claude Code