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: #98 retries can be picked up by wrong handler #100

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

acaloiaro
Copy link
Owner

@acaloiaro acaloiaro commented Oct 20, 2023

Fixes #98

p.announceJob(ctx, queue, jid)
}(jobID)
p.announceJob(ctx, j.Queue, jid)
}(jobID, job)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the crux of the bug. queue was captured by the above function, but queue was not necessarily the the job's queue.

@@ -31,21 +31,20 @@ import (
var migrationsFS embed.FS

const (
PendingJobIDQuery = `SELECT id
JobQuery = `SELECT id,fingerprint,queue,status,deadline,payload,retries,max_retries,run_after,ran_at,created_at,error
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff does a poor job of explaining this change. I've eliminated PendingJobQuery and replaced it with JobQuery, which has fewer filters, and should perform better.

Because of the setting in which PendingJobQuery was executed, it never needed to have the filter run_after <= NOW(). When that query is executed, the jobID is already pre-vetted as a pending job.

@acaloiaro acaloiaro force-pushed the fix-98-retries-picked-up-by-wrong-handler branch from cbcf27e to f854991 Compare October 20, 2023 14:49
@acaloiaro acaloiaro force-pushed the fix-98-retries-picked-up-by-wrong-handler branch 2 times, most recently from 7a3e7ac to 6a48598 Compare October 20, 2023 15:21
@acaloiaro acaloiaro self-assigned this Oct 20, 2023
Fixes a bug that can allow retries to end up on
the wrong queue in settings where there are
multiple handlers.
@acaloiaro acaloiaro force-pushed the fix-98-retries-picked-up-by-wrong-handler branch from 6a48598 to 4ec478c Compare October 21, 2023 13:25
@acaloiaro acaloiaro merged commit e95311e into main Oct 21, 2023
2 checks passed
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.

Panic when moving to neoq_dead_jobs
1 participant