From b6f2488b8814ce8a4a8ff0fee8d2d325b9c77ce6 Mon Sep 17 00:00:00 2001 From: sanjibani <18418553+sanjibani@users.noreply.github.com> Date: Sun, 5 Jul 2026 21:10:01 +0530 Subject: [PATCH] fix(api): make single-entity delete resilient to file-cleanup failures Closes #1033. The DELETE /v2/projects//knowledge/entities/ endpoint returns 500 for non-markdown entities when the file cleanup step cannot resolve the file path (cloud storage target mismatch, missing local mirror, or already-deleted after a concurrent delete-directory pass). The DB delete is what callers actually need, so wrap file_service.delete_entity_file in try/except and log a warning instead of propagating the exception. Mirrors how delete_directory catches per-entity errors. --- src/basic_memory/services/entity_service.py | 14 ++++++++-- tests/services/test_entity_service.py | 31 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/basic_memory/services/entity_service.py b/src/basic_memory/services/entity_service.py index e1e3316a3..0b1614e8f 100644 --- a/src/basic_memory/services/entity_service.py +++ b/src/basic_memory/services/entity_service.py @@ -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: + 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) diff --git a/tests/services/test_entity_service.py b/tests/services/test_entity_service.py index 45719a7c3..4e0d8a1eb 100644 --- a/tests/services/test_entity_service.py +++ b/tests/services/test_entity_service.py @@ -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."""