Skip to content

[fix] race condition in NextRun() when multiple jobs complete simultaneously#907

Merged
JohnRoesler merged 6 commits intogo-co-op:v2from
barkhayot:i-904/fix/job-next-run
Jan 28, 2026
Merged

[fix] race condition in NextRun() when multiple jobs complete simultaneously#907
JohnRoesler merged 6 commits intogo-co-op:v2from
barkhayot:i-904/fix/job-next-run

Conversation

@barkhayot
Copy link
Contributor

What does this do?

When multiple jobs completed at the same time, NextRun() could return stale/incorrect values due to a race condition in the nextScheduled cleanup logic. The issue occurred because two different methods (selectExecJobsOutCompleted and updateNextScheduled) were cleaning up the nextScheduled slice with different logic:

  • One used !t.Before(now) (keeps times >= now)
  • One used t.After(now) (keeps times > now)

When these methods ran concurrently on different jobs, their writes could interleave and overwrite each other with inconsistent states.

Solution

Unified both methods to use identical cleanup logic: t.After(now) (keep only strictly future times). This makes the cleanup idempotent - both methods now produce the same result regardless of execution order, eliminating the race condition.

Which issue(s) does this PR fix/relate to?

Testing

All existing tests pass. New regression tests verify NextRun() returns correct values when multiple jobs complete simultaneously.

Notes

@barkhayot barkhayot marked this pull request as ready for review January 28, 2026 10:43
Copy link
Contributor

@JohnRoesler JohnRoesler left a comment

Choose a reason for hiding this comment

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

This is great!

// from nextScheduled after it completes. So all intervals should be 14 days
// (2 weeks as configured).
diff := time.Hour * 14 * 24
if iteration == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lint is complaining that the iteration value passed in is unused now. Looks like it can be removed entirely from the assertion func

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching! Made quick fix here 0f55e2a based on feedback 🙏

@barkhayot barkhayot requested a review from JohnRoesler January 28, 2026 12:38
Copy link
Contributor

@JohnRoesler JohnRoesler left a comment

Choose a reason for hiding this comment

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

💯 thank you!

@JohnRoesler JohnRoesler merged commit 93ffead into go-co-op:v2 Jan 28, 2026
3 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.

2 participants