Browse Source

fix(settings): accept five-field crontab for report and LDAP schedules

The panel scheduler runs WithSeconds(), whose parser rejects a standard
five-field crontab such as "30 8 * * *". An admin who typed one into the
Telegram/Discord notify time or the LDAP sync schedule saved it without
error, AddJob then failed at startup, and the job silently never ran.

Admin-typed schedules now go through internal/util/cronspec, which takes
an optional seconds column plus the @descriptors and hands the scheduler
a parsed cron.Schedule. CheckValid rejects an unparseable schedule on
save, and a stored legacy value falls back to the job's default instead
of dropping the job.
MHSanaei 22 hours ago
parent
commit
b124af2a1b

+ 4 - 10
frontend/src/pages/settings/NotifyTimeField.tsx

@@ -3,11 +3,8 @@ import { useTranslation } from 'react-i18next';
 import { Input, InputNumber, Select, Space } from 'antd';
 import { onNumber } from '@/utils/onNumber';
 
-// The notification schedule is fed straight to robfig/cron's AddJob (see
-// web.go startTask), which accepts @every <duration>, the @hourly/@daily/...
-// macros, and full crontab expressions. This builder covers the common cases
-// with dropdowns so users don't have to memorise the syntax, while "Custom"
-// preserves the raw crontab escape hatch.
+// The schedule is parsed by internal/util/cronspec: @every, the @macros, and
+// five- or six-field crontab; "Custom" keeps the raw crontab escape hatch.
 export type Unit = 's' | 'm' | 'h';
 export type Macro = '@hourly' | '@daily' | '@weekly' | '@monthly';
 export type Mode = 'every' | Macro | 'custom';
@@ -44,11 +41,8 @@ export function composeRunTime(s: RunTime): string {
   return s.mode;
 }
 
