refactor(cron): use UTC as default timezone, log invalid TZ warning
Address reviewer feedback (yinwm): - Change hard-coded default from Asia/Shanghai to UTC, which is the industry standard for open-source projects (Kubernetes, AWS, etc.) - Add warning log when time.LoadLocation fails for a configured timezone to help users debug misconfiguration - Update tests to reflect UTC default
This commit is contained in:
parent
a8f2666b23
commit
c3281a536b
3 changed files with 20 additions and 15 deletions
|
|
@ -580,7 +580,7 @@ type WebToolsConfig struct {
|
||||||
type CronToolsConfig struct {
|
type CronToolsConfig struct {
|
||||||
ToolConfig ` envPrefix:"PICOCLAW_TOOLS_CRON_"`
|
ToolConfig ` envPrefix:"PICOCLAW_TOOLS_CRON_"`
|
||||||
ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout
|
ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout
|
||||||
DefaultTimezone string ` env:"PICOCLAW_TOOLS_CRON_DEFAULT_TIMEZONE" json:"default_timezone"` // default timezone for cron expressions, e.g. "UTC"
|
DefaultTimezone string ` env:"PICOCLAW_TOOLS_CRON_DEFAULT_TIMEZONE" json:"default_timezone"` // default timezone for cron expressions, e.g. "UTC"
|
||||||
}
|
}
|
||||||
|
|
||||||
type ExecConfig struct {
|
type ExecConfig struct {
|
||||||
|
|
|
||||||
|
|
@ -267,16 +267,18 @@ func (cs *CronService) computeNextRun(schedule *CronSchedule, nowMS int64) *int6
|
||||||
|
|
||||||
// Use gronx to calculate next run time
|
// Use gronx to calculate next run time
|
||||||
now := time.UnixMilli(nowMS)
|
now := time.UnixMilli(nowMS)
|
||||||
// Apply timezone: schedule.TZ > service default > "Asia/Shanghai"
|
// Apply timezone: schedule.TZ > service default > UTC
|
||||||
tz := schedule.TZ
|
tz := schedule.TZ
|
||||||
if tz == "" {
|
if tz == "" {
|
||||||
tz = cs.defaultTZ
|
tz = cs.defaultTZ
|
||||||
}
|
}
|
||||||
if tz == "" {
|
if tz == "" {
|
||||||
tz = "Asia/Shanghai"
|
tz = "UTC"
|
||||||
}
|
}
|
||||||
if loc, err := time.LoadLocation(tz); err == nil {
|
if loc, err := time.LoadLocation(tz); err == nil {
|
||||||
now = now.In(loc)
|
now = now.In(loc)
|
||||||
|
} else {
|
||||||
|
log.Printf("[cron] warning: failed to load timezone '%s': %v, using UTC", tz, err)
|
||||||
}
|
}
|
||||||
nextTime, err := gronx.NextTickAfter(schedule.Expr, now, false)
|
nextTime, err := gronx.NextTickAfter(schedule.Expr, now, false)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|
@ -326,7 +328,7 @@ func (cs *CronService) SetOnJob(handler JobHandler) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// SetDefaultTimezone sets the default timezone for cron expressions.
|
// SetDefaultTimezone sets the default timezone for cron expressions.
|
||||||
// If empty, falls back to "Asia/Shanghai".
|
// If empty, falls back to UTC.
|
||||||
func (cs *CronService) SetDefaultTimezone(tz string) {
|
func (cs *CronService) SetDefaultTimezone(tz string) {
|
||||||
cs.mu.Lock()
|
cs.mu.Lock()
|
||||||
defer cs.mu.Unlock()
|
defer cs.mu.Unlock()
|
||||||
|
|
|
||||||
|
|
@ -33,9 +33,9 @@ func TestComputeNextRun_CronTimezone(t *testing.T) {
|
||||||
wantHour: 1, // 9:00 CST = 1:00 UTC
|
wantHour: 1, // 9:00 CST = 1:00 UTC
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "empty TZ defaults to Asia/Shanghai",
|
name: "empty TZ defaults to UTC",
|
||||||
tz: "",
|
tz: "",
|
||||||
wantHour: 1, // should default to Asia/Shanghai
|
wantHour: 9, // should default to UTC
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "US/Eastern timezone",
|
name: "US/Eastern timezone",
|
||||||
|
|
@ -73,13 +73,16 @@ func TestComputeNextRun_CronTimezone(t *testing.T) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestComputeNextRun_DefaultTZ_NotUTC verifies that when TZ is empty,
|
// TestComputeNextRun_DefaultTZ_AppliesWhenSet verifies that when TZ is empty
|
||||||
// the computed next run differs from a pure UTC computation.
|
// and no service default is set, the computation uses UTC.
|
||||||
func TestComputeNextRun_DefaultTZ_NotUTC(t *testing.T) {
|
func TestComputeNextRun_DefaultTZ_AppliesWhenSet(t *testing.T) {
|
||||||
tmpDir := t.TempDir()
|
tmpDir := t.TempDir()
|
||||||
storePath := filepath.Join(tmpDir, "jobs.json")
|
storePath := filepath.Join(tmpDir, "jobs.json")
|
||||||
cs := NewCronService(storePath, nil)
|
cs := NewCronService(storePath, nil)
|
||||||
|
|
||||||
|
// Set a non-UTC default to verify it takes effect
|
||||||
|
cs.SetDefaultTimezone("Asia/Shanghai")
|
||||||
|
|
||||||
now := time.Date(2026, 3, 4, 2, 0, 0, 0, time.UTC) // 02:00 UTC = 10:00 CST
|
now := time.Date(2026, 3, 4, 2, 0, 0, 0, time.UTC) // 02:00 UTC = 10:00 CST
|
||||||
nowMS := now.UnixMilli()
|
nowMS := now.UnixMilli()
|
||||||
|
|
||||||
|
|
@ -93,12 +96,12 @@ func TestComputeNextRun_DefaultTZ_NotUTC(t *testing.T) {
|
||||||
t.Fatal("computeNextRun returned nil")
|
t.Fatal("computeNextRun returned nil")
|
||||||
}
|
}
|
||||||
|
|
||||||
// With empty TZ (default Asia/Shanghai), 9:00 CST already passed (it's 10:00 CST),
|
// With service default Asia/Shanghai, 9:00 CST already passed (it's 10:00 CST),
|
||||||
// so next run should be tomorrow 9:00 CST = today 01:00 UTC + 24h.
|
// so next run should be tomorrow 9:00 CST.
|
||||||
// With UTC, 9:00 UTC hasn't happened yet (it's 02:00 UTC), so next run = today 9:00 UTC.
|
// With explicit UTC, 9:00 UTC hasn't happened yet (it's 02:00 UTC).
|
||||||
// They must differ.
|
// They must differ, proving SetDefaultTimezone takes effect.
|
||||||
if *nextEmpty == *nextUTC {
|
if *nextEmpty == *nextUTC {
|
||||||
t.Errorf("empty TZ and UTC TZ produced same next run time, default TZ is not working")
|
t.Errorf("service default TZ (Asia/Shanghai) and UTC produced same next run time")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -115,7 +118,7 @@ func TestComputeNextRun_ServiceDefaultTZ(t *testing.T) {
|
||||||
now := time.Date(2026, 3, 4, 0, 0, 0, 0, time.UTC)
|
now := time.Date(2026, 3, 4, 0, 0, 0, 0, time.UTC)
|
||||||
nowMS := now.UnixMilli()
|
nowMS := now.UnixMilli()
|
||||||
|
|
||||||
// Schedule with empty TZ should use service default (US/Eastern), not Asia/Shanghai
|
// Schedule with empty TZ should use service default (US/Eastern), not UTC
|
||||||
scheduleEmpty := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: ""}
|
scheduleEmpty := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: ""}
|
||||||
scheduleCST := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: "Asia/Shanghai"}
|
scheduleCST := &CronSchedule{Kind: "cron", Expr: "0 9 * * *", TZ: "Asia/Shanghai"}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue