Repository: go-co-op/gocron
Pull request #435 (2023-03-15 04:03:39)

Fix a bug with scheduling next runs on daylight savings change days
### What does this do?
This fixes a bug in how gocron schedules scheduled next day, week/weekday, month tasks. Previously, the next run was calculated by rounding the current date to midnight, adding the scheduled time ("at time" represented as a duration), and then adding the necessary day, week, or month interval. This methodology fails on daylight savings transition days. 

For example, suppose gocron was run on March 12, 2023 (first day of daylight savings time) and needs to set up a task that runs at 8pm every night. The initial calculation of the next run date time will calculate the next run by rounding the current time to midnight (March 12, 2023 at 00:00) and adding the scheduled time ("at time" of 20:00 represented as 20 hours in duration). Normally, this yields 8pm but since daylight savings time started earlier at 3am, the actual result is 9pm. Thus, the scheduled tasks will run 1 hour late until either a power cycle of gocron at 12:00am or after the next run on March 13, 2023.

This PR fixes the issue by removing the addition operation and instead, populate a new `time.Time` object with the last run's date and the deconstructed hours, minutes, seconds from the "at time" duration.

### Which issue(s) does this PR fix/relate to?
N/A

### List any changes that modify/break current functionality
I don't think this breaks current functionality.

### Have you included tests for your changes?
Yes

### Did you document any new/modified functionality?
No

- [ ] Updated `example_test.go`
- [ ] Updated `README.md`

### Notes
It's possible I missed a few locations but I think I covered most of it. Please let me know if I missed any scenarios.


