Skip to content

fix(gateway): keep stale route when recovery lookup fails - #63265

Open
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/session-prune-recovery-fail-safe
Open

necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/session-prune-recovery-fail-safe

Conversation

@necoweb3

Copy link
Copy Markdown
Contributor

Summary

SessionStore._prune_stale_sessions_locked() could delete the only gateway routing entry when startup stale-session recovery hit a transient lookup error.

When sessions.json points at an ended parent session, startup pruning tries to recover the latest live gateway child through _recover_session_from_db(). If that recovery lookup fails unexpectedly, the current code treats it the same as “no recoverable row exists” and prunes the routing key. That loses the only fast routing handle for the peer, so the next gateway message can mint a fresh empty session instead of retrying recovery.

Why

This is the same fail-safe shape as the recent session-state P1 fixes: an indeterminate safety/recovery check should not destroy session routing state.

Keeping the stale entry on lookup failure is safer than deleting it, because the runtime stale guard can retry recovery on the next message. The existing behavior for a clean “no recovery found” result is unchanged.

Changes

  • Add an opt-in raise_on_lookup_error path to _recover_session_from_db().
  • Use it from startup stale pruning so lookup exceptions are distinguishable from “no recovered row.”
  • Keep the stale routing entry when recovery lookup is indeterminate.
  • Add a regression test proving the routing key survives a recovery lookup exception.

Tests

python -m pytest -p no:cacheprovider --basetemp=.pytest_tmp_session_prune tests/gateway/test_session_store_stale_prune.py -k "recovery_lookup_raises or repoints_stale_compression_parent or prunes_stale_entry_when_recovery_only_finds_same_ended_session" -q --timeout-method=thread
3 passed, 11 deselected

python -m pytest -p no:cacheprovider --basetemp=.pytest_tmp_session_prune tests/gateway/test_session_store_stale_prune.py tests/gateway/test_session_store_runtime_stale_guard.py -q --timeout-method=thread
25 passed

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused recovery fail-safe. Current main still swallows a recovery finder exception and returns None in gateway/session.py:1413-1415; startup pruning then treats that outcome as an unrecoverable route and queues the key for deletion in gateway/session.py:1157-1200. The PR changes that precise ambiguity while retaining the existing clean-no-match behavior.

The runtime path does retry recovery after detecting an ended route (gateway/session.py:1903-1920, gateway/session.py:1943-1956), consistent with the PR's intended preservation behavior. No blocking correctness or design-fit issues identified. The change is narrowly scoped and should be mechanically salvageable onto current main.

Automated hermes-sweeper review.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

Eight PRs address or reference this session-routing complex: #52808/#54138 implement crash-startup pruning, #54916/#55232/#55485 cover live routing and eviction variants, #62012/#62038 target ended-session resurrection, and #63265 preserves a stale route when recovery lookup is indeterminate. The diffs separate three causes: stale-key creation, recovery that reopens ended rows, and lookup failure being mistaken for a clean no-match.

Related pull requests

Duplicates

#52808 and #54138 implement substantially the same startup-pruning fix, with #54138 the merged reimplementation. #54916 and #55232 overlap in the older #54878 lane and were superseded by the more general merged #55485; #62038 competes with #62012 but is a partial alternative rather than an equivalent duplicate.

Suggested consolidation

Keep #63265 open with a salvage path: preserve its narrow exception-versus-no-match distinction and regression test, consistent with the maintainer-bot keep_open verdict. Keep recorded best fix #62012 open with the durable supersession and real-DB first-message test pending contributor re-review; for #62038, author action: rebase onto main, or split out a shared startup/runtime recovery decision that addresses its contributor review, since the current diff only delays reopening. #52808/#54916/#55232 should remain closed, while #54138/#55485 remain relevant merged references.

Cross-PR triage: Reviewed 8 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 86 kB of PR diffs, 32 kB of issue/PR text, 22 kB of discussion (25 comments), 14 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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