-// The panel's cron runs with seconds enabled (cron.WithSeconds() in web.go), so
-// crontab expressions are 6-field: "second minute hour day month weekday". When
-// the user drops into Custom we seed the box with the crontab equivalent of the
-// current selection rather than a bare @macro, so they get a real expression to
-// edit (and one that the 6-field parser accepts).
+// Custom is seeded with the six-field ("second minute hour day month weekday")
+// equivalent of the current selection, so the admin edits a real expression.
 export function toCrontab(s: RunTime): string {
   switch (s.mode) {
     case '@hourly':

+ 24 - 0
internal/util/cronspec/cronspec.go

@@ -0,0 +1,24 @@
+// Package cronspec parses the cron schedules an admin types into the settings.
+package cronspec
+
+import (
+	"strings"
+
+	"github.com/robfig/cron/v3"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/util/common"
+)
+
+// The panel scheduler runs WithSeconds(), whose parser rejects five-field crontab,
+// so admin schedules are parsed here with the seconds column optional.
+var parser = cron.NewParser(
+	cron.SecondOptional | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow | cron.Descriptor,
+)
+
+func Parse(spec string) (cron.Schedule, error) {
+	schedule, err := parser.Parse(strings.TrimSpace(spec))
+	if err != nil {
+		return nil, common.NewErrorf("cron schedule <%v> is not valid: %v", spec, err)
+	}
+	return schedule, nil
+}

+ 47 - 0
internal/util/cronspec/cronspec_test.go

@@ -0,0 +1,47 @@
+package cronspec
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+// #1648 upstream: the panel's scheduler runs WithSeconds(), whose parser refused
+// a standard five-field crontab, so an admin's "30 8 * * *" never fired.
+func TestParseNextFire(t *testing.T) {
+	from := time.Date(2026, 10, 11, 7, 0, 0, 0, time.UTC)
+	cases := []struct {
+		spec string
+		want time.Time
+	}{
+		{"30 8 * * *", time.Date(2026, 10, 11, 8, 30, 0, 0, time.UTC)},
+		{" 15 30 8 * * * ", time.Date(2026, 10, 11, 8, 30, 15, 0, time.UTC)},
+		{"@daily", time.Date(2026, 10, 12, 0, 0, 0, 0, time.UTC)},
+		{"@every 90m", from.Add(90 * time.Minute)},
+	}
+	for _, c := range cases {
+		t.Run(c.spec, func(t *testing.T) {
+			schedule, err := Parse(c.spec)
+			if err != nil {
+				t.Fatalf("Parse(%q): %v", c.spec, err)
+			}
+			if got := schedule.Next(from); !got.Equal(c.want) {
+				t.Errorf("Parse(%q).Next = %v, want %v", c.spec, got, c.want)
+			}
+		})
+	}
+}
+
+func TestParseRejects(t *testing.T) {
+	for _, spec := range []string{"", "   ", "every day", "61 * * * *", "* * * *"} {
+		t.Run(spec, func(t *testing.T) {
+			_, err := Parse(spec)
+			if err == nil {
+				t.Fatalf("Parse(%q): want error, got nil", spec)
+			}
+			if !strings.Contains(err.Error(), "cron schedule") {
+				t.Errorf("Parse(%q) error = %q, want it to name the cron schedule", spec, err)
+			}
+		})
+	}
+}

+ 25 - 0
internal/web/entity/check_valid_test.go

@@ -78,3 +78,28 @@ func TestCheckValidIPOrCIDRLists(t *testing.T) {
 		t.Errorf("trustedProxyCIDRs error = %v, want it to name the trusted-proxy list and the bad entry", err)
 	}
 }
+
+// A schedule the scheduler cannot parse used to be saved anyway, and the report
+// or sync job then silently never ran.
+func TestCheckValidSchedules(t *testing.T) {
+	set := map[string]func(*AllSetting, string){
+		"tgRunTime":      func(s *AllSetting, v string) { s.TgRunTime = v },
+		"discordRunTime": func(s *AllSetting, v string) { s.DiscordRunTime = v },
+		"ldapSyncCron":   func(s *AllSetting, v string) { s.LdapSyncCron = v },
+	}
+	for name, apply := range set {
+		for _, v := range []string{"", "@daily", "30 8 * * *", "0 30 8 * * *", "@every 1m"} {
+			s := &AllSetting{WebPort: 2053, SubPort: 2096}
+			apply(s, v)
+			if err := s.CheckValid(); err != nil {
+				t.Errorf("%s=%q: unexpected error %v", name, v, err)
+			}
+		}
+		s := &AllSetting{WebPort: 2053, SubPort: 2096}
+		apply(s, "every morning")
+		err := s.CheckValid()
+		if err == nil || !strings.Contains(err.Error(), name) {
+			t.Errorf("%s=%q: want an error naming %s, got %v", name, "every morning", name, err)
+		}
+	}
+}

+ 14 - 0
internal/web/entity/entity.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/mhsanaei/3x-ui/v3/internal/util/common"
+	"github.com/mhsanaei/3x-ui/v3/internal/util/cronspec"
 )
 
 type Msg struct {
@@ -352,6 +353,19 @@ func (s *AllSetting) CheckValid() error {
 		s.SubClashPath += "/"
 	}
 
+	for _, sched := range []struct{ name, spec string }{
+		{"tgRunTime", s.TgRunTime},
+		{"discordRunTime", s.DiscordRunTime},
+		{"ldapSyncCron", s.LdapSyncCron},
+	} {
+		if strings.TrimSpace(sched.spec) == "" {
+			continue
+		}
+		if _, err := cronspec.Parse(sched.spec); err != nil {
+			return common.NewError(sched.name+":", err)
+		}
+	}
+
 	if err := checkIPOrCIDRList(s.TrustedProxyCIDRs, "trusted proxy CIDR is not valid:"); err != nil {
 		return err
 	}

+ 36 - 0
internal/web/schedule_test.go

@@ -0,0 +1,36 @@
+package web
+
+import (
+	"testing"
+	"time"
+
+	"github.com/robfig/cron/v3"
+)
+
+// A five-field crontab used to be refused by the WithSeconds() scheduler, so the
+// Telegram/Discord report and LDAP sync were never scheduled at all.
+func TestScheduleSettingJob(t *testing.T) {
+	from := time.Date(2026, 10, 11, 7, 0, 0, 0, time.UTC)
+	cases := []struct {
+		name, spec string
+		want       time.Time
+	}{
+		{"five-field crontab", "30 8 * * *", time.Date(2026, 10, 11, 8, 30, 0, 0, time.UTC)},
+		{"six-field crontab", "15 30 8 * * *", time.Date(2026, 10, 11, 8, 30, 15, 0, time.UTC)},
+		{"empty falls back", "  ", time.Date(2026, 10, 12, 0, 0, 0, 0, time.UTC)},
+		{"invalid falls back", "every morning", time.Date(2026, 10, 12, 0, 0, 0, 0, time.UTC)},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			sched := cron.New(cron.WithSeconds(), cron.WithLocation(time.UTC))
+			id := scheduleSettingJob(sched, "test job", c.spec, "@daily", cron.FuncJob(func() {}))
+			entry := sched.Entry(id)
+			if entry.Schedule == nil {
+				t.Fatalf("spec %q: job was not scheduled", c.spec)
+			}
+			if got := entry.Schedule.Next(from); !got.Equal(c.want) {
+				t.Errorf("spec %q: next run %v, want %v", c.spec, got, c.want)
+			}
+		})
+	}
+}

