From eac30001a0677a3b44030ae1459e9bc218b9a34b Mon Sep 17 00:00:00 2001 From: Shaun Date: Sat, 17 Jan 2026 11:42:40 -0600 Subject: [PATCH] Improve crontab sync reliability and debugging --- internal/cron/schedules.go | 51 +++++++++++++++++++---- internal/cron/schedules_test.go | 74 +++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 7 deletions(-) diff --git a/internal/cron/schedules.go b/internal/cron/schedules.go index a9e8d24..cbcca82 100644 --- a/internal/cron/schedules.go +++ b/internal/cron/schedules.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "os/exec" + "path/filepath" + "strings" "github.com/sirupsen/logrus" ) @@ -13,10 +15,16 @@ import ( const ( DefaultSchedulesFilePath = "/usr/local/share/schedules.json" executeCommand = "/usr/local/bin/process-job" - cronFilePath = "/data/crontab" + defaultCronFilePath = "/data/crontab" defaultCommandTimeout = 30 ) +type crontabRunner func(path string) ([]byte, error) + +func defaultRunCrontab(path string) ([]byte, error) { + return exec.Command("crontab", path).CombinedOutput() +} + // SyncSchedules reads schedules from a file and syncs them with the store func SyncSchedules(ctx context.Context, store *Store, log *logrus.Logger, schedulesFilePath string) error { if schedulesFilePath == "" { @@ -77,16 +85,27 @@ func SyncSchedules(ctx context.Context, store *Store, log *logrus.Logger, schedu // SyncCrontab queries the store for enabled schedules and writes them to the crontab file func SyncCrontab(ctx context.Context, store *Store, log *logrus.Logger) error { + return syncCrontab(ctx, store, log, defaultCronFilePath, defaultRunCrontab) +} + +func syncCrontab(ctx context.Context, store *Store, log *logrus.Logger, cronFilePath string, runCrontab crontabRunner) error { schedules, err := store.ListEnabledSchedules(ctx) if err != nil { return fmt.Errorf("failed to list schedules: %w", err) } - file, err := os.Create(cronFilePath) + // Write to a temp file first so a single invalid schedule doesn't clobber the + // last known-good crontab file. + cronDir := filepath.Dir(cronFilePath) + if err := os.MkdirAll(cronDir, 0o755); err != nil { + return fmt.Errorf("failed to create crontab dir: %w", err) + } + + file, err := os.CreateTemp(cronDir, "crontab-*") if err != nil { - return fmt.Errorf("failed to open crontab file: %w", err) + return fmt.Errorf("failed to create temp crontab file: %w", err) } - defer func() { _ = file.Close() }() + defer func() { _ = os.Remove(file.Name()) }() for _, schedule := range schedules { entry := fmt.Sprintf("%s %s %d\n", schedule.Schedule, executeCommand, schedule.ID) @@ -96,13 +115,31 @@ func SyncCrontab(ctx context.Context, store *Store, log *logrus.Logger) error { } } - if err := exec.Command("crontab", cronFilePath).Run(); err != nil { + if err := file.Sync(); err != nil { + return fmt.Errorf("failed to sync temp crontab file: %w", err) + } + if err := file.Close(); err != nil { + return fmt.Errorf("failed to close temp crontab file: %w", err) + } + + tempPath := file.Name() + if out, err := runCrontab(tempPath); err != nil { + msg := strings.TrimSpace(string(out)) + if msg != "" { + log.WithError(err).Errorf("crontab install failed: %s", msg) + return fmt.Errorf("failed to sync crontab: %w: %s", err, msg) + } + log.WithError(err).Error("crontab install failed") return fmt.Errorf("failed to sync crontab: %w", err) } - log.Printf("Synced %d schedule(s) to crontab", len(schedules)) + // Best-effort: keep the installed crontab content at the known path for debugging/ops. + if err := os.Rename(tempPath, cronFilePath); err != nil { + log.WithError(err).Warnf("failed to replace %s with generated crontab", cronFilePath) + } - return file.Sync() + log.Printf("Synced %d schedule(s) to crontab", len(schedules)) + return nil } func findScheduleByName(schedules []Schedule, name string) *Schedule { diff --git a/internal/cron/schedules_test.go b/internal/cron/schedules_test.go index d48cc3a..1053b55 100644 --- a/internal/cron/schedules_test.go +++ b/internal/cron/schedules_test.go @@ -2,7 +2,10 @@ package cron import ( "context" + "fmt" "os" + "path/filepath" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -365,6 +368,77 @@ func TestSyncSchedules(t *testing.T) { }) } +func TestSyncCrontab(t *testing.T) { + ctx := context.TODO() + log := logrus.New() + + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "test.db") + store, err := InitializeStore(ctx, dbPath, "../../migrations") + if err != nil { + t.Fatal(err) + } + defer func() { _ = store.Close() }() + + if err := store.CreateSchedule(ctx, Schedule{ + Name: "test", + AppName: "app", + Schedule: "* * * * *", + Command: "uptime", + CommandTimeout: 60, + Region: "iad", + Enabled: true, + Config: fly.MachineConfig{}, + }); err != nil { + t.Fatal(err) + } + + cronPath := filepath.Join(tmpDir, "crontab") + t.Run("does not clobber on install failure", func(t *testing.T) { + if err := os.WriteFile(cronPath, []byte("KNOWN_GOOD\n"), 0o644); err != nil { + t.Fatal(err) + } + + runCrontab := func(path string) ([]byte, error) { + return []byte("bad minute"), fmt.Errorf("exit status 1") + } + + if err := syncCrontab(ctx, store, log, cronPath, runCrontab); err == nil { + t.Fatalf("expected error") + } + + b, err := os.ReadFile(cronPath) + if err != nil { + t.Fatal(err) + } + if string(b) != "KNOWN_GOOD\n" { + t.Fatalf("expected existing crontab to remain unchanged, got %q", string(b)) + } + }) + + t.Run("replaces on success", func(t *testing.T) { + runCrontab := func(path string) ([]byte, error) { + return nil, nil + } + + if err := syncCrontab(ctx, store, log, cronPath, runCrontab); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + b, err := os.ReadFile(cronPath) + if err != nil { + t.Fatal(err) + } + got := string(b) + if got == "" { + t.Fatalf("expected crontab file to be written") + } + if wantSub := executeCommand; !strings.Contains(got, wantSub) { + t.Fatalf("expected crontab to contain %q, got %q", wantSub, got) + } + }) +} + func createSchedulesFile(schedules []byte) (*os.File, error) { // Write schedules to a temp file tmpFile, err := os.CreateTemp("./", "schedules.json")