Skip to content

fix(gateway): adopt stranded bot sessions from the default store on profile resume - #93369

Closed
kshitijk4poor wants to merge 2 commits into
NousResearch:mainfrom
kshitijk4poor:fix/stranded-bot-session-adoption
Closed

kshitijk4poor wants to merge 2 commits into
NousResearch:mainfrom
kshitijk4poor:fix/stranded-bot-session-adoption

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Summary

Completes the Bot Mode reliability train for EXISTING installs: profile-scoped session.resume now adopts a session stranded in the default profile's store instead of hard-failing 4001/4007 forever.

The gap this closes

#93296/#93311 fixed RPC routing (dispatch by the session the RPC targets). But that fix is only forward-looking: every profile bot that ran BEFORE it accumulated its canonical session in the DEFAULT profile's state.db (the misroute wrote it there). After updating, the profile backend correctly receives the resume — and correctly reports it has no such session. The user experience is unchanged: the bot chat still doesn't work, now because the fix made the stranded conversation unreachable rather than misrouted.

Real-world shape (Teknium's Windows install, session c93770): weeks of Developer-bot history in root state.db; post-update resumes 4007 on the developer backend. Without this PR his options are "discard the chat and start over."

Changes

  • hermes_state_portability.pySessionDB.adopt_session_lineage_from(donor_db, session_id): composes the EXISTING export_session_lineage()import_sessions() primitives (no new import/export machinery). Donor rows are archived — never deleted — with end_reason='adopted_by_profile', deliberately NOT in RECOVERABLE_END_REASONS so fix(bot-mode): resurrect canonical Bot Chat archived by recoverable reasons on reopen (#92687) #93217's canonical-lookup resurrection cannot undo an adoption. Idempotent: re-running skips already-present ids.
  • tui_gateway/methods_session.py — profile-scoped session.resume (the owns_db branch only) falls back to adoption from the default store immediately before the 4007. Ids unknown to BOTH stores still 4007 exactly as before; launch-profile resumes never consult the fallback; adoption failure logs and falls through to the original error (fail-open to the old behavior, never a new failure mode).
  • tests/tui_gateway/test_stranded_session_adoption.py — 10 tests: 7 unit (move+messages, donor archived-not-deleted, non-resurrectable archive, idempotency, missing-donor reporting, compression-lineage adopted as a unit, unrelated sessions untouched) + 3 handler-level through the real server.handle_request (the live repro shape, unknown-everywhere still 4007, launch-profile path untouched).
  • tests/tui_gateway/test_session_resume_db_ownership.py — the not-found leak test now filters to path-scoped opens: the adoption probe may lazily construct the SHARED launch handle (db_path=None), which is never closed by design.

Validation

Suite Result
new adoption tests (red-first verified: handler test fails without the wiring) 10 passed
full tests/tui_gateway/ (1 pre-existing failure on clean main deselected) 573 passed
tests/test_hermes_state.py 237 passed
ruff on touched files clean

Follow-up to #93296 / #93311; fourth leg of #93091 after #93080 (venv/adapter), #93217 (resurrection), #93296+#93311 (routing).

…rofile resume

Pre-NousResearch#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 NousResearch#93296/NousResearch#93311; part of NousResearch#93091.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/sessions Session lifecycle, resume, persistence, history area/profiles Multi-profile isolation, HERMES_HOME scoping sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 24, 2026
…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.
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Overall: careful heal for a real data-stranding bug — composition over new machinery, archive-never-delete retirement, exact-id-only donors, owns_db gating, and an unusually strong test suite including the title-collision and non-vacuous gating regressions. Two residual windows:

  1. hermes_state_portability.py:353-366 — the divergence guard compares the export-time payload counts against the local store, but the export happens at the top of the same call (:339) and another backend process can append donor messages between export and retirement (:389-391). That growth gets archived behind a non-recoverable stamp — exactly what H2 was meant to prevent, just via a narrower race. Since the donor rows are still open at retire time, a cheap close-out would be: inside the retire loop, re-read len(donor_db.get_messages(seg_id)) versus len(self.get_messages(seg_id)) right before end_session, bailing to donor_retired=False on any donor-ahead signal. Same reasoning covers equal-count-but-diverged content (donor rewind/branch), which count comparison can't see — worth a comment acknowledging that limit even if unaddressed.

  2. tui_gateway/methods_session.py:571 — probing the default store goes through _get_db(), which lazily constructs the shared launch handle as a side effect on a path that previously never needed it (the updated leak test documents this). Benign today because the shared handle is never closed by design, but it means a profile resume of an unknown id now instantiates the default DB file where it didn't before — worth confirming no startup-order assumptions break (e.g., first-run systems where ~/.hermes/state.db doesn't exist yet).

Minor: the adoption log derives segment count from imported_ids + skipped_ids; verified those keys exist on the import result, so this is fine — just noting tests only pin imported/skipped, so a key rename there would silently zero the log rather than fail a test.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via ring-2 consolidated PR #93430 — both commits cherry-picked with authorship preserved (adopt stranded bot sessions from the default store on profile resume; your non-recoverable adopted_by_profile design kept intact). Live-verified against a real gateway before merge. Thanks @kshitijk4poor!

@kshitijk4poor

Copy link
Copy Markdown
Contributor Author

Verified both points rather than dismissing:

  1. Real — fixed in fix(adoption): re-check donor growth at retire time, not just at export #93739. The export-to-retire append window was exactly as described: the guard compared export-time counts, so donor growth in that window would be stamped behind the non-recoverable archive. The retire loop now re-reads live donor vs local counts immediately before end_session and bails to donor_retired=False on any donor-ahead signal (red-first-verified regression simulates the race by appending from inside an export_session_lineage wrapper). The equal-count content-divergence limit of count comparison is now acknowledged in-code as accepted — bytes stay in the donor store either way, only reachability differs.

  2. Verified benign, no change. The lazy _get_db() construction on the unknown-id path creates the default store the same way every launch-profile RPC already does (SessionDB.__init__ mkdirs + initializes schema idempotently); first-run systems get an empty valid store, and the handle is shared/never-closed by design (documented in the ownership leak test).

Minor point on the log deriving counts from imported_ids/skipped_ids: noted — #93739 keeps it as-is since a key rename there would break import_sessions' own contract tests first.

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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history comp/tui Terminal UI (ui-tui/ + tui_gateway/) 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.

4 participants