Skip to content

fix(state): avoid config-writer waits in journal-policy checks - #103214

Closed
dokterdok wants to merge 3 commits into
NousResearch:mainfrom
dokterdok:fix/state-config-writer-contention-20260904
Closed

dokterdok wants to merge 3 commits into
NousResearch:mainfrom
dokterdok:fix/state-config-writer-contention-20260904

Conversation

@dokterdok

@dokterdok dokterdok commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Avoid unnecessary database waits while settings are saved

While settings are being saved, opening an established Kanban database can wait unnecessarily just to check its journal policy. This bounded SQLite/config fix uses the safely observed mode for an initialized database instead of waiting for the full config loader. Found during #97681 work, it changes no execution ownership and does not make every database-opening or Stop path nonblocking.

Behavior and limits

  • Existing WAL stays WAL, with its companion settings and deduplicated vulnerability warning.
  • An initialized DELETE database may retain DELETE during contention when strict WAL is not required.
  • New, empty, unknown-mode and strict non-WAL cases still require real policy. Configuration-driven transitions are deferred, not guessed.
  • Cold imports do not invert the module/config locks, and a successfully acquired config lock is released on every exit.

This does not make all SessionDB opening or read-pool expansion independent of config writes. Those paths also call apply_database_pragmas(), whose separate config load can still wait. That pre-existing boundary is unchanged.

No timeout, retry loop, config key, new SQLite BUSY classification or live journal downgrade is introduced.

Verification

Refreshed by a normal merge onto main b9271bcb34e1. The original two-file patch is byte-for-byte unchanged.

Check Result
Canonical focused SQLite/config suite on this candidate 79 passed /0 failed, four files, zero retries
Unchanged authored regression on main b9271bcb34e1 Five expected config-lock timeouts, four passes
Same regression on the candidate 9 passed /0 failed; already included in the 79
Scoped Ruff, whitespace and independent reconciliation review Passed; review bound to tree 134152f492ec4979215765f769ebc60391f686c7

The earlier three independent probes remain historical evidence from the previous refresh; they are not new tests added to these totals. No full-repository, live Stop, platform or release acceptance is claimed. GitHub CI remains a separate gate.

Exact source, command and attribution

Current head: 029ca5a064a0. Two-file comparison against the tested main.

Run with disposable state:

HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh -j 2 \
  tests/test_state_config_contention.py \
  tests/hermes_state/test_hermes_state_wal_fallback.py \
  tests/hermes_state/test_journal_mode_config.py \
  tests/hermes_state/test_sqlite_wal_reset_gate.py

The sibling test paths follow main's test-directory reorganization. Coverage retains old rows and journal modes, deferred upgrades, strict initialization, concurrent readers, cold imports and warning behavior. Both changed files remain below 2K lines.

Original published head 22131bb68787 and the previous refresh 43ea3c841fdc remain ancestors. No source history was rewritten. #98073 carries an authored equivalent: land or salvage the behavior once with attribution. #101384 concerns a separate concurrent WAL-transition race; its retry logic is neither imported nor duplicated.

A Group Chat Stop can open state.db while another thread is finishing an atomic config publication. Preserve an initialized database in its observed WAL or DELETE mode instead of waiting for the config writer; never select a new mode from stale settings. Cold, unknown and strict-WAL initialization keep the normal policy path. Avoid the config/module cold-import lock inversion and retain the existing deduplicated WAL-reset warning. The four-step reviewed source lineage is preserved at14bb260e2c,9fcbe65e66,0a03d0f3b6,61eb4625e6. Two independent final reviews clear the product and runtime-independent tests.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/config Config system, migrations, profiles area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 4, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Summary

Avoids stalling initialized databases on a held config lock: when _CONFIG_LOCK is contended, apply_wal_with_fallback preserves the on-disk journal mode for established databases (with WAL-reset-bug guards) instead of blocking on resolve_journal_mode(); unestablished/require_wal paths still wait for real policy. Also guards against lock-order inversion during partial hermes_cli.config import.

Findings

  • Non-blocking — hermes_state_wal.py:11-14: the __spec__._initializing guard prevents using a half-initialized config module's lock — the subprocess test pins the exact inversion scenario. Careful.
  • Non-blocking — hermes_state_wal.py:16-28: non-blocking acquire + observed-mode preservation means a config writer's policy change is deferred, not lost — test_deferred_mode_change_is_applied_after_writer_finishes proves the next open applies it. Correct eventual-consistency design.
  • Non-blocking — stale-read consideration: while the writer holds the lock, the opener keeps the observed mode even if the pending write changes policy. Bounded (one open) and resolved on the next call. Fine.
  • Non-blocking — WAL-reset-vulnerable branch still forces DELETE logging (_log_wal_reset_bug_once) on the fast path. Safety logging preserved under contention. Good.

Verdict

Correct lock-contention design with thorough concurrency tests. No blocking issues.

dokterdok and others added 2 commits September 13, 2026 14:13
Preserve the original two-file fix and authored history unchanged.

79 focused SQLite/config tests pass without retries. The unchanged
regression still reproduces five config-lock timeouts on the new base.

This removes the journal-policy wait, not every configuration-dependent
SessionDB opening step. No live Stop or full-release acceptance claimed.
@dokterdok

Copy link
Copy Markdown
Contributor Author

Superseded by the same two-file implementation and regression test in open runtime PR #106742 (commit e06d714). The source and replayed commit have identical file blobs, and that commit remains reachable from #106742 at 485d5f6. Closing this duplicate review entry as incorporated in that open PR—not merged into main or deployed. The source branch and history are retained; if #106742 drops the fix, revisit this closure.

@dokterdok dokterdok closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants