Skip to content

fix(sessions): finalize abandoned session rows - #93803

Open
zaczzk wants to merge 1 commit into
NousResearch:mainfrom
zaczzk:fix/abandoned-session-finalize
Open

zaczzk wants to merge 1 commit into
NousResearch:mainfrom
zaczzk:fix/abandoned-session-finalize

Conversation

@zaczzk

@zaczzk zaczzk commented Aug 24, 2026

Copy link
Copy Markdown

Summary

  • finalize old message-bearing session rows that have no end marker or activity heartbeat as terminal abandoned sessions
  • run the bounded sweep at gateway startup and expiry cadence, including every served multiplex profile under its own config and database scope
  • add state-layer and gateway regressions for eligibility, exclusions, idempotence, recovery behavior, configured idle windows, and cross-profile isolation

Validation

  • uv run --frozen --extra dev pytest -q tests/hermes_state/test_orphan_gateway_session_repair.py tests/gateway/test_abandoned_session_sweep.py (20 passed)
  • uv run --frozen --extra dev ruff check hermes_state.py gateway/run.py tests/hermes_state/test_orphan_gateway_session_repair.py tests/gateway/test_abandoned_session_sweep.py
  • git diff --check

Scope

This PR completes the bounded state-layer repair. Broader exit-path lifecycle changes for ACP/bridge ownership remain a separate follow-up.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 24, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ad0ea473b8846fdc059252b983c9426c043703b9 against base ec5e369fe68f4abb3188767c751d6166d2139f0a and current main 8e46e2c4c49391737e311aad12be829062364bac. The profile scoping and awaited AsyncSessionDB forwarding path check out. I found one P1 state-authority blocker.

P1 — missing heartbeat plus old start time is not authority to make a session permanently non-recoverable

GatewayRunner._sweep_abandoned_sessions() runs unconditionally at startup and on every expiry-watcher cadence, but passes only default_reset_policy.idle_minutes into the database sweep. It does not honor SessionResetPolicy.mode, per-platform reset overrides, current/sibling-process liveness, source ownership, or the newest transcript timestamp. SessionDB.finalize_abandoned_sessions() then selects on started_at < cutoff AND last_activity_at IS NULL and assigns terminal end_reason='abandoned', which this PR deliberately excludes from stale-route recovery.

The new real-DB multiplex regression proves the unsafe case rather than preventing it: it constructs GatewayConfig(multiplex_profiles=True) (default reset mode none), appends "hello" at the current time, backdates only started_at, clears last_activity_at, and expects both rows to be finalized. A persistent profile can therefore lose canonical routing to a transcript with a fresh message merely because heartbeat metadata is absent. This is a permanent session-boundary transition, not harmless cleanup metadata.

That conflicts with the merged startup-sweep contract in #93430: its #65422 / @halaprix lineage requires both started_at and newest message time to be stale, limits the source set, excludes live in-memory IDs, and uses recoverable startup_orphan_reap; #93430 also preserves #65478 / @hansai-art for the WS-sidecar/Desktop coverage. Open #92744 by @daglimmer is the complementary stale-open backlog surface and already uses the shared freshest-of-heartbeat/latest-message/start recency rule while archiving reversibly. #56603 by @adityachaudhary99 is the adjacent actual-expiry finalizer and explicitly leaves mode: none sessions alone until a real expiry event.

Required before merge:

  • Bind this mutation to explicit policy/ownership authority: honor mode: none and per-source overrides, or introduce a separate opt-in maintenance policy instead of borrowing idle_minutes while ignoring its mode.
  • Select and revalidate on canonical real recency (the existing _sql_session_last_active shape), not started_at alone.
  • Do not assign a permanently non-recoverable reason without positive owner-death evidence; ambiguous legacy backlog should use the recoverable/archive lane.
  • Add adversarial regressions proving that default mode: none, old-start + fresh-message + null-heartbeat, per-platform overrides, and a live owner all survive, while a genuinely stale/dead row finalizes.

Exact-head hosted CI/status evidence is absent at review time; the PR reports 20 focused local passes plus Ruff. Even a hosted green run would not close this blocker while the real-DB regression continues to assert the unsafe outcome.

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

3 participants