fix(run_agent): clamp _last_flushed_db_idx after scaffolding drop (#31507) - #31517
Closed
0xsir0000 wants to merge 1 commit into
Closed
fix(run_agent): clamp _last_flushed_db_idx after scaffolding drop (#31507)#315170xsir0000 wants to merge 1 commit into
0xsir0000 wants to merge 1 commit into
Conversation
…usResearch#31507) `_persist_session` runs `_drop_trailing_empty_response_scaffolding` before `_flush_messages_to_session_db`. If an earlier intermediate flush advanced `_last_flushed_db_idx` to a length that included the scaffolding tail, the drop leaves the index strictly greater than `len(messages)`. The next flush computes `flush_from = max(start_idx, _last_flushed_db_idx) >= len(messages)`, slices `messages[flush_from:]` to an empty list, and silently writes nothing — including any final assistant response appended after the drop. CLI sessions that reuse the same `AIAgent` across turns are the common trigger, which is why state.db is missing assistant rows while the WebUI mirror keeps the full transcript. Resetting `flush_from` to `start_idx` (as the report suggested) would re-flush messages already persisted, so instead clamp `_last_flushed_db_idx` to `len(messages)` right after the drop. New messages appended in the next turn are then flushed exactly once. Tests cover both the direct invariant (clamp happens, flush is invoked) and an end-to-end SessionDB scenario where the post-drop assistant response must land in the DB. Fixes NousResearch#31507
alt-glitch
added
type/bug
Contributor
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.
Summary
_persist_sessionruns_drop_trailing_empty_response_scaffoldingbefore_flush_messages_to_session_db. If an earlier intermediate flush advanced_last_flushed_db_idxto a length that included the scaffolding tail, the drop leaves the index strictly greater thanlen(messages). The next flush computesflush_from = max(start_idx, _last_flushed_db_idx) >= len(messages), slicesmessages[flush_from:]to empty, and silently writes nothing — including any final assistant response appended after the drop. CLI sessions that reuse the sameAIAgentacross turns are the common trigger, which is why state.db loses assistant rows while the WebUI mirror keeps the full transcript.Fix
Resetting
flush_fromtostart_idx(as the report suggested) would re-flush messages already persisted, so instead clamp_last_flushed_db_idxdown tolen(messages)right after the scaffolding drop. New messages appended on the next turn are then flushed exactly once — preserving the #860 dedup invariant.run_agent.py: clamp_last_flushed_db_idxafter_drop_trailing_empty_response_scaffoldingtests/run_agent/test_empty_response_recovery_persistence.py: two new regression tests (one direct, one end-to-end against a realSessionDB); existing stub fixture updated to seed the new attribute it now readsTest plan
pytest tests/run_agent/test_empty_response_recovery_persistence.py— 5 passed (2 existing + 3 new)pytest tests/run_agent/test_860_dedup.py tests/run_agent/test_compression_persistence.py tests/run_agent/test_message_sequence_repair.py tests/run_agent/test_413_compression.py— 40 passed (dedup / compression / repair invariants unchanged)pytest tests/cli/test_branch_command.py tests/cli/test_cli_new_session.py— 23 passed (other_last_flushed_db_idxconsumers unaffected)Fixes #31507