+ 29 - 34
internal/web/web.go

@@ -24,6 +24,7 @@ import (
 	"github.com/mhsanaei/3x-ui/v3/internal/mtproto"
 	"github.com/mhsanaei/3x-ui/v3/internal/tuic"
 	"github.com/mhsanaei/3x-ui/v3/internal/util/common"
+	"github.com/mhsanaei/3x-ui/v3/internal/util/cronspec"
 	"github.com/mhsanaei/3x-ui/v3/internal/util/sys"
 	"github.com/mhsanaei/3x-ui/v3/internal/web/controller"
 	"github.com/mhsanaei/3x-ui/v3/internal/web/job"
@@ -385,13 +386,9 @@ func (s *Server) startTask(restartXray bool, loc *time.Location) {
 
 	// LDAP sync scheduling
 	if ldapEnabled, _ := s.settingService.GetLdapEnable(); ldapEnabled {
-		runtime, err := s.settingService.GetLdapSyncCron()
-		if err != nil || runtime == "" {
-			runtime = "@every 1m"
-		}
-		j := job.NewLdapSyncJob()
+		runtime, _ := s.settingService.GetLdapSyncCron()
 		// job has zero-value services with method receivers that read settings on demand
-		_, _ = s.cron.AddJob(runtime, j)
+		scheduleSettingJob(s.cron, "LDAP sync", runtime, "@every 1m", job.NewLdapSyncJob())
 	}
 
 	// Telegram-bot–dependent jobs: periodic stats report + callback-hash cleanup.
@@ -399,16 +396,9 @@ func (s *Server) startTask(restartXray bool, loc *time.Location) {
 	if (err == nil) && isTgbotenabled {
 		runtime, err := s.settingService.GetTgbotRuntime()
 		if err != nil {
-			logger.Warningf("Add NewStatsNotifyJob: failed to load runtime: %v; using default @daily", err)
-			runtime = "@daily"
-		} else if strings.TrimSpace(runtime) == "" {
-			logger.Warning("Add NewStatsNotifyJob runtime is empty, using default @daily")
-			runtime = "@daily"
-		}
-		logger.Infof("Tg notify enabled,run at %s", runtime)
-		if _, err = s.cron.AddJob(runtime, job.NewStatsNotifyJob()); err != nil {
-			logger.Warningf("Add NewStatsNotifyJob: failed to schedule runtime %q: %v", runtime, err)
+			logger.Warningf("Telegram notify: failed to load runtime: %v", err)
 		}
+		scheduleSettingJob(s.cron, "Telegram notify", runtime, "@daily", job.NewStatsNotifyJob())
 
 		// check for Telegram bot callback query hash storage reset
 		_, _ = s.cron.AddJob(cadenceCheckHash, job.NewCheckHashStorageJob())
@@ -419,18 +409,9 @@ func (s *Server) startTask(restartXray bool, loc *time.Location) {
 	if (err == nil) && isDiscordEnabled {
 		runtime, err := s.settingService.GetDiscordRunTime()
 		if err != nil {
-			logger.Warningf("Add NewDiscordNotifyJob: failed to load runtime: %v; using default @daily", err)
-			runtime = "@daily"
-		} else if strings.TrimSpace(runtime) == "" {
-			logger.Warning("Add NewDiscordNotifyJob runtime is empty, using default @daily")
-			runtime = "@daily"
-		}
-		logger.Infof("Discord notify enabled, run at %s", runtime)
-		if entryID, err := s.cron.AddJob(runtime, job.NewDiscordNotifyJob(s.discordService)); err != nil {
-			logger.Warningf("Add NewDiscordNotifyJob: failed to schedule runtime %q: %v", runtime, err)
-		} else {
-			s.discordNotifyEntryID = entryID
+			logger.Warningf("Discord notify: failed to load runtime: %v", err)
 		}
+		s.discordNotifyEntryID = scheduleSettingJob(s.cron, "Discord notify", runtime, "@daily", job.NewDiscordNotifyJob(s.discordService))
 	}
 
 	// CPU monitor publishes cpu.high events; register it whenever any notifier
@@ -710,16 +691,10 @@ func (s *Server) start(restartXray bool, startTgBot bool) (err error) {
 			return
 		}
 		runtime, err := s.settingService.GetDiscordRunTime()
-		if err != nil || strings.TrimSpace(runtime) == "" {
-			runtime = "@daily"
-		}
-		entryID, err := s.cron.AddJob(runtime, job.NewDiscordNotifyJob(s.discordService))
 		if err != nil {
-			logger.Warningf("Reload Discord notify: failed to schedule runtime %q: %v", runtime, err)
-		} else {
-			s.discordNotifyEntryID = entryID
-			logger.Infof("Discord notify rescheduled, run at %s", runtime)
+			logger.Warningf("Discord notify: failed to load runtime: %v", err)
 		}
+		s.discordNotifyEntryID = scheduleSettingJob(s.cron, "Discord notify", runtime, "@daily", job.NewDiscordNotifyJob(s.discordService))
 
 		if s.discordGateway != nil && !s.discordGateway.IsRunning() {
 			_ = s.discordGateway.Start(s.ctx)
@@ -859,3 +834,23 @@ func (s *Server) GetWSHub() any {
 func (s *Server) RestartXray() error {
 	return s.xrayService.RestartXray(true)
 }
+
+// scheduleSettingJob schedules j on an admin-typed spec, running it on fallback
+// when the spec is empty or a legacy value that no longer parses.
+func scheduleSettingJob(c *cron.Cron, name, spec, fallback string, j cron.Job) cron.EntryID {
+	if strings.TrimSpace(spec) == "" {
+		logger.Warningf("%s: schedule is empty, using default %s", name, fallback)
+		spec = fallback
+	}
+	schedule, err := cronspec.Parse(spec)
+	if err != nil {
+		logger.Warningf("%s: %v; using default %s", name, err, fallback)
+		spec = fallback
+		if schedule, err = cronspec.Parse(fallback); err != nil {
+			logger.Errorf("%s: default schedule %q: %v", name, fallback, err)
+			return 0
+		}
+	}
+	logger.Infof("%s scheduled, run at %s", name, spec)
+	return c.Schedule(schedule, j)
+}