Merge pull request cockroachdb#74893 from adityamaru/backport21.1-74880

release-21.1: release-21.2: backupccl,jobs: handle panics from invalid cron parsing

pkg/ccl/backupccl/create_scheduled_backup.go

@@ -154,8 +154,15 @@ func computeScheduleRecurrence(
 			`error parsing schedule expression: %q; it must be a valid cron expression`,
 			cron)
 	}
-	nextRun := expr.Next(now)
-	frequency := expr.Next(nextRun).Sub(nextRun)
+	nextRun, err := jobs.CronParseNext(expr, now, cron)
+	if err != nil {
+		return nil, err
+	}
+	nextToNextRun, err := jobs.CronParseNext(expr, nextRun, cron)
+	if err != nil {
+		return nil, err
+	}
+	frequency := nextToNextRun.Sub(nextRun)
 	return &scheduleRecurrence{cron, frequency}, nil
 }
 

pkg/ccl/backupccl/create_scheduled_backup_test.go

@@ -389,6 +389,12 @@ func TestSerializesScheduledBackupExecutionArgs(t *testing.T) {
 			query:  `CREATE SCHEDULE FOR BACKUP INTO 'foo' WITH encryption_passphrase=$1 RECURRING '@hourly'`,
 			errMsg: "failed to evaluate backup encryption_passphrase",
 		},
+		{
+			name:   "malformed-cron-expression",
+			query:  "CREATE SCHEDULE backup_schedule FOR BACKUP INTO 'userfile:///a' RECURRING '* 12-8 * * *';",
+			user:   freeUser,
+			errMsg: "pq: failed to parse cron expression: \"\\* 12-8 \\* \\* \\*\"",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -826,9 +832,10 @@ INSERT INTO t values (1), (10), (100);
 		// to the next scheduled recurrence.
 		for _, id := range []int64{fullID, incID} {
 			s := loadSchedule(t, id)
-			require.EqualValues(t,
-				cronexpr.MustParse(s.ScheduleExpr()).Next(th.env.Now()).Round(time.Microsecond),
-				s.NextRun())
+			expr := cronexpr.MustParse(s.ScheduleExpr())
+			nextRun, err := jobs.CronParseNext(expr, th.env.Now(), s.ScheduleExpr())
+			require.NoError(t, err)
+			require.EqualValues(t, nextRun.Round(time.Microsecond), s.NextRun())
 		}
 
 		// We expect that, eventually, both backups would succeed.

pkg/jobs/BUILD.bazel

@@ -17,6 +17,7 @@ go_library(
         "scheduled_job_executor.go",
         "testing_knobs.go",
         "update.go",
+        "utils.go",
         "validate.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/jobs",

pkg/jobs/executor_impl_test.go

@@ -34,8 +34,13 @@ func TestInlineExecutorFailedJobsHandling(t *testing.T) {
 		expectedNextRun time.Time
 	}{
 		{
-			onError:         jobspb.ScheduleDetails_RETRY_SCHED,
-			expectedNextRun: cronexpr.MustParse("@daily").Next(h.env.Now()).Round(time.Microsecond),
+			onError: jobspb.ScheduleDetails_RETRY_SCHED,
+			expectedNextRun: func() time.Time {
+				expr := cronexpr.MustParse("@daily")
+				nextRun, err := CronParseNext(expr, h.env.Now(), "@daily")
+				require.NoError(t, err)
+				return nextRun.Round(time.Microsecond)
+			}(),
 		},
 		{
 			onError:         jobspb.ScheduleDetails_RETRY_SOON,

pkg/jobs/job_scheduler_test.go

@@ -74,7 +74,7 @@ func TestJobSchedulerReschedulesRunning(t *testing.T) {
 				}))
 
 			// Verify the job has expected nextRun time.
-			expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now())
+			expectedRunTime := getNextRunTime(t, "@hourly", h.env.Now())
 			loaded := h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
 
@@ -92,7 +92,7 @@ func TestJobSchedulerReschedulesRunning(t *testing.T) {
 			if wait == jobspb.ScheduleDetails_WAIT {
 				expectedRunTime = h.env.Now().Add(recheckRunningAfter)
 			} else {
-				expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now())
+				expectedRunTime = getNextRunTime(t, "@hourly", h.env.Now())
 			}
 			loaded = h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
@@ -132,7 +132,7 @@ func TestJobSchedulerExecutesAfterTerminal(t *testing.T) {
 				}))
 
 			// Verify the job has expected nextRun time.
