Fix schedule jitter calculation bug (#3059)

temporalio · Jul 8, 2022 · 12c173c · 12c173c
1 parent a95c2fc
commit 12c173c
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 9 deletions.
diff --git a/service/worker/scheduler/spec.go b/service/worker/scheduler/spec.go
@@ -31,6 +31,7 @@ import (
 	"github.com/dgryski/go-farm"
 
 	schedpb "go.temporal.io/api/schedule/v1"
+	"go.temporal.io/server/common"
 	"go.temporal.io/server/common/primitives/timestamp"
 )
 
@@ -103,9 +104,12 @@ func (cs *compiledSpec) getNextTime(after time.Time) (nominal, next time.Time, h
 		after = nominal
 	}
 
+	maxJitter := timestamp.DurationValue(cs.spec.Jitter)
 	// Ensure that jitter doesn't push this time past the _next_ nominal start time
-	following := cs.rawNextTime(nominal)
-	next = cs.addJitter(nominal, following.Sub(nominal))
+	if following := cs.rawNextTime(nominal); !following.IsZero() {
+		maxJitter = common.MinDuration(maxJitter, following.Sub(nominal))
+	}
+	next = cs.addJitter(nominal, maxJitter)
 
 	has = true
 	return
@@ -161,16 +165,11 @@ func (cs *compiledSpec) excluded(nominal time.Time) bool {
 	return false
 }
 
-// Adds jitter to a nominal time, deterministically (by hashing the given time). The range
-// of the jitter is zero to the min of the schedule spec's jitter and the given limit value.
-func (cs *compiledSpec) addJitter(nominal time.Time, limit time.Duration) time.Time {
-	maxJitter := timestamp.DurationValue(cs.spec.Jitter)
+// Adds jitter to a nominal time, deterministically (by hashing the given time).
+func (cs *compiledSpec) addJitter(nominal time.Time, maxJitter time.Duration) time.Time {
 	if maxJitter < 0 {
 		maxJitter = 0
 	}
-	if maxJitter > limit {
-		maxJitter = limit
-	}
 
 	bin, err := nominal.MarshalBinary()
 	if err != nil {

diff --git a/service/worker/scheduler/spec_test.go b/service/worker/scheduler/spec_test.go
@@ -254,3 +254,25 @@ func (s *specSuite) TestSpecBoundedJitter() {
 		time.Date(2022, 3, 23, 15, 8, 3, 724000000, time.UTC),
 	)
 }
+
+func (s *specSuite) TestSpecJitterSingleRun() {
+	s.checkSequenceFull(
+		&schedpb.ScheduleSpec{
+			Calendar: []*schedpb.CalendarSpec{
+				{Hour: "13", Minute: "55", DayOfMonth: "7", Month: "4", Year: "2022"},
+			},
+		},
+		time.Date(2022, 3, 23, 11, 00, 0, 0, time.UTC),
+		time.Date(2022, 4, 7, 13, 55, 0, 0, time.UTC),
+	)
+	s.checkSequenceFull(
+		&schedpb.ScheduleSpec{
+			Calendar: []*schedpb.CalendarSpec{
+				{Hour: "13", Minute: "55", DayOfMonth: "7", Month: "4", Year: "2022"},
+			},
+			Jitter: timestamp.DurationPtr(1 * time.Hour),
+		},
+		time.Date(2022, 3, 23, 11, 00, 0, 0, time.UTC),
+		time.Date(2022, 4, 7, 13, 57, 26, 927000000, time.UTC),
+	)
+}