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: use the constructor function every time a periodic job is scheduled, instead of when its registered #420

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

elee1766
Copy link
Contributor

@elee1766 elee1766 commented Jul 4, 2024

right now, the constructor gets called when converting to an internal job, which causes the constructor to use the value at add time.

however, the comments say

// The constructor function is invoked each time a periodic job's schedule
// elapses, returning job arguments to insert along with optional insertion
// options.

so im assuming this is whats intended?

@elee1766 elee1766 changed the title Properly use the constructor function. use the constructor function every time a periodic job is scheduled, instead of when its registered Jul 4, 2024
@elee1766 elee1766 changed the title use the constructor function every time a periodic job is scheduled, instead of when its registered fix: use the constructor function every time a periodic job is scheduled, instead of when its registered Jul 4, 2024
@brandur
Copy link
Contributor

brandur commented Jul 4, 2024

Oops, yes, thanks! I'll try to add a test case to verify this.

@brandur brandur merged commit d5b5960 into riverqueue:master Jul 4, 2024
10 checks passed
brandur added a commit that referenced this pull request Jul 4, 2024
We haven't cut a new release in a while, and the bug in #420 is kind of
bad, so now that there's a fix it's not a bad time to cut a new release.

I'm also including a test case that catches the bug from #420.
Apparently none of our other test cases can catch the problem, so it's
good to have a regression guard.
brandur added a commit that referenced this pull request Jul 4, 2024
We haven't cut a new release in a while, and the bug in #420 is kind of
bad, so now that there's a fix it's not a bad time to cut a new release.

I'm also including a test case that catches the bug from #420.
Apparently none of our other test cases can catch the problem, so it's
good to have a regression guard.
brandur added a commit that referenced this pull request Jul 4, 2024
We haven't cut a new release in a while, and the bug in #420 is kind of
bad, so now that there's a fix it's not a bad time to cut a new release.

I'm also including a test case that catches the bug from #420.
Apparently none of our other test cases can catch the problem, so it's
good to have a regression guard.
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.

2 participants