From e27f888876c2d2d67184ce27a585a64239ccfdb2 Mon Sep 17 00:00:00 2001 From: Henry Date: Sun, 13 Sep 2026 03:41:22 +0800 Subject: [PATCH] fix(office): correct cron DOM/DOW OR semantics, DST fire policy, and unsatisfiable expressions (#3514) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../internal/office/repository/sqlite/base.go | 2 +- .../repository/sqlite/base_migrations.go | 15 + ...grations_routine_timezone_postgres_test.go | 68 +++ .../base_migrations_routine_timezone_test.go | 206 +++++++++ .../office/repository/sqlite/routines.go | 2 +- .../internal/office/routines/handler.go | 7 +- .../handler_trigger_validation_test.go | 80 ++++ .../internal/office/routines/service.go | 39 +- .../service_cron_advance_failure_test.go | 169 +++++++ .../routines/trigger_validation_test.go | 81 ++++ apps/backend/internal/office/shared/cron.go | 428 +++++++++--------- .../office/shared/cron_edge_cases_test.go | 32 ++ .../internal/office/shared/cron_test.go | 408 +++++++++++++++-- docs/specs/office/requirements/scheduler.md | 4 + .../office/system-design/scheduler-01.md | 5 +- .../office/system-design/scheduler-02.md | 2 +- 16 files changed, 1290 insertions(+), 258 deletions(-) create mode 100644 apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_postgres_test.go create mode 100644 apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_test.go create mode 100644 apps/backend/internal/office/routines/handler_trigger_validation_test.go create mode 100644 apps/backend/internal/office/routines/service_cron_advance_failure_test.go create mode 100644 apps/backend/internal/office/routines/trigger_validation_test.go create mode 100644 apps/backend/internal/office/shared/cron_edge_cases_test.go diff --git a/apps/backend/internal/office/repository/sqlite/base.go b/apps/backend/internal/office/repository/sqlite/base.go index 3024306f73..a850fc9ded 100644 --- a/apps/backend/internal/office/repository/sqlite/base.go +++ b/apps/backend/internal/office/repository/sqlite/base.go @@ -467,7 +467,7 @@ func (r *Repository) createRoutineTables() error { routine_id TEXT NOT NULL, kind TEXT NOT NULL, cron_expression TEXT DEFAULT '', - timezone TEXT DEFAULT '', + timezone TEXT DEFAULT 'UTC', public_id TEXT DEFAULT '', signing_mode TEXT DEFAULT '', secret TEXT DEFAULT '', diff --git a/apps/backend/internal/office/repository/sqlite/base_migrations.go b/apps/backend/internal/office/repository/sqlite/base_migrations.go index fa8c5f2237..2e1638d730 100644 --- a/apps/backend/internal/office/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/office/repository/sqlite/base_migrations.go @@ -65,6 +65,21 @@ func (r *Repository) runMigrations() error { if err := r.migrate.Err(); err != nil { return err } + if err := r.backfillRoutineTriggerTimezones(); err != nil { + return err + } + return nil +} + +// backfillRoutineTriggerTimezones sets an explicit "UTC" on cron triggers +// created before the column default changed from empty to 'UTC', so the +// empty string no longer means anything at read time. +func (r *Repository) backfillRoutineTriggerTimezones() error { + if _, err := r.db.Exec( + `UPDATE office_routine_triggers SET timezone = 'UTC' WHERE kind = 'cron' AND timezone = ''`, + ); err != nil { + return fmt.Errorf("office_routine_triggers.timezone backfill: %w", err) + } return nil } diff --git a/apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_postgres_test.go b/apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_postgres_test.go new file mode 100644 index 0000000000..d8afefa867 --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_postgres_test.go @@ -0,0 +1,68 @@ +package sqlite_test + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/repository/sqlite" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/testutil" +) + +// TestPostgresBackfillRoutineTriggerTimezones_LegacyEmptyTimezoneBecomesUTC +// is the PostgreSQL replay twin of the SQLite backfill test. It skips unless +// KANDEV_TEST_POSTGRES_DSN is set. +func TestPostgresBackfillRoutineTriggerTimezones_LegacyEmptyTimezoneBecomesUTC(t *testing.T) { + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + ctx := context.Background() + + if _, err := taskrepo.NewWithDB(db, db, nil); err != nil { + t.Fatalf("init task repo: %v", err) + } + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("first init: %v", err) + } + + routine := &models.Routine{ + WorkspaceID: "ws-postgres", + Name: "Legacy Cron PostgreSQL", + TaskTemplate: "{}", + Status: "active", + ConcurrencyPolicy: "skip_if_active", + Variables: "{}", + } + if err := repo.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + trigger := &models.RoutineTrigger{ + RoutineID: routine.ID, + Kind: "cron", + CronExpression: "0 9 * * *", + Timezone: "UTC", + Enabled: true, + } + if err := repo.CreateRoutineTrigger(ctx, trigger); err != nil { + t.Fatalf("create trigger: %v", err) + } + if _, err := db.ExecContext(ctx, + `UPDATE office_routine_triggers SET timezone = '' WHERE id = $1`, trigger.ID, + ); err != nil { + t.Fatalf("simulate legacy empty timezone: %v", err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("replay migrations: %v", err) + } + + var gotTimezone string + if err := db.GetContext(ctx, &gotTimezone, + `SELECT timezone FROM office_routine_triggers WHERE id = $1`, trigger.ID, + ); err != nil { + t.Fatalf("read post-migration timezone: %v", err) + } + if gotTimezone != "UTC" { + t.Errorf("timezone after backfill = %q, want %q", gotTimezone, "UTC") + } +} diff --git a/apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_test.go b/apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_test.go new file mode 100644 index 0000000000..1377a19e2d --- /dev/null +++ b/apps/backend/internal/office/repository/sqlite/base_migrations_routine_timezone_test.go @@ -0,0 +1,206 @@ +package sqlite_test + +import ( + "context" + "testing" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + settingsstore "github.com/kandev/kandev/internal/agent/settings/store" + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/repository/sqlite" +) + +// TestBackfillRoutineTriggerTimezones_LegacyEmptyTimezoneBecomesUTC reproduces +// a pre-migration row (a cron trigger created before the column default +// changed from empty to 'UTC') and asserts a migration replay backfills it, +// mirroring the table-rebuild replay pattern used elsewhere in this package +// (e.g. TestInitSchema_ExecutionProfileRoutingColumnsReplay). +func TestBackfillRoutineTriggerTimezones_LegacyEmptyTimezoneBecomesUTC(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("settings store init: %v", err) + } + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("first init: %v", err) + } + + ctx := context.Background() + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Legacy Cron", + TaskTemplate: "{}", + Status: "active", + ConcurrencyPolicy: "skip_if_active", + Variables: "{}", + } + if err := repo.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + trigger := &models.RoutineTrigger{ + RoutineID: routine.ID, + Kind: "cron", + CronExpression: "0 9 * * *", + Timezone: "UTC", + Enabled: true, + } + if err := repo.CreateRoutineTrigger(ctx, trigger); err != nil { + t.Fatalf("create trigger: %v", err) + } + + // Simulate the legacy state the backfill exists to fix: a cron trigger + // persisted before the column default changed from '' to 'UTC'. + if _, err := db.Exec(`UPDATE office_routine_triggers SET timezone = '' WHERE id = ?`, trigger.ID); err != nil { + t.Fatalf("simulate legacy empty timezone: %v", err) + } + var preTimezone string + if err := db.Get(&preTimezone, `SELECT timezone FROM office_routine_triggers WHERE id = ?`, trigger.ID); err != nil { + t.Fatalf("read pre-migration timezone: %v", err) + } + if preTimezone != "" { + t.Fatalf("pre-migration timezone = %q, want empty string", preTimezone) + } + + // Replay migrations on the same DB handle, as a restart would. + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("replay migrations: %v", err) + } + + var gotTimezone string + if err := db.Get(&gotTimezone, `SELECT timezone FROM office_routine_triggers WHERE id = ?`, trigger.ID); err != nil { + t.Fatalf("read post-migration timezone: %v", err) + } + if gotTimezone != "UTC" { + t.Errorf("timezone after backfill = %q, want %q", gotTimezone, "UTC") + } +} + +func TestBackfillRoutineTriggerTimezones_ReportsUpdateFailure(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("settings store init: %v", err) + } + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("first init: %v", err) + } + + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Legacy Cron Failure", + TaskTemplate: "{}", + Status: "active", + ConcurrencyPolicy: "skip_if_active", + Variables: "{}", + } + if err := repo.CreateRoutine(context.Background(), routine); err != nil { + t.Fatalf("create routine: %v", err) + } + trigger := &models.RoutineTrigger{ + RoutineID: routine.ID, + Kind: "cron", + CronExpression: "0 9 * * *", + Timezone: "", + Enabled: true, + } + if err := repo.CreateRoutineTrigger(context.Background(), trigger); err != nil { + t.Fatalf("create trigger: %v", err) + } + if _, err := db.Exec(`UPDATE office_routine_triggers SET timezone = '' WHERE id = ?`, trigger.ID); err != nil { + t.Fatalf("simulate legacy empty timezone: %v", err) + } + if _, err := db.Exec(` + CREATE TRIGGER fail_routine_timezone_backfill + BEFORE UPDATE OF timezone ON office_routine_triggers + WHEN OLD.id = NEW.id AND OLD.timezone = '' AND NEW.timezone = 'UTC' + BEGIN + SELECT RAISE(ABORT, 'injected backfill failure'); + END; + `); err != nil { + t.Fatalf("create failure trigger: %v", err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err == nil { + t.Fatal("migration replay succeeded after timezone backfill failure") + } +} + +// TestBackfillRoutineTriggerTimezones_LeavesNonEmptyAndNonCronAlone verifies +// the backfill only touches cron triggers with an empty timezone, not a +// deliberately-set timezone or a non-cron (webhook/manual) trigger kind. +func TestBackfillRoutineTriggerTimezones_LeavesNonEmptyAndNonCronAlone(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + + if _, _, err := settingsstore.Provide(db, db, nil); err != nil { + t.Fatalf("settings store init: %v", err) + } + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("first init: %v", err) + } + + ctx := context.Background() + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Mixed Triggers", + TaskTemplate: "{}", + Status: "active", + ConcurrencyPolicy: "skip_if_active", + Variables: "{}", + } + if err := repo.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + + cronExplicit := &models.RoutineTrigger{RoutineID: routine.ID, Kind: "cron", CronExpression: "0 9 * * *", Timezone: "America/New_York", Enabled: true} + if err := repo.CreateRoutineTrigger(ctx, cronExplicit); err != nil { + t.Fatalf("create explicit-timezone trigger: %v", err) + } + webhook := &models.RoutineTrigger{RoutineID: routine.ID, Kind: "webhook", PublicID: "wh-1", SigningMode: "none", Enabled: true} + if err := repo.CreateRoutineTrigger(ctx, webhook); err != nil { + t.Fatalf("create webhook trigger: %v", err) + } + if _, err := db.Exec(`UPDATE office_routine_triggers SET timezone = '' WHERE id = ?`, webhook.ID); err != nil { + t.Fatalf("simulate empty timezone on webhook trigger: %v", err) + } + + if _, err := sqlite.NewWithDB(db, db, nil); err != nil { + t.Fatalf("replay migrations: %v", err) + } + + var cronTZ, webhookTZ string + if err := db.Get(&cronTZ, `SELECT timezone FROM office_routine_triggers WHERE id = ?`, cronExplicit.ID); err != nil { + t.Fatalf("read cron timezone: %v", err) + } + if cronTZ != "America/New_York" { + t.Errorf("explicit cron timezone = %q, want unchanged %q", cronTZ, "America/New_York") + } + if err := db.Get(&webhookTZ, `SELECT timezone FROM office_routine_triggers WHERE id = ?`, webhook.ID); err != nil { + t.Fatalf("read webhook timezone: %v", err) + } + if webhookTZ != "" { + t.Errorf("webhook timezone = %q, want unchanged empty string (backfill is cron-only)", webhookTZ) + } +} diff --git a/apps/backend/internal/office/repository/sqlite/routines.go b/apps/backend/internal/office/repository/sqlite/routines.go index db61f8d320..6cc98e9c83 100644 --- a/apps/backend/internal/office/repository/sqlite/routines.go +++ b/apps/backend/internal/office/repository/sqlite/routines.go @@ -31,7 +31,7 @@ func (r *Repository) CreateRoutineTrigger(ctx context.Context, t *models.Routine ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `), t.ID, t.RoutineID, t.Kind, t.CronExpression, t.Timezone, t.PublicID, t.SigningMode, t.Secret, t.NextRunAt, t.LastFiredAt, - t.Enabled, t.CreatedAt, t.UpdatedAt) + boolToInt(t.Enabled), t.CreatedAt, t.UpdatedAt) return err } diff --git a/apps/backend/internal/office/routines/handler.go b/apps/backend/internal/office/routines/handler.go index e6a591b8d7..6e700df539 100644 --- a/apps/backend/internal/office/routines/handler.go +++ b/apps/backend/internal/office/routines/handler.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -183,7 +184,11 @@ func (h *Handler) createTrigger(c *gin.Context) { Enabled: true, } if err := h.svc.CreateRoutineTrigger(c.Request.Context(), trigger); err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + status := http.StatusInternalServerError + if errors.Is(err, ErrInvalidTrigger) { + status = http.StatusBadRequest + } + c.JSON(status, gin.H{"error": err.Error()}) return } trigger.Secret = "" // redact before sending response diff --git a/apps/backend/internal/office/routines/handler_trigger_validation_test.go b/apps/backend/internal/office/routines/handler_trigger_validation_test.go new file mode 100644 index 0000000000..690959760a --- /dev/null +++ b/apps/backend/internal/office/routines/handler_trigger_validation_test.go @@ -0,0 +1,80 @@ +package routines_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/office/routines" +) + +// newTestRouter mounts routine routes over a fresh in-memory-SQLite-backed +// RoutineService. +func newTestRouter(t *testing.T) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + svc := newTestRoutineService(t) + router := gin.New() + routines.RegisterRoutes(router.Group("/api/v1"), routines.NewHandler(svc)) + return router +} + +func doRequest(t *testing.T, router *gin.Engine, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +func createTestRoutineViaHTTP(t *testing.T, router *gin.Engine) string { + t.Helper() + rec := doRequest(t, router, http.MethodPost, "/api/v1/workspaces/ws-1/routines", `{ + "name": "r1", + "task_template": "{}" + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("create routine status = %d, body = %s", rec.Code, rec.Body.String()) + } + var resp routines.RoutineResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode routine response: %v", err) + } + return resp.Routine.ID +} + +// TestCreateTrigger_InvalidCronExpression_Returns400 verifies a validation +// failure (unsatisfiable cron expression) is a client error, not a server +// error. +func TestCreateTrigger_InvalidCronExpression_Returns400(t *testing.T) { + router := newTestRouter(t) + routineID := createTestRoutineViaHTTP(t, router) + + rec := doRequest(t, router, http.MethodPost, "/api/v1/routines/"+routineID+"/triggers", `{ + "kind": "cron", + "cron_expression": "0 0 30 2 *" + }`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body = %s", rec.Code, http.StatusBadRequest, rec.Body.String()) + } +} + +// TestCreateTrigger_ValidCronExpression_Returns201 is the control case: a +// well-formed cron trigger still creates successfully. +func TestCreateTrigger_ValidCronExpression_Returns201(t *testing.T) { + router := newTestRouter(t) + routineID := createTestRoutineViaHTTP(t, router) + + rec := doRequest(t, router, http.MethodPost, "/api/v1/routines/"+routineID+"/triggers", `{ + "kind": "cron", + "cron_expression": "0 9 * * *" + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want %d; body = %s", rec.Code, http.StatusCreated, rec.Body.String()) + } +} diff --git a/apps/backend/internal/office/routines/service.go b/apps/backend/internal/office/routines/service.go index 3879a42d4b..5145268635 100644 --- a/apps/backend/internal/office/routines/service.go +++ b/apps/backend/internal/office/routines/service.go @@ -27,6 +27,10 @@ import ( // imports office/repository/sqlite directly (see routineWakeupAdapter). var ErrWakeupAlreadyRequested = errors.New("wakeup request already requested") +// ErrInvalidTrigger indicates a trigger create request that failed +// validation (a client error, not a server failure). +var ErrInvalidTrigger = errors.New("invalid routine trigger") + // Repository is the persistence interface required by RoutineService. type Repository interface { CreateRoutine(ctx context.Context, routine *Routine) error @@ -329,11 +333,20 @@ func (s *RoutineService) ListRoutinesFromConfig(ctx context.Context, workspaceID // -- Trigger management -- // CreateRoutineTrigger creates a trigger and computes next_run_at for cron. +// A cron trigger requires a satisfiable expression: an empty expression, or +// one that can never fire, is rejected here rather than becoming a silent +// no-op or a wrong daily fallback at tick time. func (s *RoutineService) CreateRoutineTrigger(ctx context.Context, t *RoutineTrigger) error { - if t.Kind == "cron" && t.CronExpression != "" { + if t.Timezone == "" { + t.Timezone = "UTC" + } + if t.Kind == "cron" { + if t.CronExpression == "" { + return fmt.Errorf("%w: cron trigger requires a cron_expression", ErrInvalidTrigger) + } next, err := shared.NextCronTime(t.CronExpression, t.Timezone, time.Now().UTC()) if err != nil { - return fmt.Errorf("invalid cron expression: %w", err) + return fmt.Errorf("%w: invalid cron expression: %v", ErrInvalidTrigger, err) } t.NextRunAt = &next } @@ -402,11 +415,25 @@ func (s *RoutineService) processCronTrigger(ctx context.Context, trigger *Routin // expressions (no fixed interval — we walk NextCronTime). runCount, advanceTo, err := computeRoutineMissed(trigger, routine, now) if err != nil { - s.logger.Warn("compute routine catch-up failed", + if errors.Is(err, shared.ErrUnsatisfiableCron) { + // The expression can never fire again. ClaimTrigger already + // cleared next_run_at; leave it cleared rather than re-arming, + // which would dispatch nothing but retry (and fail) forever. + s.logger.Error("cron expression unsatisfiable; trigger permanently disarmed", + zap.String("trigger_id", trigger.ID), zap.Error(err)) + return err + } + // Any other failure (e.g. the timezone database is temporarily + // unavailable) is presumed recoverable: re-arm to the original due + // time so the next tick retries instead of leaving the trigger + // disarmed forever once the underlying issue clears. + if rearmErr := s.repo.UpdateTriggerNextRun(ctx, trigger.ID, trigger.NextRunAt); rearmErr != nil { + s.logger.Warn("re-arm trigger after recoverable catch-up failure failed", + zap.String("trigger_id", trigger.ID), zap.Error(rearmErr)) + } + s.logger.Warn("compute routine catch-up failed; will retry next tick", zap.String("trigger_id", trigger.ID), zap.Error(err)) - // Fall through with sane defaults: fire once, advance one tick. - runCount = 1 - advanceTo = now + return err } if err := s.repo.UpdateTriggerNextRun(ctx, trigger.ID, &advanceTo); err != nil { s.logger.Warn("update trigger next_run_at failed", diff --git a/apps/backend/internal/office/routines/service_cron_advance_failure_test.go b/apps/backend/internal/office/routines/service_cron_advance_failure_test.go new file mode 100644 index 0000000000..b7e0a69439 --- /dev/null +++ b/apps/backend/internal/office/routines/service_cron_advance_failure_test.go @@ -0,0 +1,169 @@ +package routines_test + +import ( + "context" + "testing" + "time" + + "github.com/jmoiron/sqlx" + _ "github.com/mattn/go-sqlite3" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/office/models" + "github.com/kandev/kandev/internal/office/repository/sqlite" + "github.com/kandev/kandev/internal/office/routines" +) + +// TestTickScheduledTriggers_UnsatisfiableExpression_DisarmsInsteadOfLooping +// is a regression test for a legacy trigger row whose cron expression can +// never fire (e.g. persisted before create-time validation existed, or +// hand-edited). On a catch-up computation failure the trigger must not +// dispatch and must not re-arm next_run_at to "now": ClaimTrigger already +// clears next_run_at when claiming, and re-arming it to "now" makes the +// trigger due again on the very next 30s cron tick, dispatching a run +// forever. +func TestTickScheduledTriggers_UnsatisfiableExpression_DisarmsInsteadOfLooping(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("new repo: %v", err) + } + svc := routines.NewRoutineService(repo, logger.Default(), &noopActivity{}) + ctx := context.Background() + + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Legacy unsatisfiable", + TaskTemplate: "", // lightweight + AssigneeAgentProfileID: "agent-1", + Status: "active", + ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + } + if err := svc.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + + // Bypass service-level create-time validation (routines.ErrInvalidTrigger) + // by writing straight through the repository, simulating a row that was + // persisted before that validation existed. + due := time.Now().UTC().Add(-time.Minute) + trigger := &models.RoutineTrigger{ + RoutineID: routine.ID, + Kind: "cron", + CronExpression: "0 0 30 2 *", // February 30th: never matches. + Timezone: "UTC", + Enabled: true, + NextRunAt: &due, + } + if err := repo.CreateRoutineTrigger(ctx, trigger); err != nil { + t.Fatalf("create trigger (bypassing validation): %v", err) + } + + enq := &fakeWakeupEnqueuer{} + svc.SetWakeupEnqueuer(enq) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + t.Fatalf("tick scheduled triggers: %v", err) + } + if len(enq.created) != 0 { + t.Fatalf("expected no dispatch for an unsatisfiable trigger, got %d", len(enq.created)) + } + + triggers, err := repo.ListTriggersByRoutineID(ctx, routine.ID) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + if len(triggers) != 1 { + t.Fatalf("expected 1 trigger, got %d", len(triggers)) + } + if triggers[0].NextRunAt != nil { + t.Fatalf("next_run_at = %v, want nil (trigger must stay disarmed, not re-armed to now)", *triggers[0].NextRunAt) + } + + // A second tick must not dispatch either — proves this isn't a + // re-arm-to-now loop that would fire again on the very next 30s tick. + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC().Add(time.Minute)); err != nil { + t.Fatalf("second tick scheduled triggers: %v", err) + } + if len(enq.created) != 0 { + t.Fatalf("expected still no dispatch after second tick, got %d", len(enq.created)) + } +} + +// TestTickScheduledTriggers_RecoverableCatchUpFailure_RearmsForRetry is a +// regression test for a catch-up failure that is NOT expression +// unsatisfiability (e.g. the timezone database is transiently unavailable). +// Unlike the unsatisfiable case, this must re-arm next_run_at to the +// original due time so the next tick retries once the underlying issue +// clears, instead of leaving the trigger permanently disarmed. +func TestTickScheduledTriggers_RecoverableCatchUpFailure_RearmsForRetry(t *testing.T) { + db, err := sqlx.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + repo, err := sqlite.NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("new repo: %v", err) + } + svc := routines.NewRoutineService(repo, logger.Default(), &noopActivity{}) + ctx := context.Background() + + routine := &models.Routine{ + WorkspaceID: "ws-1", + Name: "Bad timezone", + TaskTemplate: "", + AssigneeAgentProfileID: "agent-1", + Status: "active", + ConcurrencyPolicy: models.ConcurrencyPolicyAlwaysCreate, + } + if err := svc.CreateRoutine(ctx, routine); err != nil { + t.Fatalf("create routine: %v", err) + } + + // Bypass service-level create-time validation by writing straight + // through the repository, simulating a timezone that was valid at + // create time but whose lookup now fails (e.g. tzdata unavailable). + due := time.Now().UTC().Add(-time.Minute) + trigger := &models.RoutineTrigger{ + RoutineID: routine.ID, + Kind: "cron", + CronExpression: "*/5 * * * *", + Timezone: "Not/AZone", + Enabled: true, + NextRunAt: &due, + } + if err := repo.CreateRoutineTrigger(ctx, trigger); err != nil { + t.Fatalf("create trigger (bypassing validation): %v", err) + } + + enq := &fakeWakeupEnqueuer{} + svc.SetWakeupEnqueuer(enq) + + if err := svc.TickScheduledTriggers(ctx, time.Now().UTC()); err != nil { + t.Fatalf("tick scheduled triggers: %v", err) + } + if len(enq.created) != 0 { + t.Fatalf("expected no dispatch on a recoverable catch-up failure, got %d", len(enq.created)) + } + + triggers, err := repo.ListTriggersByRoutineID(ctx, routine.ID) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + if len(triggers) != 1 { + t.Fatalf("expected 1 trigger, got %d", len(triggers)) + } + if triggers[0].NextRunAt == nil { + t.Fatalf("next_run_at = nil, want re-armed to the original due time so the trigger retries") + } + if !triggers[0].NextRunAt.Equal(due) { + t.Fatalf("next_run_at = %v, want re-armed to original due time %v", *triggers[0].NextRunAt, due) + } +} diff --git a/apps/backend/internal/office/routines/trigger_validation_test.go b/apps/backend/internal/office/routines/trigger_validation_test.go new file mode 100644 index 0000000000..7dab03a33c --- /dev/null +++ b/apps/backend/internal/office/routines/trigger_validation_test.go @@ -0,0 +1,81 @@ +package routines_test + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/office/routines" +) + +// TestCreateRoutineTrigger_EmptyCronExpression_Rejected verifies a cron +// trigger with no expression is rejected at create time instead of +// persisting as a trigger that silently never fires. +func TestCreateRoutineTrigger_EmptyCronExpression_Rejected(t *testing.T) { + svc := newTestRoutineService(t) + routine := createTestRoutine(t, svc, "empty-cron", "skip_if_active") + + trigger := &routines.RoutineTrigger{RoutineID: routine.ID, Kind: "cron", CronExpression: ""} + err := svc.CreateRoutineTrigger(context.Background(), trigger) + if err == nil { + t.Fatal("expected error for empty cron expression") + } + if !errors.Is(err, routines.ErrInvalidTrigger) { + t.Errorf("expected ErrInvalidTrigger, got %v", err) + } +} + +// TestCreateRoutineTrigger_UnsatisfiableCron_Rejected verifies an impossible +// expression (Feb 30th) is rejected at create time instead of becoming a +// silent daily-at-midnight job. +func TestCreateRoutineTrigger_UnsatisfiableCron_Rejected(t *testing.T) { + svc := newTestRoutineService(t) + routine := createTestRoutine(t, svc, "unsatisfiable-cron", "skip_if_active") + + trigger := &routines.RoutineTrigger{RoutineID: routine.ID, Kind: "cron", CronExpression: "0 0 30 2 *"} + err := svc.CreateRoutineTrigger(context.Background(), trigger) + if err == nil { + t.Fatal("expected error for unsatisfiable cron expression") + } + if !errors.Is(err, routines.ErrInvalidTrigger) { + t.Errorf("expected ErrInvalidTrigger, got %v", err) + } +} + +// TestCreateRoutineTrigger_EmptyTimezone_NormalizedToUTC verifies an empty +// timezone is made explicit at write time rather than persisted as ”. +func TestCreateRoutineTrigger_EmptyTimezone_NormalizedToUTC(t *testing.T) { + svc := newTestRoutineService(t) + routine := createTestRoutine(t, svc, "empty-tz", "skip_if_active") + + trigger := &routines.RoutineTrigger{RoutineID: routine.ID, Kind: "cron", CronExpression: "0 9 * * *"} + if err := svc.CreateRoutineTrigger(context.Background(), trigger); err != nil { + t.Fatalf("create trigger: %v", err) + } + if trigger.Timezone != "UTC" { + t.Errorf("Timezone = %q, want %q", trigger.Timezone, "UTC") + } + + triggers, err := svc.ListRoutineTriggers(context.Background(), routine.ID) + if err != nil { + t.Fatalf("list triggers: %v", err) + } + if len(triggers) != 1 || triggers[0].Timezone != "UTC" { + t.Fatalf("persisted triggers = %+v, want one trigger with Timezone=UTC", triggers) + } +} + +// TestCreateRoutineTrigger_ValidCron_Succeeds is the control case: a +// well-formed cron trigger still creates successfully. +func TestCreateRoutineTrigger_ValidCron_Succeeds(t *testing.T) { + svc := newTestRoutineService(t) + routine := createTestRoutine(t, svc, "valid-cron", "skip_if_active") + + trigger := &routines.RoutineTrigger{RoutineID: routine.ID, Kind: "cron", CronExpression: "*/5 * * * *"} + if err := svc.CreateRoutineTrigger(context.Background(), trigger); err != nil { + t.Fatalf("create trigger: %v", err) + } + if trigger.NextRunAt == nil { + t.Error("expected NextRunAt to be set") + } +} diff --git a/apps/backend/internal/office/shared/cron.go b/apps/backend/internal/office/shared/cron.go index 712878f28e..9cb0d08317 100644 --- a/apps/backend/internal/office/shared/cron.go +++ b/apps/backend/internal/office/shared/cron.go @@ -1,251 +1,247 @@ package shared import ( + "errors" "fmt" - "strconv" "strings" "time" + + "github.com/robfig/cron/v3" ) -// NextCronTime computes the next fire time after `after` for the given cron expression. -// Supports standard 5-field cron: minute hour day-of-month month day-of-week. -// Timezone is optional; defaults to UTC. +// ErrUnsatisfiableCron indicates a syntactically valid but impossible cron +// expression (e.g. "0 0 30 2 *", February 30th) that can never match a +// wall-clock date. +var ErrUnsatisfiableCron = errors.New("cron expression can never fire") + +// cronParser accepts the standard 5-field cron syntax: minute hour +// day-of-month month day-of-week. No descriptors (@daily, @every) - Office's +// contract is strictly 5 fields. +var cronParser = cron.NewParser( + cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow, +) + +// NextCronTime computes the next fire time strictly after `after` for the +// given 5-field cron expression, in the given timezone (empty means UTC). +// +// Day-of-month and day-of-week are ORed when both are restricted, matching +// crontab(5): "0 0 13 * 5" means the 13th of the month OR any Friday, not +// only Friday the 13th. +// +// DST policy: a wall-clock slot fires at most once. A slot that does not +// exist (spring-forward gap) is skipped. A slot that occurs twice (fall-back +// repeated hour) fires only on its first occurrence. func NextCronTime(expression, timezone string, after time.Time) (time.Time, error) { - fields := strings.Fields(expression) - if len(fields) != 5 { - return time.Time{}, fmt.Errorf("expected 5 cron fields, got %d", len(fields)) - } - loc := time.UTC - if timezone != "" { - var err error - loc, err = time.LoadLocation(timezone) - if err != nil { - return time.Time{}, fmt.Errorf("invalid timezone %q: %w", timezone, err) - } - } - spec, err := parseCronSpec(fields) + loc, err := resolveLocation(timezone) if err != nil { return time.Time{}, err } - return findNextMatch(spec, after.In(loc)), nil -} - -type cronSpec struct { - minutes []int - hours []int - daysOfMonth []int - months []int - daysOfWeek []int -} - -// dayOfWeekNames maps abbreviated day names to their numeric cron value. -// Case-insensitive; lookup is performed after lowercasing the token. -var dayOfWeekNames = map[string]int{ - "sun": 0, "mon": 1, "tue": 2, "wed": 3, "thu": 4, "fri": 5, "sat": 6, -} - -// monthNames maps abbreviated month names to their numeric cron value. -// Case-insensitive; lookup is performed after lowercasing the token. -var monthNames = map[string]int{ - "jan": 1, "feb": 2, "mar": 3, "apr": 4, "may": 5, "jun": 6, - "jul": 7, "aug": 8, "sep": 9, "oct": 10, "nov": 11, "dec": 12, -} - -func parseCronSpec(fields []string) (*cronSpec, error) { - minutes, err := parseCronField(fields[0], 0, 59, nil) + trimmed := strings.TrimSpace(expression) + if len(strings.Fields(trimmed)) != 5 { + return time.Time{}, fmt.Errorf("parse cron expression: %q: must be exactly 5 whitespace-separated fields (minute hour day-of-month month day-of-week); descriptors and TZ/CRON_TZ prefixes are not supported", expression) + } + schedule, err := cronParser.Parse(trimmed) if err != nil { - return nil, fmt.Errorf("minute field: %w", err) + return time.Time{}, fmt.Errorf("parse cron expression: %w", err) } - hours, err := parseCronField(fields[1], 0, 23, nil) - if err != nil { - return nil, fmt.Errorf("hour field: %w", err) - } - dom, err := parseCronField(fields[2], 1, 31, nil) - if err != nil { - return nil, fmt.Errorf("day-of-month field: %w", err) - } - months, err := parseCronField(fields[3], 1, 12, monthNames) - if err != nil { - return nil, fmt.Errorf("month field: %w", err) - } - dow, err := parseCronField(fields[4], 0, 6, dayOfWeekNames) - if err != nil { - return nil, fmt.Errorf("day-of-week field: %w", err) - } - return &cronSpec{minutes, hours, dom, months, dow}, nil -} - -// parseCronField parses a single cron field (supports *, N, N-M, */N, N-M/N, comma lists). -// If names is non-nil, alphabetic tokens are resolved to numbers via the map -// before numeric parsing (e.g. MON->1, JAN->1). Lookup is case-insensitive. -func parseCronField(field string, min, max int, names map[string]int) ([]int, error) { - var result []int - for _, part := range strings.Split(field, ",") { - vals, err := parseCronPart(part, min, max, names) - if err != nil { - return nil, err - } - result = append(result, vals...) - } - return dedupSort(result), nil -} - -func parseCronPart(part string, min, max int, names map[string]int) ([]int, error) { - step, base, err := extractStep(part) - if err != nil { - return nil, err - } - resolved, err := resolveNames(base, names) - if err != nil { - return nil, err - } - lo, hi, err := parseRange(resolved, min, max) - if err != nil { - return nil, err - } - if lo < min || hi > max || lo > hi { - return nil, fmt.Errorf("value out of range [%d-%d] in %q", min, max, part) - } - var vals []int - for i := lo; i <= hi; i += step { - vals = append(vals, i) - } - return vals, nil -} - -// resolveNames translates any alphabetic tokens in a cron part (e.g. "MON", -// "mon-fri") into their numeric equivalents using the provided name map. -// Wildcards are passed through. If names is nil and the part contains -// non-numeric tokens, an error is returned by the downstream numeric parser. -func resolveNames(part string, names map[string]int) (string, error) { - if part == "*" || !containsAlpha(part) { - return part, nil - } - if names == nil { - return "", fmt.Errorf("alphabetic name %q not allowed in this field", part) - } - if idx := strings.Index(part, "-"); idx >= 0 { - lo, err := resolveToken(part[:idx], names) - if err != nil { - return "", err - } - hi, err := resolveToken(part[idx+1:], names) - if err != nil { - return "", err - } - return fmt.Sprintf("%d-%d", lo, hi), nil - } - val, err := resolveToken(part, names) - if err != nil { - return "", err - } - return strconv.Itoa(val), nil -} - -func resolveToken(tok string, names map[string]int) (int, error) { - if !containsAlpha(tok) { - v, err := strconv.Atoi(tok) - if err != nil { - return 0, fmt.Errorf("invalid value %q", tok) - } - return v, nil - } - v, ok := names[strings.ToLower(tok)] + specSchedule, ok := schedule.(*cron.SpecSchedule) if !ok { - return 0, fmt.Errorf("unknown name %q", tok) + // cronParser is configured with only the 5 standard fields (no + // descriptors), so Parse always returns *SpecSchedule for a + // well-formed 5-field expression. This would only trip if + // robfig/cron's internal type changed — fail loudly rather than + // silently degrading matchesWallClock to "everything matches". + return time.Time{}, fmt.Errorf("internal: unexpected schedule type %T", schedule) } - return v, nil + start := after.In(loc) + candidate := schedule.Next(start) + if candidate.IsZero() { + return time.Time{}, fmt.Errorf("%w: %q", ErrUnsatisfiableCron, expression) + } + for isAmbiguousFallBack(candidate) || !matchesWallClock(specSchedule, candidate) { + candidate = schedule.Next(candidate) + if candidate.IsZero() { + return time.Time{}, fmt.Errorf("%w: %q", ErrUnsatisfiableCron, expression) + } + } + if earlier, ok := findEarlierMatchAcrossSubHourTransition(specSchedule, loc, start, candidate); ok { + candidate = earlier + } + return candidate.UTC(), nil } -func containsAlpha(s string) bool { - for _, r := range s { - if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') { +// findEarlierMatchAcrossSubHourTransition recovers a fire that +// schedule.Next skipped over because its hour-advancing loop steps by an +// absolute 1h and desynchronizes from a zone transition whose offset delta +// is not a whole hour (only Australia/Lord_Howe, +10:30<->+11:00, does this +// among IANA zones): the loop can jump straight past a day that has a +// genuinely matching slot. matchesWallClock cannot recover this on its own +// because it only validates candidates schedule.Next actually returns. +// +// This rescans only the local days around each affected transition at minute +// granularity, so a sparse schedule cannot turn the shared scheduler tick +// into a multi-year scan. +func findEarlierMatchAcrossSubHourTransition(spec *cron.SpecSchedule, loc *time.Location, after, candidate time.Time) (time.Time, bool) { + if spec == nil { + return time.Time{}, false + } + + var earlier time.Time + found := walkZoneTransitions(loc, after, candidate, func(transition time.Time) bool { + delta := zoneOffsetAt(transition) - zoneOffsetAt(transition.Add(-time.Second)) + if delta < 0 { + delta = -delta + } + if delta%3600 == 0 { + return false + } + + windowStart, windowEnd := transitionDayWindow(loc, transition) + t := after.Truncate(time.Minute) + if !t.After(after) { + t = t.Add(time.Minute) + } + if t.Before(windowStart) { + t = windowStart + } + if candidate.Before(windowEnd) { + windowEnd = candidate + } + for t.Before(windowEnd) { + if !isAmbiguousFallBack(t) && matchesWallClock(spec, t) { + earlier = t + return true + } + t = t.Add(time.Minute) + } + return false + }) + return earlier, found +} + +const maxZoneTransitionHops = 512 + +// walkZoneTransitions visits each transition strictly between after and +// candidate. Some TZif files use a POSIX extension whose ZoneBounds result +// can stop advancing at a leap-year boundary, so a non-progressing result is +// advanced to the next local year before the next lookup. The hop limit is a +// final guard against malformed or unexpectedly long timezone data. +func walkZoneTransitions(loc *time.Location, after, candidate time.Time, visit func(time.Time) bool) bool { + if loc == nil || visit == nil || !after.Before(candidate) { + return false + } + t := after.In(loc) + for hops := 0; hops < maxZoneTransitionHops; hops++ { + _, transition := t.ZoneBounds() + if transition.IsZero() || !transition.Before(candidate) { + return false + } + if !transition.After(t) { + nextYear := time.Date(t.Year()+1, time.January, 1, 0, 0, 0, 0, loc) + if !nextYear.After(t) { + return false + } + t = nextYear + continue + } + if visit(transition) { return true } + t = transition } return false } -func extractStep(part string) (int, string, error) { - idx := strings.Index(part, "/") - if idx < 0 { - return 1, part, nil - } - step, err := strconv.Atoi(part[idx+1:]) - if err != nil || step <= 0 { - return 0, "", fmt.Errorf("invalid step in %q", part) - } - return step, part[:idx], nil +func transitionDayWindow(loc *time.Location, transition time.Time) (time.Time, time.Time) { + local := transition.In(loc) + start := time.Date(local.Year(), local.Month(), local.Day()-1, 0, 0, 0, 0, loc) + end := time.Date(local.Year(), local.Month(), local.Day()+2, 0, 0, 0, 0, loc) + return start, end } -func parseRange(part string, min, max int) (int, int, error) { - if part == "*" { - return min, max, nil +func zoneOffsetAt(t time.Time) int { + _, offset := t.Zone() + return offset +} + +// matchesWallClock reports whether candidate's local wall-clock fields +// actually satisfy spec. In a sub-hour DST zone (e.g. Australia/Lord_Howe, +// +10:30/+11:00), robfig's minute-increment loop advances in absolute time: +// crossing a spring-forward gap can shift the hour by 30 minutes without +// re-entering the hour-matching loop, so the returned instant can satisfy +// the minute bitmask under a different, unmatched hour. NextCronTime's own +// type-assertion guard means spec is never nil on that call path; the nil +// check here is defense against a future caller. +func matchesWallClock(spec *cron.SpecSchedule, candidate time.Time) bool { + if spec == nil { + return true } - if idx := strings.Index(part, "-"); idx >= 0 { - lo, err := strconv.Atoi(part[:idx]) - if err != nil { - return 0, 0, fmt.Errorf("invalid range in %q", part) - } - hi, err := strconv.Atoi(part[idx+1:]) - if err != nil { - return 0, 0, fmt.Errorf("invalid range in %q", part) - } - return lo, hi, nil + if 1< 0 + dowMatch := 1< 0 + if spec.Dom&starBit > 0 || spec.Dow&starBit > 0 { + return domMatch && dowMatch + } + return domMatch || dowMatch +} + +// resolveLocation maps an empty timezone to UTC, matching +// internal/automation/scheduler.go's nextCronFire. +func resolveLocation(timezone string) (*time.Location, error) { + if timezone == "" { + return time.UTC, nil + } + loc, err := time.LoadLocation(timezone) if err != nil { - return 0, 0, fmt.Errorf("invalid value %q", part) + return nil, fmt.Errorf("invalid timezone %q: %w", timezone, err) } - return val, val, nil + return loc, nil } -func dedupSort(vals []int) []int { - seen := make(map[int]bool, len(vals)) - var result []int - for _, v := range vals { - if !seen[v] { - seen[v] = true - result = append(result, v) - } +// isAmbiguousFallBack reports whether candidate is the second occurrence of +// a local wall-clock time made ambiguous by a DST fall-back transition. +// +// time.Date's disambiguation of ambiguous wall-clock fields is documented as +// implementation-defined ("the choice of time zone, and therefore the time, +// is not guaranteed"), and in practice resolves to different occurrences in +// different zone families (e.g. it picks the earlier instant in +// America/New_York but the later one in zones with a UTC+0 winter offset +// such as Europe/London). This instead reasons from candidate's own zone +// transition: candidate's period starts at the most recent offset change; +// if that change was a fall-back (offset decreased), the first +// offsetDelta-wide slice of the new period repeats wall-clock times already +// seen under the old offset. +func isAmbiguousFallBack(candidate time.Time) bool { + start, _ := candidate.ZoneBounds() + if start.IsZero() { + return false } - // Simple insertion sort for small slices. - for i := 1; i < len(result); i++ { - for j := i; j > 0 && result[j] < result[j-1]; j-- { - result[j], result[j-1] = result[j-1], result[j] - } + _, currentOffset := candidate.Zone() + _, priorOffset := start.Add(-time.Second).Zone() + if priorOffset <= currentOffset { + return false // not a fall-back transition (spring-forward or no change) } - return result -} - -func findNextMatch(spec *cronSpec, after time.Time) time.Time { - // Start from the next minute after `after`. - t := after.Truncate(time.Minute).Add(time.Minute) - // Search up to 366 days ahead. - deadline := after.Add(366 * 24 * time.Hour) - for t.Before(deadline) { - if matchesSpec(spec, t) { - return t.UTC() - } - t = t.Add(time.Minute) - } - // Fallback: return 24h from now. - return after.Add(24 * time.Hour).UTC() -} - -func matchesSpec(spec *cronSpec, t time.Time) bool { - return contains(spec.minutes, t.Minute()) && - contains(spec.hours, t.Hour()) && - contains(spec.daysOfMonth, t.Day()) && - contains(spec.months, int(t.Month())) && - contains(spec.daysOfWeek, int(t.Weekday())) -} - -func contains(vals []int, v int) bool { - for _, val := range vals { - if val == v { - return true - } - } - return false + repeatedWindow := time.Duration(priorOffset-currentOffset) * time.Second + return candidate.Before(start.Add(repeatedWindow)) } diff --git a/apps/backend/internal/office/shared/cron_edge_cases_test.go b/apps/backend/internal/office/shared/cron_edge_cases_test.go new file mode 100644 index 0000000000..22eb2cdf15 --- /dev/null +++ b/apps/backend/internal/office/shared/cron_edge_cases_test.go @@ -0,0 +1,32 @@ +package shared + +import ( + "testing" + "time" +) + +func TestNextCronTime_PastLeapYearPOSIXExtensionTerminates(t *testing.T) { + const expression = "0 12 1 1 *" + after := time.Date(2040, time.January, 2, 0, 0, 0, 0, time.UTC) + want := time.Date(2041, time.January, 1, 17, 0, 0, 0, time.UTC) + + done := make(chan struct{}) + var got time.Time + var err error + go func() { + got, err = NextCronTime(expression, "America/New_York", after) + close(done) + }() + + select { + case <-done: + if err != nil { + t.Fatalf("NextCronTime returned error: %v", err) + } + if !got.Equal(want) { + t.Fatalf("NextCronTime = %s, want %s", got, want) + } + case <-time.After(2 * time.Second): + t.Fatal("NextCronTime did not terminate across a POSIX timezone year boundary") + } +} diff --git a/apps/backend/internal/office/shared/cron_test.go b/apps/backend/internal/office/shared/cron_test.go index 5b6e61f45b..74aa756e42 100644 --- a/apps/backend/internal/office/shared/cron_test.go +++ b/apps/backend/internal/office/shared/cron_test.go @@ -1,6 +1,7 @@ package shared import ( + "errors" "strings" "testing" "time" @@ -77,36 +78,6 @@ func TestNextCronTime_StepExpression(t *testing.T) { } } -func TestParseCronField_Wildcard(t *testing.T) { - vals, err := parseCronField("*", 0, 59, nil) - if err != nil { - t.Fatal(err) - } - if len(vals) != 60 { - t.Errorf("expected 60 values, got %d", len(vals)) - } -} - -func TestParseCronField_Range(t *testing.T) { - vals, err := parseCronField("1-5", 0, 6, nil) - if err != nil { - t.Fatal(err) - } - if len(vals) != 5 { - t.Errorf("expected 5 values, got %d", len(vals)) - } -} - -func TestParseCronField_CommaList(t *testing.T) { - vals, err := parseCronField("1,3,5", 0, 6, nil) - if err != nil { - t.Fatal(err) - } - if len(vals) != 3 { - t.Errorf("expected 3 values, got %d", len(vals)) - } -} - func TestNextCronTime_DayNames(t *testing.T) { // 2026-04-25 is a Saturday; first MON-FRI window opens on Mon 2026-04-27. after := time.Date(2026, 4, 25, 10, 0, 0, 0, time.UTC) @@ -212,3 +183,380 @@ func TestNextCronTime_DayNameInMonthField_Rejected(t *testing.T) { t.Fatal("expected error when day name MON appears in month field") } } + +// TestNextCronTime_DomDowOred verifies crontab(5) semantics: when both +// day-of-month and day-of-week are restricted, a match on either field +// fires, not just a match on both ("Friday the 13th"). +func TestNextCronTime_DomDowOred(t *testing.T) { + after := time.Date(2025, 12, 31, 23, 59, 0, 0, time.UTC) + want := []time.Time{ + time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC), // Friday + time.Date(2026, 1, 9, 0, 0, 0, 0, time.UTC), // Friday + time.Date(2026, 1, 13, 0, 0, 0, 0, time.UTC), // the 13th (Tuesday) + time.Date(2026, 1, 16, 0, 0, 0, 0, time.UTC), // Friday + time.Date(2026, 1, 23, 0, 0, 0, 0, time.UTC), // Friday + } + cursor := after + for i, w := range want { + next, err := NextCronTime("0 0 13 * 5", "", cursor) + if err != nil { + t.Fatalf("fire %d: unexpected error: %v", i+1, err) + } + if !next.Equal(w) { + t.Errorf("fire %d: got %v, want %v", i+1, next, w) + } + cursor = next + } +} + +// TestNextCronTime_DSTSpringForward_Skip verifies that a wall-clock slot +// which does not exist (spring-forward gap hour) is skipped, not fired at +// an adjusted time. +func TestNextCronTime_DSTSpringForward_Skip(t *testing.T) { + loc, err := time.LoadLocation("America/New_York") + if err != nil { + t.Fatalf("load location: %v", err) + } + // 2026-03-08 is spring-forward in America/New_York; 02:00 does not exist. + after := time.Date(2026, 3, 7, 12, 0, 0, 0, loc) + next, err := NextCronTime("0 2 * * *", "America/New_York", after) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := time.Date(2026, 3, 9, 2, 0, 0, 0, loc) + if !next.Equal(want) { + t.Errorf("got %v, want %v (2026-03-08 should be skipped)", next, want) + } +} + +// TestNextCronTime_DSTFallBack_FiresOnce verifies that an ambiguous +// wall-clock slot (fall-back repeated hour) fires exactly once. +func TestNextCronTime_DSTFallBack_FiresOnce(t *testing.T) { + loc, err := time.LoadLocation("America/New_York") + if err != nil { + t.Fatalf("load location: %v", err) + } + // 2026-11-01 is fall-back in America/New_York; 01:30 occurs twice. + after := time.Date(2026, 10, 31, 12, 0, 0, 0, loc) + fire1, err := NextCronTime("30 1 * * *", "America/New_York", after) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + wantFire1 := time.Date(2026, 11, 1, 1, 30, 0, 0, loc) + if !fire1.Equal(wantFire1) { + t.Fatalf("fire1: got %v, want %v", fire1, wantFire1) + } + + fire2, err := NextCronTime("30 1 * * *", "America/New_York", fire1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + wantFire2 := time.Date(2026, 11, 2, 1, 30, 0, 0, loc) + if !fire2.Equal(wantFire2) { + t.Errorf("fire2: got %v, want %v (second 01:30 occurrence must be suppressed)", fire2, wantFire2) + } +} + +// TestNextCronTime_DSTFallBack_DenseExpression_NoDoubleFire is a regression +// test for a fall-back suppression that only re-checked the single +// replacement candidate: an expression with multiple matching slots inside +// the repeated hour must not double-fire on any of them. +func TestNextCronTime_DSTFallBack_DenseExpression_NoDoubleFire(t *testing.T) { + loc, err := time.LoadLocation("America/New_York") + if err != nil { + t.Fatalf("load location: %v", err) + } + // 2026-11-01 is fall-back in America/New_York; 01:00 and 01:30 each + // occur twice (EDT, then EST). + after := time.Date(2026, 10, 31, 12, 0, 0, 0, loc) + + fire1, err := NextCronTime("0,30 1 * * *", "America/New_York", after) + if err != nil { + t.Fatalf("fire1: unexpected error: %v", err) + } + wantFire1 := time.Date(2026, 11, 1, 1, 0, 0, 0, loc) + if !fire1.Equal(wantFire1) { + t.Fatalf("fire1: got %v, want %v", fire1, wantFire1) + } + + fire2, err := NextCronTime("0,30 1 * * *", "America/New_York", fire1) + if err != nil { + t.Fatalf("fire2: unexpected error: %v", err) + } + wantFire2 := time.Date(2026, 11, 1, 1, 30, 0, 0, loc) + if !fire2.Equal(wantFire2) { + t.Fatalf("fire2: got %v, want %v", fire2, wantFire2) + } + + // Both repeated fall-back occurrences (EST 01:00 and 01:30) must be + // suppressed: the next fire is the following day's 01:00, not a + // same-day repeat of either slot. + fire3, err := NextCronTime("0,30 1 * * *", "America/New_York", fire2) + if err != nil { + t.Fatalf("fire3: unexpected error: %v", err) + } + wantFire3 := time.Date(2026, 11, 2, 1, 0, 0, 0, loc) + if !fire3.Equal(wantFire3) { + t.Errorf("fire3: got %v, want %v (both repeated fall-back slots must be suppressed)", fire3, wantFire3) + } +} + +// TestNextCronTime_DSTFallBack_ZeroOffsetZone is a regression test for +// disambiguation logic that assumed time.Date resolves an ambiguous wall +// clock to its pre-transition offset. That assumption does not hold in +// zones whose standard-time UTC offset is zero (e.g. Europe/London), where +// it resolves to the later, repeated occurrence instead. +func TestNextCronTime_DSTFallBack_ZeroOffsetZone(t *testing.T) { + loc, err := time.LoadLocation("Europe/London") + if err != nil { + t.Fatalf("load location: %v", err) + } + // 2026-10-25 is fall-back in Europe/London; 01:30 occurs twice (BST + // then GMT). Build expectations from explicit UTC instants so the + // assertions don't themselves depend on ambiguous local reconstruction. + after := time.Date(2026, 10, 24, 12, 0, 0, 0, loc) + + fire1, err := NextCronTime("30 1 * * *", "Europe/London", after) + if err != nil { + t.Fatalf("fire1: unexpected error: %v", err) + } + // First occurrence: 01:30 BST (UTC+1) = 00:30 UTC. + wantFire1 := time.Date(2026, 10, 25, 0, 30, 0, 0, time.UTC) + if !fire1.Equal(wantFire1) { + t.Fatalf("fire1: got %v (%s), want %v", fire1, fire1.In(loc), wantFire1) + } + + fire2, err := NextCronTime("30 1 * * *", "Europe/London", fire1) + if err != nil { + t.Fatalf("fire2: unexpected error: %v", err) + } + // The second occurrence (01:30 GMT = 01:30 UTC) must be suppressed: + // the next fire is the following day, not the same-day repeat. + wantFire2 := time.Date(2026, 10, 26, 1, 30, 0, 0, time.UTC) + if !fire2.Equal(wantFire2) { + t.Errorf("fire2: got %v (%s), want %v", fire2, fire2.In(loc), wantFire2) + } +} + +// TestNextCronTime_LordHoweSpringForward_MatchesWallClock is a regression +// test for a wall-clock mismatch unique to Australia/Lord_Howe, the only IANA +// zone with a 30-minute DST shift (+10:30 <-> +11:00 spring-forward). robfig's +// minute loop advances in absolute time: searching forward from the first +// occurrence of "30 1 * * *" (2026-10-04 01:30, still before the gap) for the +// *next* occurrence crosses the gap and can land on a minute that satisfies +// the schedule's minute mask under an hour the schedule never asked for +// (local 02:30 on the same spring-forward day, instead of the following +// day's 01:30). +func TestNextCronTime_LordHoweSpringForward_MatchesWallClock(t *testing.T) { + loc, err := time.LoadLocation("Australia/Lord_Howe") + if err != nil { + t.Fatalf("load location: %v", err) + } + // 2026-10-04 is spring-forward in Australia/Lord_Howe: 02:00 -> 02:30. + after := time.Date(2026, 10, 3, 12, 0, 0, 0, loc) + + cases := []struct { + name string + expr string + wantFire1Min int + wantFire2Day int + wantFire2Min int + }{ + {"single slot", "30 1 * * *", 30, 5, 30}, + {"first of two slots", "0,30 1 * * *", 0, 4, 30}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fire1, err := NextCronTime(tc.expr, "Australia/Lord_Howe", after) + if err != nil { + t.Fatalf("fire1: unexpected error: %v", err) + } + local1 := fire1.In(loc) + if local1.Day() != 4 || local1.Hour() != 1 || local1.Minute() != tc.wantFire1Min { + t.Fatalf("fire1: expr %q: got local %v, want 2026-10-04 01:%02d", tc.expr, local1, tc.wantFire1Min) + } + + fire2, err := NextCronTime(tc.expr, "Australia/Lord_Howe", fire1) + if err != nil { + t.Fatalf("fire2: unexpected error: %v", err) + } + local2 := fire2.In(loc) + if local2.Day() != tc.wantFire2Day || local2.Hour() != 1 || local2.Minute() != tc.wantFire2Min { + t.Errorf("fire2: expr %q: got local %v (hour=%d), want 2026-10-%02d 01:%02d", + tc.expr, local2, local2.Hour(), tc.wantFire2Day, tc.wantFire2Min) + } + }) + } +} + +// TestNextCronTime_LordHoweSweep_WallClockMatchesExpression sweeps every fire +// of several hour/minute expressions across calendar 2026 in +// Australia/Lord_Howe and asserts each fire's local wall clock actually +// satisfies the expression. Fire completeness (every genuinely-existing slot +// is returned) is covered separately by +// TestNextCronTime_LordHoweDailyExpression_FiresEveryDay. +func TestNextCronTime_LordHoweSweep_WallClockMatchesExpression(t *testing.T) { + loc, err := time.LoadLocation("Australia/Lord_Howe") + if err != nil { + t.Fatalf("load location: %v", err) + } + cases := []struct { + expr string + wantHour int + wantMinutes map[int]bool + }{ + {"0 0 * * *", 0, map[int]bool{0: true}}, + {"0 2 * * *", 2, map[int]bool{0: true}}, + {"30 1 * * *", 1, map[int]bool{30: true}}, + {"0,30 1 * * *", 1, map[int]bool{0: true, 30: true}}, + } + yearEnd := time.Date(2027, 1, 1, 0, 0, 0, 0, loc) + for _, tc := range cases { + t.Run(tc.expr, func(t *testing.T) { + cursor := time.Date(2026, 1, 1, 0, 0, 0, 0, loc) + count := 0 + for { + next, err := NextCronTime(tc.expr, "Australia/Lord_Howe", cursor) + if err != nil { + t.Fatalf("unexpected error at %v: %v", cursor, err) + } + if !next.Before(yearEnd) { + break + } + local := next.In(loc) + if local.Hour() != tc.wantHour || !tc.wantMinutes[local.Minute()] { + t.Errorf("expr %q: fire %v has hour=%d minute=%d, not one of the expression's slots", + tc.expr, local, local.Hour(), local.Minute()) + } + cursor = next + count++ + if count > 800 { + t.Fatalf("too many fires for a daily/twice-daily expression over one year; possible infinite loop") + } + } + if count == 0 { + t.Fatalf("expected at least one fire for %q", tc.expr) + } + }) + } +} + +// TestNextCronTime_LordHoweDailyExpression_FiresEveryDay is the regression +// test for the sub-hour-transition rescan: robfig/cron/v3's day-loop DST +// correction nudges by whole hours, so in Australia/Lord_Howe (the only IANA +// zone with a 30-minute DST shift) it can skip every fire scheduled anywhere +// on the transition day, not merely fires inside the 30-minute transition +// window. NextCronTime recovers those fires with a minute-granularity rescan +// gated on the sub-hour transition, so "0 12 * * *" now fires on every day of +// 2026, including both transition days, matching the +// TestNextCronTime_ChathamDailyExpression_FiresEveryDay control below. +func TestNextCronTime_LordHoweDailyExpression_FiresEveryDay(t *testing.T) { + loc, err := time.LoadLocation("Australia/Lord_Howe") + if err != nil { + t.Fatalf("load location: %v", err) + } + + fallBack, err := NextCronTime("0 12 * * *", "Australia/Lord_Howe", time.Date(2026, 10, 3, 12, 0, 0, 0, loc)) + if err != nil { + t.Fatalf("fall-back: unexpected error: %v", err) + } + if got := fallBack.In(loc); got.Year() != 2026 || got.Month() != time.October || got.Day() != 4 || got.Hour() != 12 || got.Minute() != 0 { + t.Errorf("fall-back: got %v, want 2026-10-04 12:00", got) + } + + springForward, err := NextCronTime("0 12 * * *", "Australia/Lord_Howe", time.Date(2026, 4, 4, 12, 0, 0, 0, loc)) + if err != nil { + t.Fatalf("spring-forward: unexpected error: %v", err) + } + if got := springForward.In(loc); got.Year() != 2026 || got.Month() != time.April || got.Day() != 5 || got.Hour() != 12 || got.Minute() != 0 { + t.Errorf("spring-forward: got %v, want 2026-04-05 12:00", got) + } + + cursor := time.Date(2026, 1, 1, 0, 0, 0, 0, loc) + yearEnd := time.Date(2027, 1, 1, 0, 0, 0, 0, loc) + days := 0 + for { + next, err := NextCronTime("0 12 * * *", "Australia/Lord_Howe", cursor) + if err != nil { + t.Fatalf("unexpected error at %v: %v", cursor, err) + } + if !next.Before(yearEnd) { + break + } + days++ + cursor = next + } + if days != 365 { + t.Errorf("expected a fire on every one of 2026's 365 days, got %d", days) + } +} + +// TestNextCronTime_ChathamDailyExpression_FiresEveryDay is the control for +// TestNextCronTime_LordHoweDailyExpression_FiresEveryDay: Pacific/Chatham +// has a whole-hour DST shift, so it never needed the sub-hour-transition +// rescan — "0 12 * * *" fires on every day of 2026, including both +// transition days. +func TestNextCronTime_ChathamDailyExpression_FiresEveryDay(t *testing.T) { + loc, err := time.LoadLocation("Pacific/Chatham") + if err != nil { + t.Fatalf("load location: %v", err) + } + cursor := time.Date(2026, 1, 1, 0, 0, 0, 0, loc) + yearEnd := time.Date(2027, 1, 1, 0, 0, 0, 0, loc) + days := 0 + for { + next, err := NextCronTime("0 12 * * *", "Pacific/Chatham", cursor) + if err != nil { + t.Fatalf("unexpected error at %v: %v", cursor, err) + } + if !next.Before(yearEnd) { + break + } + days++ + cursor = next + } + if days != 365 { + t.Errorf("expected a fire on every one of 2026's 365 days, got %d", days) + } +} + +// TestNextCronTime_Unsatisfiable verifies an impossible expression (Feb 30th) +// returns ErrUnsatisfiableCron instead of a silent +24h fallback. +func TestNextCronTime_Unsatisfiable(t *testing.T) { + _, err := NextCronTime("0 0 30 2 *", "", time.Date(2026, 4, 25, 10, 0, 0, 0, time.UTC)) + if err == nil { + t.Fatal("expected error for unsatisfiable expression") + } + if !errors.Is(err, ErrUnsatisfiableCron) { + t.Errorf("expected ErrUnsatisfiableCron, got %v", err) + } +} + +func TestNextCronTime_InvalidTimezone(t *testing.T) { + _, err := NextCronTime("* * * * *", "Not/AZone", time.Now()) + if err == nil { + t.Fatal("expected error for invalid timezone") + } +} + +// TestNextCronTime_RejectsCronTZPrefix verifies that a caller-supplied +// TZ=/CRON_TZ= prefix is rejected rather than silently accepted. robfig/cron +// strips the prefix in Parse() before any field-mask check, so without this +// guard the expression's own prefix would override the trigger's timezone +// column and bypass the DST fall-back suppression (the candidate would carry +// the column's location while the schedule actually ran in the prefix's +// zone). +func TestNextCronTime_RejectsCronTZPrefix(t *testing.T) { + exprs := []string{ + "CRON_TZ=America/New_York 30 1 * * *", + "TZ=Asia/Tokyo 0 9 * * *", + } + for _, expr := range exprs { + t.Run(expr, func(t *testing.T) { + _, err := NextCronTime(expr, "UTC", time.Now()) + if err == nil { + t.Fatalf("expected error for prefixed expression %q, got none", expr) + } + }) + } +} diff --git a/docs/specs/office/requirements/scheduler.md b/docs/specs/office/requirements/scheduler.md index ba98bc1d5f..27b9c2c231 100644 --- a/docs/specs/office/requirements/scheduler.md +++ b/docs/specs/office/requirements/scheduler.md @@ -39,6 +39,10 @@ queue](../../tasks/requirements/run-scheduling.md) and - **AC-OFFICE-SCHEDULER-001.6:** Has a `source` discriminator (see table below) plus a typed payload. - **AC-OFFICE-SCHEDULER-001.7:** Carries an `idempotency_key`. The queue uses a 24-hour lookup for recent duplicates and a durable unique key for persisted identities. - **AC-OFFICE-SCHEDULER-001.8:** Is coalesced into an in-flight run when one exists for the same agent (claim-time merge). +- **AC-OFFICE-SCHEDULER-001.9:** A cron routine trigger's day-of-month and day-of-week fields are ORed when both are restricted, matching `crontab(5)` (`0 0 13 * 5` fires on the 13th of the month OR any Friday, not only Friday the 13th). +- **AC-OFFICE-SCHEDULER-001.10:** A cron routine trigger's wall-clock slot fires at most once across a DST transition: a slot that does not exist (spring-forward) is skipped, a slot that occurs twice (fall-back) fires only on its first occurrence, and no existing slot is ever lost. This holds for every IANA zone, including `Australia/Lord_Howe`, the only zone with a 30-minute DST shift. +- **AC-OFFICE-SCHEDULER-001.11:** A cron routine trigger that can never fire (an impossible date, or an empty expression) is rejected at trigger-create time with a client error, not accepted as a silent no-op or a wrong daily fallback. +- **AC-OFFICE-SCHEDULER-001.12:** A routine trigger's timezone defaults to UTC when not supplied; there is no workspace-level timezone. ## System design diff --git a/docs/specs/office/system-design/scheduler-01.md b/docs/specs/office/system-design/scheduler-01.md index 117cdadd82..45bd8aec06 100644 --- a/docs/specs/office/system-design/scheduler-01.md +++ b/docs/specs/office/system-design/scheduler-01.md @@ -118,7 +118,7 @@ Routine fields: - `variables`: declared template variables (type, default, required). Triggers: -- **Schedule (cron)**: `cron_expression`, `timezone`, computed `next_run_at`, `last_fired_at`. +- **Schedule (cron)**: `cron_expression`, `timezone`, computed `next_run_at`, `last_fired_at`. `cron_expression` is the standard 5-field syntax (minute hour day-of-month month day-of-week), computed via `robfig/cron/v3`. Day-of-month and day-of-week are ORed when both are restricted, matching `crontab(5)` (`0 0 13 * 5` fires on the 13th of the month OR any Friday). A wall-clock slot fires at most once across a DST transition: a spring-forward slot that does not exist is skipped, and a fall-back slot that occurs twice fires only on its first occurrence. Every returned fire is validated against the expression's own wall-clock fields (minute/hour/month/day-of-month-or-day-of-week), guarding against the underlying library returning a fire under the wrong hour when a DST shift crosses a match boundary. In `Australia/Lord_Howe` (the only IANA zone with a 30-minute DST shift, +10:30 <-> +11:00), `robfig/cron/v3`'s day-loop DST correction — which nudges by whole hours — can skip a candidate that genuinely exists; a minute-granularity rescan of the gap, gated on the interval containing a non-whole-hour offset transition, recovers it, so no zone loses an existing slot. `timezone` defaults to UTC when empty. An expression that can never fire (an impossible date, or empty) is rejected at trigger-create time. - **Webhook**: `public_id` (URL path component), `signing_mode` (`none` | `bearer` | `hmac_sha256`), `secret`. URL: `POST /api/routine-triggers//fire`. Webhook payload is available as variables. - **Manual**: fired only via UI or API. @@ -165,7 +165,8 @@ variables: [] trigger: kind: schedule cron_expression: "*/5 * * * *" - timezone: (workspace TZ, fall back to UTC) + timezone: UTC (no workspace-level timezone exists; a trigger's timezone + defaults to UTC unless the user sets one explicitly) enabled: true ``` diff --git a/docs/specs/office/system-design/scheduler-02.md b/docs/specs/office/system-design/scheduler-02.md index 96d2756f8d..12b8530d8e 100644 --- a/docs/specs/office/system-design/scheduler-02.md +++ b/docs/specs/office/system-design/scheduler-02.md @@ -86,7 +86,7 @@ id TEXT PRIMARY KEY routine_id TEXT NOT NULL kind TEXT NOT NULL -- schedule | webhook | manual cron_expression TEXT -- when kind=schedule -timezone TEXT +timezone TEXT -- default 'UTC'; empty on legacy rows is backfilled to 'UTC' next_run_at TIMESTAMP -- computed; atomically claimed for cron scheduling last_fired_at TIMESTAMP public_id TEXT UNIQUE -- when kind=webhook