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

Fix race condition causing incorrect status not_found #362

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

iamlikeme
Copy link
Contributor

@iamlikeme iamlikeme commented Nov 19, 2022

Checking job status consited of three separate calls to Redis, where the first one checked if result is present and the second checked if job is in progress. If a job got completed between these two calls Job.status() would erroneously return status not_found.

The solution is to make all the calls to Redis in one transaction.

Fixes #329

Checking job status consited of three separate calls to Redis,
where the first one checked if result is present and the second
checked if job is in progress. If a job got completed between
these two calls Job.status() would erroneously return status
not_found.

The solution is to make all the calls to Redis in one transaction.
@iamlikeme iamlikeme changed the title Fix race condition causing status not_found Fix race condition causing incorrect status not_found Nov 19, 2022
@codecov
Copy link

codecov bot commented Nov 19, 2022

Codecov Report

Merging #362 (0a46032) into main (1495be6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
- Coverage   98.80%   98.80%   -0.01%     
==========================================
  Files          11        9       -2     
  Lines        1007      921      -86     
  Branches      185      167      -18     
==========================================
- Hits          995      910      -85     
  Misses          6        6              
+ Partials        6        5       -1     
Impacted Files Coverage Δ
arq/jobs.py 98.05% <100.00%> (+0.05%) ⬆️
arq/cron.py
arq/__init__.py
arq/worker.py 99.55% <0.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1495be6...0a46032. Read the comment docs.

@iamlikeme
Copy link
Contributor Author

I had a hard time writing good tests for this change. I could reproduce the issue by patching some implementation details in Worker.finish_job() and Job.status() but after fixing the problem the patches no longer made sense (because implementation changed). So there is no proof that issue #329 is fixed, but I believe it is reasonable to believe it is.

@samuelcolvin samuelcolvin merged commit 6f68086 into python-arq:main Dec 1, 2022
@samuelcolvin
Copy link
Member

thanks so much.

@iamlikeme iamlikeme deleted the fix-race-condition branch December 18, 2022 20:20
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.

Status statistically return not found for job that just ended
2 participants