Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/agents/extensions/memory/advanced_sqlite_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,11 @@ def _validate_turn():
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):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Make the duplicate-branch check atomic

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 👍 / 👎.

raise ValueError(
f"Branch '{branch_name}' already exists in session '{self.session_id}'. "
"Pass a different branch_name, or omit it to generate one."
)

# Copy messages before the branch point to the new branch
await self._copy_messages_to_new_branch(branch_name, turn_number)
Expand Down Expand Up @@ -1087,6 +1092,25 @@ def _list_branches_sync():

return await asyncio.to_thread(_list_branches_sync)

async def _branch_exists(self, branch_id: str) -> bool:
"""Whether any message in this session is already tagged with ``branch_id``."""

def _branch_exists_sync() -> bool:
with self._locked_connection() as conn:
with closing(conn.cursor()) as cursor:
cursor.execute(
"""
SELECT 1
FROM message_structure
WHERE session_id = ? AND branch_id = ?
LIMIT 1
""",
(self.session_id, branch_id),
)
return cursor.fetchone() is not None

return await asyncio.to_thread(_branch_exists_sync)

async def _copy_messages_to_new_branch(self, new_branch_id: str, from_turn_number: int) -> None:
"""Copy messages before the branch point to the new branch.

Expand Down
26 changes: 26 additions & 0 deletions tests/extensions/memory/test_advanced_sqlite_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,32 @@ async def test_create_branch_logging_respects_model_data_policy(monkeypatch, red
session.close()


async def test_create_branch_rejects_existing_branch_id():
session = AdvancedSQLiteSession(session_id="s4150", create_tables=True)
try:
await session.add_items(
[
{"role": "user", "content": "one"},
{"role": "assistant", "content": "a"},
{"role": "user", "content": "two"},
{"role": "assistant", "content": "b"},
]
)
await session.create_branch_from_turn(2, "dup")
# Return to a branch where turn 2 still resolves, so the only thing
# left for the second call to object to is the duplicate branch id.
await session.switch_to_branch("main")
before = {b["branch_id"]: b["message_count"] for b in await session.list_branches()}

with pytest.raises(ValueError, match="already exists"):
await session.create_branch_from_turn(2, "dup")

after = {b["branch_id"]: b["message_count"] for b in await session.list_branches()}
assert after == before, "a rejected branch creation must not modify any branch"
finally:
session.close()


async def test_advanced_session_respects_custom_table_names():
"""AdvancedSQLiteSession should consistently use configured table names."""
session = AdvancedSQLiteSession(
Expand Down