fix: correct event constant in rollback path, reorder delete operations

Aureliolo · claude · Aureliolo · commit c4cb8b08e2d4 · 2026-03-31T15:54:04.000+02:00
- Use PERSISTENCE_ARTIFACT_SAVE_FAILED (not _STORED) in PersistenceError
  handler -- success event must not be emitted on failure
- Delete storage content before repo metadata so inconsistency is
  detectable (metadata present + no blob) rather than invisible
  (no metadata + orphaned blob)
- Test: use asyncio.run(storage.store(...)) instead of internal dict

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/src/synthorg/api/controllers/artifacts.py b/src/synthorg/api/controllers/artifacts.py
@@ -20,6 +20,7 @@
 from synthorg.observability.events.persistence import (
     PERSISTENCE_ARTIFACT_CONTENT_MISSING,
     PERSISTENCE_ARTIFACT_DELETED,
+    PERSISTENCE_ARTIFACT_SAVE_FAILED,
     PERSISTENCE_ARTIFACT_SAVED,
     PERSISTENCE_ARTIFACT_STORAGE_DELETE_FAILED,
     PERSISTENCE_ARTIFACT_STORAGE_ROLLBACK_FAILED,
@@ -219,15 +220,17 @@ async def delete_artifact(
             200 on success, 404 if not found.
         """
         repo = state.app_state.persistence.artifacts
-        deleted = await repo.delete(artifact_id)
-        if not deleted:
+        artifact = await repo.get(artifact_id)
+        if artifact is None:
             return Response(
                 content=ApiResponse[None](
                     error=f"Artifact {artifact_id!r} not found",
                 ),
                 status_code=404,
             )
-        logger.info(PERSISTENCE_ARTIFACT_DELETED, artifact_id=artifact_id)
+        # Delete storage content first -- if this fails, metadata still
+        # exists so the inconsistency is detectable (vs. the reverse
+        # order where metadata is gone but orphaned blob is invisible).
         storage = state.app_state.artifact_storage
         try:
             await storage.delete(artifact_id)
@@ -237,6 +240,8 @@ async def delete_artifact(
                 artifact_id=artifact_id,
                 error=str(exc),
             )
+        await repo.delete(artifact_id)
+        logger.info(PERSISTENCE_ARTIFACT_DELETED, artifact_id=artifact_id)
         return Response(
             content=ApiResponse[None](data=None),
             status_code=200,
@@ -306,7 +311,7 @@ async def upload_content(
             await repo.save(updated)
         except PersistenceError as exc:
             logger.warning(
-                PERSISTENCE_ARTIFACT_STORED,
+                PERSISTENCE_ARTIFACT_SAVE_FAILED,
                 artifact_id=artifact_id,
                 error=str(exc),
                 note="metadata save failed, rolling back content",
diff --git a/tests/unit/api/controllers/test_artifacts.py b/tests/unit/api/controllers/test_artifacts.py
@@ -149,7 +149,9 @@ def test_delete_artifact_not_found(self, test_client: TestClient[Any]) -> None:
         assert resp.status_code == 404
 
     def test_download_content(self, test_client: TestClient[Any]) -> None:
-        """Pre-populate storage directly to test download path."""
+        """Pre-populate storage via public API, then test download."""
+        import asyncio
+
         create_resp = test_client.post(
             "/api/v1/artifacts",
             json={
@@ -163,9 +165,8 @@ def test_download_content(self, test_client: TestClient[Any]) -> None:
         )
         artifact_id = create_resp.json()["data"]["id"]
         payload = b"hello world"
-        # Pre-populate the fake storage dict to avoid multipart PUT.
         storage = test_client.app.state.app_state.artifact_storage
-        storage._store[artifact_id] = payload
+        asyncio.run(storage.store(artifact_id, payload))
         dl_resp = test_client.get(f"/api/v1/artifacts/{artifact_id}/content")
         assert dl_resp.status_code == 200
         assert dl_resp.content == payload
