fix(agent): persist messages by intrinsic marker to stop id() reuse data loss by rrevenanttt · Pull Request #50372 · NousResearch/hermes-agent

rrevenanttt · 2026-06-21T19:41:23Z

What does this PR do?

_flush_messages_to_session_db decided which messages were already durable
purely by object identity: if id(msg) in flushed_ids: continue. On a
long-lived AIAgent (CLI/TUI/gateway) that dedup set survives across turns,
and that is dangerous. Once a previously-flushed message dict is dropped from
the live list — which happens routinely via
_drop_trailing_empty_response_scaffolding popping orphaned tool/assistant
pairs, in-place compaction replacing tool outputs, and repair_message_sequence
compaction — the dict is garbage-collected and CPython is free to hand its
address to a brand-new message. The new (real) assistant or tool message then
has an id() that already lives in flushed_ids, so it is treated as
already-written and is silently never persisted to state.db.

The impact is severe and hard to detect: non-deterministic, silent loss of
real conversation turns from the durable transcript. The user sees a normal
session, but on resume the turn is simply gone — no error, no warning. Because
it depends on allocator address reuse it surfaces intermittently, exactly the
kind of corruption that erodes trust in session persistence.

The fix removes the reusable address from the trust path entirely. Persistence
is now tracked with an intrinsic per-message marker (_db_persisted) stamped
on the dict itself the moment it is written. A marker bound to the object
cannot be aliased onto a recycled address, so a real turn can never be skipped.
This is safe by construction: the marker uses the mandatory _ prefix, and the
wire sanitizers already strip every top-level _-prefixed key before a request
leaves the process, so it never reaches a strict OpenAI-compatible gateway.
The existing _flushed_db_message_ids seed contract (gateway shutdown, tests
populate it with live-object ids right before the flush) is preserved: those
ids are valid at seed time, so we translate them into durable markers once and
then clear the set, guaranteeing no stale id can accumulate across turns and
alias a future message.

Related Issue

N/A

Type of Change

🐛 Bug fix (non-breaking change that fixes an issue)
✨ New feature (non-breaking change that adds functionality)
🔒 Security fix
📝 Documentation update
✅ Tests (adding or improving test coverage)
♻️ Refactor (no behavior change)
🎯 New skill (bundled or hub)

Changes Made

run_agent.py: added the _DB_PERSISTED_MARKER constant documenting the
id()-reuse hazard and the wire-sanitizer guarantee.
run_agent.py: rewrote the dedup loop in _flush_messages_to_session_db to
skip on the intrinsic _db_persisted marker instead of id(msg), treat
_flushed_db_message_ids as a one-shot seed (translated to markers), and
clear that set after every flush so no recyclable id outlives its turn.
tests/run_agent/test_identity_flush.py: added
test_flush_does_not_retain_object_ids_across_turns and
test_recycled_id_in_dedup_set_still_persists_new_message covering the data
loss and the marker-based dedup contract.

How to Test

Run the focused suite: scripts/run_tests.sh tests/run_agent/test_identity_flush.py.
The two new tests fail against the old id()-keyed logic (a stale id aliases
a fresh message and it is dropped) and pass with the marker-based dedup.
Regression sweep — persistence, dedup and compaction paths stay green:
scripts/run_tests.sh tests/run_agent/ tests/gateway/test_13121_shutdown_inflight_transcript_flush.py tests/gateway/test_agent_cache.py.

Checklist

Code

I've read the Contributing Guide
My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
I searched for existing PRs to make sure this isn't a duplicate
My PR contains only changes related to this fix/feature (no unrelated commits)
I've run pytest tests/ -q and all tests pass
I've added tests for my changes (required for bug fixes, strongly encouraged for features)
I've tested on my platform: macOS 15 (Darwin 25.5), Python 3.13

Documentation & Housekeeping

