fix(agent): a chat whose session row was deleted recreates it instead of dropping every later turn - #124117
Merged
kshitijk4poor merged 14 commits intoSep 26, 2026
Conversation
…ch#123583) `_ensure_db_session` trusts the cached `_session_db_created` flag as proof the row exists, and the flush path only retries row creation while that flag is False. Any store-side removal of the row under a live agent — `hermes sessions delete`, the Desktop/web delete, bulk prune, a profile-repair move, an in-place store rebuild — leaves the flag stale, so every later turn's append fails the FK and is dropped with one WARNING per turn. The agent keeps answering; the durable transcript silently stops growing, and because the failed transaction leaves no rows behind there is no post-hoc trace in the store. Classification (`hermes_state_errors.py`): add the `session_row_missing` cause — matched by `SQLITE_CONSTRAINT_FOREIGNKEY` (787) when the code survives, else the RPC-wrapped phrase — so the turn-end explanation names the real failure instead of "unknown". Heal (`agent/session_persistence.py::_db_flush_failed`): on that cause, drop the stale flag, reset the flush markers, call `_ensure_db_session()`, and replay once within the flush's existing `_adoption_budget`. Because the deletion erased the session's message rows too, the replay clears the per-message persisted markers so the FULL in-memory transcript lands on the recreated row, not just the current tail (mirrors `_db_flush_adopt_compression_tip`). If row creation also fails, return False without appending into a guaranteed rollback — fail-open, batch stays unmarked for the next flush. No new fail-closed path. (cherry picked from commit ad97047)
The heal retry passed the caller's conversation_history through, so _db_flush_collect's id()-based history shortcut treated the prefix as already durable and skipped it. The row was recreated with only the new tail and the flush returned True: a silent partial restore. On the session_row_missing retry, pass conversation_history=None so the full in-memory transcript lands on the recreated row. Co-authored-by: 赵桂雄 <daniel21436@hotmail.com>
The heal predicate (errno 787 or a 'foreign key constraint' substring) and the classifier phrase were two separate definitions of the same failure. Move the SQLITE_CONSTRAINT_FOREIGNKEY code check into classify_persistence_error, document the bucket, and branch the heal on agent._last_persistence_error_cause == 'session_row_missing' so the two can't drift.
Without entries in _STORAGE_FAILURES and _PERSISTENCE_CAUSE_EXPLANATIONS a failed heal fell back to the generic disk/lock advice, which sends users chasing the wrong problem.
Deleting a parent session cascades to its delegate children. A live child then can never be recreated: create_session(parent_session_id=...) hits the parent FK on every flush for the agent's lifetime. When the recreate fails and the parent row is gone, create once without the parent for that call only; agent._parent_session_id is restored because the relay and hooks still key on it.
Rewrite the heal test to the real turn shape (messages = history + tail with conversation_history=history), which is the shape that exposed the partial-replay bug, and fold the 'deleted twice' case in as extra delete rounds. Keep the fail-open Scenario B test. Stack budget is two tests.
A session deleted while its chat is still running is recreated under the same id with the full in-memory transcript on the next save (the gateway session-key mapping expects the id to be stable). Say so next to `hermes sessions delete`, in English and the zh-Hans mirror. Fixes NousResearch#123583 Co-authored-by: 赵桂雄 <daniel21436@hotmail.com>
…n row The session-row heal retried with conversation_history=None so the history prefix would be replayed onto the recreated row. On a muted notification-reply turn that also made _db_flush_collect treat every history row as new: each replayed row was written display_kind=hidden and the flag was stamped onto the live in-memory history dicts, hiding the user's whole past transcript from pollers from then on. Keep the history set on the heal retry and pass an explicit replay_history flag instead: history rows are written again (not stamped durable) but keep their original visibility, and only this turn's new rows get the mute treatment.
The FK classifier behind session_row_missing also matches the sessions
table's own parent_session_id / system_prompt_hash FKs, and session
create is an upsert. Healing on the classification alone could replay
the whole transcript into a still-live row and duplicate it. Heal only
when get_session(session_id) confirms the row is gone.
The delegate-child branch's get_session(parent_id) was unguarded: a
store error there escaped the flush's error handler. Route both lookups
through one guarded helper (as the compression-tip helper does); a failed
lookup is not proof of deletion, so it falls through to the existing
"will retry next flush" return.
While here: _db_flush_failed now returns which retry to take
("adopted" / "healed" / None) instead of the caller string-comparing
_last_persistence_error_cause; its docstring covers the heal branch;
messages is required (the only caller always passes it); the marker
reset reuses _strip_persistence_markers. Tests drop the change-detector
assertion on the lingering cause after a successful heal and the Scenario
B docstring now says fail-closed, which is what it asserts.
The getattr(..., 787) fallback guarded a constant every supported Python (>=3.11) always defines; reference it directly and drop the module-level alias.
The two heal tests stayed green with agent/session_persistence.py reverted to before the muted-turn and confirmed-missing fixes, so neither fix was guarded. Extend the existing tests (no new test functions): - recreate test: a heal during a muted notification turn must hide only that turn's new rows, not the replayed history (reverted: history rows and their in-memory dicts turn 'hidden'). - fails-closed test (renamed from ..._fails_open_... to match its docstring): an FK failure while the row still exists must not heal or duplicate the transcript (reverted: `one, a1, one, two`), and a raising parent-row lookup for a delegate child must fail the flush closed instead of leaking OperationalError.
When the session row is gone and the heal cannot recreate it, the flush fails closed with the flush markers already stripped. The next flush then recreates the row up front via _ensure_db_session, so no FK error fires, no heal runs, and _db_flush_collect stamps the history prefix as durable without writing it. The recreated row ends up holding only the new tail (probe: rows ['a2'] instead of ['one', 'a1', 'two', 'a2']). Remember the failed heal per session id and have the next flush for that session replay the history prefix; clear it once a flush succeeds. Co-authored-by: 赵桂雄 <daniel21436@hotmail.com>
… heal When the heal recreated the session row but its single retry write then failed (database locked, turn lease, disk), the replay was carried only by the per-call _replay_history argument. The next flush found a live row, hit no FK error, ran no heal, and stamped the history prefix durable without writing it: the durable transcript silently lost everything but the tail (scratch probe: rows ['a2'] instead of ['one','a1','two','a2']). Set _session_row_replay_pending in the heal branch right after the markers are stripped, so any exit after a heal (recreate failed, retry failed) leaves the replay pending until a write succeeds. That makes the separate recreate-failed assignment and the _replay_history parameter redundant; the retry is a plain _adoption_budget=0 call. The fails-closed test now covers FK -> heal -> locked retry -> full replay on the next flush. Co-authored-by: 赵桂雄 <daniel21436@hotmail.com>
Gate mutation showed deleting the clear of _session_row_replay_pending after a successful write survived the suite. A flag that never clears re-appends any unmarked history dicts on every flush (e.g. history rehydrated per turn on a cached agent). Assert it is None after the turn-3 flush.
kshitijk4poor
enabled auto-merge (rebase)
September 26, 2026 16:54
This was referenced Sep 26, 2026
alt-glitch
added
type/bug
6 of 12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A running chat whose session was deleted from storage (Desktop delete,
hermes sessions delete,--delete-after-verified) no longer silently stops saving. Its next save recreates the session under the same id with the full in-memory transcript. Fixes #123583. This salvages #123641 by @strzhao (赵桂雄).Why
_session_db_created=Trueafter another surface deletes its session row. The next transcript append fails the sessions foreign key, the failure is only logged, and every later turn in that chat is dropped fromstate.db.Changes
session_row_missingcause._db_flush_failedheals only when the row is confirmed gone: it clears the stale row flag and flush cursors, re-runs_ensure_db_session()and retries once within the existing retry budget.get_sessionconfirms the row is gone._parent_session_iditself is kept, because the relay and hooks key on it. jonpol01 found this.session_row_missinggets its own text in the storage-failure message and the turn explainer, so a failed heal no longer says the database couldn't be opened.sessions.md). Deleting a session still open in a running chat doesn't stop that chat; close it first if you want the session gone.Behaviour notes
Validation
tests/agent/test_session_row_under_live_agent_persist.py: 2 invariant tests. Both fail on main.test_compression_closed_adoption.py: 7 passed. The 1 failure comes from the local test home guard (the local venv sits under~/.hermes), not this change; it passes in an isolated copy.SessionDB).['one','a1','two','a2']. The first version stored only['a2'], and main stores nothing after the delete.Not included
_write_guards_rejectguard main already has. The right home is an opt-in flag ondelete_session/delete_sessionswired through the web, API-server, CLI and TUI delete paths. fix(sessions): refuse to delete a session row a live turn still owns (#123583) #123725 stays open for that follow-up.Credit
@strzhao's commit from #123641 is kept with authorship, and follow-ups that rework it carry a Co-authored-by trailer. Review findings came from ehz0ah (the history replay and user copy) and jonpol01 (the delegate child and docs).