procrastinate-org / procrastinate

PostgreSQL-based Task Queue for Python
https://procrastinate.readthedocs.io/
MIT License
859 stars 53 forks source link

Set job to failed when task raises and job is aborting (regardless retry) #1133

Closed medihack closed 2 months ago

medihack commented 3 months ago

Closes #1131

Successful PR Checklist:

PR label(s):

This time, a job with the status aborting of a task that raises is set to failed (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 status aborting to aborted when the task raises, but one to failed that 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.

github-actions[bot] commented 3 months ago

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  procrastinate
  worker.py
Project Total  

This report was generated by python-coverage-comment-action

medihack commented 3 months ago

That's great! I believe there may be a bit of conflict when we merge the worker refactor, though.

@onlyann When I commit this (later today), can you include it in the worker refactoring?

Thank you so much for the time you spent and doing it all over again after my comment!

No prob, the comment was very relevant. And I am happy that we don't need a migration 🙂.

onlyann commented 2 months ago

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

medihack commented 2 months ago

Alright, I will write a note in the documentation, merge it, and make a patch release.

ewjoachim commented 2 months ago

:+1: