fix(agent): persist messages by intrinsic marker to stop id() reuse data loss (salvage #50372) - #56284
Merged
Merged
Conversation
…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>
… (review) Phase 2c review follow-up on the id()-reuse persistence fix: - test_recycled_id_in_dedup_set_still_persists_new_message seeded an EMPTY dedup set, so it never injected a collision and passed under id-based dedup too (couldn't distinguish the designs). Replace with test_stale_seed_id_from_prior_flush_cannot_suppress_new_message, which asserts the durable invariant: the seed is empty after every flush (mutation-checked: removing the post-flush reset now fails BOTH id-reuse tests). - Refresh the _flush_messages_to_session_db docstring: it still described the old per-session identity tracking; document the intrinsic-marker mechanism, that _flushed_db_message_ids is now a one-shot seed, and the shared-dict mutation safety note.
kshitijk4poor
enabled auto-merge (rebase)
July 1, 2026 10:30
alt-glitch
added
type/bug
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.
Salvage of #50372 (@rrevenanttt), rebased onto current main.
Issue
_flush_messages_to_session_dbdeduped persisted messages with a retained{id(msg)}set 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 whoseid()collides with the stale entry — so the real turn is silently never written to state.db (#46053-adjacent data loss).Fix
Replace the retained id-set with an intrinsic
_DB_PERSISTED_MARKERstamped on each dict. The id-set is demoted to a one-shot seed (valid only while the caller's objects are alive), translated to markers, and cleared after every flush — so noid()outlives a flush to alias a future message. The marker is_-prefixed so the wire sanitizers strip it before any request leaves the process. Preserves the existing_is_ephemeral_scaffoldingskip landed since the PR was written.Verification
2 new tests: (a) no id() lingers past a flush + all written dicts carry the marker; (b) a recycled-id collision still persists the real message. Mutation-checked: removing the seed reset fails the retention test. 31 persistence tests pass (identity-flush + sequence-repair).
Closes #50372.
Co-authored-by: rrevenanttt 290873280+rrevenanttt@users.noreply.github.com