Skip to content
Open
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
14 changes: 12 additions & 2 deletions src/basic_memory/services/entity_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -805,8 +805,18 @@ async def delete_entity(self, permalink_or_id: str | int) -> bool:
exc_info=True,
)

# Delete file
await self.file_service.delete_entity_file(entity)
# Delete file (best-effort: file may be on a remote/None storage target in cloud mode,
# or already gone after a concurrent delete-directory pass). Mirrors the resilience
# of delete_directory, which catches per-entity errors instead of failing the batch.
try:
await self.file_service.delete_entity_file(entity)
except Exception:

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 Preserve failed file deletes in directory results

When delete_directory calls delete_entity, any real local filesystem failure from delete_entity_file (for example PermissionError on a read-only project or invalid file path state) is now swallowed here, so delete_directory increments successful_deletes and downstream MCP JSON reports deleted: true because failed_deletes == 0, even though the file was not removed. Since files are the source of truth and the next sync treats a remaining file with no DB row as new, narrow this suppression to the missing/remote cleanup cases or return a status that lets directory deletes report the per-file failure.

Useful? React with 👍 / 👎.

logger.warning(
"File cleanup failed for entity (continuing with DB delete)",
permalink_or_id=permalink_or_id,
file_path=entity.file_path,
exc_info=True,
)

# Delete from DB (this will cascade to observations/relations)
# Trigger: repository.delete returns False when entity is already gone (NoResultFound)
Expand Down
31 changes: 31 additions & 0 deletions tests/services/test_entity_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,37 @@ async def test_delete_nonexistent_entity(entity_service: EntityService):
assert await entity_service.delete_entity("test/non_existent") is True


@pytest.mark.asyncio
async def test_delete_entity_succeeds_when_file_cleanup_raises(
entity_service: EntityService, monkeypatch: pytest.MonkeyPatch
):
"""Single-entity delete must succeed even when the file cannot be removed.

Regression for #1033: non-markdown entities (note_type='file', sync-conflict
files, etc.) hit `delete_entity_file` on a path the file service cannot
resolve (cloud storage mismatch, missing local mirror, etc.). The DB row
should still be deleted so callers see a 200 instead of a 500.
"""

entity_data = EntitySchema(
title="FileTypeDeleteTarget",
directory="test",
note_type="file",
)
created = await entity_service.create_entity(entity_data)

async def _raise_on_file_delete(_entity): # pragma: no cover - exercised via assert below
raise FileNotFoundError("simulated: file path not present on this storage target")

monkeypatch.setattr(entity_service.file_service, "delete_entity_file", _raise_on_file_delete)

# Delete must return True (caller treats this as success) instead of propagating.
assert await entity_service.delete_entity(created.id) is True

with pytest.raises(EntityNotFoundError):
await entity_service.get_by_permalink(_permalink(entity_data))


@pytest.mark.asyncio
async def test_create_entity_with_special_chars(entity_service: EntityService):
"""Test entity creation with special characters in name and description."""
Expand Down