fix: gateway persists transcript to state.db itself instead of relying on agent's internal flush - #46471
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Gateway now persists transcript to state.db directly instead of relying on the agent's internal flush. More reliable persistence. Small, focused change.
Looks Good
- Single fix, clean implementation
- Moves persistence responsibility to the correct layer
Reviewed by Hermes Agent
Regression Risk: Duplicate DB WritesThis PR removes Current state on main: lines ~9149–9193 still use Concrete risk: After this PR, for every agent turn, both the gateway's Suggested approach: If the goal is to make the gateway the single source of truth for persistence, the agent's The other |
The agent's _flush_messages_to_session_db() runs in the same killable process. When pm2/systemd restart or OOM intervenes between the agent's flush and the gateway's response delivery, skip_db=True drops the assistant message from state.db. Instead of skipping the DB write, always write from the gateway and deduplicate by checking the last message in the session. If role and content match, the agent already persisted it — skip to avoid the duplicate-row bug (NousResearch#860). If they don't match, the agent's flush never ran (or was interrupted) and the gateway's write is the safety net. - gateway/run.py: Remove agent_persisted skip_db mechanism - gateway/session.py: Add last-message dedup in append_to_transcript() - hermes_state.py: Add SessionDB.get_last_message()
fd40989 to
1fbcbba
Compare
|
Good catch, thanks for the review. I've revised the approach: Instead of removing skip_db without counterpart: Gateway always writes to the DB, and now has a last-message dedup — before inserting, it checks whether the most recent message in the session already has the same role + content. If the agent's already persisted it, we skip (no duplicate row). If the agent was killed mid-flush and never wrote it, the gateway's write is the safety net. Changes:
The inconsistency concern is also addressed — there are no other call sites left in the file after this PR (the flag variable was only used in the four calls being changed). |
|
Thanks for this @rayjun — the bug in #46088 is real and well-diagnosed: if the gateway process dies (pm2/OOM) between the agent's Closing this particular fix though, because the dedup mechanism doesn't hold up and the direction fights an intentional (recently re-hardened) design:
The right fix targets the actual gap — the flush/delivery ordering window — using a per-message idempotency key rather than a tail role+content compare. Your reproduction and root-cause writeup were spot on — thank you. |
Root Cause
gateway/run.pyusesskip_db=Trueon allappend_to_transcript()calls, relying on the agent's internal_flush_messages_to_session_db()to persist messages. When pm2/systemd restart or OOM intervenes between the agent's flush and the gateway's response delivery, the assistant message is permanently lost from state.db — leaving a session of consecutiverole=usermessages with no replies.Fix
Remove the
skip_dbmechanism and letappend_to_transcriptalways write to SQLite. The gateway and agent already share the sameSessionDBinstance; the gateway writing its own entries is idempotent for messages the agent already persisted, and a safety net for the ones it didn't.Changes
gateway/run.py: Removeagent_persistedflag and allskip_db=agent_persistedargumentsgateway/session.py: Updateappend_to_transcriptdocstring to reflect new semanticsTesting
Pre-existing test suite passes with no new failures.
Closes #46088