diff --git a/cmd/nightshift/commands/daemon.go b/cmd/nightshift/commands/daemon.go index d082b0e..6e062e3 100644 --- a/cmd/nightshift/commands/daemon.go +++ b/cmd/nightshift/commands/daemon.go @@ -230,9 +230,13 @@ func runDaemonLoop(cfg *config.Config) error { return fmt.Errorf("start scheduler: %w", err) } - log.InfoCtx("daemon running", map[string]any{ - "next_run": sched.NextRun().Format(time.RFC3339), - }) + if next := sched.NextRun(); next.IsZero() { + log.Warnf("daemon running, no upcoming run: %v", scheduler.ErrNoRunInWindow) + } else { + log.InfoCtx("daemon running", map[string]any{ + "next_run": next.Format(time.RFC3339), + }) + } // Wait for context cancellation <-ctx.Done() diff --git a/cmd/nightshift/commands/doctor.go b/cmd/nightshift/commands/doctor.go index 1a0f0ee..2427fc8 100644 --- a/cmd/nightshift/commands/doctor.go +++ b/cmd/nightshift/commands/doctor.go @@ -116,6 +116,10 @@ func checkSchedule(cfg *config.Config, add func(string, checkStatus, string)) { return } nextRuns, err := sched.NextRuns(1) + if errors.Is(err, scheduler.ErrNoRunInWindow) { + add("schedule", statusWarn, err.Error()) + return + } if err != nil || len(nextRuns) == 0 { add("schedule", statusWarn, "unable to compute next run") return diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 278ca71..a4273cc 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -22,8 +22,13 @@ var ( ErrNoSchedule = errors.New("no schedule configured (need cron or interval)") ErrAlreadyRunning = errors.New("scheduler already running") ErrNotRunning = errors.New("scheduler not running") + ErrNoRunInWindow = errors.New("cron schedule never fires inside time window") ) +// cronSearchHorizon bounds the search for a cron tick inside the window. +// Matches robfig/cron, which gives up after five years. +const cronSearchHorizon = 5 * 365 * 24 * time.Hour + // Job is a function to execute on schedule. type Job func(ctx context.Context) error @@ -333,9 +338,9 @@ func (s *Scheduler) NextRuns(n int) ([]time.Time, error) { } current := now for i := 0; i < n; i++ { - next := schedule.Next(current) - if window != nil && !window.Contains(next) { - next = nextWindowStartForWindow(window, next) + next, ok := nextCronRunInWindow(schedule, window, current) + if !ok { + return nil, ErrNoRunInWindow } runs = append(runs, next) current = next @@ -369,16 +374,45 @@ func (s *Scheduler) updateNextRunLocked() { now := time.Now().In(s.location) if s.cronExpr != "" && s.cron != nil { + // Cron only fires on its own ticks; runs outside the window are + // skipped, so the next real run is the first tick inside it. entry := s.cron.Entry(s.entryID) - s.nextRun = entry.Next - } else if s.interval > 0 { + next, ok := nextCronRunInWindow(entry.Schedule, s.window, now) + if !ok { + next = time.Time{} + } + s.nextRun = next + return + } + + if s.interval > 0 { s.nextRun = now.Add(s.interval) + // The interval loop sleeps until nextRun, so snapping to the + // window start is accurate here. + if s.window != nil && !s.window.Contains(s.nextRun) { + s.nextRun = s.nextWindowStartLocked(s.nextRun) + } } +} - // Adjust for window if needed (use internal check, no lock) - if s.window != nil && !s.window.Contains(s.nextRun) { - s.nextRun = s.nextWindowStartLocked(s.nextRun) +// nextCronRunInWindow returns the first cron tick after t that falls inside +// window. Ticks outside the window are skipped by runJobs, so they are not +// real runs. Returns false if no such tick exists within cronSearchHorizon. +func nextCronRunInWindow(schedule cron.Schedule, window *Window, t time.Time) (time.Time, bool) { + if schedule == nil { + return time.Time{}, false + } + limit := t.Add(cronSearchHorizon) + next := schedule.Next(t) + for !next.IsZero() && !next.After(limit) { + if window == nil || window.Contains(next) { + return next, true + } + // Jump to just before the next window opening rather than walking + // every tick in between. + next = schedule.Next(nextWindowStartForWindow(window, next).Add(-time.Second)) } + return time.Time{}, false } // IsInWindow checks if the given time is within the allowed execution window. diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index e28a714..ae9f27c 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -2,12 +2,14 @@ package scheduler import ( "context" + "errors" "fmt" "sync/atomic" "testing" "time" "github.com/marcus/nightshift/internal/config" + "github.com/robfig/cron/v3" ) func TestParseTimeOfDay(t *testing.T) { @@ -579,3 +581,113 @@ func TestScheduleInterval_Legacy(t *testing.T) { t.Errorf("len(jobs) = %d, want 1", len(s.jobs)) } } + +func TestNextCronRunInWindow(t *testing.T) { + loc := time.UTC + parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) + at := func(day, hour, min int) time.Time { + return time.Date(2026, 1, day, hour, min, 0, 0, loc) + } + + tests := []struct { + name string + cron string + start string + end string + after time.Time + want time.Time + wantOK bool + }{ + { + name: "next cron tick already inside window", + cron: "0 * * * *", start: "22:00", end: "06:00", + after: at(5, 23, 10), want: at(6, 0, 0), wantOK: true, + }, + { + name: "skips ticks outside window to first tick inside", + cron: "0 * * * *", start: "22:30", end: "06:00", + after: at(5, 7, 0), want: at(5, 23, 0), wantOK: true, + }, + { + name: "does not snap to window start when cron never fires there", + cron: "15 */2 * * *", start: "09:00", end: "17:00", + after: at(5, 18, 0), want: at(6, 10, 15), wantOK: true, + }, + { + name: "weekday-only cron skips weekend", + cron: "0 23 * * 1-5", start: "22:00", end: "06:00", + after: at(9, 23, 30), want: at(12, 23, 0), wantOK: true, // Fri -> Mon + }, + { + name: "cron never fires inside window", + cron: "0 2 * * *", start: "09:00", end: "17:00", + after: at(5, 12, 0), wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sched, err := parser.Parse(tt.cron) + if err != nil { + t.Fatalf("parse cron: %v", err) + } + start, _ := ParseTimeOfDay(tt.start) + end, _ := ParseTimeOfDay(tt.end) + w := &Window{Start: start, End: end, Location: loc} + + got, ok := nextCronRunInWindow(sched, w, tt.after) + if ok != tt.wantOK { + t.Fatalf("ok = %v, want %v (got %v)", ok, tt.wantOK, got) + } + if ok && !got.Equal(tt.want) { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} + +func TestNextRuns_CronWithWindow(t *testing.T) { + cfg := &config.ScheduleConfig{ + Cron: "0 * * * *", + Window: &config.WindowConfig{Start: "09:30", End: "17:00", Timezone: "UTC"}, + } + s, err := NewFromConfig(cfg) + if err != nil { + t.Fatalf("NewFromConfig() error = %v", err) + } + + // 24 runs always span time outside the window, so any snapping to the + // window start (09:30) would show up here. + runs, err := s.NextRuns(24) + if err != nil { + t.Fatalf("NextRuns() error = %v", err) + } + if len(runs) != 24 { + t.Fatalf("NextRuns() returned %d runs, want 24", len(runs)) + } + for i, r := range runs { + if r.Minute() != 0 { + t.Errorf("run %d at %v is not a cron tick (minute != 0)", i, r) + } + if !s.IsInWindow(r) { + t.Errorf("run %d at %v is outside the window", i, r) + } + if i > 0 && !r.After(runs[i-1]) { + t.Errorf("run %d at %v not after previous run %v", i, r, runs[i-1]) + } + } +} + +func TestNextRuns_CronNeverInWindow(t *testing.T) { + cfg := &config.ScheduleConfig{ + Cron: "0 2 * * *", + Window: &config.WindowConfig{Start: "09:00", End: "17:00", Timezone: "UTC"}, + } + s, err := NewFromConfig(cfg) + if err != nil { + t.Fatalf("NewFromConfig() error = %v", err) + } + if _, err := s.NextRuns(1); !errors.Is(err, ErrNoRunInWindow) { + t.Errorf("NextRuns() error = %v, want ErrNoRunInWindow", err) + } +}