Fix: recurring cron jobs becoming one-time tasks (#1043)

Fixed an issue where recurring cron jobs (every_seconds/cron_expr) would
become one-time tasks after execution. The root cause was in the
executeJobByID function where computeNextRun could return nil (due to
validation errors or other issues) and this nil value was always
assigned to job.State.NextRunAtMS, effectively stopping recurrence.

This fix implements:
1. Check if computed nextRun is nil before assigning it
2. Use fallback schedule logic for 'every' type jobs to preserve recurrence
3. Properly handle invalid cron expressions by disabling those jobs
4. Improved logging to diagnose scheduling issues

The fix prevents recurring jobs from accidentally becoming one-time tasks,
while still appropriately handling truly invalid scheduling configurations.

Added tests to verify recurring jobs preserve schedule after execution.

Signed-off-by: liugangjian <liugangjian@pku.edu.cn>

pkg/cron/service.go

@@ -235,7 +235,42 @@ func (cs *CronService) executeJobByID(jobID string) {
 		}
 	} else {
 		nextRun := cs.computeNextRun(&job.Schedule, time.Now().UnixMilli())
-		job.State.NextRunAtMS = nextRun
+		if nextRun != nil {
+			// Successfully calculated next run, update as usual
+			job.State.NextRunAtMS = nextRun
+		} else {
+			// Failed to compute next run - this indicates an issue with the schedule, prevent recurring->one-time conversion
+			log.Printf("[cron] Failed to compute next run for recurring job '%s' (%s), kind: %s", 
+				job.Name, job.ID, job.Schedule.Kind)
+			// Handle differently based on schedule type to preserve recurrence
+			switch job.Schedule.Kind {
+			case "every":
+				// For every seconds jobs, compute fallback to prevent breaking recurrence due to temporary calculation issues
+				currentTime := time.Now().UnixMilli()
+				if job.Schedule.EveryMS != nil && *job.Schedule.EveryMS > 0 {
+					fallbackNextRun := currentTime + *job.Schedule.EveryMS
+					job.State.NextRunAtMS = &fallbackNextRun
+				} else {
+					// If interval is invalid, cannot schedule properly
+					job.State.NextRunAtMS = nil
+					job.Enabled = false  // Also disable as invalid configuration
+				}
+			case "cron":
+				// For cron expression jobs, check if expression is invalid and disable appropriately
+				if job.Schedule.Expr == "" {
+					log.Printf("[cron] Empty cron expression for job '%s' (%s), disabling job", job.Name, job.ID)
+					job.Enabled = false
+				} else {
+					// Try to validate the expression further and potentially disable if repeatedly invalid
+					log.Printf("[cron] Invalid cron expression '%s' for job '%s' (%s), disabling job", 
+						job.Schedule.Expr, job.Name, job.ID)
+					job.Enabled = false  // Disable to prevent continuous errors from malformed cron expression
+				}
+			default:
+				// For other unrecognized schedule types, preserve as nil
+				job.State.NextRunAtMS = nil
+			}
+		}
 	}
 
 	if err := cs.saveStoreUnsafe(); err != nil {

pkg/cron/service_test.go

@@ -33,6 +33,88 @@ func TestSaveStore_FilePermissions(t *testing.T) {
 	}
 }
 
+// TestRecurringJobsPreserveSchedule verifies that recurring jobs maintain their schedule after execution.
+func TestRecurringJobsPreserveSchedule(t *testing.T) {
+	tmpDir := t.TempDir()
+	storePath := filepath.Join(tmpDir, "cron", "jobs.json")
+
+	handler := func(job *CronJob) (string, error) {
+		return "done", nil
+	}
+
+	cs := NewCronService(storePath, handler)
+
+	// Add an 'every' type recurring job
+	everyMS := int64(1000)  // 1 second intervals
+	job, err := cs.AddJob("recurring-test", CronSchedule{Kind: "every", EveryMS: &everyMS}, "hello recurring", false, "cli", "direct")
+	if err != nil {
+		t.Fatalf("AddJob failed: %v", err)
+	}
+
+	// Initially, the job should be enabled and have a next run time
+	if !job.Enabled {
+		t.Errorf("Job should be enabled initially");
+	}
+	if job.State.NextRunAtMS == nil {
+		t.Errorf("Job should have a next run time initially");
+	}
+
+	// Execute the job manually once
+	cs.executeJobByID(job.ID)
+
+	// Verify the job still exists, is enabled, and has next run time
+	foundJob := false
+	for _, j := range cs.ListJobs(true) { // include disabled jobs
+		if j.ID == job.ID {
+			foundJob = true
+			// Check that it's still enabled and has NextRunAtMS
+			if !j.Enabled {
+				t.Errorf("Recurring 'every' job was disabled after execution - this indicates a bug where recurring jobs become one-time.")
+			}
+			if j.State.NextRunAtMS == nil {
+				 t.Errorf("Recurring 'every' job lost its next run time after execution");
+			}
+			break
+		}
+	}
+
+	if !foundJob {
+		t.Errorf("Job not found after execution - it was incorrectly removed.")
+	}
+}
+
+// TestInvalidCronExpressionIsHandledGracefully checks that cron jobs with invalid expressions are handled appropriately
+func TestInvalidCronExpressionIsHandledGracefully(t *testing.T) {
+	tmpDir := t.TempDir()
+	storePath := filepath.Join(tmpDir, "cron", "jobs.json")
+
+	cs := NewCronService(storePath, func(job *CronJob) (string, error) {
+		return "ok", nil
+	})
+
+	// Add a cron job with invalid expression
+	job, err := cs.AddJob("invalid-cron-test", CronSchedule{Kind: "cron", Expr: "not-a-valid-cron-expr"}, "invalid cron", false, "cli", "direct")
+	if err != nil {
+		t.Fatalf("AddJob for invalid cron job failed: %v", err)
+	}
+
+	// Execute the job - this should not crash and should handle gracefully
+	cs.executeJobByID(job.ID)
+
+	// For invalid cron expressions, the job should possibly be disabled after execution based on our fix
+	var jobFound *CronJob
+	for _, j := range cs.ListJobs(true) {
+		if j.ID == job.ID {
+			jobFound = &j
+			break
+		}
+	}
+
+	if jobFound == nil {
+		t.Fatalf("Job not found after execution")
+	}
+}
+
 func int64Ptr(v int64) *int64 {
 	return &v
 }