Skip to content

Commit 85484f9

Browse files
phernandezclaude
andcommitted
Fix race condition in UPSERT entity implementation
Improve IntegrityError handling to distinguish between file_path and permalink conflicts, addressing a race condition vulnerability where: 1. Initial check finds no entity with file_path 2. Another process creates/deletes entity with same file_path 3. Insert fails with IntegrityError 4. We incorrectly assume it's a permalink conflict Changes: - Check for file_path conflict after IntegrityError using second query - Handle race condition by updating the existing entity - Only generate unique permalink if it's truly a permalink conflict - Add comprehensive test for race condition scenario This ensures robust conflict resolution regardless of timing between concurrent operations on the same file_path. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent d525af2 commit 85484f9

2 files changed

Lines changed: 99 additions & 2 deletions

File tree

src/basic_memory/repository/entity_repository.py

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,47 @@ async def upsert_entity(self, entity: Entity) -> Entity:
172172
return found
173173

174174
except IntegrityError:
175-
# Permalink conflict with different file - generate unique permalink
175+
# Could be either file_path or permalink conflict
176176
await session.rollback()
177-
return await self._handle_permalink_conflict(entity, session)
177+
178+
# Check if it's a file_path conflict (race condition)
179+
existing_by_path_check = await session.execute(
180+
select(Entity).where(
181+
Entity.file_path == entity.file_path,
182+
Entity.project_id == entity.project_id
183+
)
184+
)
185+
race_condition_entity = existing_by_path_check.scalar_one_or_none()
186+
187+
if race_condition_entity:
188+
# Race condition: file_path conflict detected after our initial check
189+
# Update the existing entity instead
190+
for key, value in {
191+
'title': entity.title,
192+
'entity_type': entity.entity_type,
193+
'entity_metadata': entity.entity_metadata,
194+
'content_type': entity.content_type,
195+
'permalink': entity.permalink,
196+
'checksum': entity.checksum,
197+
'updated_at': entity.updated_at,
198+
}.items():
199+
setattr(race_condition_entity, key, value)
200+
201+
await session.flush()
202+
# Return the updated entity with relationships loaded
203+
query = (
204+
select(Entity)
205+
.where(Entity.file_path == entity.file_path)
206+
.options(*self.get_load_options())
207+
)
208+
result = await session.execute(query)
209+
found = result.scalar_one_or_none()
210+
if not found: # pragma: no cover
211+
raise RuntimeError(f"Failed to retrieve entity after race condition update: {entity.file_path}")
212+
return found
213+
else:
214+
# Must be permalink conflict - generate unique permalink
215+
return await self._handle_permalink_conflict(entity, session)
178216

179217
async def _handle_permalink_conflict(self, entity: Entity, session: AsyncSession) -> Entity:
180218
"""Handle permalink conflicts by generating a unique permalink."""

tests/repository/test_entity_repository_upsert.py

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,65 @@ async def test_upsert_entity_multiple_permalink_conflicts(entity_repository: Ent
146146
assert len(set(entity_ids)) == 3
147147

148148

149+
@pytest.mark.asyncio
150+
async def test_upsert_entity_race_condition_file_path(entity_repository: EntityRepository):
151+
"""Test that upsert handles race condition where file_path conflict occurs after initial check."""
152+
from unittest.mock import patch
153+
from sqlalchemy.exc import IntegrityError
154+
155+
# Create an entity first
156+
entity1 = Entity(
157+
project_id=entity_repository.project_id,
158+
title="Original Entity",
159+
entity_type="note",
160+
permalink="test/original",
161+
file_path="test/race-file.md",
162+
content_type="text/markdown",
163+
created_at=datetime.now(timezone.utc),
164+
updated_at=datetime.now(timezone.utc),
165+
)
166+
167+
result1 = await entity_repository.upsert_entity(entity1)
168+
original_id = result1.id
169+
170+
# Create another entity with different file_path and permalink
171+
entity2 = Entity(
172+
project_id=entity_repository.project_id,
173+
title="Race Condition Test",
174+
entity_type="note",
175+
permalink="test/race-entity",
176+
file_path="test/different-file.md", # Different initially
177+
content_type="text/markdown",
178+
created_at=datetime.now(timezone.utc),
179+
updated_at=datetime.now(timezone.utc),
180+
)
181+
182+
# Now simulate race condition: change file_path to conflict after the initial check
183+
original_add = entity_repository.session_maker().add
184+
call_count = 0
185+
186+
def mock_add(obj):
187+
nonlocal call_count
188+
if isinstance(obj, Entity) and call_count == 0:
189+
call_count += 1
190+
# Simulate race condition by changing file_path to conflict
191+
obj.file_path = "test/race-file.md" # Same as entity1
192+
# This should trigger IntegrityError for file_path constraint
193+
raise IntegrityError("UNIQUE constraint failed: entity.file_path", None, None)
194+
return original_add(obj)
195+
196+
# Mock session.add to simulate the race condition
197+
with patch.object(entity_repository.session_maker().__class__, 'add', side_effect=mock_add):
198+
# This should handle the race condition gracefully by updating the existing entity
199+
result2 = await entity_repository.upsert_entity(entity2)
200+
201+
# Should return the updated original entity (same ID)
202+
assert result2.id == original_id
203+
assert result2.title == "Race Condition Test" # Updated title
204+
assert result2.file_path == "test/race-file.md" # Same file path
205+
assert result2.permalink == "test/race-entity" # Updated permalink
206+
207+
149208
@pytest.mark.asyncio
150209
async def test_upsert_entity_gap_in_suffixes(entity_repository: EntityRepository):
151210
"""Test that upsert finds the next available suffix even with gaps."""

0 commit comments

Comments
 (0)