Skip to content

fix(gateway): preserve and name a pending message whose session is gone, instead of an FK rejection - #133366

Draft
devtroniu wants to merge 3 commits into
NousResearch:mainfrom
devtroniu:fix/pending-message-recovery-survives-missing-session
Draft

devtroniu wants to merge 3 commits into
NousResearch:mainfrom
devtroniu:fix/pending-message-recovery-survives-missing-session

Conversation

@devtroniu

@devtroniu devtroniu commented Oct 5, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fixes a boot-time recovery pass that can never succeed for some payloads and reports each of them, on every restart, as a bare database error.

gateway/shutdown_flush.recover_pending_to_db() replays flush files by calling SessionDB.append_message(). messages.session_id foreign-keys sessions.id, so a payload whose session has no row in the target store — state.db rebuilt from a backup while the shutdown spool survived, a session pruned/archived out, a profile store retired — can never be replayed. Until now the append was attempted anyway, the rejection was caught by the generic handler, and the operator got:

WARNING gateway.shutdown_flush: Failed to recover pending message from /srv/hermes/home/pending_messages/pending-090b734e….json: FOREIGN KEY constraint failed

once per payload, per restart, indefinitely. The file is correctly kept (it is the only surviving copy of the message), but nothing says why it will never be recovered or which session is gone.

Measured on a live deployment (2026-10-05): two cap-dropped transcript spools, pending-090b734e12244ee4b1c3528a3f991e89.json and pending-492f148d17874ce5a9512200cfa1c4be.json, warned at 09:16:49Z and again at 10:40:26Z. Their session 20260929_140547_994b400d has no row in any store on the host (checked against every state.db under the Hermes home).

The fix probes the target store for the payload's session before appending, and translates a foreign-key rejection that races past the probe (session row vanishing mid-pass). A payload that cannot be replayed is kept, counted, and reported by condition, so the line names what an operator can act on:

WARNING gateway.shutdown_flush: Preserved 2 pending message(s) whose session is absent from the store they target:
pending-090b734e….json (session 20260929_140547_994b400d), pending-492f148d….json (session 20260929_140547_994b400d)
— kept in /srv/hermes/home/pending_messages for operator recovery (no message was dropped).

The payload itself is untouched and still on disk, so the existing operator path (re-home or archive by hand) works on a named, explained file. Nothing is dropped, and no append is attempted for a session that cannot accept one — the raw driver message is never echoed.

Related Issue

Fixes # (no issue filed for this; the report and the two spool files come from a self-hosted firm deployment, and get_session was used as the probe instead of inventing a new API — happy to file an issue first if maintainers prefer that order.)

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/shutdown_flush.py
    • MissingSessionError — the named condition (carries session_id), replacing the raw rejection.
    • _store_holds_session() — probes db.get_session(session_id); a store that cannot be probed (no method, or a probe that raises) counts as holding the session, so duck-typed backends keep the previous behaviour.
    • _append_or_name_the_condition() — appends, and raises MissingSessionError when the driver rejects with a foreign-key failure (the probe→append race).
    • recover_pending_to_db() — catches MissingSessionError per file, keeps the file, accumulates the stranded payloads, and logs one WARNING per pass naming each file and its session. The generic Failed to recover pending message … path is unchanged for every other failure.
  • tests/gateway/test_shutdown_flush.py — four tests: the measured transcript-spool shape, the slot shape, the probe/append race, and the property that matters most in one pass (a stranded payload must not strand the recoverable ones beside it). Each asserts the file survives, the session id is named, and neither FOREIGN KEY nor the old Failed to recover pending message text is logged.

How to Test

  1. Take the readings the tests encode: python -m pytest tests/gateway/test_shutdown_flush.py -q → 15 passed (11 pre-existing + 4 new); scripts/run_tests.sh tests/gateway/test_pending_queue_spool.py tests/gateway/test_profile_spool_recovery.py tests/gateway/test_session_messages_shutdown_preserve.py → 9 passed at the base revision and unchanged on the branch.
  2. RED witness: detach at the parent revision, materialise only the branch's test file, run it → 4 failed, 11 passed (the four fail as assertions — no collection error, since the file imports nothing new).
  3. Real-data probe (the payloads from the deployment, replayed against a temp state.db with no matching session row): at the parent revision it prints FOREIGN KEY lines: 2; on this branch FOREIGN KEY lines: 0, one WARNING naming both files and the session, both files still on disk with their JSON intact.

Checklist

