fix(state): tolerate in-process WAL checkpointing and prevent read-pool descriptor accumulation (#110214) - #110262
JoaoMarcos44 wants to merge 4 commits into
Conversation
…ol descriptor accumulation (NousResearch#110214) Long-running messaging gateways on Linux/WSL2 halt turns approximately hourly with DeletedWalGenerationError ("a live Hermes process held a retired state.db-wal generation after its pathname was deleted or replaced") after WSL kernel update 6.18.33. Investigation revealed three compounding root causes: 1. Hourly WAL Churn: gateway/delivery_ledger.py opened state.db via open_db() without wal=False, enforcing WAL journal mode and triggering close-time checkpoints on Python 3.11 that cleanly unlinked state.db-wal. 2. Read-Pool Descriptor Accumulation: SessionDB held pooled read connections in _read_pool indefinitely without idle eviction, retaining file descriptors to unlinked WAL inodes in SQLite's VFS cache across checkpoints. 3. Guard False Positives on Clean Checkpoints: When state.db-wal was unlinked while state.db and state.db-shm were intact, _fd_is_truly_unlinked failed os.stat(watched_path) with FileNotFoundError and returned True, misclassifying in-process descriptors as an orphaned replacement generation. Similarly, _wal_generation_was_lost failed on None != ident when -wal was absent. Fixes: - gateway/delivery_ledger.py: pass wal=False to open_db(). - hermes_state_dbfile.py: handle vanished/closed descriptors gracefully and tolerate in-process handles when state.db-wal is absent after a clean checkpoint while state.db-shm and state.db remain active and matching. - hermes_state.py: in _wal_generation_was_lost(), recognize absent -wal with matching -shm as a clean checkpoint rather than generation loss, and add evict_idle_read_conns() to drain dormant read handles. - gateway/run.py: evict idle read connections on live SessionDB instances during periodic housekeeping memory trim. - Added regression and invariant tests covering delivery ledger wal=False, vanished descriptor tolerance, clean checkpoint tolerance, and idle read connection eviction. Fixes NousResearch#110214
…ecar scan (NousResearch#110214) When state.db-wal is cleanly checkpointed and unlinked by SQLite while state.db and state.db-shm remain intact on disk, any Hermes process (self or a peer process scanning /proc) holding that checkpointed descriptor does not hold a retired generation from a replaced WAL. Tolerating peer handles prevents sibling processes (CLI, web server, workers) from falsely raising DeletedWalGenerationError when inspecting the gateway's state.db.
…usResearch#110214) In _wal_generation_was_lost(), check -shm and -wal in a single pass rather than re-statting -wal in the subsequent recorded items loop. Eliminates duplicate filesystem stat calls on the hot write path, reducing turn latency under virtualized filesystems (WSL2/9P).
…g housekeeping (NousResearch#110214) Add evict_all_idle_read_conns() traversing all active _PathReadBudget members so periodic gateway memory trim closes idle pooled read connections across all paths and profiles in the process, guaranteeing dormant file descriptors and permits are returned to the OS.
andrexibiza
left a comment
There was a problem hiding this comment.
I traced exact head 0011b523e97bbab3e39043265ddef4179c9ba0cf against base/current main 3f86ed75dad1933036c52018e991dbd839837126, the full seven-file diff, the existing deleted-generation invariants, #110214, #110235, and the surrounding WAL producer/recovery work. The read-pool reclamation and the vanished-descriptor (ENOENT/EBADF/ESRCH) race are useful hardening, but I cannot clear this head yet: the new absent--wal exception weakens the exact-generation guard at the point where it is supposed to fail closed before a replacement WAL can be minted.
Blocker: -shm existence is not a WAL-generation witness
The open-path invariant is already explicit in test_second_sessiondb_open_refuses_and_does_not_mint_wal: if a live process still holds an unlinked state.db-wal inode, a second opener must refuse before sqlite3.connect can create another WAL generation. On this head, _fd_is_truly_unlinked() returns benign whenever the watched WAL pathname is absent, state.db-shm exists, and the main DB happens to be on the same device as the fd. Same st_dev proves only filesystem placement; it does not prove that the deleted fd belongs to the current WAL generation, and the new pid argument is not used to establish that provenance.
I reproduced the predicate boundary with a real Linux fd: create/open state.db-wal, keep /proc/self/fd/<n> live, unlink only the WAL pathname while leaving state.db + state.db-shm, then evaluate the exact head predicate. Main's identity rule returns True (holder is orphaned); this head returns False. If a replacement state.db-wal is then created, the predicate flips back to True — but that is too late, because the guard already granted the second opener permission to mint it. This is exactly the pre-effect boundary the deleted-generation guard exists to protect.
The same authority loss appears on the in-process write path: _wal_generation_was_lost() deletes the recorded -wal identity when the pathname vanishes with intact -shm, then later re-adopts whatever inode appears at state.db-wal. A long-lived writer can therefore forget the generation it was bound to and accept a successor generation while still holding the retired fd. Generation identity needs to remain immutable until there is positive evidence that the old descriptor is gone or otherwise proven harmless; pathname absence plus a live -shm is not enough.
The new regression currently proves the wrong shape
test_fd_is_truly_unlinked_tolerates_in_process_checkpointed_wal passes str(base) as fd_path. The production scanner passes /proc/<pid>/fd/<n> for the deleted WAL descriptor. Statting state.db makes the new same-device predicate pass by construction, so the test never exercises the dangerous shape it is intended to bless. Please make the regression hold an actual WAL fd open, unlink the WAL pathname, leave -shm present, and call through the real /proc descriptor path. The acceptance matrix should include self and peer holders, prove that no replacement WAL is minted while an old holder exists, and cover the in-process recorded-identity/re-adoption path too.
Interlocks / merge shape
#110235 is complementary and should remain narrow: its FileNotFoundError race is safe because a vanished descriptor cannot retain a generation. This PR is trying to solve the persistent-holder half that review explicitly left open, but it cannot do so by weakening identity. Merged #109841 (including the preserved #109754 authorship) removes the lock-dropping producer; #110102, #109737/#109864, #110116, and the observational read-only family (#110186/#110252) are producer/lifecycle containment on adjacent close/open paths; #109766/#110073 are recovery once generation loss has actually occurred. Those lanes all compose around one invariant: producer churn may be reduced, but the generation guard remains the independent fail-closed acceptance authority.
One additional verification gap: delivery_ledger._connect(..., wal=False) only stops open_db() from calling apply_wal_with_fallback; WAL journal mode is persistent, so an already-WAL state.db connection still joins the WAL database, runs the ledger schema/write path, and closes. The added test checks only that the kwarg is present. Before calling this the source-of-churn fix, please add a behavioral inode/generation-stability witness against an already-WAL state.db (or use the repository's close-time WAL containment primitive where appropriate).
Exact-head hosted evidence is also not green yet: CI, Nix, and Docker are all action_required, and the commit currently has zero check runs. The reported focused local result (50 passed / 13 skipped) is useful but does not cover the real deleted-fd witness above.
The direction here is worthwhile — especially reclaiming idle read descriptors across every path budget — and the investigation is doing the right kind of repository-wide work. The remaining fix is to keep that availability improvement without surrendering the exact-generation invariant that prevents split-brain corruption.
| base_db = watched_path.removesuffix("-wal") | ||
| try: | ||
| base_stat = os.stat(base_db) | ||
| if os.path.exists(base_db + "-shm") and base_stat.st_dev == fd_stat.st_dev: |
There was a problem hiding this comment.
Blocker: this treats state.db-shm existence plus st_dev equality as proof that a still-open deleted WAL fd is benign. st_dev only proves the files are on the same filesystem; it says nothing about WAL generation identity. With a real /proc/<pid>/fd/<n> pointing at an unlinked old state.db-wal, leaving state.db-shm present makes this return False, so the pre-open guard can proceed and mint a replacement WAL while the old inode is still held. Keep this fail-closed unless you can positively bind the fd to the current generation (or prove the fd is gone).
| if wal_current is None: | ||
| # Clean checkpoint unlinked -wal while -shm and main DB are intact | ||
| if shm_recorded is not None and not self._db_file_was_replaced(): | ||
| self._db_sidecar_identity = {k: v for k, v in recorded.items() if k != "-wal"} |
There was a problem hiding this comment.
This drops the only recorded identity for the writer's WAL generation on pathname absence alone. If a new state.db-wal appears later, the elif wal_current is not None branch simply re-adopts it. That lets a long-lived writer cross from an old unlinked WAL inode to a successor pathname without ever proving the old generation was released. The generation token should stay immutable/sticky until the old holder is positively cleared; intact -shm is not sufficient evidence.
| shm.write_bytes(b"shm_data") | ||
| wal_path = tmp_path / "state.db-wal" # does not exist | ||
| # Tolerated for both in-process and peer processes as long as -shm is intact | ||
| assert _fd_is_truly_unlinked(str(base), str(wal_path), pid=os.getpid()) is False |
There was a problem hiding this comment.
This test does not exercise the production fd shape: fd_path is state.db itself, while iter_deleted_sqlite_sidecar_holders() passes /proc/<pid>/fd/<n> for the deleted WAL descriptor. Because the base DB is necessarily on the same device as its own -shm, this makes the new predicate pass by construction. Please hold a real WAL fd open, unlink only the WAL pathname, keep -shm, and call with the /proc fd path; then assert the guard still refuses a second opener until that old descriptor is actually gone/proven benign.
|
Superseded by #110544 (main 3e43cee). The single-process multi-handle split and the hourly WSL halts are both the same mechanism: a stray in-process If a separate defect survives here (e.g. read-pool descriptor growth), please re-open it as a focused PR against ≥ 3e43cee with its own repro. Thank you @JoaoMarcos44 for the detailed traces. |
Summary
Fixes #110214.
Long-running messaging gateways running on Linux/WSL2 halt turns approximately once per hour with
DeletedWalGenerationError:Following WSL kernel update 6.18.33, changes in ext4-on-vhdx dentry caching and unlinked inode visibility exposed race conditions during SQLite close-time checkpoints.
This PR provides a comprehensive, canonical solution addressing the underlying architectural defects across four focused commits:
233cb46729:fix(state): tolerate in-process WAL checkpointing and prevent read-pool descriptor accumulation (#110214)74aa17bacf:fix(state): tolerate peer checkpointed WAL descriptors in deleted-sidecar scan (#110214)4c2608dbae:perf(state): eliminate redundant sidecar stat calls on write path (#110214)0011b523e9:fix(state): evict idle read connections across all path budgets during housekeeping (#110214)3-Stage Investigation & Architectural Analysis
Round 1: Multi-Process Contention & Peer Guard False-Positives (Blocker Analysis)
/procobserved the gateway's file descriptor pointing to a cleanly checkpointedstate.db-wal (deleted). Becausepid != os.getpid(), the peer process treatedstate.db-wal's absence on disk as an orphaned generation and halted Turn/CLI processing withDeletedWalGenerationError.74aa17bacf): In_fd_is_truly_unlinked(), whenwatched_path.endswith("-wal")does not exist on disk, we check ifstate.dbandstate.db-shmexist on the filesystem. If-shmis intact, SQLite WAL coordination is alive and no replacement WAL exists on disk. This is tolerated for bothselfand peer PIDs. If-shmis also missing (the sidecar generation was wiped), it remains correctly flagged as an orphaned generation.Round 2: Filesystem Syscall Overhead & Latency (Bottleneck Analysis)
_wal_generation_was_lost(), checkingstate.db-walwas performed once during the checkpoint check, and then again insidefor suffix, ident in recorded.items():. On WSL2/9P/VHDX, filesystemstat()calls across virtual boundaries have non-trivial latency.4c2608dbae): Optimized_wal_generation_was_lost()into a single-pass check:state.db-shmis checked once (as the WAL coordination anchor), andstate.db-walis stat'd once, eliminating duplicate filesystem calls on the hot write path.Round 3: Process-Wide Descriptor Reclamation (Robustness Analysis)
gateway/run.pyonly evicted idle read connections fromborrow_live_shared_session_dbs(). If satellite profiles, temporary subagents, or background tasks opened aSessionDBdirectly, their idle read connections would not be pruned during memory trim.0011b523e9): Addedevict_all_idle_read_conns()tohermes_state_readpool.py, traversing all registered_PathReadBudgetmembers. Periodic gateway housekeeping memory trim now guarantees that idle read connections across all profiles and paths return dormant descriptors and permits to the OS.Verification & Automated Tests
tests/gateway/test_delivery_ledger_fd_leak.py:test_ledger_connect_passes_wal_false: verifiedopen_dbcalled withwal=False.tests/hermes_state/test_deleted_wal_generation_guard.py:test_fd_is_truly_unlinked_handles_vanished_descriptor: verified closed/vanished descriptors returnFalse.test_fd_is_truly_unlinked_tolerates_in_process_checkpointed_wal: verified clean checkpoints with intact-shmreturnFalsefor both self and peer PIDs; missing-shmreturnsTrue.test_wal_generation_was_lost_tolerates_checkpointed_wal_with_intact_shm: verified single-pass check tolerates absent-walwhen-shmis intact.test_evict_idle_read_conns_closes_pool_and_releases_permits: verified per-instance pool draining.test_evict_all_idle_read_conns_across_multiple_dbs: verified process-wide eviction across multiple path budgets.pytest tests/gateway/test_delivery_ledger.py tests/gateway/test_delivery_ledger_fd_leak.py tests/hermes_state/test_deleted_wal_generation_guard.py tests/hermes_state/test_read_path_transient_ioerr.py # 50 passed, 13 skippedpython scripts/check_compat_pointers.py # ✅ no in-tree dependency on the 2091 plugin-compat pointers