schedule: Prevent spinning on schedule filter#161
Conversation
|
Warning Rate limit exceeded@andrewwormald has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes modify scheduling behaviour: the scheduler now derives the initial last-run time from the latest stored record (or now if none), advances lastRun on each cycle even when a trigger is skipped, and only calls Trigger when the schedule deems it due. waitUntil returns immediately for non‑positive durations. Tests update the filter and introduce a multi‑phase scenario verifying skip/allow transitions and RunID changes. Many module go.mod files bump the Go toolchain from 1.24.2 to 1.25.3. .gitlab-ci.yml is removed and .gitignore gains a new pattern. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
schedule.go (2)
84-92: Skip-path now avoids spinning; consider basing the next window onnextRunand confirm!okfallback is intentionalThis change achieves the PR goal: when the filter skips a run, the scheduler now blocks until a future time instead of returning
nilimmediately and spinning. Behaviour here looks sensible.Two small points you might want to tighten up:
- You already have
nextRun(the window we just waited for). Using that as the base for the following window is slightly clearer and avoids any subtle dependence on how far pastnextRunw.clock.Now()happens to be when the filter executes (especially around DST boundaries or coarse schedules).- When
schedule.Nextfrom “now” returns!ok, you fall back topollingFrequency, whereas earlier (Line 53–56) a!okresult is treated as a hard configuration error. If there are cron specs that can legitimately exhaust future occurrences after the scheduler has been running for a while, this fallback may be exactly what you want; otherwise, it might be worth making that explicit in a comment so future readers do not wonder about the different behaviours.If you like the first suggestion, the change is minimal:
- if !shouldTrigger { - // Get the next scheduled time from now as this run is being skipped. - sleepTill, ok := schedule.Next(w.clock.Now()) - if !ok { - sleepTill = w.clock.Now().Add(w.defaultOpts.pollingFrequency) - } - // Filter excludes this run. Wait till the next scheduled time to attempt to trigger again. - return waitUntil(ctx, w.clock, sleepTill) - } + if !shouldTrigger { + // Get the next scheduled time relative to the window we just waited for. + sleepTill, ok := schedule.Next(nextRun) + if !ok { + sleepTill = w.clock.Now().Add(w.defaultOpts.pollingFrequency) + } + // Filter excludes this run. Wait till the next scheduled time to attempt to trigger again. + return waitUntil(ctx, w.clock, sleepTill) + }
109-121:waitUntilis fine; optionally short-circuit zero/negative waits to avoid unnecessary timers
waitUntilcorrectly uses the injectedclock.Clockand respects context cancellation. One minor robustness/clarity improvement you could consider is avoiding creating a timer whenuntilis already in the past or effectively “now” (which can happen due to small clock skews or coarse schedules):func waitUntil(ctx context.Context, clock clock.Clock, until time.Time) error { - timeDiffAsDuration := until.Sub(clock.Now()) - - t := clock.NewTimer(timeDiffAsDuration) + timeDiffAsDuration := until.Sub(clock.Now()) + if timeDiffAsDuration <= 0 { + // Nothing to wait for; still honour any already‑cancelled context. + select { + case <-ctx.Done(): + return ctx.Err() + default: + return nil + } + } + + t := clock.NewTimer(timeDiffAsDuration) defer t.Stop()This keeps the semantics the same while avoiding a
NewTimercall for effectively zero sleep, and should work with both real and fakeclock.Clockimplementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
adapters/sqlite/tmp/example.dbis excluded by!**/*.db
📒 Files selected for processing (1)
schedule.go(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
schedule_test.go (1)
172-178: Clarify initialskipValand filter semanticsHere
skipValis initialised totrue, so the filter will returntruefrom the very first evaluation. Given the variable is namedshouldSkipand later comments refer to "Skip until the next run", it would be good to confirm whethertruehere really means "skip this schedule" or "allow this schedule".If
truemeans "skip", you may want to start withskipVal := false(no skipping at start) and only flip it totruewhen you actually want to exercise the filtered‑out case; otherwise the test may be asserting behaviour for a permanently filtered schedule rather than a temporarily filtered one.If
trueinstead means "run", then the naming and comments are misleading and should be adjusted for clarity to avoid future confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
schedule.go(2 hunks)schedule_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- schedule.go
🧰 Additional context used
🧬 Code graph analysis (1)
schedule_test.go (2)
workflow_test.go (1)
StatusEnd(54-54)_examples/webui/main.go (1)
StatusEnd(82-82)
🪛 GitHub Actions: Run
schedule_test.go
[error] 236-236: TestWorkflow_ScheduleFilter failed. Should not be: "dcf16fda-d6ae-11f0-a0f4-7c1e52c322bd".
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
adapters/jlog/go.mod (1)
3-3: Consistent Go version upgrade.The version bump to 1.25.3 is applied consistently across all
go.modfiles in the repository, which is good for maintainability. However, consider the same upgrade path as noted in the previous file.
🧹 Nitpick comments (1)
_examples/gettingstarted/go.mod (1)
3-3: Consider upgrading to Go 1.25.5 for the latest security patches.Go 1.25.3 was released on 2025-10-13 and includes fixes to the crypto/x509 package, but Go 1.25.5 (released 2025-12-02) includes two security fixes to the crypto/x509 package, as well as bug fixes to the mime and os packages. Since your PR is being reviewed in December 2025, upgrading the entire codebase to 1.25.5 would be advisable for better security coverage, especially given that Go's support model means point releases may receive important patches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
_examples/callback/go.mod(1 hunks)_examples/connector/go.mod(1 hunks)_examples/gettingstarted/go.mod(1 hunks)_examples/schedule/go.mod(1 hunks)_examples/timeout/go.mod(1 hunks)_examples/webui/go.mod(1 hunks)adapters/jlog/go.mod(1 hunks)adapters/kafkastreamer/go.mod(1 hunks)adapters/reflexstreamer/go.mod(1 hunks)adapters/rinkrolescheduler/go.mod(1 hunks)adapters/sqlstore/go.mod(1 hunks)adapters/sqltimeout/go.mod(1 hunks)adapters/webui/go.mod(1 hunks)go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- _examples/timeout/go.mod
- adapters/webui/go.mod
- adapters/rinkrolescheduler/go.mod
- adapters/kafkastreamer/go.mod
🔇 Additional comments (8)
adapters/sqlstore/go.mod (1)
3-3: Toolchain version update is consistent across all modules.All go.mod files in the PR are updated uniformly from Go 1.24.2 to 1.25.3 with no dependency changes, which maintains consistency across the monorepo.
adapters/reflexstreamer/go.mod (1)
3-3: Toolchain version consistent with repository-wide update.The Go version bump mirrors the uniform update across all modules without altering dependencies.
_examples/connector/go.mod (1)
3-3: Go toolchain version updated consistently.The version bump is aligned with all other modules in the PR.
_examples/callback/go.mod (1)
3-3: Go version aligned with repository-wide update.The version bump is consistent across the codebase.
_examples/schedule/go.mod (1)
3-3: Go toolchain version bumped consistently with other modules.The update to Go 1.25.3 is applied uniformly across all modules.
_examples/webui/go.mod (1)
3-3: Version updated consistently.The Go toolchain version aligns with the repository-wide update to 1.25.3.
adapters/sqltimeout/go.mod (1)
3-3: Go version bump is consistent with repository-wide update.The change maintains parity across all modules.
go.mod (1)
3-3: Root module Go version aligned with all modules.The Go toolchain version is updated consistently across the entire repository to 1.25.3, with no dependency version changes.
|
1 participant
Currently if the schedule is filtered out it can spin by returning nil to the runner. The runner immediately attempts to restart the process as there was no error.