Code

  • I've read the Contributing Guide
  • My commit message follows Conventional Commits (fix(gateway): …)
  • I searched for existing PRs — nothing addresses a foreign-key-rejected recovery payload
  • My PR contains only changes related to this fix (three commits — the fix, a code-health waiver on its boundary catch, and the merge of upstream main that kept the branch current; two files)
  • pytest tests/ -q — not run in full here (the checkout's full suite is far beyond a single-session budget); the touched surface and its guard suites are quoted above
  • python scripts/check — see the run quoted in the PR conversation
  • Tests added
  • Tested on macOS 26.6.2 (arm64, Python 3.14.7, pytest 9.1.1)

Documentation & Housekeeping

  • Docstrings updated in the touched module (recover_pending_to_db, the new helpers) — no README/docs change applies
  • No config keys added/changed — N/A
  • No architecture/workflow change — N/A
  • Cross-platform: pure stdlib + existing store API, no path or process assumptions added
  • No tool schema/description change — N/A

Screenshots / Logs

Parent revision, real payloads, temp store:

WARNING gateway.shutdown_flush: Failed to recover pending message from …/pending-090b734e12244ee4b1c3528a3f991e89.json: FOREIGN KEY constraint failed
WARNING gateway.shutdown_flush: Failed to recover pending message from …/pending-492f148d17874ce5a9512200cfa1c4be.json: FOREIGN KEY constraint failed
--- verdict ---  FOREIGN KEY lines: 2   files kept: 2   recovered: 0

On this branch, identical inputs:

DEBUG  gateway.shutdown_flush: Preserved pending message pending-090b734e12244ee4b1c3528a3f991e89.json: session 20260929_140547_994b400d has no row in the target store
DEBUG  gateway.shutdown_flush: Preserved pending message pending-492f148d17874ce5a9512200cfa1c4be.json: session 20260929_140547_994b400d has no row in the target store
WARNING gateway.shutdown_flush: Preserved 2 pending message(s) whose session is absent from the store they target: pending-090b734e12244ee4b1c3528a3f991e89.json (session 20260929_140547_994b400d), pending-492f148d17874ce5a9512200cfa1c4be.json (session 20260929_140547_994b400d) — kept in …/pending_messages for operator recovery (no message was dropped).
--- verdict ---  FOREIGN KEY lines: 0   lines naming the stranded session: 3   files kept: 2   recovered: 0

…ne, instead of an FK rejection

Recovery replays flush files through `SessionDB.append_message`, and `messages.session_id`
foreign-keys `sessions.id`. A payload whose session row no longer exists — state.db rebuilt from a
backup while the spool survived, a session pruned or its store retired — can never be replayed, so
the append was rejected and the operator got one bare

    WARNING gateway.shutdown_flush: Failed to recover pending message from <path>: FOREIGN KEY constraint failed

per payload, per restart, forever; the file (the only surviving copy of the message) stayed in
place, unnamed and unexplained.

Measured on a firm instance: two cap-dropped transcript spools warned at 2026-10-05 09:16:49Z and
again 10:40:26Z, their session `20260929_140547_994b400d` absent from every store on the host.

The pass now probes the target store for the payload's session (`get_session`) before appending,
and translates a foreign-key rejection that races past the probe. A stranded payload is kept,
counted and reported by condition — one WARNING per pass naming the file and the session, one DEBUG
per payload — so the line names what the operator can act on. No message is dropped, and no append
is attempted for a session that cannot accept one.

Tests: tests/gateway/test_shutdown_flush.py covers the measured transcript-spool shape, the slot
shape, the probe/append race, and the case that matters most in a pass — a stranded payload must not
strand the recoverable ones next to it.
…ason on the line

`scripts/check` (code health) is right: `_store_holds_session` catches `Exception`. The catch is the
point of the helper — a store that cannot be probed must not be read as "the session is gone", so
every failure means the append decides — and the reviewed waiver carries that reason.
@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 Oct 5, 2026
@whyyagswhy

Copy link
Copy Markdown

Verified at 4ef8fa0: shutdown-flush suite passes 15/15.

@marioGusmao

Copy link
Copy Markdown

Another data point from a self-hosted install (v0.21.6+305, upstream 0670ba45). Two transcript_cap_drop spool files containing only metadata (data.message.role: session_meta, no user text) survived a corruption-related rebuild of state.db, but their session rows did not. Since then the gateway logs FOREIGN KEY constraint failed recovery warnings on every start.

This PR improves the diagnosis and keeps the original files for recovery, but they remain eligible for replay on every later start. In the installed revision, is_row_rejection() deliberately leaves constraint failures retryable.

Could the new warning point operators to a documented action that preserves the evidence? For example, once the operator has confirmed which store owns the session and that the session will not be restored, renaming pending-<id>.json to pending-<id>.json.bad keeps the file out of both replay globs and retains it for manual recovery. Moving it out of the replay directory would work too.

I would avoid quarantining automatically just because get_session() returns None. That shows the row is missing from the selected store, not that it is gone for good from the owning store. Payloads with unresolved ownership or a temporary routing failure should stay retryable.

Since #133932 has merged, it would also be worth testing this change against its ordered, full-fidelity recovery and its per-session hold-back.

This branch has not been deployed

No deployments
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.

4 participants