fix(state): warn when configured journal_mode=delete is overridden by on-disk WAL

[paoloantinori]

Closes #85608.

What

When database.journal_mode: delete is configured but the on-disk DB is already WAL, apply_wal_with_fallback() honors the never-live-downgrade rule and keeps WAL. That is correct (a live downgrade under open connections causes mixed-mode corruption), but the operator has no signal that their configured mode had no effect, and on a WAL-incompatible filesystem (virtiofs/NFS/SMB) the DB then corrupts on the next crash/sleep — exactly what they configured to prevent.

Real-world trigger: we hit this during the v0.20.0 (v2026.8.3) upgrade. Setting journal_mode=delete on inherited-WAL state.db files right after the bounce left them silently still in WAL (no signal), and one agent's state.db then corrupted on the next Mac sleep. The corruption cause is the pre-existing WAL-on-virtiofs vulnerability, not the upgrade; the upgrade was the crash/sleep trigger at the moment the new resolve_journal_mode() knob (#68912) had us set delete. See #85608.

This PR adds a once-per-process-per-db_label ERROR telling the operator the config did not apply and they must convert the DB header offline (PRAGMA journal_mode=DELETE with no open connections). See #85608 for the full motivation and the two affected code paths.

Changes

• New _log_configured_delete_overridden_once(db_label) helper (mirrors the existing _log_wal_fallback_once / _log_wal_reset_bug_once idiom: module-level set + lock, deduped per process per db_label, ERROR level).
• Emit it at both return-WAL paths when the configured mode is delete:
  1. The WAL-reset-vulnerable path (_apply_delete_for_wal_reset_bug): previously warned only about the vulnerability with an "upgrade SQLite" remedy that does not help when the cause is the filesystem. The new warning fires AFTER that one, so the actionable message is last.
  2. The read-only probe path (non-vulnerable runtime): previously returned WAL with no signal at all.
• The never-live-downgrade behavior is unchanged (return values, exceptions, side effects identical to main; this is log-only).

Tests

All behavioral (real sqlite3 on tmp files, caplog assertions), no mocks at the boundary:

• test_configured_delete_never_live_downgrades_existing_wal (updated): vulnerable path keeps WAL AND warns.
• test_configured_delete_overridden_warns_on_non_vulnerable_runtime_too: probe path also warns (the previously fully-silent case), with dedup.
• test_configured_delete_overridden_warning_fires_once_per_db: dedup to one per process per db_label.
• test_configured_delete_with_require_wal_and_existing_wal_returns_wal: pins the require_wal=True + configured=delete + on-disk WAL edge (returns wal, warns, no WalUnsupportedError).
• Autouse fixture resets the dedup set so tests are order-independent.

Full WAL test suites pass (test_journal_mode_config.py, test_hermes_state_wal_fallback.py, test_sqlite_wal_reset_gate.py) in both run orders.

Notes

