Skip to content
Merged
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
23 changes: 21 additions & 2 deletions src/basic_memory/services/entity_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,10 @@ async def delete_entity(self, permalink_or_id: str | int) -> bool:
entity = await self.get_by_permalink(permalink_or_id)
else:
entities = await self.get_entities_by_id([permalink_or_id])
if len(entities) == 0:
# Entity already deleted (concurrent delete or race condition)
logger.info("Entity already deleted", entity_id=permalink_or_id)
return True
if len(entities) != 1: # pragma: no cover
logger.error(
"Entity lookup error", entity_id=permalink_or_id, found_count=len(entities)
Expand All @@ -746,13 +750,28 @@ async def delete_entity(self, permalink_or_id: str | int) -> bool:

# Delete from search index first (if search_service is available)
if self.search_service:
await self.search_service.handle_delete(entity)
try:
await self.search_service.handle_delete(entity)
except Exception:
# Search cleanup is best-effort during concurrent deletes.
# Relationships may have been cascade-deleted by a concurrent request.
logger.warning(
"Search cleanup failed for entity (likely concurrent delete)",
permalink_or_id=permalink_or_id,
exc_info=True,
)

# Delete file
await self.file_service.delete_entity_file(entity)

# Delete from DB (this will cascade to observations/relations)
return await self.repository.delete(entity.id)
# Trigger: repository.delete returns False when entity is already gone (NoResultFound)
# Why: concurrent delete_directory requests can race to delete the same entity
# Outcome: treat as success since the entity is deleted either way
deleted = await self.repository.delete(entity.id)
if not deleted:
logger.info("Entity already removed from DB", entity_id=permalink_or_id)
return True

except EntityNotFoundError:
logger.info(f"Entity not found: {permalink_or_id}")
Expand Down
113 changes: 113 additions & 0 deletions tests/services/test_entity_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -2426,3 +2426,116 @@ async def test_fast_write_entity_null_user_id(entity_service: EntityService):
entity = await entity_service.fast_write_entity(schema, external_id=str(uuid.uuid4()))
assert entity.created_by is None
assert entity.last_updated_by is None


# --- Concurrent Delete Resilience ---


@pytest.mark.asyncio
async def test_delete_entity_by_id_already_deleted(entity_service: EntityService):
"""delete_entity returns True when entity was already deleted (concurrent delete)."""
entity_data = EntitySchema(
title="ConcurrentDeleteTarget",
directory="test",
note_type="test",
)
created = await entity_service.create_entity(entity_data)
entity_id = created.id

# Delete once - should succeed
assert await entity_service.delete_entity(entity_id) is True

# Delete again by ID - should return True (already deleted), not raise ValueError
assert await entity_service.delete_entity(entity_id) is True


@pytest.mark.asyncio
async def test_delete_entity_by_permalink_already_deleted(entity_service: EntityService):
"""delete_entity returns True when entity was already deleted by permalink."""
entity_data = EntitySchema(
title="ConcurrentDeleteByPermalink",
directory="test",
note_type="test",
)
created = await entity_service.create_entity(entity_data)

# Delete once
assert await entity_service.delete_entity(created.id) is True

# Delete again by permalink - should return True (EntityNotFoundError caught)
assert await entity_service.delete_entity(entity_data.permalink) is True


@pytest.mark.asyncio
async def test_delete_directory_concurrent_resilience(entity_service: EntityService):
"""delete_directory succeeds even when entities are concurrently deleted."""
# Create entities in a directory
entities = []
for i in range(5):
entity_data = EntitySchema(
title=f"DirEntity{i}",
directory="concurrent-test",
note_type="test",
)
created = await entity_service.create_entity(entity_data)
entities.append(created)

# Delete some entities directly to simulate concurrent delete
await entity_service.delete_entity(entities[0].id)
await entity_service.delete_entity(entities[2].id)

# Now delete the directory - should handle already-deleted entities gracefully
result = await entity_service.delete_directory("concurrent-test")

# Only 3 remain in DB (2 were already deleted before the directory query)
assert result.total_files == 3
assert result.successful_deletes == 3
assert result.failed_deletes == 0
assert len(result.errors) == 0


@pytest.mark.asyncio
async def test_delete_directory_all_already_deleted(entity_service: EntityService):
"""delete_directory handles case where all entities were concurrently deleted."""
# Create entities
created_entities = []
for i in range(3):
entity_data = EntitySchema(
title=f"AllGone{i}",
directory="all-deleted",
note_type="test",
)
created = await entity_service.create_entity(entity_data)
created_entities.append(created)

# Delete all entities directly
for e in created_entities:
await entity_service.delete_entity(e.id)

# Directory delete should report empty (entities no longer found in prefix query)
result = await entity_service.delete_directory("all-deleted")
assert result.total_files == 0
assert result.successful_deletes == 0
assert result.failed_deletes == 0


@pytest.mark.asyncio
async def test_delete_directory_entity_deleted_between_query_and_delete(
entity_service: EntityService,
):
"""Simulates the real race condition: entity exists in prefix query but is deleted
by a concurrent request before delete_entity is called."""
# Create entities
entity_data = EntitySchema(title="RaceTarget", directory="race-dir", note_type="test")
created = await entity_service.create_entity(entity_data)

# Get the entities via prefix query (as delete_directory does)
entities = await entity_service.repository.find_by_directory_prefix("race-dir")
assert len(entities) == 1

# Now delete the entity behind the scenes (simulating a concurrent request)
await entity_service.delete_entity(created.id)

# Call delete_entity with the stale entity ID - should return True, not raise
result = await entity_service.delete_entity(entities[0].id)
assert result is True
Loading