-			expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now())
+			expectedRunTime := getNextRunTime(t, "@hourly", h.env.Now())
 			loaded := h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
 
@@ -146,7 +146,7 @@ func TestJobSchedulerExecutesAfterTerminal(t *testing.T) {
 					return s.executeSchedules(ctx, allSchedules, txn)
 				}))
 
-			expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now())
+			expectedRunTime = getNextRunTime(t, "@hourly", h.env.Now())
 			loaded = h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
 		})
@@ -172,7 +172,7 @@ func TestJobSchedulerExecutesAndSchedulesNextRun(t *testing.T) {
 		}))
 
 	// Verify the job has expected nextRun time.
-	expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now())
+	expectedRunTime := getNextRunTime(t, "@hourly", h.env.Now())
 	loaded := h.loadSchedule(t, j.ScheduleID())
 	require.Equal(t, expectedRunTime, loaded.NextRun())
 
@@ -186,11 +186,19 @@ func TestJobSchedulerExecutesAndSchedulesNextRun(t *testing.T) {
 			return s.executeSchedules(ctx, allSchedules, txn)
 		}))
 
-	expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now())
+	expectedRunTime = getNextRunTime(t, "@hourly", h.env.Now())
 	loaded = h.loadSchedule(t, j.ScheduleID())
 	require.Equal(t, expectedRunTime, loaded.NextRun())
 }
 
+func getNextRunTime(t *testing.T, cron string, fromNow time.Time) time.Time {
+	t.Helper()
+	expr := cronexpr.MustParse(cron)
+	nextRun, err := CronParseNext(expr, fromNow, cron)
+	require.NoError(t, err)
+	return nextRun
+}
+
 func TestJobSchedulerDaemonInitialScanDelay(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -534,7 +542,11 @@ func TestJobSchedulerRetriesFailed(t *testing.T) {
 	}{
 		{jobspb.ScheduleDetails_PAUSE_SCHED, time.Time{}},
 		{jobspb.ScheduleDetails_RETRY_SOON, execTime.Add(retryFailedJobAfter).Round(time.Microsecond)},
-		{jobspb.ScheduleDetails_RETRY_SCHED, cron.Next(execTime).Round(time.Microsecond)},
+		{jobspb.ScheduleDetails_RETRY_SCHED, func() time.Time {
+			nextRun, err := CronParseNext(cron, execTime, "@hourly")
+			require.NoError(t, err)
+			return nextRun.Round(time.Microsecond)
+		}()},
 	} {
 		t.Run(tc.onError.String(), func(t *testing.T) {
 			h.env.SetTime(startTime)

pkg/jobs/scheduled_job.go

@@ -183,8 +183,14 @@ func (j *ScheduledJob) Frequency() (time.Duration, error) {
 			"parsing schedule expression: %q; it must be a valid cron expression",
 			j.rec.ScheduleExpr)
 	}
-	next := expr.Next(j.env.Now())
-	nextNext := expr.Next(next)
+	next, err := CronParseNext(expr, j.env.Now(), j.rec.ScheduleExpr)
+	if err != nil {
+		return 0, err
+	}
+	nextNext, err := CronParseNext(expr, next, j.rec.ScheduleExpr)
+	if err != nil {
+		return 0, err
+	}
 	return nextNext.Sub(next), nil
 }
 
@@ -198,7 +204,11 @@ func (j *ScheduledJob) ScheduleNextRun() error {
 	if err != nil {
 		return errors.Wrapf(err, "parsing schedule expression: %q", j.rec.ScheduleExpr)
 	}
-	j.SetNextRun(expr.Next(j.env.Now()))
+	nextRun, err := CronParseNext(expr, j.env.Now(), j.rec.ScheduleExpr)
+	if err != nil {
+		return err
+	}
+	j.SetNextRun(nextRun)
 	return nil
 }
 

pkg/jobs/utils.go

@@ -0,0 +1,33 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package jobs
+
+import (
+	"time"
+
+	"github.com/cockroachdb/errors"
+	"github.com/gorhill/cronexpr"
+)
+
+// CronParseNext is a helper that parses the next time for the given expression
+// but captures any panic that may occur in the underlying library.
+func CronParseNext(
+	e *cronexpr.Expression, fromTime time.Time, cron string,
+) (t time.Time, err error) {
+	defer func() {
+		if recover() != nil {
+			t = time.Time{}
+			err = errors.Newf("failed to parse cron expression: %q", cron)
+		}
+	}()
+
+	return e.Next(fromTime), nil
+}