From 9279b2ebd6e51fe1e129bf569e1cb7096b9b5302 Mon Sep 17 00:00:00 2001 From: Thomas Barth Date: Tue, 3 Nov 2026 23:12:00 +0100 Subject: [PATCH] fix(scheduler): report real cron runs when a time window is set With a cron schedule plus a window, NextRun/NextRuns snapped any tick outside the window to the window start. Cron never fires at that time, so `preview`, `doctor` and the daemon's startup log showed runs that would not happen (e.g. cron "0 * * * *" with window 22:30-06:00 showed 22:30 instead of 23:00; cron "0 2 * * *" with window 09:00-17:00 showed a daily 09:00 run although the job never runs). Now cron mode returns the first cron tick inside the window, and ErrNoRunInWindow when none exists. Interval mode keeps snapping, since its loop really does wake at the window start. Co-Authored-By: Claude Opus 5.5 --- cmd/nightshift/commands/daemon.go | 10 ++- cmd/nightshift/commands/doctor.go | 4 + internal/scheduler/scheduler.go | 50 ++++++++++-- internal/scheduler/scheduler_test.go | 112 +++++++++++++++++++++++++++ 4 files changed, 165 insertions(+), 11 deletions(-) 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) + } +}