fix(advanced-sqlite-session): cleanup branch-only messages on delete_branch#3531
Open
LeSingh1 wants to merge 1 commit into
Open
fix(advanced-sqlite-session): cleanup branch-only messages on delete_branch#3531LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…branch (fixes openai#3346) AdvancedSQLiteSession.delete_branch removed turn_usage and message_structure rows for the deleted branch but left the underlying agent_messages rows in the table when those messages were only referenced by the deleted branch. After deletion those rows were invisible to advanced branch queries such as get_items() yet still took up space, so branch-heavy sessions accumulated orphaned rows. The existing _cleanup_orphaned_messages_sync helper already scopes to self.session_id and only removes messages with no message_structure reference, so it preserves messages that the main branch (or any other branch) still shares via _copy_messages_to_new_branch. Reuse it inside delete_branch's transaction, after the message_structure delete, so the cleanup commits with the rest of the deletion. Adds two regression tests: one for the branch-only cleanup case from the issue, and one that verifies messages still referenced by main are preserved when a forked branch is deleted.
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
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.
Fixes #3346.
AdvancedSQLiteSession.delete_branchdeletes the matchingturn_usageandmessage_structurerows but never touchedagent_messages. When messages were only referenced by the deleted branch, theiragent_messagesrows survived the deletion. After the delete:get_items()correctly omit them (nomessage_structurerow), andFor branch-heavy sessions that meant invisible orphans accumulating.
Repro from the issue:
Before: four rows (the branch-only pair is still there). After this PR: two rows (only main).
The existing
_cleanup_orphaned_messages_synchelper already does exactly the right thing — it scopes toself.session_idand removes only rows with nomessage_structurereference, so it preserves messages that other branches still share via_copy_messages_to_new_branch. The fix calls it insidedelete_branch's existing transaction, after themessage_structuredelete, so the cleanup commits with the rest of the deletion.Tests:
test_delete_branch_cleans_branch_only_messagesreproduces the issue and asserts only the main messages remain after the branch is deleted.test_delete_branch_preserves_messages_shared_with_mainforks a branch at turn 2, deletes it, and confirms turn 1's messages stay because main still references them.Verification:
uv run pytest tests/extensions/memory/test_advanced_sqlite_session.py→ 38 passed.make format/make lint/make typecheckclean on the touched files.