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.
alt-glitch
added
type/bug
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Request changes conclusion (GitHub does not allow a formal change request from this account)
Motivation
This PR addresses a real persistence failure. A live agent can keep _session_db_created=True after another surface deletes its session row. The next transcript append then fails the session foreign key. The current code logs the failure and drops every later durable turn. Recreating the row in the agent layer is the right boundary because the agent owns the complete session identity.
Approach
The patch adds a typed session_row_missing cause. _db_flush_failed() clears the stale row flag and flush cursors, calls _ensure_db_session(), and uses the existing one-retry budget. It also tries to clear all per-message durable markers so the deleted transcript can be rebuilt. This reuses the existing row creation and retry machinery.
Concrete changes
agent/session_persistence.py adds foreign-key recovery to the flush failure owner. hermes_state_errors.py adds the new cause. tests/agent/test_session_row_under_live_agent_persist.py covers row recreation, repeated deletion, and failed recreation with a real SessionDB.
Risk to main
There is one P1 blocker. The retry keeps the original conversation_history, and _db_flush_collect() treats matching object identities as already durable. The production turn builder uses messages = list(conversation_history), which preserves those identities. After deletion removes the row and all messages, the recovery still skips the historical prefix and writes only the new tail. A real-store regression reproduced this exact result: the call returned True, but only turn two was stored instead of turn one, answer one, and turn two.
There is also one P2 copy defect. The new typed cause has no entry in hermes_state_user_copy._STORAGE_FAILURES or agent.turn_explainers._PERSISTENCE_CAUSE_EXPLANATIONS. If recreation or the bounded retry fails, users still receive generic database-open, disk, or lock advice.
Validation at ad97047da0f9801a4fdca4478bfd68085f02c3a8: 52 committed and adjacent persistence tests passed; 7 storage-copy tests passed; Ruff passed; Python compilation passed; the Windows footgun scan passed; and git diff --check passed. No native Windows validation was performed. The focused reviewer regression failed as described above. No hosted checks are reported. Current upstream main equals the PR base.
Overall assessment
Request changes. Keep the agent-owned recovery design. Make the forced retry ignore the old-history identity shortcut, and add a regression using messages = list(history) plus a new tail. Add cause-specific user copy and tests for both error-copy consumers. The issue still has a maintainer decision open about whether explicit deletion should recreate the same session id or rotate it.
English verdict: REQUEST_CHANGES at exact head ad97047da0f9801a4fdca4478bfd68085f02c3a8. The recovery silently restores only the new tail in the real turn shape, so it does not yet prevent the reported transcript loss.
| return True | ||
| except Exception as e: | ||
| if _db_flush_failed(self, e, batch_rows, _adoption_budget): | ||
| if _db_flush_failed(self, e, batch_rows, _adoption_budget, messages): |
There was a problem hiding this comment.
P1 — The forced replay still skips the deleted historical prefix. This recursive call preserves conversation_history. _db_flush_collect() builds history_ids from it and skips any matching message object as already durable. The production turn builder uses messages = list(conversation_history), so those dict identities match even though deleting the session row also deleted every message row. A real SessionDB reproduction returned True but stored only the new turn two row, not the prior turn one / answer one rows. Pass a recovery signal that disables the history-identity shortcut (for example, retry with conversation_history=None) and add a regression with messages = list(history) plus a new tail.
There was a problem hiding this comment.
Fixed in #124117. The heal retry replays the history prefix via an explicit replay flag that stays pending (keyed to the session id) until a write succeeds. A real-SessionDB probe now stores all four rows. The kept test pins it, including a failed recreate and a failed retry.
| PERSISTENCE_ERROR_CAUSES = ( | ||
| "locked", "compression", "compression_closed", "turn_lease", "corrupt", "fts_index", | ||
| "replaced", "deleted_wal", "disk", "unknown", | ||
| "replaced", "deleted_wal", "disk", "session_row_missing", "unknown", |
There was a problem hiding this comment.
P2 — Add user-facing handling for the new cause. session_row_missing is now a public persistence cause, but neither hermes_state_user_copy._STORAGE_FAILURES nor agent.turn_explainers._PERSISTENCE_CAUSE_EXPLANATIONS handles it. If row creation or the bounded retry fails, describe_storage_failure() says the database could not be opened and recommends doctor --fix, while the turn explainer suggests disk or lock trouble. Add cause-specific entries and tests for both consumers so the new classification produces the promised diagnosis.
There was a problem hiding this comment.
Fixed in #124117. session_row_missing now has entries in both hermes_state_user_copy and agent.turn_explainers.
jonpol01
left a comment
jonpol01
left a comment
There was a problem hiding this comment.
Agent-side heal for #123583: on an FK reject in agent/session_persistence.py::_db_flush_failed, drop the stale _session_db_created, reset the flush cursors, re-run run_agent.py::AIAgent._ensure_db_session, and replay once within the existing _adoption_budget. That is the layer the triage on #123583 picked, and it mirrors _db_flush_adopt_compression_tip. Verified with scripts/run_tests.sh -j 2 tests/agent/test_session_row_under_live_agent_persist.py: 3 failed on main d0288be5b3 (fix files reverted, PR test kept) and 3 passed on head ad97047da0. Adjacent files on head (test_compression_closed_adoption, test_flush_diverts_on_corrupt_state_db, test_in_place_persist_marker, test_turn_completion_explainer, test_compression_persistence, test_rotation_flush_persisted_boundary, tests/hermes_state/test_deleted_wal_generation_guard, test_state_db_corrupt_quarantine, test_storage_health_latch): 90 passed, 0 failed, 13 skipped. ehz0ah's P1 (the history-identity shortcut skips the deleted prefix) and P2 (no user copy for the new cause) still stand, and nothing below repeats them. Preflight: #113999 edits the same spot in _db_flush_failed (right after classify_persistence_error(e)) and adds a row after (("locked", "busy"), "locked") in _PERSISTENCE_CAUSE_BY_PHRASE, so both files will conflict textually. #120862 touches _db_flush_write in the same file, in a separate hunk. #123725 is the entry-side half (a delete is refused while a turn lease is live) and complements this PR.
- [nonblocking] The heal never succeeds for a live delegate child whose parent row was deleted —
agent/session_persistence.py:325(_db_flush_failed, heal branch). Risk/repro, with a realSessionDB, a realAIAgent(session_id="C", parent_session_id="P")and the_delegate_frommarker thattools/delegate_tool.py:277sets: flush one turn, thendb.delete_session("P"), which cascades delegate children through_delete_delegate_children. Head output:{"t2": false, "t3": false, "C_row": false, "C_rows": 0, "cause": "session_row_missing"}, plusSession DB creation failed (will retry next turn): FOREIGN KEY constraint failedand...could not be recreated; will retry next flushon every flush.db.delete_sessions(["P", "C"])(the bulk endpoint) and two single deletes give the same result. The recreate passesparent_session_id=self._parent_session_id(run_agent.py:375), which points at the deletedP, so the row's ownsessions.parent_session_idFK rejects it on every retry. The comment at :327 calls this transient, but it is permanent for that agent's lifetime. The store already has a rule for a surviving child of a deleted row:delete_sessionnulls itsparent_session_id(hermes_state_sessions.py:1577-1578), andimport_moved_sessiondrops a missing parent the same way (hermes_state_profile_repair.py:217-219). Suggested fix: when the heal's create fails and the parent row is gone, create this row once without the parent. Do it for the create call only; don't clearagent._parent_session_iditself, becauseagent/turn_facade.py:66-70andagent/turn_context.py:766pass it to the relay and to plugin hooks as the subagent's parent. - [nonblocking] No docs for the user-visible change —
agent/session_persistence.py:316. After this PR, a session deleted withhermes sessions delete, the Desktop delete or--delete-after-verifiedcomes back under the same id on the owning agent's next flush if an agent still has it open.website/docs/user-guide/sessions.md("Delete a Session", and the--delete-after-verifiedparagraph at :459, which says deletion is verified inside the delete transaction) describes deletion as final. If the maintainers answer the open same-id-or-rotate question on #123583 with same-id, add one sentence there and to the zh-Hans mirrorwebsite/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/sessions.md, and put a "Docs:" line in the PR body.
Minor
- Test count (
tests/agent/test_session_row_under_live_agent_persist.py:75): there are three tests. The rootAGENTS.mdsets 1–2 invariant tests per fix.test_flush_recovers_when_row_deleted_between_turns_twiceruns the same path as the first test with a one-message list. Please confirm it can be folded into the first test as a second delete round. - Heal predicate vs classifier (
agent/session_persistence.py:309-310): the heal matches errorcode 787 or"foreign key constraint", butclassify_persistence_errormatches only"foreign key constraint failed"and never reads the errorcode (hermes_state_errors.py:290does the errorcode check only for the lock codes). Keying the branch onagent._last_persistence_error_cause == "session_row_missing"(set two lines above) and moving the 787 check into the classifier would give the heal and the turn-end explanation one definition. The classifier docstring (hermes_state_errors.py:269-276) lists every bucket and doesn't list the new one yet. - PR body lists "a profile-repair move" as a trigger.
hermes sessions repair-profiles --applyrefuses while a gateway owning any touched store is live (hermes_cli/sessions_repair_profiles.py:25). For a live CLI or TUI agent on the source store, this heal re-creates the moved row in the store the repair just emptied, which leaves the duplicate that thehermes_state_profile_repair.pymodule docstring (:17-20) leaves for the next run to settle. Please confirm that is intended.
Related — #123725 (entry-side delete refusal on a live turn lease; with it, finding 1's parent delete is refused only while P holds a live lease and only on the surfaces it guards), #113999 (same function and cause table, see preflight), #44266 (open; swallows the FK in the single-row append_message, a different direction from this PR's).
| agent._flushed_db_message_ids = set() | ||
| agent._last_flushed_db_idx = 0 | ||
| agent._session_db_created = False | ||
| agent._ensure_db_session() |
There was a problem hiding this comment.
[nonblocking] The heal never succeeds for a live delegate child whose parent row was deleted.
Repro on this head: a real SessionDB and AIAgent(session_id="C", parent_session_id="P") with the _delegate_from marker (tools/delegate_tool.py:277). Flush one turn, then db.delete_session("P"), which cascades delegate children. Result: {"t2": false, "t3": false, "C_row": false, "C_rows": 0}, with Session DB creation failed (will retry next turn): FOREIGN KEY constraint failed on every flush. _ensure_db_session recreates with parent_session_id=self._parent_session_id (run_agent.py:375), which still points at the deleted P, so the row's own sessions.parent_session_id FK rejects every retry. This isn't the transient case the comment below describes.
Suggested fix: when this create fails and the parent row is gone, create the row once without the parent. That is the rule delete_session applies to surviving children (hermes_state_sessions.py:1577-1578) and import_moved_session applies to a missing parent (hermes_state_profile_repair.py:217-219). Scope it to the create call; agent._parent_session_id itself feeds the relay and plugin hooks (agent/turn_facade.py:66-70, agent/turn_context.py:766).
There was a problem hiding this comment.
Fixed in #124117. When the recreate fails and the parent row is gone, it creates the row once without the parent for that call only; _parent_session_id is kept for the relay and hooks.
| # cached agent, so the cached `_session_db_created` flag is stale and every later append hits | ||
| # the FK). The deletion already erased the session's message rows with it, so the durable | ||
| # transcript is empty: drop the stale flag, reset the flush markers, and replay the FULL | ||
| # in-memory transcript onto the recreated row — not just the current tail (#123583). |
There was a problem hiding this comment.
[nonblocking] No docs for the user-visible change. A session deleted while an agent still has it open (hermes sessions delete, the Desktop delete, --delete-after-verified) now reappears under the same id on the owning agent's next flush. website/docs/user-guide/sessions.md ("Delete a Session", and :459) describes deletion as final. If the maintainers pick same-id on #123583, add one sentence there and to the zh-Hans mirror (website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/user-guide/sessions.md), and put a "Docs:" line in the PR body.
There was a problem hiding this comment.
Added in #124117: one sentence in website/docs/user-guide/sessions.md and the zh-Hans mirror.
|
Thanks @strzhao. Your heal lands through #124117 with authorship kept. Follow-ups there, in response to the review threads:
Closing in favour of #124117. |
|
Thanks @kshitijk4poor — clean landing, and the follow-ups folded into #124117 (full-prefix replay, replay-pending-until-write, confirmed-missing-only heal, plus the muted-turn and delegate-child edges) each close a real gap. This deployment tracks main, so once 819c624 soaks here I'm happy to report back if that's useful. |
5 participants
Fixes #123583
Root cause
_ensure_db_sessiontreats the cached_session_db_createdflag as proof the row exists (run_agent.py:350), sets it once at first creation (run_agent.py:378), and never re-checks it. The flush path only retries row creation while the flag is False (agent/session_persistence.py:407-408).Every store-side path that removes a session row is real and none is visible to the cached agent:
hermes sessions delete→db.delete_session(...)(hermes_cli/sessions_cmd.py:561,:587)hermes_cli/web_routers/sessions.py:715)tui_gateway/methods_session.py:1071)hermes_state_gateway.py:401)After any of these, every later turn's flush fails the FK and is dropped with one WARNING per turn (
Session DB append_message failed: FOREIGN KEY constraint failed). The agent keeps answering normally; the durable transcript silently stops growing; and because the failed transaction leaves no rows behind, there is nothing to find in the store after the fact.Fix
Follows the maintainer triage direction on the issue (teknium1, 2026-09-26):
Classification (
hermes_state_errors.py): newsession_row_missingcause inPERSISTENCE_ERROR_CAUSES— matched bysqlite_errorcode == SQLITE_CONSTRAINT_FOREIGNKEY(787) when the code survives, else the RPC-wrapped"foreign key constraint"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_session_db_createdflag, reset the flush markers (_flushed_db_message_ids = set(),_last_flushed_db_idx = 0), strip the per-message persisted markers, call_ensure_db_session(), and replay once within the flush's existing_adoption_budget(mirrors_db_flush_adopt_compression_tip). Because the row deletion erased the session's message rows with it, the replay is FULL — the whole in-memory transcript lands on the recreated row, not just the current tail. If row creation also fails, returnFalsewithout appending into a guaranteed rollback (scenario B) — fail-open, batch stays unmarked for the next flush. No new fail-closed path.Cost: ~+32 lines in one file + a 2-line cause registration, reusing the flush's existing single-retry machinery; no store-layer changes (the agent already holds every identity field via
_ensure_db_session, which is why store-side upserts mintsource='unknown'phantoms and the agent-side layer is the class fix).Tests
tests/agent/test_session_row_under_live_agent_persist.py— realAIAgent+ realSessionDBat a temp path, real store API (delete_session, the onehermes sessions delete/Desktop delete call):delete_sessionremoves the row, turn 2 flush returnsFalsewith the FK warning and 0 rows written — the exact silent-drop signature from the issue (maintainer's live probe:sqlite_errorcode=787)._ensure_db_session), cause set tosession_row_missing, and the FULL 4-message transcript lands (not just the deleted-then-retried tail).Falseand the flag staysFalse— no append into a guaranteed rollback.Adjacency: 73 existing persistence/flush/scheduler tests pass unchanged (compression persistence, in-place persist markers, rotation boundary, oneshot resume, CLI shutdown flush, session activity, incremental turns).
Relationship to other open work
The two open PRs matching
_session_db_created(#122704, #72694) both changeacp_adapter/session.py(ACP persist-ownership and /compress semantics) — different files, no overlap with this fix's flush-classification leg; this PR deliberately does not touch ACP semantics.