fix(sessions): gateway restarts no longer strand rows, and closing sockets can't unbind live sessions (ring 2, 5 salvages) - #93430
Merged
Merged
Conversation
…rofile resume Pre-#93296, the desktop routed session RPCs by the focused tile, so a profile bot's turns executed on the default backend and its canonical session accumulated in the DEFAULT profile's state.db. Post-fix, the profile backend correctly receives the resume — but its store has never seen the session, so the same chat 4001s forever (unreachable instead of misrouted). Live repro: Teknium's Developer bot, session c93770. - hermes_state_portability: SessionDB.adopt_session_lineage_from() — composes the existing export_session_lineage()/import_sessions() primitives; donor rows are archived (never deleted) with end_reason=adopted_by_profile, which is deliberately NOT in RECOVERABLE_END_REASONS so canonical-lookup resurrection cannot undo an adoption. Idempotent (already-present ids skip). - tui_gateway/methods_session: profile-scoped session.resume falls back to adoption from the default store right before the 4007; ids unknown to BOTH stores still 4007 exactly as before, and launch-profile resumes never consult the fallback. - tests: 10 new (7 unit on the primitive incl. compression-lineage unit adoption + non-resurrectable archive; 3 handler-level through server.handle_request incl. the live repro shape); db-ownership leak test taught that the shared launch handle probe is by design. Follow-up to #93296/#93311; part of #93091.
…guard, honest donor_retired Review batch (3 reviewers) on the final diff surfaced: - H1: title-based donor matching could adopt AND non-recoverably retire an UNRELATED default-store conversation (bot titles collide by design; get_session_by_title has no archived filter/ordering). Donor probe is now exact-id only — the stranded repro always has the id. - H2: re-adoption after a partial run could retire a donor that had accumulated NEWER messages than the profile copy (skip-based idempotency never merges). New divergence guard compares message counts and refuses retirement when the donor is ahead (still adopts). - M1: donor_retired reported True even when every retirement step failed under suppress. Now per-segment tracked + warn-logged; True only when all applied. - M3: adopted=False (e.g. import validation limits) was silent — now warn-logged with import errors. - M4: archived donors are never re-adopted (no cross-profile cloning). - Dead 'from pathlib import Path' dropped; contextlib no longer needed. 5 new red-first-verified regressions (title-collision immunity, archived-donor immunity, non-vacuous owns_db gating with a real donor seeded, divergent-donor retirement refusal, donor_retired truthfulness). tests/tui_gateway: 578 passed. ruff clean.
ensureGroupChatSession's resume loop caught ANY session.resume error (stored sid, then title lookup) identically and fell through to session.create — the same bug findExistingCanonicalChat was fixed for hours earlier (87b645f) in the same file: a transient failure (the backend still warming up after a restart, a network blip on a cross-connection lookup, an oversized-resume refusal) read as "no session, mint a new one". That forks the member's real session AND silently overwrites room.sessions[key], making the original unreachable from the room. ensureGroupChatSession is actually more exposed than the 1:1 case: it runs every group turn (runGroupChatMemberTurn), with two independent swallow points. Distinguish "genuinely doesn't exist" from "transient failure" the same way the gateway itself does: session.resume's own handler (tui_gateway/methods_session.py) returns JSON-RPC code 4007 only when the target truly isn't found; every other failure (including 4130, "session too large to resume" — a session that DOES exist) now surfaces instead of being silently swallowed. The existing outer try/catch at the call site already treats a thrown error as "this member passes the round" (recordGroupActivity kind: 'failed'), so nothing new needs to catch it — a transient hiccup now costs one skipped round instead of a permanent fork.
Messages sent into a session whose in-memory runtime was detached on WS disconnect and orphan-reaped vanished silently: _sess_nowait returned 4001 with no log line, so 'request arrived and was rejected' was indistinguishable from 'request never arrived' in a 'message vanished' report. Log a WARNING with the session id and request id on every session-scoped RPC rejected against an unknown runtime id. Adds a regression test asserting the 4001 response and the warning. Closes #90428
… startup Close session rows left ended_at IS NULL when the in-process websocket orphan timer dies with the process (#65194). Dual-clock staleness (started_at AND newest message), desktop included, live in-memory sessions excluded, scheduled once from both entry.main and the WS sidecar so desktop/dashboard boots also run the sweep.
…to dashboard.* Follow-up to the #65422 salvage: - startup_orphan_reap joins _RECOVERABLE_END_REASONS (kept distinct from ws_orphan_reap for forensics): every recovery fence (find_latest_gateway_session_for_peer, unarchive_recoverable_session, promote_to_session_reset) now treats a startup-swept row as an accidental end, so a sweep never makes a session unresumable. - Config key moves from sessions.orphan_reaper to dashboard.startup_orphan_sweep in DEFAULT_CONFIG, next to its siblings ws_ping_interval / ws_ping_timeout / ws_orphan_reap_grace_s; the raw loader in tui_gateway.server reads the new key (fail-open on missing). cli-config.yaml.example and website/docs/user-guide/configuration.md follow the dashboard.* documentation pattern. - New regression test: a stranded 'active' row (ended_at NULL, no live runtime) is swept AND still recoverable via peer-keyed lookup and fully revivable via reopen_session afterward.
…ing on WS disconnect Reimplements the concept from #77129 on the current structure (viewer rebinding from #83716 and _client_gone_interrupt_requested clearing are preserved). _close_sessions_for_transport snapshots owned sessions under _sessions_lock, then wrote session['transport'] = _detached_ws_transport WITHOUT re-checking that the session still pointed at the disconnecting transport. A session.resume that rebinds the session to a new live transport between the snapshot and the stomp got knocked back onto the drop sentinel with an orphan-reap Timer armed against a client that is attached right now. The park now happens under _sessions_lock and first revalidates ownership: if the session already moved to a different live transport, the disconnect has nothing to tear down — skip the sentinel park AND the reap scheduling. Regression test simulates the rebind landing between snapshot and stomp. Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
…r detection) Without SO_KEEPALIVE a silently-dropped client (SSH tunnel reset, laptop sleep, NAT timeout) leaves the TCP leg half-open forever: receive_text() blocks indefinitely and the disconnect teardown (detach, orphan reap, resume replay) never runs. The server then leaks the session and never reclaims its orphans. _disable_nagle already reaches the raw socket, so enable keepalive there: SO_KEEPALIVE on, plus TCP_KEEPIDLE=30s / TCP_KEEPINTVL=10s / TCP_KEEPCNT=3 on Linux and TCP_KEEPALIVE=30s on macOS. A dead peer is now detected in ~60s instead of never. Best-effort like the Nagle tuning — any failure to reach the socket is logged at debug and skipped. Tests: new tests/tui_gateway/test_ws_keepalive.py fakes the socket and pins SO_KEEPALIVE + the platform-specific idle tuning, plus the no-transport no-raise path. tests/tui_gateway: 336 passed.
Contributor
૮ >ﻌ< ა ci reviewran on f7b20ac — chore: map contributor emails for ring-2 salvage (A2chitect,
|
alt-glitch
added
type/bug
andrexibiza
reviewed
Aug 24, 2026
andrexibiza
left a comment
andrexibiza
left a comment
Contributor
There was a problem hiding this comment.
Reviewed exact head f7b20ac9869b699905c02c9740c41d31db514432 against base 654d537088aa499e14b2a859534652dc80b16e7f and current main 8c8193dce5faf71d80bf98eb52bd896ee7be8a27. Current main is one formatter commit ahead of the PR base and only changes four Desktop test/store files outside this diff, so it does not change the finding below. Exact-head CI 32680878271, Docker 32680877901, and Nix 32680877909 are green.
Blocker — the startup sweep can end a session that is still live in another process
_sweep_orphaned_session_rows() calls SessionDB.sweep_orphaned_sessions() with exclude_ids=tuple(_live_session_ids()), but _live_session_ids() is deliberately only this process's _sessions. The DB, however, is shared by sibling Hermes processes on the same profile. The new DB predicate then treats ended_at IS NULL + source in tui/desktop/subagent + both clocks older than _SESSION_TTL_S as sufficient proof of orphanhood.
That is not equivalent to “owning process is dead.” The existing in-memory TTL contract shows the missing axis: _session_is_evictable() refuses to reap any session whose transport is still live, regardless of age. A concrete reachable interleaving is:
- Process A owns a live standalone TUI/WS session whose transport is healthy but which has been idle for >6h. Its own idle reaper correctly preserves it because
_transport_is_dead(session["transport"])is false. - Process B starts on the same profile/state.db and schedules the new startup sweep.
- B cannot see A's runtime in
_live_session_ids(). The persisted row satisfies both age predicates, so B stamps itended_at=now, end_reason='startup_orphan_reap'while A still owns the live runtime.
That makes durable lifecycle state disagree with a live authority and can expose the same stored session as recoverable/reopenable while the original runtime still exists. This is exactly the “other side” of #65194: the issue asks for rows whose owning websocket/process is provably gone, and is labeled sweeper:risk-session-state; hours-scale inactivity is evidence of quietness, not process death.
The repo already has the shape needed for an exact proof in hermes_cli.active_sessions: entries carry session_id, PID, and process start time and prune PID-reuse-safe dead owners. Today that registry is only materialized when the concurrent-session cap is enabled, so it cannot simply be assumed present. The repair should either decouple durable session ownership/liveness from that optional cap registry or introduce equivalent process/generation ownership evidence for the sweep. Then sweep only when the recorded owner is demonstrably dead. Please add an adversarial cross-process regression with (a) a TTL-stale row held by a live sibling owner that must be spared and (b) the same row after that owner dies that must be swept. The current test_spares_live_in_memory_and_gateway_rows only proves the process-local case and therefore cannot catch this failure.
What I checked beyond that blocker
The disconnect/rebind repair is pointed at the right TOCTOU boundary: the detach path now revalidates session["transport"] under _sessions_lock before sentinel-parking/scheduling reap, so a session already rebound to a different live transport is left alone. That is the correct composition with merged #93361/#93405 and with #77129's independently-developed race diagnosis (credit to @JoaoMarcos44 is preserved here).
The Bot Mode group-resume change also has the right failure polarity: only JSON-RPC 4007 is treated as “absent”; transient/other failures surface instead of minting a fork, with both non-4007 and genuine-4007 regressions. The stranded-session adoption keeps exact-ID-only donor selection, blocks readoption of archived donors, carries compression lineage, and has explicit divergence/retirement-failure tests. TCP keepalive is additive dead-peer detection rather than a replacement for the application-level reconnect/reap machinery. I did not find another merge blocker in those paths on this head.
Interlocks / provenance
- #93361 and #93405 are merged foundation; #93430 is a follow-up, not duplicate work.
- #93369 by @kshitijk4poor is the direct stranded-session-adoption predecessor being salvaged here.
- #77129 by @JoaoMarcos44 is the direct/concept predecessor for the disconnect/rebind race and is correctly co-authored/credited.
- #65422 by @halaprix is the immediate startup-sweep code donor, but its own PR explicitly says it folded in @hansai-art's competing #65478 work for the WS-sidecar scheduling site and desktop-source coverage. #93430 currently summarizes this as “#65422 + maintainer follow-up”; please carry #65478/@hansai-art forward explicitly when this consolidation lands/closes the predecessors. That is a superseded/competing lineage, not unrelated work.
- The positive
tui/desktop/subagentsource allowlist is the right boundary against the #60609 messaging-session routing loop; the blocker is ownership/liveness within that allowed set, not the source partition itself.
Once cross-process ownership is proven at the sweep boundary and covered by the live-owner/dead-owner regression, this ring looks structurally coherent.
This was referenced Aug 24, 2026
kshitijk4poor
added a commit
to kshitijk4poor/hermes-agent
that referenced
this pull request
Aug 24, 2026
Closes the TOCTOU window flagged in review on NousResearch#93369 (merged via NousResearch#93430): the divergence guard compared EXPORT-TIME message counts, but another backend can append donor messages between the export snapshot and the retire loop — that growth would be stamped behind the non-recoverable adopted_by_profile archive, the exact H2 class the guard exists to prevent, just via a narrower race. The retire loop now re-reads live donor vs local counts immediately before end_session and leaves the donor unretired (donor_retired=False, warn-logged) on any donor-ahead signal; the next resume's export-time guard then handles the divergence normally. Equal-count CONTENT divergence (donor rewind+rewrite) remains invisible to count comparison — documented as accepted: bytes stay in the donor store either way. New red-first-verified regression simulates the exact race by appending to the donor from inside an export_session_lineage wrapper. adoption+ownership suites: 25 passed; ruff clean.
kshitijk4poor
added a commit
that referenced
this pull request
Aug 24, 2026
Closes the TOCTOU window flagged in review on #93369 (merged via #93430): the divergence guard compared EXPORT-TIME message counts, but another backend can append donor messages between the export snapshot and the retire loop — that growth would be stamped behind the non-recoverable adopted_by_profile archive, the exact H2 class the guard exists to prevent, just via a narrower race. The retire loop now re-reads live donor vs local counts immediately before end_session and leaves the donor unretired (donor_retired=False, warn-logged) on any donor-ahead signal; the next resume's export-time guard then handles the divergence normally. Equal-count CONTENT divergence (donor rewind+rewrite) remains invisible to count comparison — documented as accepted: bytes stay in the donor store either way. New red-first-verified regression simulates the exact race by appending to the donor from inside an export_session_lineage wrapper. adoption+ownership suites: 25 passed; ruff clean.
6 tasks
and7777
pushed a commit
to and7777/hermes-agent
that referenced
this pull request
Aug 27, 2026
Closes the TOCTOU window flagged in review on NousResearch#93369 (merged via NousResearch#93430): the divergence guard compared EXPORT-TIME message counts, but another backend can append donor messages between the export snapshot and the retire loop — that growth would be stamped behind the non-recoverable adopted_by_profile archive, the exact H2 class the guard exists to prevent, just via a narrower race. The retire loop now re-reads live donor vs local counts immediately before end_session and leaves the donor unretired (donor_retired=False, warn-logged) on any donor-ahead signal; the next resume's export-time guard then handles the divergence normally. Equal-count CONTENT divergence (donor rewind+rewrite) remains invisible to count comparison — documented as accepted: bytes stay in the donor store either way. New red-first-verified regression simulates the exact race by appending to the donor from inside an export_session_lineage wrapper. adoption+ownership suites: 25 passed; ruff clean.
melon-xf
added a commit
to melon-xf/hermes-agent
that referenced
this pull request
Sep 3, 2026
Closes the TOCTOU window flagged in review on NousResearch#93369 (merged via NousResearch#93430): the divergence guard compared EXPORT-TIME message counts, but another backend can append donor messages between the export snapshot and the retire loop — that growth would be stamped behind the non-recoverable adopted_by_profile archive, the exact H2 class the guard exists to prevent, just via a narrower race. The retire loop now re-reads live donor vs local counts immediately before end_session and leaves the donor unretired (donor_retired=False, warn-logged) on any donor-ahead signal; the next resume's export-time guard then handles the divergence normally. Equal-count CONTENT divergence (donor rewind+rewrite) remains invisible to count comparison — documented as accepted: bytes stay in the donor store either way. New red-first-verified regression simulates the exact race by appending to the donor from inside an export_session_lineage wrapper. adoption+ownership suites: 25 passed; ruff clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ring 2 of the session-reconnect family
Summary
The remaining verified gaps in the session-reconnect family are closed: stranded rows from a dead gateway process are swept at startup (and stay recoverable), a closing socket can no longer knock a just-rebound session back onto the reap sentinel, bot-mode group resumes fail closed instead of forking history, stranded bot sessions are adopted on profile resume, silent 4001 rejections are logged, and WS sockets get OS-level dead-peer detection. Five contributor PRs salvaged with authorship preserved + one concept reimplemented with co-authorship.
Follow-up to the merged #93361/#93405 cluster; verified live against the real gateway (see Validation).
Changes
adopt_session_lineage_from; donor archived non-recoverable so resurrection can't undo adoption).ensureGroupChatSessionfails closed on transient resume errors — only 4007 means absent (mirrors the mergedfindExistingCanonicalChatsibling)._sess_nowaitlogs 4001 session-not-found rejections for diagnosability.startup_orphan_reapa recoverable end reason, moves config todashboard.startup_orphan_sweep, docs updated in the same diff._close_sessions_for_transportrevalidates under the sessions lock that the session still points at the disconnecting transport before sentinel-parking — a resume that rebound between snapshot and stomp is left alone (no park, no reap).dashboard.ws_ping_*.Validation
startup_orphan_reapAND it resumes live afterward; rebind-race: session survives its original socket's close (no sentinel, no reclaim); storm re-check: 3 drop/resume cycles, zero reclaimsResolves
Fixes #65194. Salvages (close with credit after merge): #93369, #92870, #90466, #65422, #77977; #77129 concept credited via Co-authored-by.
Infographic