From 5cd1fcb49237defc59d3f1c9af4bd5307b7c9e5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 20 Mar 2026 08:57:13 -0400 Subject: [PATCH] feat(scheduler): add crontab(5) random ~ syntax support (#5233) * feat(scheduler): add CrontabSchedule with crontab(5) random ~ syntax Implement ParseCrontab() that extends robfig/cron with support for the crontab(5) random ~ operator (e.g., 0~30 * * * *). Random values are resolved fresh on each Next() call for load spreading. Supports A~B, ~B, A~, and bare ~ forms in all 6 fields (including seconds). Expressions without ~ delegate to robfig's standard parser with zero overhead. Integrates into scheduler.Add() and conf.validateSchedule() so that scanner.schedule and backup.schedule config values accept ~ syntax. * refactor(scheduler): resolve random ~ values once at parse time Change from per-Next() randomization to per-parse randomization, matching crontab(5) semantics. This prevents double-firing within the same period when random values land after the current time. ParseCrontab now resolves ~ fields to concrete values, substitutes them into the spec string, and delegates to robfig's parser. This eliminates CrontabSchedule, randomField, and resolveField entirely. * test(scheduler): replace WaitGroup with channel for job execution synchronization Signed-off-by: Deluan --------- Signed-off-by: Deluan --- conf/configuration.go | 10 +- scheduler/crontab_schedule.go | 133 ++++++++++++++++++++ scheduler/crontab_schedule_test.go | 194 +++++++++++++++++++++++++++++ scheduler/scheduler.go | 3 +- scheduler/scheduler_test.go | 40 +++--- 5 files changed, 347 insertions(+), 33 deletions(-) create mode 100644 scheduler/crontab_schedule.go create mode 100644 scheduler/crontab_schedule_test.go diff --git a/conf/configuration.go b/conf/configuration.go index 9efa352b..a5c25374 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -16,8 +16,8 @@ import ( "github.com/kr/pretty" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/scheduler" "github.com/navidrome/navidrome/utils/run" - "github.com/robfig/cron/v3" "github.com/spf13/viper" ) @@ -570,15 +570,9 @@ func validateBackupSchedule() error { } func validateSchedule(schedule, field string) (string, error) { - if _, err := time.ParseDuration(schedule); err == nil { - schedule = "@every " + schedule - } - c := cron.New() - id, err := c.AddFunc(schedule, func() {}) + _, err := scheduler.ParseCrontab(schedule) if err != nil { log.Error(fmt.Sprintf("Invalid %s. Please read format spec at https://pkg.go.dev/github.com/robfig/cron#hdr-CRON_Expression_Format", field), "schedule", schedule, err) - } else { - c.Remove(id) } return schedule, err } diff --git a/scheduler/crontab_schedule.go b/scheduler/crontab_schedule.go new file mode 100644 index 00000000..5de1ae14 --- /dev/null +++ b/scheduler/crontab_schedule.go @@ -0,0 +1,133 @@ +package scheduler + +import ( + "fmt" + "math/rand/v2" + "strconv" + "strings" + "time" + + "github.com/robfig/cron/v3" +) + +var parser = cron.NewParser( + cron.SecondOptional | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow | cron.Descriptor, +) + +// ParseCrontab parses a cron expression with support for the crontab(5) random ~ syntax. +// Random values are resolved once at parse time. If no ~ is present, it delegates to +// robfig/cron's standard parser. Duration strings (e.g., "5m") are converted to "@every 5m". +func ParseCrontab(spec string) (cron.Schedule, error) { + if spec == "" { + return nil, fmt.Errorf("empty spec string") + } + + if _, err := time.ParseDuration(spec); err == nil { + spec = "@every " + spec + } + + if !strings.Contains(spec, "~") { + return parser.Parse(spec) + } + + // Handle TZ=/CRON_TZ= prefix + var tzPrefix string + if strings.HasPrefix(spec, "TZ=") || strings.HasPrefix(spec, "CRON_TZ=") { + i := strings.Index(spec, " ") + if i == -1 { + return nil, fmt.Errorf("missing spec after timezone") + } + tzPrefix = spec[:i] + " " + spec = strings.TrimSpace(spec[i:]) + } + + // @ descriptors cannot contain ~ + if strings.HasPrefix(spec, "@") { + return nil, fmt.Errorf("random ~ syntax cannot be used with descriptors: %s", spec) + } + + fields := strings.Fields(spec) + fields, err := normalizeFields(fields) + if err != nil { + return nil, err + } + + // Resolve each ~ field to a concrete random value + for i, field := range fields { + if !strings.Contains(field, "~") { + continue + } + if strings.ContainsAny(field, ",/") { + return nil, fmt.Errorf("random ~ cannot be combined with lists or steps: %s", field) + } + v, parseErr := resolveRandomField(field, fieldBounds[i]) + if parseErr != nil { + return nil, parseErr + } + fields[i] = strconv.FormatUint(uint64(v), 10) + } + + // Re-assemble and parse with robfig + resolved := tzPrefix + strings.Join(fields, " ") + return parser.Parse(resolved) +} + +type bounds struct { + min, max uint +} + +var fieldBounds = [6]bounds{ + {0, 59}, // Second + {0, 59}, // Minute + {0, 23}, // Hour + {1, 31}, // Dom + {1, 12}, // Month + {0, 6}, // Dow +} + +// resolveRandomField parses a ~ field and returns a random value within the range. +func resolveRandomField(field string, b bounds) (uint, error) { + parts := strings.SplitN(field, "~", 2) + + min := b.min + max := b.max + + if parts[0] != "" { + v, err := strconv.ParseUint(parts[0], 10, 0) + if err != nil { + return 0, fmt.Errorf("invalid random range start: %s", parts[0]) + } + min = uint(v) + } + + if parts[1] != "" { + v, err := strconv.ParseUint(parts[1], 10, 0) + if err != nil { + return 0, fmt.Errorf("invalid random range end: %s", parts[1]) + } + max = uint(v) + } + + if min < b.min { + return 0, fmt.Errorf("random range start (%d) below minimum (%d): %s", min, b.min, field) + } + if max > b.max { + return 0, fmt.Errorf("random range end (%d) above maximum (%d): %s", max, b.max, field) + } + if min > max { + return 0, fmt.Errorf("random range start (%d) beyond end (%d): %s", min, max, field) + } + + return min + uint(rand.IntN(int(max-min+1))), nil //nolint:gosec // Cryptographic randomness not needed for schedule jitter +} + +func normalizeFields(fields []string) ([]string, error) { + switch len(fields) { + case 5: + return append([]string{"0"}, fields...), nil + case 6: + return fields, nil + default: + return nil, fmt.Errorf("expected 5 or 6 fields, found %d: %v", len(fields), fields) + } +} diff --git a/scheduler/crontab_schedule_test.go b/scheduler/crontab_schedule_test.go new file mode 100644 index 00000000..b1e26f1d --- /dev/null +++ b/scheduler/crontab_schedule_test.go @@ -0,0 +1,194 @@ +package scheduler + +import ( + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/robfig/cron/v3" +) + +var _ = Describe("ParseCrontab", func() { + Describe("standard expressions", func() { + It("parses a 5-field expression", func() { + sched, err := ParseCrontab("5 * * * *") + Expect(err).ToNot(HaveOccurred()) + Expect(sched).To(BeAssignableToTypeOf(&cron.SpecSchedule{})) + }) + + It("parses a 6-field expression with seconds", func() { + sched, err := ParseCrontab("30 5 * * * *") + Expect(err).ToNot(HaveOccurred()) + Expect(sched).To(BeAssignableToTypeOf(&cron.SpecSchedule{})) + }) + + It("converts duration string to @every", func() { + sched, err := ParseCrontab("5m") + Expect(err).ToNot(HaveOccurred()) + Expect(sched).To(BeAssignableToTypeOf(cron.ConstantDelaySchedule{})) + }) + + It("returns error for empty string", func() { + _, err := ParseCrontab("") + Expect(err).To(HaveOccurred()) + }) + }) + + Describe("random ~ syntax", func() { + It("resolves A~B to a value within range", func() { + sched, err := ParseCrontab("0~30 * * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + minute := findSetBit(spec.Minute) + Expect(minute).To(BeNumerically(">=", 0)) + Expect(minute).To(BeNumerically("<=", 30)) + }) + + It("resolves ~ alone to full field range", func() { + sched, err := ParseCrontab("~ * * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + minute := findSetBit(spec.Minute) + Expect(minute).To(BeNumerically(">=", 0)) + Expect(minute).To(BeNumerically("<=", 59)) + }) + + It("resolves ~B as min~B", func() { + sched, err := ParseCrontab("~15 * * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + minute := findSetBit(spec.Minute) + Expect(minute).To(BeNumerically(">=", 0)) + Expect(minute).To(BeNumerically("<=", 15)) + }) + + It("resolves A~ as A~max", func() { + sched, err := ParseCrontab("15~ * * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + minute := findSetBit(spec.Minute) + Expect(minute).To(BeNumerically(">=", 15)) + Expect(minute).To(BeNumerically("<=", 59)) + }) + + It("resolves multiple random fields independently", func() { + sched, err := ParseCrontab("0~30 0~12 * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + Expect(findSetBit(spec.Minute)).To(BeNumerically("<=", 30)) + Expect(findSetBit(spec.Hour)).To(BeNumerically("<=", 12)) + }) + + It("resolves ~ in DOM field with correct bounds", func() { + sched, err := ParseCrontab("0 0 ~ * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + dom := findSetBit(spec.Dom) + Expect(dom).To(BeNumerically(">=", 1)) + Expect(dom).To(BeNumerically("<=", 31)) + }) + + It("resolves ~ in month field with correct bounds", func() { + sched, err := ParseCrontab("0 0 1 ~ *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + month := findSetBit(spec.Month) + Expect(month).To(BeNumerically(">=", 1)) + Expect(month).To(BeNumerically("<=", 12)) + }) + + It("resolves ~ in DOW field with correct bounds", func() { + sched, err := ParseCrontab("0 0 * * ~") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + dow := findSetBit(spec.Dow) + Expect(dow).To(BeNumerically(">=", 0)) + Expect(dow).To(BeNumerically("<=", 6)) + }) + + It("preserves TZ= prefix through resolution", func() { + sched, err := ParseCrontab("TZ=America/New_York 0~30 * * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + nyc, _ := time.LoadLocation("America/New_York") + Expect(spec.Location).To(Equal(nyc)) + }) + + It("preserves non-random fields", func() { + sched, err := ParseCrontab("0~30 10 * * *") + Expect(err).ToNot(HaveOccurred()) + spec := sched.(*cron.SpecSchedule) + Expect(spec.Hour & (1 << 10)).ToNot(BeZero()) + }) + + It("resolves to a stable value across repeated Next calls", func() { + sched, err := ParseCrontab("0~30 * * * *") + Expect(err).ToNot(HaveOccurred()) + + ref := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) + first := sched.Next(ref) + for range 50 { + Expect(sched.Next(ref)).To(Equal(first)) + } + }) + }) + + Describe("error cases", func() { + It("rejects min > max", func() { + _, err := ParseCrontab("30~0 * * * *") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("beyond end")) + }) + + It("rejects value above field maximum", func() { + _, err := ParseCrontab("0~60 * * * *") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("above maximum")) + }) + + It("rejects value below field minimum", func() { + _, err := ParseCrontab("0 0 0~15 * *") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("below minimum")) + }) + + It("rejects ~ mixed with comma (list)", func() { + _, err := ParseCrontab("0~30,45 * * * *") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot be combined")) + }) + + It("rejects ~ mixed with slash (step)", func() { + _, err := ParseCrontab("0~30/5 * * * *") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot be combined")) + }) + + It("rejects @ descriptor with ~", func() { + _, err := ParseCrontab("@every 0~30m") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("descriptor")) + }) + + It("rejects wrong number of fields", func() { + _, err := ParseCrontab("0~30 * *") + Expect(err).To(HaveOccurred()) + }) + + It("rejects non-numeric range values", func() { + _, err := ParseCrontab("a~b * * * *") + Expect(err).To(HaveOccurred()) + }) + }) +}) + +// findSetBit returns the lowest bit position set in v, ignoring the starBit (bit 63). +func findSetBit(v uint64) int { + v &^= 1 << 63 // clear starBit + for i := 0; i < 63; i++ { + if v&(1<