I've updated relevant documentation (README, docs/, docstrings) — or N/A
I've updated cli-config.yaml.example if I added/changed config keys — or N/A
I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…ata loss ## What does this PR do? `_flush_messages_to_session_db` decided which messages were already durable purely by object identity: `if id(msg) in flushed_ids: continue`. On a long-lived `AIAgent` (CLI/TUI/gateway) that dedup set survives across turns, and that is dangerous. Once a previously-flushed message dict is dropped from the live list — which happens routinely via `_drop_trailing_empty_response_scaffolding` popping orphaned tool/assistant pairs, in-place compaction replacing tool outputs, and `repair_message_sequence` compaction — the dict is garbage-collected and CPython is free to hand its address to a brand-new message. The new (real) assistant or tool message then has an `id()` that already lives in `flushed_ids`, so it is treated as already-written and is silently never persisted to `state.db`. The impact is severe and hard to detect: non-deterministic, silent loss of real conversation turns from the durable transcript. The user sees a normal session, but on resume the turn is simply gone — no error, no warning. Because it depends on allocator address reuse it surfaces intermittently, exactly the kind of corruption that erodes trust in session persistence. The fix removes the reusable address from the trust path entirely. Persistence is now tracked with an intrinsic per-message marker (`_db_persisted`) stamped on the dict itself the moment it is written. A marker bound to the object cannot be aliased onto a recycled address, so a real turn can never be skipped. This is safe by construction: the marker uses the mandatory `_` prefix, and the wire sanitizers already strip every top-level `_`-prefixed key before a request leaves the process, so it never reaches a strict OpenAI-compatible gateway. The existing `_flushed_db_message_ids` seed contract (gateway shutdown, tests populate it with live-object ids right before the flush) is preserved: those ids are valid at seed time, so we translate them into durable markers once and then clear the set, guaranteeing no stale id can accumulate across turns and alias a future message. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) - [ ] ✨ New feature (non-breaking change that adds functionality) - [ ] 🔒 Security fix - [ ] 📝 Documentation update - [ ] ✅ Tests (adding or improving test coverage) - [ ] ♻️ Refactor (no behavior change) - [ ] 🎯 New skill (bundled or hub) ## Changes Made - `run_agent.py`: added the `_DB_PERSISTED_MARKER` constant documenting the id()-reuse hazard and the wire-sanitizer guarantee. - `run_agent.py`: rewrote the dedup loop in `_flush_messages_to_session_db` to skip on the intrinsic `_db_persisted` marker instead of `id(msg)`, treat `_flushed_db_message_ids` as a one-shot seed (translated to markers), and clear that set after every flush so no recyclable id outlives its turn. - `tests/run_agent/test_identity_flush.py`: added `test_flush_does_not_retain_object_ids_across_turns` and `test_recycled_id_in_dedup_set_still_persists_new_message` covering the data loss and the marker-based dedup contract. ## How to Test 1. Run the focused suite: `scripts/run_tests.sh tests/run_agent/test_identity_flush.py`. 2. The two new tests fail against the old `id()`-keyed logic (a stale id aliases a fresh message and it is dropped) and pass with the marker-based dedup. 3. Regression sweep — persistence, dedup and compaction paths stay green: `scripts/run_tests.sh tests/run_agent/ tests/gateway/test_13121_shutdown_inflight_transcript_flush.py tests/gateway/test_agent_cache.py`. ## Checklist ### Code - [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md) - [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.) - [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate - [x] My PR contains **only** changes related to this fix/feature (no unrelated commits) - [x] I've run `pytest tests/ -q` and all tests pass - [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features) - [x] I've tested on my platform: macOS 15 (Darwin 25.5), Python 3.13 ### Documentation & Housekeeping - [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A - [x] I've considered cross-platform impact (Windows, macOS) per the [compatibility guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md#cross-platform-compatibility) — or N/A - [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…ata loss _flush_messages_to_session_db deduped persisted messages with a retained {id(msg)} set (_flushed_db_message_ids) kept across turns. Once a flushed dict is dropped from the live list (scaffolding rewind / in-place compaction) and GC'd, CPython recycles its address onto a new assistant/tool dict whose id() collides with the stale entry — so the real turn is silently never written to state.db. Replace the retained id-set with an intrinsic _DB_PERSISTED_MARKER stamped on each dict. The id-set is demoted to a one-shot seed (valid only while the caller's objects are alive) that is translated to markers and cleared after every flush, so no id() outlives a flush to alias a future message. The marker is _-prefixed so the wire sanitizers strip it before any request leaves. Preserves the existing _is_ephemeral_scaffolding skip. Salvaged from #50372. Co-authored-by: rrevenanttt <290873280+rrevenanttt@users.noreply.github.com>

