Improve crontab sync reliability and debugging

internal/cron/schedules.go

@@ -6,17 +6,25 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"path/filepath"
+	"strings"
 
 	"github.com/sirupsen/logrus"
 )
 
 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 {

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")