fix(scheduler): prevent next_run_at=NULL permanently silencing schedules (#722)

platform/internal/handlers/org.go

@@ -643,9 +643,18 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, defa
 			log.Printf("Org import: schedule '%s' on %s has empty prompt (neither prompt nor prompt_file set) — skipping insert", sched.Name, ws.Name)
 			continue
 		}
-		nextRun, _ := scheduler.ComputeNextRun(sched.CronExpr, tz, time.Now())
+		// #722: capture ComputeNextRun error; pass *time.Time (nil=NULL) so
+		// the driver writes NULL instead of zero-time (0001-01-01). The startup
+		// repair in scheduler.Start() will patch any NULL rows on next boot.
+		nextRun, nextRunErr := scheduler.ComputeNextRun(sched.CronExpr, tz, time.Now())
+		var nextRunPtr *time.Time
+		if nextRunErr != nil {
+			log.Printf("Org import: ComputeNextRun failed for schedule '%s' (expr=%q tz=%q): %v — next_run_at will be NULL (repaired at scheduler startup)", sched.Name, sched.CronExpr, tz, nextRunErr)
+		} else {
+			nextRunPtr = &nextRun
+		}
 		if _, err := db.DB.ExecContext(context.Background(), orgImportScheduleSQL,
-			id, sched.Name, sched.CronExpr, tz, prompt, enabled, nextRun); err != nil {
+			id, sched.Name, sched.CronExpr, tz, prompt, enabled, nextRunPtr); err != nil {
 			log.Printf("Org import: failed to upsert schedule '%s' for %s: %v", sched.Name, ws.Name, err)
 		} else {
 			log.Printf("Org import: schedule '%s' (%s, %d chars) upserted for %s (source=template)", sched.Name, sched.CronExpr, len(prompt), ws.Name)

platform/internal/scheduler/scheduler.go

@@ -87,6 +87,59 @@ func (s *Scheduler) Healthy() bool {
 	return time.Since(t) < 2*pollInterval
 }
 
+// repairNullNextRunAt patches enabled schedules whose next_run_at is NULL.
+// This can happen when a previous ComputeNextRun call failed at fire-time or
+// import-time and the protective COALESCE wasn't in place (issue #722 Bug 3).
+// Called once at startup, before the first tick, so affected schedules are
+// never permanently silenced — the poll loop would skip them forever because
+// tick() filters WHERE next_run_at IS NOT NULL.
+func (s *Scheduler) repairNullNextRunAt(ctx context.Context) {
+	rows, err := db.DB.QueryContext(ctx, `
+		SELECT id, cron_expr, timezone
+		FROM workspace_schedules
+		WHERE enabled = true AND next_run_at IS NULL
+	`)
+	if err != nil {
+		log.Printf("Scheduler: repairNullNextRunAt: query error: %v", err)
+		return
+	}
+	defer rows.Close()
+
+	repaired := 0
+	for rows.Next() {
+		var id, cronExpr, tz string
+		if err := rows.Scan(&id, &cronExpr, &tz); err != nil {
+			log.Printf("Scheduler: repairNullNextRunAt: scan error: %v", err)
+			continue
+		}
+		nextRun, nextErr := ComputeNextRun(cronExpr, tz, time.Now())
+		if nextErr != nil {
+			log.Printf("Scheduler: repairNullNextRunAt: ComputeNextRun failed for %s (expr=%q tz=%q): %v — leaving NULL", short(id, 12), cronExpr, tz, nextErr)
+			continue
+		}
+		if _, err := db.DB.ExecContext(ctx,
+			`UPDATE workspace_schedules SET next_run_at = $2, updated_at = now() WHERE id = $1`,
+			id, nextRun,
+		); err != nil {
+			log.Printf("Scheduler: repairNullNextRunAt: update error for %s: %v", short(id, 12), err)
+			continue
+		}
+		repaired++
+		log.Printf("Scheduler: repairNullNextRunAt: repaired %s → next_run_at=%s", short(id, 12), nextRun.Format(time.RFC3339))
+	}
+	// rows.Err() surfaces any error that cut the iteration short (network blip,
+	// context cancel, etc.). Without this check a partial repair would be
+	// silently treated as complete — some NULL-next_run_at schedules would stay
+	// silenced until the next startup.
+	if err := rows.Err(); err != nil {
+		log.Printf("Scheduler: repairNullNextRunAt: row iteration error: %v", err)
+		return
+	}
+	if repaired > 0 {
+		log.Printf("Scheduler: repairNullNextRunAt: repaired %d schedule(s)", repaired)
+	}
+}
+
 // Start runs the scheduler poll loop. Blocks until ctx is cancelled.
 //
 // Defends against panics inside tick() so a single bad row / bad cron
@@ -98,6 +151,9 @@ func (s *Scheduler) Start(ctx context.Context) {
 	ticker := time.NewTicker(s.tickInterval)
 	defer ticker.Stop()
 
+	// Repair any schedules silenced by a prior ComputeNextRun failure (#722).
+	s.repairNullNextRunAt(ctx)
+
 	log.Printf("Scheduler: started (poll interval=%s)", s.tickInterval)
 
 	tickWithRecover := func() {
@@ -279,12 +335,17 @@ func (s *Scheduler) fireSchedule(ctx context.Context, sched scheduleRow) {
 	var nextRunPtr *time.Time
 	if nextErr == nil {
 		nextRunPtr = &nextRun
+	} else {
+		// #722 Bug 1: log the failure so it's not silent; COALESCE below
+		// preserves the existing next_run_at rather than writing NULL.
+		log.Printf("Scheduler: ComputeNextRun failed for '%s' (expr=%q tz=%q): %v — preserving existing next_run_at",
+			sched.Name, sched.CronExpr, sched.Timezone, nextErr)
 	}
 
 	_, err := db.DB.ExecContext(ctx, `
 		UPDATE workspace_schedules
 		SET last_run_at = now(),
-		    next_run_at = $2,
+		    next_run_at = COALESCE($2, next_run_at),
 		    run_count = run_count + 1,
 		    last_status = $3,
 		    last_error = $4,
@@ -334,15 +395,22 @@ func (s *Scheduler) recordSkipped(ctx context.Context, sched scheduleRow, active
 	var nextRunPtr *time.Time
 	if nextErr == nil {
 		nextRunPtr = &nextRun
+	} else {
+		// #722 Bug 2: same guard as fireSchedule — log and preserve existing
+		// next_run_at via COALESCE rather than silencing the schedule with NULL.
+		log.Printf("Scheduler: ComputeNextRun failed in recordSkipped for '%s' (expr=%q tz=%q): %v — preserving existing next_run_at",
+			sched.Name, sched.CronExpr, sched.Timezone, nextErr)
 	}
 
 	// Advance next_run_at + bump run_count so the liveness view reflects
 	// that we're still ticking. last_status='skipped', last_error carries
 	// the reason for operators debugging via the schedule history API.
+	// COALESCE($2, next_run_at): if ComputeNextRun failed, preserve the
+	// existing next_run_at rather than writing NULL (#722).
 	_, _ = db.DB.ExecContext(ctx, `
 		UPDATE workspace_schedules
 		SET last_run_at = now(),
-		    next_run_at = $2,
+		    next_run_at = COALESCE($2, next_run_at),
 		    run_count = run_count + 1,
 		    last_status = 'skipped',
 		    last_error = $3,

platform/internal/scheduler/scheduler_test.go

@@ -2,6 +2,7 @@ package scheduler
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -237,6 +238,97 @@ func TestRecordSkipped_writesSkippedStatus(t *testing.T) {
 	}
 }
 
+// ── TestRepairNullNextRunAt_repairsRows (#722 Bug 3) ─────────────────────────
+// repairNullNextRunAt must query enabled schedules with next_run_at IS NULL and
+// UPDATE them with a computed next_run_at. Without this, bad cron rows are
+// permanently silenced because tick() filters WHERE next_run_at IS NOT NULL.
+
+func TestRepairNullNextRunAt_repairsRows(t *testing.T) {
+	mock := setupTestDB(t)
+	s := New(nil, nil)
+
+	// One silenced schedule with a valid cron expression — repair should fire.
+	repairRows := sqlmock.NewRows([]string{"id", "cron_expr", "timezone"}).
+		AddRow("aaaaaaaa-0000-0000-0000-000000000001", "0 * * * *", "UTC")
+	mock.ExpectQuery(`SELECT id, cron_expr, timezone\s+FROM workspace_schedules\s+WHERE enabled = true AND next_run_at IS NULL`).
+		WillReturnRows(repairRows)
+	mock.ExpectExec(`UPDATE workspace_schedules SET next_run_at = \$2, updated_at = now\(\) WHERE id = \$1`).
+		WithArgs("aaaaaaaa-0000-0000-0000-000000000001", sqlmock.AnyArg()).
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	s.repairNullNextRunAt(context.Background())
+
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestRepairNullNextRunAt_noRows — empty result set must not cause a panic or
+// unexpected DB call.
+
+func TestRepairNullNextRunAt_noRows(t *testing.T) {
+	mock := setupTestDB(t)
+	s := New(nil, nil)
+
+	mock.ExpectQuery(`SELECT id, cron_expr, timezone\s+FROM workspace_schedules\s+WHERE enabled = true AND next_run_at IS NULL`).
+		WillReturnRows(sqlmock.NewRows([]string{"id", "cron_expr", "timezone"}))
+
+	s.repairNullNextRunAt(context.Background())
+
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestRepairNullNextRunAt_badCronSkipped — a schedule whose cron expression is
+// unparseable must be skipped (no UPDATE attempted) and must not panic.
+
+func TestRepairNullNextRunAt_badCronSkipped(t *testing.T) {
+	mock := setupTestDB(t)
+	s := New(nil, nil)
+
+	repairRows := sqlmock.NewRows([]string{"id", "cron_expr", "timezone"}).
+		AddRow("bbbbbbbb-0000-0000-0000-000000000002", "not-a-cron", "UTC")
+	mock.ExpectQuery(`SELECT id, cron_expr, timezone\s+FROM workspace_schedules\s+WHERE enabled = true AND next_run_at IS NULL`).
+		WillReturnRows(repairRows)
+	// No ExpectExec — bad cron must not trigger an UPDATE.
+
+	s.repairNullNextRunAt(context.Background())
+
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestRepairNullNextRunAt_rowsErrAborts — when rows.Err() returns a non-nil
+// error (e.g. the connection drops mid-iteration), repairNullNextRunAt must
+// stop and return without logging "repaired N schedule(s)".  The codebase
+// convention (CLAUDE.md) requires rows.Err() to be checked after every loop.
+func TestRepairNullNextRunAt_rowsErrAborts(t *testing.T) {
+	mock := setupTestDB(t)
+	s := New(nil, nil)
+
+	// Return one valid row followed by a forced rows.Err() from sqlmock.
+	repairRows := sqlmock.NewRows([]string{"id", "cron_expr", "timezone"}).
+		AddRow("cccccccc-0000-0000-0000-000000000003", "0 * * * *", "UTC").
+		RowError(0, fmt.Errorf("simulated row iteration error"))
+
+	mock.ExpectQuery(`SELECT id, cron_expr, timezone\s+FROM workspace_schedules\s+WHERE enabled = true AND next_run_at IS NULL`).
+		WillReturnRows(repairRows)
+	// The UPDATE for the first row may fire before rows.Err() is surfaced;
+	// allow it but do not require it — the important assertion is that
+	// ExpectationsWereMet does not see unexpected calls and the function
+	// returns without panicking.
+	mock.ExpectExec(`UPDATE workspace_schedules SET next_run_at`).
+		WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg()).
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	s.repairNullNextRunAt(context.Background())
+
+	// The test passes as long as no panic occurs and no unexpected DB calls
+	// were made. Expectations already consumed above are fine.
+}
+
 // ── TestRecordSkipped_shortWorkspaceIDNoPanic ─────────────────────────────────
 // Guards against the short() regression: recordSkipped must not panic if
 // WorkspaceID is unexpectedly shorter than the 12-char prefix used in logs.