refactor(handle_source_conversation_deleted): utils.delete_file_object() is non-atomic

Author: cfm

API2.md

@@ -155,7 +155,7 @@ direction TB
 [*] --> CacheLookup : process(event)
 CacheLookup: status = redis.get(event.id)
 
-CacheLookup --> IdempotentBranch : status in {102 Processing, 200 OK, 207 MultiStatus}
+CacheLookup --> IdempotentBranch : status in {102 Processing, 200 OK}
 CacheLookup --> StartBranch : status == None
 
 state "Enforce idempotency" as IdempotentBranch {
@@ -176,12 +176,9 @@ state "handle_<event.type>()" as Handler {
 Handler --> OK
 state "Cache and report success" as SuccessBranch {
     OK : 200 OK
-    MultiStatus : 207 MultiStatus
-
     OK --> UpdateCache
-    MultiStatus --> UpdateCache
 
-    UpdateCache : redis.set(event.id, status, ttl)
+    UpdateCache : redis.set(event.id, OK, ttl)
     UpdateCache --> [*] : return (status, delta)
 }
 

securedrop/journalist_app/api2/events.py

@@ -1,5 +1,5 @@
 from dataclasses import asdict
-from typing import Any, Dict
+from typing import List
 
 from db import db
 from journalist_app import utils
@@ -255,26 +255,27 @@ def handle_source_conversation_truncated(event: Event, minor: int) -> EventResul
                 ),
             )
 
-        deleted: Dict[ItemUUID, Any] = {}
+        deleted: List[ItemUUID] = []
         for item in source.collection:
             if item.interaction_count <= event.data.upper_bound:
                 try:
                     utils.delete_file_object(item)
-                    deleted[item.uuid] = None
                 except ValueError:
-                    deleted[item.uuid] = item
-
-        if any(deleted.values()):
-            status = EventStatusCode.MultiStatus
-        else:
-            status = EventStatusCode.OK
+                    # `utils.delete_file_object()` is non-atomic: it guarantees
+                    # database deletion but not filesystem deletion.  The former
+                    # is all we need for consistency with the client, and the
+                    # latter will be caught by monitoring for "disconnected"
+                    # submissions.
+                    pass
+                finally:
+                    deleted.append(item.uuid)
 
         db.session.refresh(source)
         return EventResult(
             event_id=event.id,
-            status=(status, None),
+            status=(EventStatusCode.OK, None),
             sources={source.uuid: source},
-            items=deleted,
+            items={item_uuid: None for item_uuid in deleted},
         )
 
     @staticmethod

securedrop/journalist_app/api2/types.py

@@ -44,9 +44,6 @@ class EventType(StrEnum):
 class EventStatusCode(IntEnum):
     Processing = 102
     OK = 200
-    # This event decomposes into multiple actions, some of which succeeded and
-    # some of which failed.  Retries for failures must be submitted in a new event.
-    MultiStatus = 207
     # We already saw and processed this event
     AlreadyReported = 208
     BadRequest = 400

securedrop/tests/test_journalist_api2.py

@@ -1170,10 +1170,10 @@ def test_api2_source_conversation_truncated(
 
         status_code, msg = response.json["events"][event.id]
         # Because some deletes may fail (simulated) and some succeed, the handler
-        # returns 200 if all succeed, or 207 (MultiStatus) if any fail.
+        # returns 200 if all succeed.
         # The test_files fixtures never cause delete_file_object() to raise,
         # so OK (200) is expected.
-        assert status_code in (200, 207)
+        assert status_code == 200
 
         # Verify item-wise results
         returned_items = response.json["items"]