fix(gateway): honor session_reset policy when recovering sessions - #78618
Closed
hillimited wants to merge 1 commit into
Closed
hillimited wants to merge 1 commit into
hillimited wants to merge 1 commit into
Conversation
Both session recovery paths (the startup stale-entry repoint and the lazy in-message recovery) rebuilt the routing entry with updated_at=now and never consulted _should_reset, so an opt-in idle/daily session_reset policy was silently dead across any gateway restart: a recovered session always looked freshly active, and since every subsequent message bumps updated_at, a session recovered stale could then never age out at all. Fix in three parts: - SessionDB.get_last_activity(session_id) returns the last stored message timestamp, and _create_entry_from_recovered_row derives updated_at from it (falling back to created_at). An invalid or missing started_at now maps to epoch 0 instead of now — an invalid durable timestamp must look old, never freshly active. reset_had_activity is set from the durable transcript so the continuity hint stays accurate. - _recover_session_from_db evaluates _should_reset on the rebuilt entry: an overdue session is durably promoted to a reset boundary (promote_to_session_reset, falling back to end_session) and the stale mapping is dropped instead of repointed. - _query_recoverable_session no longer reopens the row; the get_or_create_session recovery phase evaluates _should_reset first and either feeds the normal auto-reset create path (reset notice, prev_session_id continuity, durable promotion) or reopens and publishes the recovered entry exactly as before. Behavior is unchanged under the default session_reset mode "none": _should_reset returns None there, so recovery still resumes every recoverable row — only users who opted into idle/daily resets see the policy actually applied across restarts. Recovery stays lock-free on the message path (TestRecoverOutsideLock), and the pre-existing session recovery suites pass unmodified.
alt-glitch
added
type/bug
7 tasks
This was referenced Aug 9, 2026
teknium1
pushed a commit
that referenced
this pull request
Aug 9, 2026
Both session recovery paths (the startup stale-entry repoint and the lazy in-message recovery) rebuilt the routing entry with updated_at=now and never consulted _should_reset, so an opt-in idle/daily session_reset policy was silently dead across any gateway restart: a recovered session always looked freshly active, and since every subsequent message bumps updated_at, a session recovered stale could then never age out at all. Fix in three parts: - _create_entry_from_recovered_row derives updated_at from the durable last_activity_at the finder already returns on the row (no extra DB round-trip; the original PR added SessionDB.get_last_activity for this, unnecessary post-#82633), falling back to created_at. An invalid or missing started_at now maps to epoch 0 instead of now — an invalid durable timestamp must look old, never freshly active. reset_had_activity is set from the row's durable activity/message signals so the continuity hint stays accurate. - _recover_session_from_db evaluates _should_reset on the rebuilt entry: an overdue session is durably promoted to a reset boundary (promote_to_session_reset, falling back to end_session) and the stale mapping is dropped instead of repointed. - _query_recoverable_session no longer reopens the row; the get_or_create_session recovery phase evaluates _should_reset first and either feeds the normal auto-reset create path (reset notice, prev_session_id continuity, durable promotion) or reopens and publishes the recovered entry exactly as before. Behavior is unchanged under the default session_reset mode "none": _should_reset returns None there, so recovery still resumes every recoverable row — only users who opted into idle/daily resets see the policy actually applied across restarts. Cherry-picked from #78618 and adapted to the #82633 finder. (cherry picked from commit 31c71f7)
teknium1
pushed a commit
that referenced
this pull request
Aug 9, 2026
Both session recovery paths (the startup stale-entry repoint and the lazy in-message recovery) rebuilt the routing entry with updated_at=now and never consulted _should_reset, so an opt-in idle/daily session_reset policy was silently dead across any gateway restart: a recovered session always looked freshly active, and since every subsequent message bumps updated_at, a session recovered stale could then never age out at all. Fix in three parts: - _create_entry_from_recovered_row derives updated_at from the durable last_activity_at the finder already returns on the row (no extra DB round-trip; the original PR added SessionDB.get_last_activity for this, unnecessary post-#82633), falling back to created_at. An invalid or missing started_at now maps to epoch 0 instead of now — an invalid durable timestamp must look old, never freshly active. reset_had_activity is set from the row's durable activity/message signals so the continuity hint stays accurate. - _recover_session_from_db evaluates _should_reset on the rebuilt entry: an overdue session is durably promoted to a reset boundary (promote_to_session_reset, falling back to end_session) and the stale mapping is dropped instead of repointed. - _query_recoverable_session no longer reopens the row; the get_or_create_session recovery phase evaluates _should_reset first and either feeds the normal auto-reset create path (reset notice, prev_session_id continuity, durable promotion) or reopens and publishes the recovered entry exactly as before. Behavior is unchanged under the default session_reset mode "none": _should_reset returns None there, so recovery still resumes every recoverable row — only users who opted into idle/daily resets see the policy actually applied across restarts. Cherry-picked from #78618 and adapted to the #82633 finder. (cherry picked from commit 31c71f7)
Collaborator
|
Merged via #82743 (rebase-merge — your commit with your authorship preserved). Thank you @hillimited — recovered sessions now keep their real timestamps instead of updated_at=now, and both recovery paths consult reset policy before reopening. One simplification during salvage: your get_last_activity DB method became unnecessary because #82633's rewritten finder already returns last_activity_at on the row — the policy check now reads it directly. Clean diagnosis and tests; both carried over. |
ma1138569845
pushed a commit
to ma1138569845/dechnicAuditor-agent
that referenced
this pull request
Aug 10, 2026
Both session recovery paths (the startup stale-entry repoint and the lazy in-message recovery) rebuilt the routing entry with updated_at=now and never consulted _should_reset, so an opt-in idle/daily session_reset policy was silently dead across any gateway restart: a recovered session always looked freshly active, and since every subsequent message bumps updated_at, a session recovered stale could then never age out at all. Fix in three parts: - _create_entry_from_recovered_row derives updated_at from the durable last_activity_at the finder already returns on the row (no extra DB round-trip; the original PR added SessionDB.get_last_activity for this, unnecessary post-NousResearch#82633), falling back to created_at. An invalid or missing started_at now maps to epoch 0 instead of now — an invalid durable timestamp must look old, never freshly active. reset_had_activity is set from the row's durable activity/message signals so the continuity hint stays accurate. - _recover_session_from_db evaluates _should_reset on the rebuilt entry: an overdue session is durably promoted to a reset boundary (promote_to_session_reset, falling back to end_session) and the stale mapping is dropped instead of repointed. - _query_recoverable_session no longer reopens the row; the get_or_create_session recovery phase evaluates _should_reset first and either feeds the normal auto-reset create path (reset notice, prev_session_id continuity, durable promotion) or reopens and publishes the recovered entry exactly as before. Behavior is unchanged under the default session_reset mode "none": _should_reset returns None there, so recovery still resumes every recoverable row — only users who opted into idle/daily resets see the policy actually applied across restarts. Cherry-picked from NousResearch#78618 and adapted to the NousResearch#82633 finder. (cherry picked from commit 31c71f7)
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
Both session recovery paths (the startup stale-entry repoint and the lazy in-message recovery) rebuilt the routing entry with updated_at=now and never consulted _should_reset, so an opt-in idle/daily session_reset policy was silently dead across any gateway restart: a recovered session always looked freshly active, and since every subsequent message bumps updated_at, a session recovered stale could then never age out at all. Fix in three parts: - _create_entry_from_recovered_row derives updated_at from the durable last_activity_at the finder already returns on the row (no extra DB round-trip; the original PR added SessionDB.get_last_activity for this, unnecessary post-NousResearch#82633), falling back to created_at. An invalid or missing started_at now maps to epoch 0 instead of now — an invalid durable timestamp must look old, never freshly active. reset_had_activity is set from the row's durable activity/message signals so the continuity hint stays accurate. - _recover_session_from_db evaluates _should_reset on the rebuilt entry: an overdue session is durably promoted to a reset boundary (promote_to_session_reset, falling back to end_session) and the stale mapping is dropped instead of repointed. - _query_recoverable_session no longer reopens the row; the get_or_create_session recovery phase evaluates _should_reset first and either feeds the normal auto-reset create path (reset notice, prev_session_id continuity, durable promotion) or reopens and publishes the recovered entry exactly as before. Behavior is unchanged under the default session_reset mode "none": _should_reset returns None there, so recovery still resumes every recoverable row — only users who opted into idle/daily resets see the policy actually applied across restarts. Cherry-picked from NousResearch#78618 and adapted to the NousResearch#82633 finder. (cherry picked from commit 31c71f7)
stefanpieter
added a commit
to stefanpieter/hermes-agent-upstream-fork
that referenced
this pull request
Aug 11, 2026
The bug
The opt-in
session_resetpolicy (idle/daily) is silently dead across any gatewayrestart, because both session recovery paths resurrect sessions as freshly active:
SessionStore._create_entry_from_recovered_rowstamps the rebuilt routing entrywith
updated_at=now(and falls back tocreated_at=nowwhen the durablestarted_atis missing or invalid)._should_reset:_prune_stale_sessions_locked→_recover_session_from_db) repoints stalesessions.jsonentries to therecovered row and reopens it unconditionally;
get_or_create_sessionphase 3 →_query_recoverable_session) reopens and publishes the recovered entryunconditionally.
Because
_should_resetmeasures idleness againstentry.updated_at, a recoveredsession always looks zero-seconds idle. And since every subsequent message bumps
updated_atagain on the healthy path, a session recovered stale can then neverage out — the miss is self-perpetuating, not just delayed by one restart.
The existing runtime stale-guard fix (see
test_stale_agent_close_overdue_policy_creates_fresh_session) only covers the casewhere a stale in-memory entry with a real
updated_atexists. The pure recoverypaths — startup repoint, and a lost mapping rebuilt from state.db — still bypassed
the policy entirely.
The fix
SessionDB.get_last_activity(session_id) -> Optional[float](new, placedafter
get_session):SELECT MAX(timestamp) FROM messages WHERE session_id = ?through the standard
_read_ctx()read path._create_entry_from_recovered_rowderivesupdated_atfrom that durablelast message timestamp, falling back to
created_at. An invalid or missingstarted_atnow maps to epoch 0 instead ofnow— an invalid durable timestampmust look old, never freshly active.
reset_had_activityis set from the durabletranscript so the channel-continuity hint stays accurate for recovered resets.
The lookup is
getattr-guarded, so an older/mocked SessionDB without the newmethod degrades to the
created_atfallback._recover_session_from_db): build the entry first, thenevaluate
_should_reset(entry, source). On a reset reason the row is durablypromoted to a reset boundary (
promote_to_session_reset, falling back toend_session) andNoneis returned, so the pruner drops the stale mappinginstead of repointing it. Otherwise the row is reopened and the entry returned
exactly as before (including the migrated-legacy peer rewrite).
_query_recoverable_sessionno longer reopens the row — it returns thecandidate un-reopened so the caller decides reset vs resume. (The
migrated-legacy peer rewrite stays here; it is key-mapping bookkeeping, already
performed today even when the publish race is lost.)
get_or_create_sessionevaluates_should_reset(recovered, source). On a reason it feeds the existing auto-reset locals(
was_auto_reset=True,auto_reset_reason,reset_had_activity=recovered.reset_had_activity,db_end_session_id=recovered.session_id,prev_session_id), so the ordinarycreate+promote tail runs — reset notice, continuity hint, and durable promotion
all behave exactly like an in-memory expiry. Otherwise it reopens and publishes
the recovered entry as before.
Why this is behavior-preserving by default
Upstream's default
session_resetmode is"none"—_should_resetreturnsNonethere, so recovery still resumes every recoverable row with the samesession_id, the same reopen call, and the same peer bookkeeping. Only users who
opted into idle/daily resets see a change: the policy they configured now actually
applies across restarts. The recovery query stays lock-free on the message path
(
TestRecoverOutsideLockpasses unmodified), and evaluating_should_resetinsidethe startup pruner matches the existing lock discipline
(
prune_old_entriesalready calls_has_active_processes_safewith the lock held).All pre-existing session recovery suites pass unmodified.
One user-visible nuance worth stating: a session reset at startup recovery
returns
Noneand the mapping is pruned, so the next inbound message creates afresh session without the
was_auto_resetnotice (there is no message context atstartup). The message-path reset keeps the full notice/continuity behavior.
New tests
tests/gateway/test_session_store_runtime_stale_guard.py—TestRecoveredSessionResetPolicy:updated_at/created_atandreset_had_activity;auto-reset metadata,
promote_to_session_reset(sid, "idle"), no reopen;mode="none"recovery resumes unchanged (reopen called, nopromote/end, same session_id);
get_last_activityfalls back tocreated_at.tests/gateway/test_session_store_stale_prune.py—TestStartupRecoveryResetPolicy: overdue recovered session at startup ispromoted to reset and the mapping pruned;
mode="none"startup repointunchanged and now carries the durable
updated_at.tests/test_hermes_state.py—test_get_last_activity(empty/missing session,max-timestamp semantics against the real schema).
Test plan
Results on this branch (Python 3.12.13, venv with
.[dev,messaging]extras):271 passed — that is all pre-existing tests in those suites unmodified plus the
9 new ones. Wider sweep (
test_session_store_prune.py,test_channel_continuity_hint.py,test_10710/48031/35809/73297,test_clean_shutdown_marker.py,test_session_load_bool.py,test_async_session_store.py,test_fresh_reset_skill_injection.py): 34 passed,1 failed —
test_session_store_default_db_uses_runtime_hermes_home— which failsidentically on the unpatched base commit when run in that same batch (passes in
isolation on both): a pre-existing test-order isolation issue upstream, unrelated
to this change.