cron: Replace external dep with internal implementation#151
cron: Replace external dep with internal implementation#151andrewwormald merged 4 commits intomainfrom
Conversation
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 4
🧹 Nitpick comments (4)
internal/cron/cron.go (2)
85-97: Allow Sunday as 7 for backwards compatibility.Many cron parsers accept both 0 and 7 as Sunday. Your implementation rejects 7, which is likely a breaking change compared to robfig/cron. Map 7→0 during weekday parsing.
Apply this change in parseCrontab to use a weekday-specific parser:
- weekday, err := parseField(fields[4], 0, 6) + weekday, err := parseWeekday(fields[4]) if err != nil { return nil, fmt.Errorf("invalid weekday: %v", err) }Add this helper (place near
parseField):func parseWeekday(field string) (int, error) { if field == "*" || field == "?" { return -1, nil } if field == "7" { return 0, nil // Sunday alias } v, err := strconv.Atoi(field) if err != nil { return 0, fmt.Errorf("invalid number: %s", field) } if v < 0 || v > 6 { return 0, fmt.Errorf("value %d out of range [0-6]", v) } return v, nil }Please confirm whether your public docs/users rely on 7=Sunday; if so, this change is necessary.
10-12: Add doc comments for exported API.Add Go doc for
ScheduleandParseto satisfy linters and clarify semantics (wildcards, DOM/DOW OR, Sunday=0/7, zero-time behaviour).Also applies to: 14-26
internal/cron/cron_test.go (2)
306-311: Test name is misleading; spec matches only Mondays.Rename for accuracy: it asserts Monday 09:00, not Mon–Fri.
- name: "Monday through Friday at 9 AM", + name: "Monday at 9 AM", spec: "0 9 * * 1",
368-387: Add negative/@every and unreachable-spec test cases.Strengthen coverage for validations proposed in cron.go.
@@ func TestParse(t *testing.T) { - } + { + name: "every negative duration rejected", + spec: "@every -5m", + baseTime: baseTime, + want: time.Time{}, + wantErr: true, + }, + { + name: "impossible date rejected (31 Feb)", + spec: "0 0 31 2 *", + baseTime: baseTime, + want: time.Time{}, + wantErr: true, + }, + }I can push a commit with these if you want me to open a quick PR against this branch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(0 hunks)internal/cron/cron.go(1 hunks)internal/cron/cron_test.go(1 hunks)schedule.go(2 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cron/cron_test.go (1)
internal/cron/cron.go (1)
Parse(14-26)
schedule.go (1)
internal/cron/cron.go (1)
Parse(14-26)
🔇 Additional comments (1)
internal/cron/cron.go (1)
159-163: Correct OR semantics for DOM vs DOW.Using OR when both day and weekday are specified matches Vixie/robfig semantics. Nice.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/cron/cron.go (2)
39-41: Good fix: disallow non‑positive @every durations.This addresses the prior review about zero/negative durations.
63-101: Reject impossible day/month combinations at parse time.Specs like “0 0 31 2 *” can never match; without validation, Next() scans 4 years then returns zero, risking upstream spin. Fail fast in Parse.
Apply this diff:
weekday, err := parseWeekday(fields[4]) if err != nil { return nil, fmt.Errorf("invalid weekday: %v", err) } + // Validate impossible month-day combinations (single-value fields only). + if month != -1 && day != -1 { + switch month { + case 4, 6, 9, 11: + if day > 30 { + return nil, fmt.Errorf("day %d out of range for month %d", day, month) + } + case 2: + if day > 29 { + return nil, fmt.Errorf("day %d out of range for February", day) + } + // day==29 is allowed; Next() will find the next leap year. + } + } + return &cronSchedule{ minute: minute, hour: hour, day: day, month: month, weekday: weekday, }, nil
🧹 Nitpick comments (5)
internal/cron/cron.go (2)
10-12: Add a brief contract doc for Schedule.Next.Document that Next must return a time strictly after the argument, uses the time’s Location, and (ideally) never returns the zero time.
Apply this diff:
type Schedule interface { - Next(time.Time) time.Time + // Next returns the first time strictly after t that matches the schedule. + // Implementations should respect t.Location() and avoid returning the zero time. + Next(time.Time) time.Time }
166-185: Clarify DOM vs DOW semantics are OR.matches() uses OR when both day and weekday are specified. This is fine, but differs from some cron variants. Document it in package comments to avoid surprises.
internal/cron/cron_test.go (3)
320-325: Fix misleading test name.Spec “0 9 * * 1” is Monday only, not Monday–Friday. Rename for accuracy.
Apply this diff:
- name: "Monday through Friday at 9 AM", + name: "Monday at 9 AM", spec: "0 9 * * 1",
8-382: Add tests for impossible day/month combos (now rejected).Strengthen coverage to ensure Parse fails fast for unreachable specs.
Apply this diff (append to the tests slice):
@@ { name: "every microsecond (minimal duration)", spec: "@every 1us", baseTime: baseTime, want: baseTime.Add(time.Microsecond), wantErr: false, }, + { + name: "invalid: 31 Feb", + spec: "0 0 31 2 *", + baseTime: baseTime, + want: time.Time{}, + wantErr: true, + }, + { + name: "invalid: 31 April", + spec: "0 0 31 4 *", + baseTime: baseTime, + want: time.Time{}, + wantErr: true, + },
8-382: Consider a regression test to ensure Next() never returns zero after Parse success.Guard against future changes that might reintroduce zero-time results.
You can add:
t.Run("no zero time after valid parse", func(t *testing.T) { s, err := Parse("0 0 1 1 *") if err != nil { t.Fatalf("parse failed: %v", err) } if s.Next(baseTime).IsZero() { t.Fatalf("Next returned zero time") } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cron/cron.go(1 hunks)internal/cron/cron_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cron/cron_test.go (1)
internal/cron/cron.go (1)
Parse(14-26)
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/cron/cron.go (1)
94-101: Validate impossible day-of-month/month combinations.Expressions like "0 0 31 2 *" can never match. These run the 4-year scan and then return false. Proactively reject impossible combos to fail fast.
month, err := parseField(fields[3], 1, 12) if err != nil { return nil, fmt.Errorf("invalid month: %v", err) } weekday, err := parseWeekday(fields[4]) if err != nil { return nil, fmt.Errorf("invalid weekday: %v", err) } + // Validate impossible combinations when month and day are both specified. + if month != -1 && day != -1 { + switch month { + case 4, 6, 9, 11: + if day > 30 { + return nil, fmt.Errorf("day %d out of range for month %d", day, month) + } + case 2: + if day > 29 { + return nil, fmt.Errorf("day %d out of range for February", day) + } + // day==29 is allowed; Next() will find the next leap year. + } + } + return &cronSchedule{ minute: minute, hour: hour, day: day, month: month, weekday: weekday, }, nilschedule.go (1)
53-57: Guard against unreachable schedules returning false.Good that you're now handling the bool return from
schedule.Next(). However, consider adding more context about why the schedule is unreachable (e.g., impossible date combinations).The error handling is correct, but we should verify that the error message provides sufficient debugging information. Let's check if there are any tests that validate this error path:
#!/bin/bash # Check for tests covering the schedule.Next() error path rg -n "no next schedule found" --type go
🧹 Nitpick comments (4)
internal/cron/cron.go (3)
103-118: Consider supporting named weekday aliases.The current implementation only supports numeric weekday values (0-6) and "7" as an alias for Sunday. Consider supporting standard cron weekday names (SUN, MON, TUE, etc.) for better compatibility.
func parseWeekday(field string) (int, error) { if field == "*" || field == "?" { return -1, nil } if field == "7" { return 0, nil // Sunday alias } + + // Support weekday name aliases + weekdayNames := map[string]int{ + "SUN": 0, "MON": 1, "TUE": 2, "WED": 3, + "THU": 4, "FRI": 5, "SAT": 6, + } + if val, ok := weekdayNames[strings.ToUpper(field)]; ok { + return val, nil + } + v, err := strconv.Atoi(field) if err != nil { return 0, fmt.Errorf("invalid number: %s", field) } if v < 0 || v > 6 { return 0, fmt.Errorf("value %d out of range [0-6]", v) } return v, nil }
120-135: Consider supporting ranges and lists in cron fields.The current implementation only supports single values or wildcards. Standard cron syntax includes ranges (e.g., "1-5") and lists (e.g., "0,15,30,45"). Adding this support would improve compatibility with existing cron specifications.
Example enhancement to support ranges and lists:
func parseField(field string, min, max int) ([]int, error) { if field == "*" || field == "?" { return nil, nil // nil indicates wildcard } var values []int for _, part := range strings.Split(field, ",") { if strings.Contains(part, "-") { // Parse range rangeParts := strings.Split(part, "-") if len(rangeParts) != 2 { return nil, fmt.Errorf("invalid range: %s", part) } start, err := strconv.Atoi(rangeParts[0]) if err != nil { return nil, fmt.Errorf("invalid range start: %s", rangeParts[0]) } end, err := strconv.Atoi(rangeParts[1]) if err != nil { return nil, fmt.Errorf("invalid range end: %s", rangeParts[1]) } if start < min || end > max || start > end { return nil, fmt.Errorf("range %d-%d out of bounds [%d-%d]", start, end, min, max) } for i := start; i <= end; i++ { values = append(values, i) } } else { // Parse single value val, err := strconv.Atoi(part) if err != nil { return nil, fmt.Errorf("invalid number: %s", part) } if val < min || val > max { return nil, fmt.Errorf("value %d out of range [%d-%d]", val, min, max) } values = append(values, val) } } return values, nil }This would require updating the cronSchedule struct and matches method to handle multiple values per field.
153-164: Consider optimising the Next() search algorithm.The current implementation performs a linear minute-by-minute search up to 4 years. For schedules with large gaps (e.g., yearly), this could check over 2 million iterations. Consider a more efficient approach that jumps to likely candidates.
For example, if the schedule is monthly (day=1, month=-1), you could jump directly to the 1st of each month rather than checking every minute:
func (c *cronSchedule) Next(t time.Time) (time.Time, bool) { next := t.Add(time.Minute).Truncate(time.Minute) // Fast path for common patterns if c.minute != -1 && c.hour != -1 && c.day != -1 && c.month == -1 && c.weekday == -1 { // Daily at specific time - jump to next occurrence candidate := time.Date(next.Year(), next.Month(), next.Day(), c.hour, c.minute, 0, 0, next.Location()) if candidate.After(t) && c.matches(candidate) { return candidate, true } candidate = candidate.AddDate(0, 0, 1) if c.matches(candidate) { return candidate, true } } // Fallback to minute-by-minute search for i := 0; i < 4*365*24*60; i++ { if c.matches(next) { return next, true } next = next.Add(time.Minute) } return time.Time{}, false }schedule.go (1)
103-115: Consider adding protection against negative durations in waitUntil.While the new
Next()implementation returnsfalsefor unreachable schedules, there's still a risk ifuntilis in the past (e.g., if system clock changes or there's a race condition). Consider adding a guard.func waitUntil(ctx context.Context, clock clock.Clock, until time.Time) error { timeDiffAsDuration := until.Sub(clock.Now()) + + // If the target time is in the past, return immediately + if timeDiffAsDuration <= 0 { + return nil + } t := clock.NewTimer(timeDiffAsDuration) defer t.Stop() select { case <-ctx.Done(): return ctx.Err() case <-t.C(): return nil } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cron/cron.go(1 hunks)internal/cron/cron_test.go(1 hunks)schedule.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cron/cron_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
schedule.go (1)
internal/cron/cron.go (1)
Parse(14-26)
1 participant
Removing the 3rd party cron spec as it's largely not needed as a 3rd party and is being offered as a first class feature. Unless the Schedule method is removed then this can exist with direct support and optimisations where needed.
Summary by CodeRabbit
New Features
Chores
Tests