fix(kanban): stop corruption amplification + spurious quarantines in the DB health guard

[jamesraddock]

What does this PR do?

Hardens the Kanban SQLite health guard against three failure modes we hit in production (WSL2, multi-worker dispatch). The three commits build on each other and ship together because the first one alone is actively harmful (details below).

1. b4a98f74-equivalent — TTL re-probe (stops the corruption amplification loop)

_guard_existing_db_is_healthy() runs PRAGMA integrity_check on connect, but skipped it for the entire process lifetime once a path was in _INITIALIZED_PATHS. That set conflates "schema is migrated" (genuinely once-per-process) with "the DB is healthy" (can change after first connect). When the file was later torn (e.g. an interrupted WAL checkpoint), a long-lived writer that first connected while healthy never re-checked — it kept checkpointing the damaged file, compounding corruption on every cycle. Observed: one torn write snowballed into ~20 quarantine copies (~150 MB) in ~10 minutes with the board hard-down (HTTP 409) throughout.

Fix: decouple health from schema-init; re-probe on a 30s TTL so writers notice post-init corruption within one window.

2. Confirm across N probes before quarantining (stops spurious quarantines)

With the TTL re-probe in place, long-lived processes probe far more often — and a read/write integrity_check under concurrent writers on a weak-durability FS (WSL2 virtual disk, no DPO/FUA) can transiently read a mid-checkpoint page as bad while the DB is fine. Observed: ~45 spurious .corrupt.bak copies of a healthy, progressing board in 20 minutes. This is why the TTL commit must not ship alone.

Fix: require _HEALTH_CONFIRM_ATTEMPTS = 3 consecutive non-ok probes (100 ms backoff) before quarantining. Real corruption reproduces on every probe; a transient clears on retry. The healthy path still costs exactly one probe.

3. Classify all-IOERR integrity failures as transient I/O, not corruption

Under sustained load the probe⇄checkpoint race can outlive the whole confirmation loop. The tell: every failure line is unable to get the page. error code=522 (SQLITE_IOERR_SHORT_READ) — the read failed, not the content is malformed. Observed: 3 quarantines of an integrity_check = ok board in 90 seconds, with the confirm loop active.

Fix: when every confirming probe fails and the report contains only IOERR-family lines (error code & 0xFF == 10, plus the Page N: never used accounting noise from unvisited pages), raise sqlite3.OperationalError — same contract as the existing lock/busy path: no quarantine copy, no fail-closed. Any content-damage line (is malformed, missing from index, …), any non-IOERR code, or any unexpected report shape still quarantines and fails closed.

Related Issue

Fixes #

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• hermes_cli/kanban_db.py
  • _LAST_HEALTH_OK TTL cache (30s) replaces lifetime health caching; stamp evicted on detected damage so subsequent connects keep re-probing
  • _run_integrity_probe() extracted; OperationalError (lock/busy) propagates raw as before — never classified as corruption
  • _HEALTH_CONFIRM_ATTEMPTS = 3 confirmation loop with backoff before any quarantine
  • _integrity_failure_is_transient_io() — strict classifier for all-IOERR reports → OperationalError instead of quarantine
• tests/hermes_cli/test_kanban_db.py — 5 regression tests covering: TTL honored then re-probed, lone transient probe failure ignored, persistent corruption still quarantined, persistent SHORT_READ raises OperationalError with no quarantine, and classifier edge cases (mixed damage+IOERR, non-IOERR codes, refused-open). Full file: 219 passed.

Testing

python -m pytest tests/hermes_cli/test_kanban_db.py
219 passed

Also soak-tested on the live multi-worker board that originally reproduced all three failure modes: zero spurious quarantines since, real-corruption path verified by injected malformed file.

[liuhao1024]

Code Review: Positive Verification

Reviewed: hermes_cli/kanban_db.py — health check TTL to prevent corruption amplification.

Findings: Well-designed fix for a real corruption amplification bug. The previous one-shot lifetime cache in _INITIALIZED_PATHS meant a long-lived writer that connected while healthy would never re-check, silently amplifying corruption from interrupted checkpoints.

Design is sound:

• _LAST_HEALTH_OK dict with 30s TTL bounds the full integrity probe window
• Cheap SQLite header check still runs on every connect (not affected by TTL)
• Cache is invalidated on remove_board, init_db, and corruption detection
• _backup_corrupt_db still runs on first corruption detection

Test coverage: test_health_guard_honors_ttl_cache_then_reprobes covers the full lifecycle — healthy probe → TTL cache → post-corruption re-probe within window → TTL expiry → fail-closed on re-probe. monkeypatch on time.monotonic avoids flaky timing.

No issues found. LGTM.

[jamesraddock]

Pushed two follow-up commits after further production soak (multi-worker dispatch on WSL2):

• e2d0a6d2 — require 3 consecutive non-ok probes before quarantining. The TTL re-probe alone increased probe frequency in long-lived processes, and a single r/w integrity_check under concurrent writers can transiently misread a mid-checkpoint page — we measured ~45 spurious .corrupt.bak copies of a healthy board in 20 min. Reviewers should consider the original TTL commit incomplete without this one.
• 88cd0fcf — classify all-IOERR integrity reports (unable to get the page. error code=522 = SQLITE_IOERR_SHORT_READ) as transient I/O → raise OperationalError like the lock/busy path instead of quarantining. The probe⇄checkpoint race can outlive the confirmation loop under sustained load; a failed read is not malformed content. Genuine damage reports ("is malformed", "missing from index", non-IOERR codes) still quarantine + fail closed.

PR description updated to cover the full stack; 219/219 tests pass.

[jamesraddock]

One more layer found in production soak, pushed as ceeb58a6: under checkpoint contention the r/w probe can also fail with a hard SQLITE_CORRUPT (database disk image is malformed) on open/first-read that clears on the next read — identical string to real damage, so it can't be pattern-matched as transient like the IOERR case. The r/w probe participates in WAL recovery; a read-only connection doesn't, and has never false-positived in our monitoring. So when the confirmation loop exhausts, a mode=ro PRAGMA integrity_check now gets the last word: ro-ok → OperationalError (no quarantine), ro-confirms-damage → quarantine + fail closed exactly as before. Lock/busy during the ro probe counts as undecided, never as evidence of corruption. 221/221 tests.

[jamesraddock]

bb99b1f1 — production falsified my previous claim: the read-only arbiter false-positived too (the quarantined copy itself later checked out integrity_check=ok). It samples at the hottest moment by construction — right after every r/w probe failed — and even ro opens can transiently report SQLITE_CORRUPT there.

The discriminator that has held across every observed failure shape is persistence: real corruption is permanent; probe⇄checkpoint contention clears within seconds. The decision window is now multi-second instead of ~300ms: 4 r/w attempts with exponential backoff (~2s), then the arbiter's damage verdict must repeat across 3 ro attempts spaced 1s apart — one clean/undecided read anywhere means transient (OperationalError, no quarantine, warning logged for visibility). Healthy DBs still cost exactly one probe; only the suspected-corrupt path sleeps.

Heads-up for reviewers: the corrupt-file tests now pay the real decision window, so test_kanban_db.py runs ~80s instead of ~25s. 222/222 pass.

[jamesraddock]

Pushed two more commits after another production soak on the WSL2 host — these invert the probe design rather than adding a fifth countermeasure:

8466a2f71 — probe read-only-first; the read/write probe is gone.
Every false-positive shape this PR has chased (single torn read, IOERR storm, hard SQLITE_CORRUPT at open, hottest-moment arbiter flicker) traced back to one cause: the scheduled probe opened the DB read/write, which participates in WAL recovery/checkpointing and therefore races every concurrent writer. Meanwhile a read-only probe at a random moment (which is exactly what a TTL expiry is) produced zero false positives across the same period — months of 5-minute cron sampling plus all manual checks.

So the guard now runs ONE read-only probe at TTL expiry with a tri-state verdict:

• ok → stamp, return (one cheap probe on the healthy path, same as before);
• undecided (lock/busy, a hot WAL a ro connection can't recover, an all-IOERR report) → fail open, no stamp: callers no longer see spurious OperationalError, the next connect re-probes, and a genuinely bad DB still surfaces through cell_size_check=ON + the write_txn page-count invariant;
• damage → must repeat on every spaced probe across a multi-second window before quarantining (real corruption is permanent; contention clears in seconds).

This subsumes the confirm-N loop and the separate ro arbiter (both deleted) while keeping the TTL re-probe that stops corruption amplification and the IOERR-family classifier (now feeding the undecided verdict). Net: −226/+187 lines, and the corrupt-path tests run faster because there's one persistence window instead of two stacked ones.

984f5012c — wal_autocheckpoint 100 → 1000.
Every checkpoint rewrites main-DB pages — that's both the torn-write window on a no-DPO/FUA virtual disk and the contention window the probes race. 100 (from 99c19eb) forced a checkpoint per ~400KB written, i.e. near-constant churn under a dispatcher swarm. 1000 is the SQLite default, now asserted explicitly; durability is unchanged since synchronous=FULL still fsyncs the WAL at commit.

Both deployed on the affected host (gateway + web UI restarted). Suite: 222/222.

[jamesraddock]

This is still in testing and may cross over to webui. May be particular to users using WSL

[teknium1]

Thanks for the detailed production investigation and the focused regression coverage. The underlying lifetime-cache premise is still present on current main, but this needs a current-main-aware salvage.

Problems

• Current main added an initialized-path fast return at hermes_cli/kanban_db.py:1722-1737 after this branch diverged. It returns before _guard_existing_db_is_healthy(), while this PR's TTL changes only affect the slow initialization path. Therefore post-init connects would still skip the re-probe.
• Current main also sets wal_autocheckpoint=100 in that fast path at hermes_cli/kanban_db.py:1730. The PR changes only the initialization-path assignment, so the requested 1000-page setting would not apply to normal subsequent connections.

Suggested changes

• Integrate the bounded health check with the current fast path and add an end-to-end connect() regression covering an initialized database after TTL expiry.
• Set and test wal_autocheckpoint on both connection paths.

Automated hermes-sweeper review.

[teknium1]

Current main added an _INITIALIZED_PATHS fast return before the guard (hermes_cli/kanban_db.py:1722-1737). This cache is therefore never consulted for ordinary post-init connect() calls after a salvage. Please wire the TTL check into that branch and cover it through connect(), not only a direct guard call.

[jamesraddock]

Good catch, and thanks for the current-main-aware framing. Pushed a fix and rebased the branch onto current main so the line numbers line up with your review.

Worth noting the fast-path bypass was actually latent in the branch itself, not only in the merge-with-main view: the _INITIALIZED_PATHS fast return already sat before _guard_existing_db_is_healthy() and set wal_autocheckpoint=100, so on the steady-state path the TTL re-probe and the 1000-page setting never applied — which defeats the whole point for the long-lived dispatcher this PR targets.

Changes

• Health check wired into the fast path. connect() now calls _guard_existing_db_is_healthy(path) before the read/write open on the _INITIALIZED_PATHS branch. The guard is read-only and TTL-gated, so on a cache hit it's a dict lookup, and it takes no cross-process init lock — so it does not reintroduce the [Bug]: Kanban dispatcher hangs forever on an unbounded fcntl.flock(LOCK_EX) in kanban_db.connect() under concurrent board access #36644 stall this fast path exists to avoid. Running it before the open means a confirmed-damage verdict quarantines and raises with no connection to unwind.
• wal_autocheckpoint=1000 on both paths. The fast path now matches the init path (was 100).

Tests (both fail without the change)

• test_connect_fast_path_reprobes_health_after_ttl — drives it end to end through connect() on an already-initialized DB: within the TTL no re-probe, past the TTL exactly one re-probe, and a persistent-damage verdict fails the connect closed.
• test_connect_sets_wal_autocheckpoint_1000_on_both_paths — asserts 1000 on both the init and already-initialized connects.

The 7 original commits are unchanged in content (just replayed onto current main), with the fast-path fix as the commit on top.

[jamesraddock]

@teknium1 — both points are addressed and the branch is rebased onto current main (new commit baaa8fdee; details in the comment above). GitHub won't let me formally re-request review from a fork PR, so tagging you here — would you mind taking another look when you get a chance? 🙏

[jamesraddock]

Author note for maintainers — please hold the wal_autocheckpoint change in this stack before merging.

Two commits here raise wal_autocheckpoint from 100 to 1000 (both connect paths) and rename the regression test to assert 1000. Field data plus a follow-up investigation on my install now argue against shipping that as-is.

What happened in the field. On 2026-07-15, ~6.5h after 1000 first went live fleet-wide — on my heaviest process-churn day on record — the shared kanban board corrupted three times in one afternoon (wrong # of entries in index idx_events_*, quarantined and auto-restored from verified snapshots each time). The board had run clean at 100 before that.

What the investigation showed (~120 controlled trials + artifact forensics). The simple causal story does not survive scrutiny in either direction:

• No process-level condition corrupted a live DB at either value: concurrent writers, SIGKILL mid-checkpoint, and naive-copy races all left the live DB intact every trial.
• The production damage signature is index entries ahead of the table with an empty WAL — mixed-generation pages that a completed checkpoint cannot produce and that a crash would have left a WAL to repair. The strongest remaining suspects are environmental (the Windows host drive backing the WSL2 vhdx was 99% full at the time — since fixed) or checkpoint bookkeeping, not burst size.
• In a fleet dominated by short-lived CLI/webui connections, checkpoint cadence is driven mostly by last-connection-close checkpoints, which run a full checkpoint regardless of this pragma — so the value matters much less than either in-tree comment assumed.

What I run now, and propose for this PR: wal_autocheckpoint=100 on both call sites as an explicitly-labeled precaution (not a mechanism-backed fix), a test asserting the two paths agree so the value can only change deliberately in both places at once, and quarantine-time forensics (WAL captured before the main-file copy; incident-context JSONL) so the next occurrence is actually diagnosable.

Happy to push the updated commits to this branch — flagging it first so the current diff doesn't merge with the 1000 assertion locked in by the test.

[jamesraddock]

Follow-up: the corruption is now REPRODUCED and the 100-vs-1000 question is settled experimentally.

I built a churn harness (~830 real short-lived connect() processes over 150s + a long-lived ticker, ~10% SIGKILLed, page-cache eviction between integrity checks) against a throwaway board with the real schema. Results across 20 ablation arms:

• wal_autocheckpoint=1000: corrupts within ~1 minute, every run, with the exact production signature (wrong # of entries in index idx_events_task/idx_events_run, index/table count deltas of 1–2, rowid-out-of-order cells).
• wal_autocheckpoint=100: clean under identical load — same churn, same kills, same eviction.
• SIGKILL and cache eviction are NOT required; plain sqlite3 connections at 1000 under the same load are clean, and the damage is byte-identical read through the page cache vs after eviction — so this is not a WSL2/storage artifact but an interaction in our own connect path: the health guard's read-only integrity probe + a fresh rw connection + the migration pass's BEGIN IMMEDIATE write transaction, executed per new process, corrupts at 1000 and is safe at 100. Removing any one ingredient (or running at 100) prevents it.

So the earlier request stands, now with direct evidence rather than field correlation: the two commits raising this to 1000 (and the test asserting 1000) must not merge. I'll push the 100-both-paths commits to this branch. Independent of the pragma value, the durable fix is to stop running the migration write-transaction on every process start when there is nothing to migrate — that removes the trigger entirely; I'll follow up with that change once it's validated against the same harness.

[jamesraddock]

Pushed as promised (append-only): adaf10ec1 restores the 100-page fast-path value, 21c9f1081 reconciles BOTH call sites to 100 (replacing the earlier justification comments, which did not survive testing, with the evidence-backed rationale) + adds quarantine-time forensics, and 5fcf6d41c removes the corruption trigger itself — the migration pass no longer opens its BEGIN IMMEDIATE backfill transaction on every process start when there is nothing to backfill. The regression test now asserts both connect paths agree at 100. With the last commit, the churn harness runs clean even at wal_autocheckpoint=1000; the 100 value stays as defense-in-depth.

[teknium1]

Merged via #68654. The backup-retention and guard-improvement ideas from your PR informed #68654 (retention cap, narrow auto-REINDEX). The TTL re-probe half didn't survive the rebase onto the current fast path; if you want to rebase that half onto current main as a focused PR, it's still welcome.

[jamesraddock]

Thanks for carrying those improvements forward and for the credit. I’ll rebase the TTL re-probe work onto current main and submit it separately as a focused PR.

…

On Tue, Jul 21, 2026 at 4:15 PM Teknium ***@***.***> wrote: *teknium1* left a comment (NousResearch/hermes-agent#41795) <#41795 (comment)> Merged via #68654 <#68654>. The backup-retention and guard-improvement ideas from your PR informed #68654 <#68654> (retention cap, narrow auto-REINDEX). The TTL re-probe half didn't survive the rebase onto the current fast path; if you want to rebase that half onto current main as a focused PR, it's still welcome. — Reply to this email directly, view it on GitHub <#41795?email_source=notifications&email_token=AAMJY5QMBT4T44V4H4CHNOL5F7FNFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBTHA3DANJWGE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5038605615>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMJY5WIVVZIUQIB6V3O2LT5F7FNFAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMRUGU2TIMRWG45US43TOVSTWNBWGEYDAMJUGM3TJILWAI> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/AAMJY5RAYTJ5SO7UWLAM5JT5F7FNFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBTHA3DANJWGE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/AAMJY5WZY3GNNCNAT3CO7535F7FNFA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBTHA3DANJWGE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

[jamesraddock]

Follow-up root-cause evidence is now tracked in #69784.

SQLite has documented a WAL-reset corruption race affecting the 3.50.4 library embedded by Hermes' current uv-managed Python. The documented trigger (multi-process WAL writers plus overlapping checkpoints/WAL reset) and failure shape match this investigation.

Using the same migwritetxnall1000 churn arm:

• SQLite 3.50.4: two 150s runs ended corrupt, with 14 and 18 mid-run anomalies and the same idx_events_task / idx_events_run disagreement seen in production.
• fixed SQLite 3.51.3: 837 workers, 38 SIGKILLs, zero anomalies, clean cached and post-eviction final checks.

That changes the root-cause priority: the guard/quarantine work here and the recovery work merged through #68654 remain valuable defense-in-depth, but they cannot prevent vulnerable SQLite from creating the damage. The upstream issue asks Hermes to ship/select a fixed SQLite runtime or gate WAL on vulnerable versions. No need to reopen this PR; linking it so the investigation history stays intact.