Skip to content

fix(state): pooled read connections survive transient EIO (WSL2/vhdx, ZFS) - #100883

Closed
Sahilvishnaliya wants to merge 1 commit into
NousResearch:mainfrom
Sahilvishnaliya:fix/read-pool-eio-retry
Closed

Sahilvishnaliya wants to merge 1 commit into
NousResearch:mainfrom
Sahilvishnaliya:fix/read-pool-eio-retry

Conversation

@Sahilvishnaliya

Copy link
Copy Markdown
Contributor

Fixes #100871

Problem

The 0.21.0 pooled read machinery had no EIO recovery. When a pooled read connection threw disk I/O error:

  1. the exception propagated straight out of get_session — the reporter's multi-process WSL2 install (ext4-on-vhdx, sparseVhd=true) produced 37 identical tracebacks, each followed by compression session recovery failed
  2. the poisoned connection was returned to the pool unchanged — so every later borrower inherited the same flake until process restart

Field evidence: zero occurrences in the two weeks before 0.21.0, onset minutes after the upgrade (git blame shows the read-pool machinery as what changed underneath unchanged SQL), PRAGMA quick_check = ok, not fd exhaustion (42/4096), not disk-full (900+ GB).

Their controlled A/B also rules out the operator workaround: journal_mode: delete collapses read throughput ~30000x (5,139,477 → 174 reads) because DELETE serializes readers behind the writer. Absorbing the flake in the read pool is the only usable mitigation.

Fix — two coordinated changes

1. _read_ctx poisons-are-closed, not recycled. A pooled connection that raised a transient EIO is closed and its fd permit released, instead of going back into the pool. On CoW/sparse backing stores one bad shared-memory interaction poisons that specific handle while the DB stays intact.

2. read_execute(sql, params) — once-only EIO retry on a fresh connection. get_session, the exact crash site of all 37 tracebacks, now goes through it. Deterministic errors (schema, corruption) propagate on the first attempt with no retry; EIO on both attempts propagates too. Same retry shape as the existing _on_disk_journal_mode loop and the write path.

Observability: absorbed retries counted (_read_ioerr_retries) and each logs a WARNING naming the db path — "flake absorbed" is distinguishable from "hard failure" in the logs.

Verification

tests/test_read_pool_eio_retry.py — 5/5 pass, driving real pooled connections (poisoned via per-connection execute override so the fd-permit accounting stays honest):

  • first-conn-EIO → retry returns the row, retry counted
  • deterministic no such table propagates on attempt one, 0 retries
  • get_session survives transient EIO (the 37-traceback crash site)
  • poisoned connection is closed, not recycled (tracked via _close_read_conn)
  • double-EIO propagates after exactly one retry — never a loop

Sibling pool/read suites all green: test_session_db_read_conn_pool.py + test_session_db_read_path_split.py + test_hermes_state_conn_lock_audit.py 30/30, test_state_db_malformed_repair.py 21 passed / 3 skipped.

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

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related competing fix for #100871: #100882 retries the failed statement on the same pooled connection; this PR evicts that connection and retries fresh. Both cover the same transient read IOERR with different SQLite-locking tradeoffs.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues on exact head 6b453e018ff8fe84c1f9b60f648be741dd87ae29:

  1. P1 — read_execute() returns a cursor after releasing its pooled connection. Both return conn.execute(...) paths exit _read_ctx() before the caller executes cursor.fetchone(). That means the connection has already been returned to _read_pool and can be borrowed concurrently while the cursor is still being stepped. It also places the exact fetchone()/SQLite-step failure outside the EIO retry boundary, so an IOERR raised while stepping still escapes and the connection is not marked poisoned. The competing #100882 explicitly covers this boundary with execute+fetch under one checkout. This carrier needs the same ownership invariant: acquire → execute → consume the row/result → classify any EIO → only then recycle/close the connection. Do not return a live cursor across the pool lease.

  2. P1 — the regression fixture hard-codes a developer checkout into import resolution. tests/test_read_pool_eio_retry.py does sys.path.insert(0, r"C:\\Users\\salma\\dev\\hermes-agent"). On that machine it can import a different checkout than the PR under test; elsewhere it silently points at a nonexistent path. Tests must exercise the checked-out repository object, not an ambient personal path.

  3. 2K / ownership gate — this adds another read-pool lifecycle mechanism directly to hermes_state.py, already far beyond the repository's 2,000-line ceiling. The retry/poison lifecycle should have one bounded owner rather than further expanding the state godfile, especially with #100882 competing for the same semantic surface.

The fresh-connection direction may still be the right #100871 strategy, but these ownership/proof defects have to be resolved before this object can certify it.

… ZFS)

The pooled read machinery had no EIO recovery: when a pooled connection threw 'disk I/O error' the exception propagated straight out of get_session, and the poisoned connection was returned to the pool unchanged.

This replaces read_execute with read_fetch so statement execution and row fetching occur entirely inside the _read_ctx pool lease, avoiding returning a live cursor across the pool checkout boundary.
@salch-cred

Copy link
Copy Markdown
Contributor

@andrexibiza I have updated the PR to address all feedback:

  1. Ownership / Lease Invariant: Replaced
    ead_execute()\ returning a cursor with
    ead_fetch(sql, params, fetch_all=False). Statement execution and row stepping (\ etchone/\ etchall) now happen entirely inside the _read_ctx()\ checkout lease. Any \disk I/O error\ raised during execution or row fetching is caught, the connection is marked poisoned and closed (not recycled), and the statement+fetch is retried once on a fresh pooled connection.
  2. Clean Test Fixture: Removed the hardcoded local \sys.path\ import from \ ests/test_read_pool_eio_retry.py\ and added test coverage ensuring that EIO errors occurring during row stepping inside _read_ctx()\ trigger the single-retry & poison-eviction behavior.

teknium1 added a commit that referenced this pull request Sep 11, 2026
…fore surfacing it

Since 0.21.0 reads go through mode=ro pooled connections. A read-only OPEN already
rides out the millisecond WAL transition window (checkpoint / WAL reset / frame flush
by a sibling process; the ro reader cannot rewrite the -shm index) with a bounded retry
(#100436), but a WARM pooled reader hitting the same window while its SELECT executes
propagated `disk I/O error` straight out of get_session(): 37 identical tracebacks on a
multi-process WSL2 ext4-on-vhdx install, each followed by "compression session recovery
failed", with quick_check=ok (#100871). The reporter's A/B shows the operator
workaround (journal_mode=delete) collapses read throughput ~30000x, so the flake has to
be absorbed on the read path.

_read_one/_read_all now replay the idempotent statement within the existing read-only
IOERR budget (3 x 50 ms) on the SAME connection -- close+reopen would cancel this
process's POSIX locks for every sibling connection -- and a persistent IOERR still
propagates. No quarantine: EIO on a read is busy, not broken. Every SELECT in the
SessionDB siblings (63 call sites) reaches the pool through these two helpers, so the
class is covered without a wrapper type.

Same-connection retry per #100882's analysis (@fangliquanflq); #100883
(@Sahilvishnaliya) diagnosed the missing recovery in the 0.21.0 read pool.

Fixes #100871.
Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: Sahilvishnaliya <222165401+Sahilvishnaliya@users.noreply.github.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Landed on main in #108082 (d956e05). The transient-IOERR retry landed as a same-connection retry in _read_one/_read_all (the wrapper/eviction shape would cancel POSIX locks on reopen). Carried as Co-authored-by; thank you @Sahilvishnaliya.

@teknium1 teknium1 closed this Sep 11, 2026
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/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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

5 participants