on_reply_download_failure() assumes exceptions have a uuid attribute

[cfm]

Description

After #2231, a NoResultFound exception can cause a further AttributeError in securedrop_client.logic.Controller.on_reply_download_failure().

Steps to Reproduce

Discovered in test runs of f6bd2c9 without this padding—

securedrop-client/client/tests/functional/test_delete_sources.py

Lines 32 to 33 in f6bd2c9

	# Wait for a full sync to give us a stable source list.
	qtbot.wait(TIME_SYNC)

—suggesting that it's a race in downloading something that's been deleted. After #2252, reproduce with:

cfm@kintsugi client % git diff tests/functional/test_delete_sources.py
diff --git a/client/tests/functional/test_delete_sources.py b/client/tests/functional/test_delete_sources.py
index cebf3ba5..d211e4d9 100644
--- a/client/tests/functional/test_delete_sources.py
+++ b/client/tests/functional/test_delete_sources.py
@@ -21,7 +21,6 @@ from tests.conftest import (
 
 
 @pytest.mark.parametrize("num_to_delete", [1, 3])
-@flaky
 def test_delete_sources(functional_test_logged_in_context, qtbot, mocker, num_to_delete):
     """
     Check that sources selected for deletion are first confirmed and then
@@ -29,9 +28,6 @@ def test_delete_sources(functional_test_logged_in_context, qtbot, mocker, num_to
     """
     gui, controller = functional_test_logged_in_context
 
-    # Wait for a full sync to give us a stable source list.
-    qtbot.wait(TIME_SYNC)
-
     def check_for_sources():
         """Check that we have enough sources to work with."""
         assert len(list(gui.main_view.source_list.source_items.keys())) >= num_to_delete
cfm@kintsugi client % rm tests/functional/data/test_delete_sources.yml 
cfm@kintsugi client % TESTS=tests/functional/test_delete_sources.py make test

Expected Behavior

Either on_reply_download_failure() should only take a DownloadException, or it should not assume all exceptions it handles have a uuid attribute.

Actual Behavior

ERROR tests/functional/test_delete_sources.py::test_delete_sources[3] - Failed: SETUP ERROR: Exceptions caught in Qt event loop:
________________________________________________________________________________
Traceback (most recent call last):
  File "/Users/cfm/Desktop/securedrop-client/client/securedrop_client/logic.py", line 919, in on_reply_download_failure
    reply = storage.get_reply(self.session, exception.uuid)
                                            ^^^^^^^^^^^^^^
AttributeError: 'NoResultFound' object has no attribute 'uuid'
________________________________________________________________________________
Traceback (most recent call last):
  File "/Users/cfm/Desktop/securedrop-client/client/securedrop_client/logic.py", line 919, in on_reply_download_failure
    reply = storage.get_reply(self.session, exception.uuid)
                                            ^^^^^^^^^^^^^^
AttributeError: 'NoResultFound' object has no attribute 'uuid'

[legoktm]

I haven't been able to reproduce this nor obviously trace what's happening...I suppose you don't have the full traceback anymore?

[legoktm]

Previously:

        try:
            reply = storage.get_reply(self.session, exception.uuid)
            self.reply_download_failed.emit(reply.source.uuid, reply.uuid, str(reply))
        except Exception as e:

Now:

        reply = storage.get_reply(self.session, exception.uuid)
        if reply:
            self.reply_download_failed.emit(reply.source.uuid, reply.uuid, str(reply))
        else:

So any old AttributeError would've been caught.

[legoktm]

Interestingly on_file_download_failure types it as just Exception while on_message_download_failure/on_reply_download_failure are specifically DownloadException.

[cfm]

Oops, sorry, @legoktm: see updated steps to reproduce after #2252 above. This particular failure mode under pytest doesn't give a full traceback without further instrumentation.

[legoktm]

thanks, I was able to reproduce now! Digging in a bit.

[legoktm]

So here's the underlying traceback of the exception that then fails in the download handling:

Traceback (most recent call last):
  File "/home/user/github/freedomofpress/securedrop-client/client/securedrop_client/queue.py", line 230, in process
    self.current_job._do_call_api(self.api_client, session)
  File "/home/user/github/freedomofpress/securedrop-client/client/securedrop_client/api_jobs/base.py", line 77, in _do_call_api
    result = self.call_api(api_client, session)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/github/freedomofpress/securedrop-client/client/securedrop_client/api_jobs/downloads.py", line 123, in call_api
    db_object = self.get_db_object(session)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/github/freedomofpress/securedrop-client/client/securedrop_client/api_jobs/downloads.py", line 245, in get_db_object
    return session.query(Reply).filter_by(uuid=self.uuid).one()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/user/.cache/pypoetry/virtualenvs/securedrop-client-OfHWkjU1-py3.11/lib64/python3.11/site-packages/sqlalchemy/orm/query.py", line 3282, in one
    raise orm_exc.NoResultFound("No row was found for one()")
sqlalchemy.orm.exc.NoResultFound: No row was found for one()

(obtained with traceback.print_exception(exception))

[legoktm]

I'm going to submit two PRs, first is the one I already posted, which broadens exception handling because there are still other exceptions that can be raised that could trigger this error in the exception handling.

The second will change DownloadJob.get_db_object() to use one_or_none() just like #2231 and then handle the None case with an explicit DownloadException.

[deeplow]

I wasn't able to reproduce this. Following the reproduction steps, I get:

ERROR tests/functional/test_delete_sources.py::test_delete_sources[1] - pytestqt.exceptions.TimeoutError: waitUntil timed out in 10000 milliseconds
ERROR tests/functional/test_delete_sources.py::test_delete_sources[3] - pytestqt.exceptions.TimeoutError: waitUntil timed out in 10000 milliseconds

[rocodes]

ugh, you even asked me @legoktm if it was okay to stop catching Exception and I said yes. Sorry, but also, thanks for figuring this out because this is better.