Patch:
diff --git a/scheduler.go b/scheduler.go
index ea1b326..ffc588f 100644
--- a/scheduler.go
+++ b/scheduler.go
@@ -239,8 +239,6 @@ func (s *Scheduler) durationToNextRun(lastRun time.Time, job *Job) nextRun {
 }
 
 func (s *Scheduler) calculateMonths(job *Job, lastRun time.Time) nextRun {
-	lastRunRoundedMidnight := s.roundToMidnight(lastRun)
-
 	// Special case: the last day of the month
 	if len(job.daysOfTheMonth) == 1 && job.daysOfTheMonth[0] == -1 {
 		return calculateNextRunForLastDayOfMonth(s, job, lastRun)
@@ -264,7 +262,7 @@ func (s *Scheduler) calculateMonths(job *Job, lastRun time.Time) nextRun {
 
 		return nextRunResult
 	}
-	next := lastRunRoundedMidnight.Add(job.getFirstAtTime()).AddDate(0, job.getInterval(), 0)
+	next := s.roundToMidnightAndAddDSTAware(lastRun, job.getFirstAtTime()).AddDate(0, job.getInterval(), 0)
 	return nextRun{duration: until(lastRun, next), dateTime: next}
 }
 
@@ -275,7 +273,7 @@ func calculateNextRunForLastDayOfMonth(s *Scheduler, job *Job, lastRun time.Time
 	addMonth := job.getInterval()
 	atTime := job.getAtTime(lastRun)
 	if testDate := lastRun.AddDate(0, 0, 1); testDate.Month() != lastRun.Month() &&
-		!s.roundToMidnight(lastRun).Add(atTime).After(lastRun) {
+		!s.roundToMidnightAndAddDSTAware(lastRun, atTime).After(lastRun) {
 		// Our last run was on the last day of this month.
 		addMonth++
 		atTime = job.getFirstAtTime()
@@ -291,7 +289,10 @@ func calculateNextRunForLastDayOfMonth(s *Scheduler, job *Job, lastRun time.Time
 func calculateNextRunForMonth(s *Scheduler, job *Job, lastRun time.Time, dayOfMonth int) nextRun {
 	atTime := job.getAtTime(lastRun)
 	natTime := atTime
-	jobDay := time.Date(lastRun.Year(), lastRun.Month(), dayOfMonth, 0, 0, 0, 0, s.Location()).Add(atTime)
+
+	hours, minutes, seconds := s.deconstructDuration(atTime)
+	jobDay := time.Date(lastRun.Year(), lastRun.Month(), dayOfMonth, hours, minutes, seconds, 0, s.Location())
+
 	difference := absDuration(lastRun.Sub(jobDay))
 	next := lastRun
 	if jobDay.Before(lastRun) { // shouldn't run this month; schedule for next interval minus day difference
@@ -320,13 +321,13 @@ func (s *Scheduler) calculateWeekday(job *Job, lastRun time.Time) nextRun {
 	if totalDaysDifference > 0 {
 		acTime = job.getFirstAtTime()
 	}
-	next := s.roundToMidnight(lastRun).Add(acTime).AddDate(0, 0, totalDaysDifference)
+	next := s.roundToMidnightAndAddDSTAware(lastRun, acTime).AddDate(0, 0, totalDaysDifference)
 	return nextRun{duration: until(lastRun, next), dateTime: next}
 }
 
 func (s *Scheduler) calculateWeeks(job *Job, lastRun time.Time) nextRun {
 	totalDaysDifference := int(job.getInterval()) * 7
-	next := s.roundToMidnight(lastRun).Add(job.getFirstAtTime()).AddDate(0, 0, totalDaysDifference)
+	next := s.roundToMidnightAndAddDSTAware(lastRun, job.getFirstAtTime()).AddDate(0, 0, totalDaysDifference)
 	return nextRun{duration: until(lastRun, next), dateTime: next}
 }
 
@@ -354,14 +355,14 @@ func (s *Scheduler) calculateTotalDaysDifference(lastRun time.Time, daysToWeekda
 
 func (s *Scheduler) calculateDays(job *Job, lastRun time.Time) nextRun {
 	if job.getInterval() == 1 {
-		lastRunDayPlusJobAtTime := s.roundToMidnight(lastRun).Add(job.getAtTime(lastRun))
+		lastRunDayPlusJobAtTime := s.roundToMidnightAndAddDSTAware(lastRun, job.getAtTime(lastRun))
 
 		if shouldRunToday(lastRun, lastRunDayPlusJobAtTime) {
 			return nextRun{duration: until(lastRun, lastRunDayPlusJobAtTime), dateTime: lastRunDayPlusJobAtTime}
 		}
 	}
 
-	nextRunAtTime := s.roundToMidnight(lastRun).Add(job.getFirstAtTime()).AddDate(0, 0, job.getInterval()).In(s.Location())
+	nextRunAtTime := s.roundToMidnightAndAddDSTAware(lastRun, job.getFirstAtTime()).AddDate(0, 0, job.getInterval()).In(s.Location())
 	return nextRun{duration: until(lastRun, nextRunAtTime), dateTime: nextRunAtTime}
 }
 
@@ -386,14 +387,6 @@ func in(scheduleWeekdays []time.Weekday, weekday time.Weekday) bool {
 }
 
 func (s *Scheduler) calculateDuration(job *Job) time.Duration {
-	if job.neverRan() && shouldRunAtSpecificTime(job) { // ugly. in order to avoid this we could prohibit setting .At() and allowing only .StartAt() when dealing with Duration types
-		now := s.time.Now(s.location)
-		next := time.Date(now.Year(), now.Month(), now.Day(), 0, 0, 0, 0, s.Location()).Add(job.getFirstAtTime())
-		if now.Before(next) || now.Equal(next) {
-			return next.Sub(now)
-		}
-	}
-
 	interval := job.getInterval()
 	switch job.getUnit() {
 	case milliseconds:
@@ -407,11 +400,6 @@ func (s *Scheduler) calculateDuration(job *Job) time.Duration {
 	}
 }
 
-func shouldRunAtSpecificTime(job *Job) bool {
-	jobLastRun := job.LastRun()
-	return job.getAtTime(jobLastRun) != 0
-}
-
 func (s *Scheduler) remainingDaysToWeekday(lastRun time.Time, job *Job) int {
 	weekDays := job.Weekdays()
 	sort.Slice(weekDays, func(i, j int) bool {
@@ -429,7 +417,7 @@ func (s *Scheduler) remainingDaysToWeekday(lastRun time.Time, job *Job) int {
 	})
 	// check atTime
 	if equals {
-		if s.roundToMidnight(lastRun).Add(job.getAtTime(lastRun)).After(lastRun) {
+		if s.roundToMidnightAndAddDSTAware(lastRun, job.getAtTime(lastRun)).After(lastRun) {
 			return 0
 		}
 		index++
@@ -450,9 +438,17 @@ func absDuration(a time.Duration) time.Duration {
 	return -a
 }
 
-// roundToMidnight truncates time to midnight
-func (s *Scheduler) roundToMidnight(t time.Time) time.Time {
-	return time.Date(t.Year(), t.Month(), t.Day(), 0, 0, 0, 0, s.Location())
+func (s *Scheduler) deconstructDuration(d time.Duration) (hours int, minutes int, seconds int) {
+	hours = int(d.Seconds()) / int(time.Hour/time.Second)
+	minutes = (int(d.Seconds()) % int(time.Hour/time.Second)) / int(time.Minute/time.Second)
+	seconds = int(d.Seconds()) % int(time.Minute/time.Second)
+	return
+}
+
+// roundToMidnightAndAddDSTAware truncates time to midnight and "adds" duration in a DST aware manner
+func (s *Scheduler) roundToMidnightAndAddDSTAware(t time.Time, d time.Duration) time.Time {
+	hours, minutes, seconds := s.deconstructDuration(d)
+	return time.Date(t.Year(), t.Month(), t.Day(), hours, minutes, seconds, 0, s.Location())
 }
 
 // NextRun datetime when the next Job should run.


Test patch:
diff --git a/scheduler_test.go b/scheduler_test.go
index 3f6f36b..d69cf1d 100644
--- a/scheduler_test.go
+++ b/scheduler_test.go
@@ -395,6 +395,71 @@ func TestWeekdayAt(t *testing.T) {
 	})
 }
 
+func TestDaylightSavingsScheduled(t *testing.T) {
+	loc, err := time.LoadLocation("US/Pacific")
+	assert.NoError(t, err)
+
+	fromRFC3339 := func(ts string) time.Time {
+		dt, err := time.Parse(time.RFC3339, ts)
+		assert.NoError(t, err)
+		return dt.In(loc)
+	}
+
+	beforeToEDT := fakeTime{onNow: func(l *time.Location) time.Time {
+		return time.Date(2023, 3, 12, 1, 50, 50, 0, loc)
+	}}
+	toEDT := fakeTime{onNow: func(l *time.Location) time.Time {
+		return time.Date(2023, 3, 12, 5, 0, 0, 0, loc)
+	}}
+	beforeToEST := fakeTime{onNow: func(l *time.Location) time.Time {
+		return time.Date(2022, 11, 6, 1, 50, 50, 0, loc)
+	}}
+	afterToEST := fakeTime{onNow: func(l *time.Location) time.Time {
+		return fromRFC3339("2022-11-06T09:00:01.000Z")
+	}}
+	toEST := fakeTime{onNow: func(l *time.Location) time.Time {
+		return time.Date(2022, 11, 6, 4, 0, 0, 0, loc)
+	}}
+
+	testCases := []struct {
+		description     string
+		ft              fakeTime
+		jobCreateFn     func(s *Scheduler) *Scheduler
+		expectedNextRun time.Time
+	}{
+		{"EST no change", beforeToEDT, func(s *Scheduler) *Scheduler { return s.Every(1).Day().At("01:59") }, time.Date(2023, 3, 12, 1, 59, 0, 0, loc)},
+		{"EST->EDT every day", toEDT, func(s *Scheduler) *Scheduler { return s.Every(1).Day().At("20:00") }, time.Date(2023, 3, 12, 20, 0, 0, 0, loc)},
+		{"EST->EDT every 2 week", toEDT, func(s *Scheduler) *Scheduler { return s.Every(2).Week().At("17:00") }, time.Date(2023, 3, 26, 17, 0, 0, 0, loc)},
+		{"EST->EDT every 2 Tuesday", toEDT, func(s *Scheduler) *Scheduler { return s.Every(2).Tuesday().At("16:00") }, time.Date(2023, 3, 14, 16, 0, 0, 0, loc)},
+		{"EST->EDT every Sunday", toEDT, func(s *Scheduler) *Scheduler { return s.Every(1).Sunday().At("04:30") }, time.Date(2023, 3, 19, 4, 30, 0, 0, loc)},
+		{"EST->EDT every month", toEDT, func(s *Scheduler) *Scheduler { return s.Every(3).Month(12).At("14:00") }, time.Date(2023, 6, 12, 14, 0, 0, 0, loc)},
+		{"EST->EDT every last day of month", toEDT, func(s *Scheduler) *Scheduler { return s.Every(1).MonthLastDay().At("13:00") }, time.Date(2023, 3, 31, 13, 0, 0, 0, loc)},
+
+		{"EDT no change", beforeToEST, func(s *Scheduler) *Scheduler { return s.Every(1).Day().At("01:59") }, time.Date(2022, 11, 6, 1, 59, 0, 0, loc)},
+		{"EDT->EST every day", toEST, func(s *Scheduler) *Scheduler { return s.Every(1).Day().At("20:00") }, time.Date(2022, 11, 6, 20, 0, 0, 0, loc)},
+		{"EDT->EST every 2 week", toEST, func(s *Scheduler) *Scheduler { return s.Every(2).Week().At("18:00") }, time.Date(2022, 11, 20, 18, 0, 0, 0, loc)},
+		{"EDT->EST every 2 Tuesday", toEST, func(s *Scheduler) *Scheduler { return s.Every(2).Tuesday().At("15:00") }, time.Date(2022, 11, 8, 15, 0, 0, 0, loc)},
+		{"EDT->EST every Sunday", afterToEST, func(s *Scheduler) *Scheduler { return s.Every(1).Sunday().At("01:30") }, time.Date(2022, 11, 13, 1, 30, 0, 0, loc)},
+		{"EDT->EST every month", toEST, func(s *Scheduler) *Scheduler { return s.Every(3).Month(6).At("14:30") }, time.Date(2023, 2, 6, 14, 30, 0, 0, loc)},
+		{"EDT->EST every last day of month", toEST, func(s *Scheduler) *Scheduler { return s.Every(1).MonthLastDay().At("13:15") }, time.Date(2022, 11, 30, 13, 15, 0, 0, loc)},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			s := NewScheduler(loc)
+			s.time = tc.ft
+
+			job, err := tc.jobCreateFn(s).Do(func() {})
+			assert.NoError(t, err)
+
+			s.setRunning(true)
+			s.scheduleNextRun(job)
+
+			assert.Equal(t, tc.expectedNextRun, job.NextRun())
+		})
+	}
+}
+
 func TestScheduler_Remove(t *testing.T) {
 	t.Run("remove from non-running", func(t *testing.T) {
 		s := NewScheduler(time.UTC)