Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle sqlalchemy error + app crash when a source that has ongoing download job is deleted #2217

Open
rocodes opened this issue Sep 9, 2024 · 0 comments · May be fixed by #2231
Open

Handle sqlalchemy error + app crash when a source that has ongoing download job is deleted #2217

rocodes opened this issue Sep 9, 2024 · 0 comments · May be fixed by #2231
Assignees
Labels
bug Something isn't working ux

Comments

@rocodes
Copy link
Contributor

rocodes commented Sep 9, 2024

Description

Starting a download for a given user, then deleting that user, has undefined behaviour; if the download job is slower than the server's acknowledgment of the deletion request, then the download will fail, but then a sqlalchemy error will be raised because the on_file_download_error presumes that the file uuid is present in the databse, when in fact it has been removed), leading to an app crash.

Steps to Reproduce

Start a large file download for a given source, then delete that source. Observe logs and possible app crash.

Expected Behavior

(Requesting eyes on this; this is what is expected from a technical point of view, not sure if it's what users would expect. If a user on one computer is downloading a file deleted by another user, the download will fail.)

  • Deletion wins, and a source that has been deleted does not throw an error when file downloads fail.
  • No disconnected database records, partial files, or file download remnants for an orphaned/delete source remain on disk.

Actual Behavior

[sd app, timestamp redacted] root:66(excepthook) ERROR: Unrecoverable error#012Traceback (most recent call last):#012  File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/securedrop_client/logic.py", line 1001, in on_file_download_failure#012    f = self.get_file(exception.uuid)#012        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#012  File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/securedrop_client/logic.py", line 1162, in get_file#012    file = storage.get_file(self.session, file_uuid)#012           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#012  File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/securedrop_client/storage.py", line 1059, in get_file#012    return session.query(File).filter_by(uuid=uuid).one()#012           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#012  File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/sqlalchemy/orm/query.py", line 3282, in one#012    raise orm_exc.NoResultFound("No row was found for one()")#012sqlalchemy.orm.exc.NoResultFound: No row was found for one()
sd-app qubes.VMShell-dom0: Traceback (most recent call last):
sd-app qubes.VMShell-dom0:   File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/securedrop_client/logic.py", line 1001, in on_file_download_failure
sd-app qubes.VMShell-dom0:     f = self.get_file(exception.uuid)
sd-app qubes.VMShell-dom0:         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sd app qubes.VMShell-dom0:   File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/securedrop_client/logic.py", line 1162, in get_file
sd app qubes.VMShell-dom0:     file = storage.get_file(self.session, file_uuid)
sd app qubes.VMShell-dom0:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sd app qubes.VMShell-dom0:   File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/securedrop_client/storage.py", line 1059, in get_file
sd-app qubes.VMShell-dom0:     return session.query(File).filter_by(uuid=uuid).one()
sd-app qubes.VMShell-dom0:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sd-app qubes.VMShell-dom0:   File "/opt/venvs/securedrop-client/lib/python3.11/site-packages/sqlalchemy/orm/query.py", line 3282, in one
sd-app qubes.VMShell-dom0:     raise orm_exc.NoResultFound("No row was found for one()")
sd-app qubes.VMShell-dom0: sqlalchemy.orm.exc.NoResultFound: No row was found for one()

Comments

This picture:

  • If a DownloadDecryptionException (or any download exception) is raised and the uuid isn't in the files table, then the file was deleted. Proper exception handling should occur, the exception should be logged, and in this case, the exception could either be generically reported to the user, or logged but not flashed in the status bar.
  • When cancellable downloads are implemented, deleting a user should send a download cancellation job for any ongoing downloads they may have.

Big picture:
sqlalchemy one() should be examined in the client codebase to ensure that we are always error-handling properly, and knowing when a missing record is fine (deleted elsewhere) vs when it is an error, and we should take holistic /design steps to avoid these types of issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant