Skip to content

fix(state): retry contended session list queries - #102621

Open
ialmeida-jera wants to merge 1 commit into
NousResearch:mainfrom
ialmeida-jera:fix/eng-1265-sqlite-lock
Open

ialmeida-jera wants to merge 1 commit into
NousResearch:mainfrom
ialmeida-jera:fix/eng-1265-sqlite-lock

Conversation

@ialmeida-jera

Copy link
Copy Markdown

What does this PR do?

Retries transient SQLite query-execution contention in the session-list path instead of returning an intermittent error after SQLite's own busy timeout expires.

The retry is intentionally narrow and bounded:

  • only SQLITE_BUSY / SQLITE_LOCKED (including extended result codes) and SQLite's canonical lock messages qualify;
  • page, pinned backfill, and count statements get two retries with short jitter;
  • retry/exhaustion logs include phase, retry count, elapsed wait, connection mode, fallback state, and DB path;
  • unrelated sqlite3.OperationalError values propagate immediately.

This is still required on current main: read-only open contention already has retry handling, but query execution does not. Open PR #101042 changes connection busy_timeout; it does not retry a statement after that timeout expires, and it targets #101035 rather than this pinned-query incident.

Related Issue

  • Linear: ENG-1265
  • Sentry: HERMES-GATEWAY-R / HERMES-GATEWAY-S

Type of Change

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

Changes Made

  • hermes_state.py — add narrow BUSY/LOCKED classification, bounded jittered retry/telemetry, and route page, pinned, and count reads through it.
  • tests/test_session_list_lock_retry.py — deterministic two-connection lock reproduction, persistent-lock budget/telemetry assertions, narrow classifier coverage, and unrelated-error sabotage.
  • tests/hermes_cli/test_web_server.py — prove GET /api/sessions recovers both from a locked page query and a lock acquired between listing and count.

How to Test

  1. Hold BEGIN EXCLUSIVE from a second DELETE-journal connection with the read connection's test-only busy_timeout=0.
  2. Call list_sessions_rich() or GET /api/sessions; release the lock during the deterministic backoff.
  3. Verify the request succeeds, persistent locks exhaust exactly two retries with classified logs, and no such table is never retried.

Commands run:

HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh -j 4 tests/test_session_list_lock_retry.py tests/test_session_db_read_conn_pool.py tests/test_session_db_read_path_split.py tests/hermes_cli/test_web_server.py -q
# 197 passed, 25 skipped, 0 failed

python3 scripts/check-windows-footguns.py --all
# No Windows footguns found (1089 files)

.venv/bin/ruff check hermes_state.py tests/test_session_list_lock_retry.py tests/hermes_cli/test_web_server.py
# All checks passed

Red/sabotage proof: reverting only hermes_state.py made tests/test_session_list_lock_retry.py fail 4 tests (database is locked / missing retry boundary); restoring the implementation returned it to green.

A full HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh run was also attempted. It failed in 83 unrelated files (248 tests) plus 53 collection/import failures due the local optional-dependency/tool environment (for example disabled lazy install for parallel-web, host sort/man behavior). All changed and adjacent state/web suites above are green.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the full test suite and all tests pass — targeted/adjacent suites pass; unrelated local full-suite failures are disclosed above
  • I've added tests for my changes
  • I've tested on my platform: macOS 26 arm64, Python 3.11

Documentation & Housekeeping

  • Documentation — N/A; behavior and observability are documented beside the implementation
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform impact considered; pure Python/SQLite and Windows-footgun scan is clean
  • Tool descriptions/schemas — N/A

Screenshots / Logs

N/A — backend concurrency fix with automated regression coverage.

@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 area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 4, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

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

PR #102621 — fix(state): retry contended session list queries

Well-constructed reliability fix: the lock classifier is deliberately narrow (exact-match + :-suffix on known messages, errorcode-masked & 0xFF), non-lock OperationalErrors propagate unretried (with a test proving it), and the exhaustion log carries phase/retry/wait/mode for diagnosis. Bounded (2 retries, ~25-75ms jitter) so worst-case added latency per phase is ~150ms.

Non-blocking nits:

  • hermes_state.py:100time.sleep(delay) runs outside the with self._read_ctx() block (good — doesn't hold the read ctx while sleeping), but note the loop re-enters _read_ctx() fresh each attempt, which may reopen/recheckout a connection per retry. Correct, just slightly more churn than reusing the handle; fine at 2 retries.
  • hermes_state.py:29error_code & 0xFF in (...): masking to the primary result code is right (extended codes share low byte), but SQLITE_LOCKED low byte vs SQLITE_BUSY — both compared correctly. One subtlety: sqlite3.SQLITE_BUSY/SQLITE_LOCKED constants exist on all supported builds, but sqlite_errorcode attr doesn't (hence the fallback). The getattr(..., None) + isinstance(int) guard handles it. Good.
  • Retry budget is per-phase (page, pinned, count each get 2 retries): a single list_sessions_rich can sleep up to ~450ms worst case across three phases. Still fine for a sidebar poll, but the budget composes — worth knowing, not changing.
  • test_web_server.py:180 — patching hermes_state.time.sleep with a function that releases the lock couples timing control to lock release; elegant for determinism, but it means the test never exercises real backoff durations. The sleeps length assertions cover counts. Acceptable.
  • hermes_state.py:87random.uniform(*_SESSION_LIST_LOCK_RETRY_BACKOFF_S) unpacks a tuple constant; if someone later edits the tuple arity this fails opaquely. Trivial; ignore unless touching.

No correctness concerns; the narrow classifier is the key thing and it's right.

@ialmeida-jera
ialmeida-jera force-pushed the fix/eng-1265-sqlite-lock branch 2 times, most recently from dbf6ec9 to 1fbb318 Compare September 4, 2026 18:05
Retry only SQLITE_BUSY/SQLITE_LOCKED failures at the list page, pinned backfill, and count query boundaries. Keep retries bounded and emit contention telemetry while unrelated OperationalErrors continue to surface.
@ialmeida-jera
ialmeida-jera force-pushed the fix/eng-1265-sqlite-lock branch from 1fbb318 to ac81f4d Compare September 5, 2026 00:49
@ialmeida-jera

Copy link
Copy Markdown
Author

Review follow-up completed on current upstream main. Exact head ac81f4d246cbae32d8d949503f43138aba4893ce passed the focused/session-state suites, Ruff, Windows-footgun gate, and an independent exact-SHA review. The automated review had no correctness findings; its non-blocking nits were assessed, and the newly enforced text-I/O encoding findings were fixed. The fork workflow suites are currently action_required and need a NousResearch maintainer to approve them before CI can run; this contributor cannot approve or merge the upstream PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists 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