-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
@@ -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): |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
0a507a3
to
ed95843
Compare
review comments have been addressed |
There was a problem hiding this 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)
ed95843
to
2e1ba89
Compare
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:
Keyring access
qubes notication popups and reply bar turn red in the guiChecklist
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: