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

Lost keys when using job.progress(...) #2286

Closed
carlos870 opened this issue Feb 10, 2022 · 4 comments
Closed

Lost keys when using job.progress(...) #2286

carlos870 opened this issue Feb 10, 2022 · 4 comments
Assignees
Labels

Comments

@carlos870
Copy link

Description

The job.progress method is not checking if the job key actually exists before updating the job's progress, which means we could end up with with an empty key left in Redis, only with the updated progress.
If the job config uses a custom jobId (based on business logic - userId), this could become problematic, since no jobs for that user would be added anymore.

A simple solution that I tried and it seems to fix the issue, was to wrap the LUA script updateProgress (HSET/PUBLISH) inside a Redis EXISTS command.

This occurred in a scenario where we get the job (getJob()), and then try to update it's progress, and sometimes the job was already removed. We are using setting removeOnComplete.

Bull version

4.4.0

@manast
Copy link
Member

manast commented Feb 13, 2022

Thanks for the report. Yes, this sounds like an oversight from my part, a job progress should only be possible to perform while a job is in active state and if the caller owns the lock.

@manast manast self-assigned this Feb 13, 2022
@manast manast added the bug label Feb 13, 2022
@carlos870
Copy link
Author

Thanks for looking into it!

Meanwhile I also noticed the same behaviour for the job.update action.

@manast
Copy link
Member

manast commented Feb 14, 2022

For job.update I do not think it is necessary to check for a lock, since usually this method is used outside of the processor, on the other hand it could require the job to exist in the first place.

@roggervalf
Copy link
Collaborator

It's fixes in this pr #2730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants