fix(memory): reject an existing branch id in create_branch_from_turn - #4179
fix(memory): reject an existing branch id in create_branch_from_turn#4179aryanputta wants to merge 1 commit into
Conversation
create_branch_from_turn() passed branch_name straight to _copy_messages_to_new_branch(), which unconditionally inserts message_structure rows tagged with that id. Passing the id of a branch that already exists therefore merged the copied turns into it and then switched to it, with no error and no way to tell the two histories apart afterwards. Check the id first and raise ValueError, which the docstring already documents as the failure mode for this method. A generated name is unaffected, since it embeds a timestamp. Closes openai#4150
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2602075824
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if branch_name is None: | ||
| timestamp = int(time.time()) | ||
| branch_name = f"branch_from_turn_{turn_number}_{timestamp}" | ||
| elif await self._branch_exists(branch_name): |
There was a problem hiding this comment.
When two coroutines call create_branch_from_turn(..., "same") concurrently from a branch where the turn resolves, this check runs in its own locked DB operation and then releases the lock before _copy_messages_to_new_branch() inserts anything, so both calls can observe that the branch is absent and then both copy rows into the same branch. That leaves the merged/duplicated branch state this change is meant to reject; the existence check needs to be performed in the same locked transaction as the copy, or enforced by a database-level uniqueness boundary for branch creation. .agents/references/session-persistence.mdL19-L20
Useful? React with 👍 / 👎.
seratch
added
duplicate
|
Thanks for trying to fix this. The issue was resolved by #4186 |
2 participants
Closes #4150.
Problem
create_branch_from_turn()passesbranch_namestraight through to_copy_messages_to_new_branch(), which unconditionally insertsmessage_structurerows tagged with that id. Passing the id of a branch that already exists merges the copied turns into it and then switches to it. There is no error, and afterwards the two histories cannot be told apart.Change
Check the id before copying and raise
ValueError. The method's docstring already documentsValueErroras its failure mode, so this uses the existing contract rather than introducing a new one. A generated name is unaffected, since it embeds a timestamp.Verification
tests/extensions/memory/test_advanced_sqlite_session.py. It asserts both halves: that the second call raises, and that branch message counts are unchanged afterwards, so a rejected call cannot have partially written."already exists". An earlier version of this test passed for the wrong reason: after the first call the session is on the new branch, where the requested turn no longer resolves, so turn validation raisedValueErroron its own. Switching back tomainfirst isolates the duplicate-id condition.ruff checkandruff format --checkclean on both files.