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

Keep trying failed replies #530

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Keep trying failed replies #530

merged 3 commits into from
Aug 21, 2019

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Aug 9, 2019

Description

Fixes #533
Towards #443

Now when SendReplyJob fails because of a timeout, the api call is retried 5 times, and then re-processed in the queue until the timeout issue is resolved.

This is the same behavior for other jobs that receive timeout errors.

Note: This does not fix #294 because that happens when replies are in the queue and never reach the server (the timeout issue is never resolved during the client session).

Test Plan

In addition to making sure the two issues are no longer reproduced, follow these steps to test:

  1. Send reply after pausing the staging app server vm, wait and observe 5 Keyring access qubes notication popups and reply bar turn red in the gui
  2. Unpause the staging app server vm, wait until the next sync happens or manually click the refresh icon and see the reply bar turn blue

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

@sssoleileraaa sssoleileraaa changed the title Send reply timeout bug Keep trying failed replies Aug 9, 2019
@@ -91,7 +91,8 @@ def test_ApiJob_other_error(mocker):
mock_api_client = mocker.MagicMock()
mock_session = mocker.MagicMock()

api_job._do_call_api(mock_api_client, mock_session)
with pytest.raises(Exception):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A failed job should always raise an exception so the queue processor can log and continue, pause the queue, or drop the job.

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm digging the direction this is going! a few comments/thoughts are inline

securedrop_client/api_jobs/uploads.py Show resolved Hide resolved
securedrop_client/api_jobs/uploads.py Outdated Show resolved Hide resolved
securedrop_client/queue.py Outdated Show resolved Hide resolved
securedrop_client/api_jobs/base.py Outdated Show resolved Hide resolved
@sssoleileraaa sssoleileraaa force-pushed the send-reply-timeout-bug branch from 0a507a3 to ed95843 Compare August 20, 2019 22:01
@sssoleileraaa
Copy link
Contributor Author

review comments have been addressed

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm now! (just needs a rebase and then feel free to merge)

@sssoleileraaa sssoleileraaa force-pushed the send-reply-timeout-bug branch from ed95843 to 2e1ba89 Compare August 21, 2019 19:51
@sssoleileraaa sssoleileraaa merged commit 57996a4 into master Aug 21, 2019
@sssoleileraaa sssoleileraaa deleted the send-reply-timeout-bug branch August 21, 2019 19:54
@sssoleileraaa sssoleileraaa mentioned this pull request Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replies missing in the UI after timeout resolves but before refresh Failed replies may disappear from UI
2 participants