Set job to failed when task raises and job is aborting (regardless retry) #1133
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
| @@ -385,6 +385,7 @@ def job_func(a, b): # pylint: disable=unused-argument | |||
| task_name="job", | |||
| queue="yay", | |||
| ) | |||
| app.job_manager.get_job_status_async = mocker.AsyncMock(return_value="doing") | |||
There was a problem hiding this comment.
Just curious: what happens if we don't mock? The InMemory implementation doesn't answer correctly ?
There was a problem hiding this comment.
It'll throw. The job is not put into the worker queue. The run_job method is tested in isolation.
@onlyann When I commit this (later today), can you include it in the worker refactoring?
No prob, the comment was very relevant. And I am happy that we don't need a migration 🙂. |
|
That's not a problem, I will resolve the conflict. I think it's fine to merge this fix but wanted to highlight that there can still be a race condition where the job is changed to "aborting" between the check and the moment the job is set to retry. Something we can look at improving if we end up removing that extra status in v3 |
|
Alright, I will write a note in the documentation, merge it, and make a patch release. |
|
👍 |
Closes #1131
Successful PR Checklist:
PR label(s):
This time, a job with the status
abortingof a task that raises is set tofailed(regardless of whether it should be retried or not). It feels a bit more consistent to me, but somehow, there is no optimal way. If we set a to-be-retried job in statusabortingtoabortedwhen the task raises, but one tofailedthat should not be retried, it seems somehow inconsistent to me.@ewjoachim A note in the documentation is still missing. Just want to hear what you think.