• This is a follow-up to state.db corruption on macOS virtiofs: checkpoint_fullfsync no-ops in Linux containers; request a configurable, centralized journal_mode #68545 / PR fix(state): configurable journal_mode + centralize all DB openers #68912, which centralized journal_mode but left inherited on-disk WAL DBs silently un-protected when an operator later sets delete.
• The never-live-downgrade rule is intentionally preserved; the PR is purely the operator-facing signal that the configured and on-disk modes disagree.
• Happy to add a hermes doctor check or a hermes db migration helper as a follow-up if maintainers want either (mentioned in database.journal_mode: delete silently ignored when the on-disk DB is already WAL (no warning, no doctor signal) #85608), but kept this PR minimal.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(state): warn when configured journal_mode=delete is overridden by on-disk WAL

1. hermes_state.py:1018-1023 — the new override warning is logged at ERROR level, matching its _log_wal_fallback_once counterpart. There is a semantic difference worth considering: the WAL-refusal path is ERROR because concurrency is genuinely lost, whereas here the DB is functioning normally (in WAL, the faster mode) and only the operator's preference failed to apply. Since the message only fires for operators who explicitly configured delete, ERROR is defensible, but WARNING would avoid tripping ERROR-based alerting for a non-degradation state.

2. hermes_state.py:1340-1371 — dedup is keyed only on the caller-supplied db_label, a free-form string. Two distinct databases that share a label (e.g. "state.db" from two profiles in the same process) will suppress the second warning. This matches the pre-existing _wal_reset_bug_warned_paths discipline, so it is consistent, but keying on the resolved DB path would be more precise.

3. Test coverage is good — the dedup is pinned by two tests (one warning after two/three calls), and the autouse fixture correctly clears the module-level set between tests.

[Enough1122]

@C:/Users/admin/AppData/Local/Temp/opencode/review-85609.md

[paoloantinori]

Addressed the review from @Enough1122 point by point (verified against the code on this branch):

• ERROR vs WARNING — keeping ERROR, for three reasons. The parent issue (database.journal_mode: delete silently ignored when the on-disk DB is already WAL (no warning, no doctor signal) #85608) framed the silence as the bug ("no warning, no doctor signal"), so severity should match what motivated the fix. An operator who configures journal_mode: delete typically does so for a data-integrity reason (the virtiofs/corruption class of state.db corruption on macOS virtiofs: checkpoint_fullfsync no-ops in Linux containers; request a configurable, centralized journal_mode #68545), so "your protective setting did not apply" is a safety signal, not cosmetics: the DB is not in the state the operator believes it is in. And the per-process dedup bounds the noise to one line per database. That said, the judgment is genuinely arguable; happy to drop to WARNING if a maintainer prefers that framing.
• Dedup keyed on db_label — verified: every state-DB call site passes the fixed string "state.db" while db_path varies per instance, so the two-profiles-in-one-process collision is real in shape. However, both sibling dedup sets (_wal_fallback_warned_paths, _wal_reset_bug_warned_paths) key on the same free-form label discipline; keying only the new set on the resolved path would diverge from the established convention, and migrating all three is out of scope for this PR. Left consistent with the siblings.
• Test-coverage note: acknowledged, no change needed.

[paoloantinori]

@Enough1122 heads-up: your later comment (13:18 UTC, #issuecomment-5307630832) contains only a local Windows temp path, "C:/Users/admin/AppData/Local/Temp/opencode/review-85609.md", instead of review content. Looks like the review automation pasted the file path where the file contents belonged, so whatever that second review said never landed. The 13:10 review arrived intact.

[jeremykane]

Cross-linking from #90950: this is exactly the operator-signal gap we hit during a corruption incident — a journal_mode: delete config pin was silently ignored for an already-on-disk WAL DB (we'd earlier run the manual exclusive PRAGMA, so it held, but the pin alone would have done nothing with no warning). Confirming the need from a real corruption case.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

Follow-up review of the current head (the earlier second-review comment only contained a stray local file path — reposting the content properly):

1. hermes_state.py:1018-1023 (apply_wal_with_fallback) — the override warning fires when current_mode == "wal" and configured == "delete", good. One ordering nit: _log_configured_delete_overridden_once runs before _apply_wal_size_limit() etc., so in a fresh log stream the "your configured delete did not apply" ERROR appears before any size-limit activity. That reads fine, but consider emitting it after the mode-keeping actions so the log narrative is "kept WAL because X, and note your delete config didn't apply". Cosmetic only.

2. hermes_state.py:1189-1200 (_apply_delete_for_wal_reset_bug) — emitting the override warning last here (with the explicit comment about actionable-message ordering) is good. The require_delete gate correctly avoids double-warning on the non-strict path where WAL retention is merely advisory.

3. Dedup keying (_delete_overridden_warned_paths) still uses db_label, same as my first review's point 2 — consistent with the existing _wal_reset_bug_warned_paths discipline, acceptable, but path-keying would be strictly more precise for multi-profile same-label DBs.

4. Test coverage: the autouse fixture clearing both warned-sets between tests is correct, and the new caplog assertions pin both call sites. Nice addition.

No blocking issues — this is ready from my side.

[Enough1122]

Thanks for the heads-up — confirmed, that comment was automation pasting a local path instead of the content. The full second review is now posted above (#85609 (comment)).