… list (NousResearch#48677) The persist user-message override was applied in place to the live messages list. On the early crash-resilience persist (which runs BEFORE api_messages is built), that stripped observed group-chat context off the live user message and silently dropped it when observe_unmentioned_group_messages was enabled. Fix at the single chokepoint: _flush_messages_to_session_db resolves the override (idx/content/timestamp) locally and applies it ONLY to the row written to the DB — the live dict is never mutated, so EVERY persist caller (early persist, mid tool-loop flush, /resume, /branch) is protected uniformly. This supersedes the earlier shallow-copy approach, which broke the intrinsic _DB_PERSISTED_MARKER idempotency (copies never propagated the marker back to the live dicts → duplicate rows) and closes the sibling class tracked in NousResearch#56303. Trailing empty-response scaffolding is still dropped from the live list in _persist_session (unchanged behavior). Salvaged from NousResearch#48817; chokepoint reworked to coexist with the marker-based dedup (NousResearch#50372). Co-authored-by: kyssta-exe <kyssta-exe@users.noreply.github.com>

… list (#48677) The persist user-message override was applied in place to the live messages list. On the early crash-resilience persist (which runs BEFORE api_messages is built), that stripped observed group-chat context off the live user message and silently dropped it when observe_unmentioned_group_messages was enabled. Fix at the single chokepoint: _flush_messages_to_session_db resolves the override (idx/content/timestamp) locally and applies it ONLY to the row written to the DB — the live dict is never mutated, so EVERY persist caller (early persist, mid tool-loop flush, /resume, /branch) is protected uniformly. This supersedes the earlier shallow-copy approach, which broke the intrinsic _DB_PERSISTED_MARKER idempotency (copies never propagated the marker back to the live dicts → duplicate rows) and closes the sibling class tracked in #56303. Trailing empty-response scaffolding is still dropped from the live list in _persist_session (unchanged behavior). Salvaged from #48817; chokepoint reworked to coexist with the marker-based dedup (#50372). Co-authored-by: kyssta-exe <kyssta-exe@users.noreply.github.com>

…ata loss _flush_messages_to_session_db deduped persisted messages with a retained {id(msg)} set (_flushed_db_message_ids) kept across turns. Once a flushed dict is dropped from the live list (scaffolding rewind / in-place compaction) and GC'd, CPython recycles its address onto a new assistant/tool dict whose id() collides with the stale entry — so the real turn is silently never written to state.db. Replace the retained id-set with an intrinsic _DB_PERSISTED_MARKER stamped on each dict. The id-set is demoted to a one-shot seed (valid only while the caller's objects are alive) that is translated to markers and cleared after every flush, so no id() outlives a flush to alias a future message. The marker is _-prefixed so the wire sanitizers strip it before any request leaves. Preserves the existing _is_ephemeral_scaffolding skip. Salvaged from NousResearch#50372. Co-authored-by: rrevenanttt <290873280+rrevenanttt@users.noreply.github.com>

… list (NousResearch#48677) The persist user-message override was applied in place to the live messages list. On the early crash-resilience persist (which runs BEFORE api_messages is built), that stripped observed group-chat context off the live user message and silently dropped it when observe_unmentioned_group_messages was enabled. Fix at the single chokepoint: _flush_messages_to_session_db resolves the override (idx/content/timestamp) locally and applies it ONLY to the row written to the DB — the live dict is never mutated, so EVERY persist caller (early persist, mid tool-loop flush, /resume, /branch) is protected uniformly. This supersedes the earlier shallow-copy approach, which broke the intrinsic _DB_PERSISTED_MARKER idempotency (copies never propagated the marker back to the live dicts → duplicate rows) and closes the sibling class tracked in NousResearch#56303. Trailing empty-response scaffolding is still dropped from the live list in _persist_session (unchanged behavior). Salvaged from NousResearch#48817; chokepoint reworked to coexist with the marker-based dedup (NousResearch#50372). Co-authored-by: kyssta-exe <kyssta-exe@users.noreply.github.com>

…ata loss _flush_messages_to_session_db deduped persisted messages with a retained {id(msg)} set (_flushed_db_message_ids) kept across turns. Once a flushed dict is dropped from the live list (scaffolding rewind / in-place compaction) and GC'd, CPython recycles its address onto a new assistant/tool dict whose id() collides with the stale entry — so the real turn is silently never written to state.db. Replace the retained id-set with an intrinsic _DB_PERSISTED_MARKER stamped on each dict. The id-set is demoted to a one-shot seed (valid only while the caller's objects are alive) that is translated to markers and cleared after every flush, so no id() outlives a flush to alias a future message. The marker is _-prefixed so the wire sanitizers strip it before any request leaves. Preserves the existing _is_ephemeral_scaffolding skip. Salvaged from NousResearch#50372. Co-authored-by: rrevenanttt <290873280+rrevenanttt@users.noreply.github.com> (cherry picked from commit e4c6d1b)