[WIP] fix: don't override DraftReply.send_status based on application…

…-level state

Since #750, application-level state transitions (logging in, logging
out, and switching into offline mode) call
securedrop_client.storage.mark_all_pending_draft_replies_as_failed().
However, one or more SendReplyJobs (and their underlying POST requests
to "/sources/<source_uuid/replies>") may be in flight on the network at
that time, whether or not the application is connected (or even running)
to receive their responses.  Until we have better (ideally generalized)
logic around upload jobs that have not yet been confirmed by the server,
these application-level events should not make assumptions about the
results of jobs that have already been dispatched.

Individual SendReplyJobs still call their own _set_status_to_failed()
method on non-timeout exceptions.

[WIP] refactor: consolidate mark_all_pending_drafts_as_failed() calls

1. when queue is cleared
2. when local storage updates after sync

securedrop_client/logic.py

@@ -361,6 +361,7 @@ def __init__(  # type: ignore [no-untyped-def]
 
         # Queue that handles running API job
         self.api_job_queue = ApiJobQueue(self.api, self.session_maker)
+        self.api_job_queue.cleared.connect(self.on_queue_cleared)
         self.api_job_queue.paused.connect(self.on_queue_paused)
         self.add_job.connect(self.api_job_queue.enqueue)
 
@@ -470,6 +471,9 @@ def call_api(  # type: ignore [no-untyped-def]
         # Start the thread and related activity.
         new_api_thread.start()
 
+    def on_queue_cleared(self) -> None:
+        self.update_failed_replies()
+
     def on_queue_paused(self) -> None:
         self.gui.update_error_status(
             _("The SecureDrop server cannot be reached. Trying to reconnect..."), duration=0
@@ -509,7 +513,6 @@ def login(self, username: str, password: str, totp: str) -> None:
         default_request_timeout for Queue API requests in ApiJobQueue in order to display errors
         faster.
         """
-        storage.mark_all_pending_drafts_as_failed(self.session)
         self.api = sdclientapi.API(
             self.hostname, username, password, totp, self.proxy, default_request_timeout=60
         )
@@ -579,7 +582,6 @@ def login_offline_mode(self) -> None:
         # may have attempted online mode login, then switched to offline)
         self.gui.clear_clipboard()
         self.gui.show_main_window()
-        storage.mark_all_pending_drafts_as_failed(self.session)
         self.update_sources()
         self.show_last_sync()
         self.show_last_sync_timer.start(TIME_BETWEEN_SHOWING_LAST_SYNC_MS)
@@ -633,6 +635,7 @@ def on_sync_success(self) -> None:
         self.gui.refresh_current_source_conversation()
         self.download_new_messages()
         self.download_new_replies()
+        self.update_failed_replies()
         self.sync_succeeded.emit()
 
         try:
@@ -775,10 +778,6 @@ def logout(self) -> None:
 
         self.invalidate_token()
 
-        failed_replies = storage.mark_all_pending_drafts_as_failed(self.session)
-        for failed_reply in failed_replies:
-            self.reply_failed.emit(failed_reply.uuid)
-
         self.api_sync.stop()
         self.api_job_queue.stop()
         self.gui.logout()
@@ -1170,3 +1169,14 @@ def on_logout_success(self, result: Exception) -> None:
 
     def on_logout_failure(self, result: Exception) -> None:
         logging.info("Client logout failure")
+
+    def update_failed_replies(self) -> None:
+        """
+        Emit an explicit `reply_failed` signal for each pending reply marked as failed by
+        the storage layer rather than the API job responsible for sending it (for example,
+        if the application quit mid-job or mid-queue).  Without this signal, the reply
+        won't be shown as failed in the GUI until the application is restarted.
+        """
+        failed_replies = storage.mark_all_pending_drafts_as_failed(self.session)
+        for failed_reply in failed_replies:
+            self.reply_failed.emit(failed_reply.uuid)

securedrop_client/storage.py

@@ -1047,9 +1047,6 @@ def get_reply(session: Session, uuid: str) -> Reply:
 
 
 def mark_all_pending_drafts_as_failed(session: Session) -> List[DraftReply]:
-    """
-    When we login (offline or online) or logout, we need to set all the pending replies as failed.
-    """
     pending_status = (
         session.query(ReplySendStatus).filter_by(name=ReplySendStatusCodes.PENDING.value).one()
     )

tests/test_logic.py

@@ -156,7 +156,6 @@ def test_Controller_login(homedir, config, mocker, session_maker):
     """
     mock_gui = mocker.MagicMock()
     mock_api = mocker.patch("securedrop_client.logic.sdclientapi.API")
-    fail_draft_replies = mocker.patch("securedrop_client.storage.mark_all_pending_drafts_as_failed")
 
     co = Controller("http://localhost", mock_gui, session_maker, homedir, None)
     co.call_api = mocker.MagicMock()
@@ -167,7 +166,6 @@ def test_Controller_login(homedir, config, mocker, session_maker):
     co.call_api.assert_called_once_with(
         mock_api().authenticate, co.on_authenticate_success, co.on_authenticate_failure
     )
-    fail_draft_replies.assert_called_once_with(co.session)
     co.show_last_sync_timer.stop.assert_called_once_with()
 
 
@@ -1016,7 +1014,8 @@ def test_Controller_invalidate_token(mocker, homedir, session_maker):
 
 def test_Controller_logout_with_pending_replies(mocker, session_maker, homedir, reply_status_codes):
     """
-    Ensure draft reply fails on logout and that the reply_failed signal is emitted.
+    Ensure pending replies not currently being processed are marked failed and emit the
+    "reply_failed" signal.
     """
     co = Controller("http://localhost", mocker.MagicMock(), session_maker, homedir, None)
     co.api_job_queue = mocker.MagicMock()
@@ -1040,10 +1039,15 @@ def test_Controller_logout_with_pending_replies(mocker, session_maker, homedir,
 
     co.logout()
 
-    for draft in session.query(db.DraftReply).all():
-        assert draft.send_status == failed_status
+    for (i, draft) in enumerate(session.query(db.DraftReply).all()):
+        if i == 0:
+            # We can't check that draft.sending_pid == os.getpid() here, because that's set by the
+            # SendReplyJob.
+            assert draft.send_status == pending_status
+        else:
+            assert draft.send_status == failed_status
+            co.reply_failed.emit.assert_called_once_with(draft.uuid)
 
-    co.reply_failed.emit.assert_called_once_with(pending_draft_reply.uuid)
     co.api_job_queue.stop.assert_called_once_with()
 
 
@@ -1059,15 +1063,13 @@ def test_Controller_logout_with_no_api(homedir, config, mocker, session_maker):
     co.api_job_queue = mocker.MagicMock()
     co.api_job_queue.stop = mocker.MagicMock()
     co.call_api = mocker.MagicMock()
-    fail_draft_replies = mocker.patch("securedrop_client.storage.mark_all_pending_drafts_as_failed")
 
     co.logout()
 
     assert not co.authenticated_user
     co.call_api.assert_not_called()
     co.api_job_queue.stop.assert_called_once_with()
     co.gui.logout.assert_called_once_with()
-    fail_draft_replies.called_once_with(co.session)
 
 
 def test_Controller_logout_success(homedir, config, mocker, session_maker):
@@ -1085,7 +1087,6 @@ def test_Controller_logout_success(homedir, config, mocker, session_maker):
     co.call_api = mocker.MagicMock()
     co.show_last_sync_timer = mocker.MagicMock()
     info_logger = mocker.patch("securedrop_client.logic.logging.info")
-    fail_draft_replies = mocker.patch("securedrop_client.storage.mark_all_pending_drafts_as_failed")
     logout_method = co.api.logout
     co.logout()
     co.call_api.assert_called_with(logout_method, co.on_logout_success, co.on_logout_failure)
@@ -1095,7 +1096,6 @@ def test_Controller_logout_success(homedir, config, mocker, session_maker):
     co.gui.logout.assert_called_once_with()
     msg = "Client logout successful"
     info_logger.assert_called_once_with(msg)
-    fail_draft_replies.called_once_with(co.session)
     co.show_last_sync_timer.start.assert_called_once_with(TIME_BETWEEN_SHOWING_LAST_SYNC_MS)
 
 
@@ -1113,7 +1113,6 @@ def test_Controller_logout_failure(homedir, config, mocker, session_maker):
     co.api_job_queue.stop = mocker.MagicMock()
     co.call_api = mocker.MagicMock()
     info_logger = mocker.patch("securedrop_client.logic.logging.info")
-    fail_draft_replies = mocker.patch("securedrop_client.storage.mark_all_pending_drafts_as_failed")
     logout_method = co.api.logout
 
     co.logout()
@@ -1125,7 +1124,6 @@ def test_Controller_logout_failure(homedir, config, mocker, session_maker):
     co.gui.logout.assert_called_once_with()
     msg = "Client logout failure"
     info_logger.assert_called_once_with(msg)
-    fail_draft_replies.called_once_with(co.session)
 
 
 def test_Controller_set_activity_status(homedir, config, mocker, session_maker):
@@ -2319,6 +2317,21 @@ def test_APICallRunner_api_call_timeout(mocker, exception):
     mock_timeout_signal.emit.assert_called_once_with()
 
 
+def test_Controller_on_queue_cleared(homedir, config, mocker, session_maker):
+    """
+    Check that clearing the queue marks as failed replies that were still pending in the queue.
+    """
+    mock_gui = mocker.MagicMock()
+    co = Controller("http://localhost", mock_gui, session_maker, homedir, None)
+    fail_draft_replies = mocker.patch("securedrop_client.storage.mark_all_pending_drafts_as_failed")
+
+    mocker.patch.object(co, "api_job_queue")
+    co.api = "not none"
+    co.show_last_sync_timer = mocker.MagicMock()
+    co.on_queue_cleared()
+    fail_draft_replies.called_once_with(co.session)
+
+
 def test_Controller_on_queue_paused(homedir, config, mocker, session_maker):
     """
     Check that a paused queue is communicated to the user via the error status bar
@@ -2701,3 +2714,27 @@ def test_get_file(mocker, session, homedir):
 
     storage.get_file.assert_called_once_with(co.session, file.uuid)
     assert obj == file
+
+
+def test_Controller_update_failed_replies(homedir, config, mocker, session, session_maker):
+    """
+    The "reply_failed" signal is emitted for each pending reply marked as failed.
+    """
+    mock_storage = mocker.patch("securedrop_client.logic.storage")
+    mock_storage.mark_all_pending_drafts_as_failed = mocker.MagicMock()
+
+    controller = Controller(
+        "http://localhost", mocker.MagicMock(), session_maker, homedir, None, None
+    )
+    controller.authenticated_user = factory.User(id=1)
+    controller.reply_failed = mocker.MagicMock()
+
+    source = factory.Source()
+    draft = factory.DraftReply(source=source, journalist_id=controller.authenticated_user.id)
+
+    failed_drafts = [draft]
+    mock_storage.mark_all_pending_drafts_as_failed.return_value = failed_drafts
+
+    controller.update_failed_replies()
+    for failed in failed_drafts:
+        controller.reply_failed.emit.assert_called_once_with(failed.uuid)