feat: make per-job timeout 0 mean "no timeout" instead of inherit
A per-job timeout now has three distinct states: unset inherits the global default, an explicit 0 means no timeout and does not inherit, and a positive value is the per-job limit. Job.TimeoutSeconds became *int so unset and 0 stay distinguishable in jobs.json. Also fixes the global default, which could not persist a 0. loadOrCreateConfig normalized DefaultTimeoutSeconds <= 0 back to 30 on every read of an existing gosentry.json, so "no timeout" only held until the next restart. The field is now written unconditionally (no omitempty) and read back as-is. Existing jobs and configs are unaffected: a job with no timeout_seconds still inherits, and a saved global default of 30 stays 30. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+9
-5
@@ -103,12 +103,16 @@ func DisplayOverlapPolicy(job domain.Job, globalPolicy domain.OverlapPolicy) str
|
||||
}
|
||||
|
||||
// DisplayTimeout formats a job's effective run timeout for the details panel.
|
||||
// When the job sets its own TimeoutSeconds it is shown as-is; when 0 (inherit),
|
||||
// the global default is shown with "(global default)" appended, mirroring
|
||||
// DisplayOverlapPolicy. A non-positive global default means no timeout at all.
|
||||
// When the job sets its own TimeoutSeconds it is shown as-is, with an explicit 0
|
||||
// rendered as "no timeout"; when unset (nil), the global default is shown with
|
||||
// "(global default)" appended, mirroring DisplayOverlapPolicy. A non-positive
|
||||
// global default means no timeout at all.
|
||||
func DisplayTimeout(job domain.Job, globalDefault int) string {
|
||||
if job.TimeoutSeconds > 0 {
|
||||
return fmt.Sprintf("%d s", job.TimeoutSeconds)
|
||||
if job.TimeoutSeconds != nil {
|
||||
if *job.TimeoutSeconds <= 0 {
|
||||
return "no timeout"
|
||||
}
|
||||
return fmt.Sprintf("%d s", *job.TimeoutSeconds)
|
||||
}
|
||||
if globalDefault <= 0 {
|
||||
return "no timeout (global default)"
|
||||
|
||||
@@ -165,11 +165,15 @@ func TestDisplayOverlapPolicy(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestDisplayTimeout(t *testing.T) {
|
||||
own := domain.Job{TimeoutSeconds: 45}
|
||||
own := domain.Job{TimeoutSeconds: domain.TimeoutSecondsPtr(45)}
|
||||
if got, want := DisplayTimeout(own, 30), "45 s"; got != want {
|
||||
t.Errorf("per-job timeout = %q, want %q", got, want)
|
||||
}
|
||||
inherit := domain.Job{TimeoutSeconds: 0}
|
||||
none := domain.Job{TimeoutSeconds: domain.TimeoutSecondsPtr(0)}
|
||||
if got, want := DisplayTimeout(none, 30), "no timeout"; got != want {
|
||||
t.Errorf("explicit per-job zero timeout = %q, want %q", got, want)
|
||||
}
|
||||
inherit := domain.Job{TimeoutSeconds: nil}
|
||||
if got, want := DisplayTimeout(inherit, 30), "30 s (global default)"; got != want {
|
||||
t.Errorf("inherited timeout = %q, want %q", got, want)
|
||||
}
|
||||
|
||||
@@ -367,8 +367,8 @@ func validateJob(job domain.Job) error {
|
||||
if policy != "" && policy != string(domain.OverlapPolicySkip) && policy != string(domain.OverlapPolicyQueue) {
|
||||
return errors.New("overlap policy must be 'skip', 'queue', or empty")
|
||||
}
|
||||
if job.TimeoutSeconds < 0 {
|
||||
return errors.New("timeout must be zero (inherit) or a positive number of seconds")
|
||||
if job.TimeoutSeconds != nil && *job.TimeoutSeconds < 0 {
|
||||
return errors.New("timeout must be zero (no timeout) or a positive number of seconds, or unset to inherit the global default")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -103,9 +103,13 @@ func TestCreateJobValidates(t *testing.T) {
|
||||
if _, err := svc.CreateJob(domain.Job{Name: "A", Schedule: "@every 1m", Command: "echo", OverlapPolicy: "invalid"}); err == nil {
|
||||
t.Error("expected error for invalid overlap policy")
|
||||
}
|
||||
if _, err := svc.CreateJob(domain.Job{Name: "A", Schedule: "@every 1m", Command: "echo", TimeoutSeconds: -1}); err == nil {
|
||||
if _, err := svc.CreateJob(domain.Job{Name: "A", Schedule: "@every 1m", Command: "echo", TimeoutSeconds: domain.TimeoutSecondsPtr(-1)}); err == nil {
|
||||
t.Error("expected error for negative per-job timeout")
|
||||
}
|
||||
// An explicit 0 is a valid choice ("no timeout"), not a rejected one.
|
||||
if _, err := svc.CreateJob(domain.Job{Name: "Zero", Schedule: "@every 1m", Command: "echo", TimeoutSeconds: domain.TimeoutSecondsPtr(0)}); err != nil {
|
||||
t.Errorf("explicit zero per-job timeout should be accepted: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateJobKeepsRuntimeAndReflectsDisable(t *testing.T) {
|
||||
|
||||
+7
-6
@@ -211,15 +211,16 @@ func (s *Service) effectiveOverlapPolicy(job *domain.Job) domain.OverlapPolicy {
|
||||
}
|
||||
|
||||
// effectiveTimeout resolves the run timeout that actually governs a job: the
|
||||
// job's own TimeoutSeconds when positive, otherwise the global
|
||||
// Config.DefaultTimeoutSeconds. A non-positive Job.TimeoutSeconds means "inherit
|
||||
// the global default", which is why normalizeJob leaves 0 rather than
|
||||
// job's own TimeoutSeconds whenever it is set — including an explicit 0, which
|
||||
// means "no timeout" and deliberately does not inherit — otherwise the global
|
||||
// Config.DefaultTimeoutSeconds. A nil Job.TimeoutSeconds means "inherit the
|
||||
// global default", which is why normalizeJob leaves it nil rather than
|
||||
// backfilling the configured value. A resolved duration of 0 means no timeout;
|
||||
// runner.RunJob treats it as "run without a deadline". The caller must hold mu.
|
||||
func (s *Service) effectiveTimeout(job *domain.Job) time.Duration {
|
||||
secs := job.TimeoutSeconds
|
||||
if secs <= 0 {
|
||||
secs = s.store.Config.DefaultTimeoutSeconds
|
||||
secs := s.store.Config.DefaultTimeoutSeconds
|
||||
if job.TimeoutSeconds != nil {
|
||||
secs = *job.TimeoutSeconds
|
||||
}
|
||||
return time.Duration(secs) * time.Second
|
||||
}
|
||||
|
||||
+12
-5
@@ -617,23 +617,30 @@ func TestRunDueQueueDrainSkippedWhenPaused(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestEffectiveTimeout verifies the inherit-or-override resolution: a zero
|
||||
// Job.TimeoutSeconds falls back to the global default, while a positive value
|
||||
// overrides it.
|
||||
// TestEffectiveTimeout verifies the three-state resolution: an unset (nil)
|
||||
// Job.TimeoutSeconds falls back to the global default, a positive value
|
||||
// overrides it, and an explicit 0 means "no timeout" without inheriting.
|
||||
func TestEffectiveTimeout(t *testing.T) {
|
||||
svc := newTempService(t, nil)
|
||||
svc.store.Config.DefaultTimeoutSeconds = 30
|
||||
|
||||
inherit := &domain.Job{TimeoutSeconds: 0}
|
||||
inherit := &domain.Job{TimeoutSeconds: nil}
|
||||
if got, want := svc.effectiveTimeout(inherit), 30*time.Second; got != want {
|
||||
t.Errorf("inherited timeout = %s, want %s", got, want)
|
||||
}
|
||||
|
||||
own := &domain.Job{TimeoutSeconds: 5}
|
||||
own := &domain.Job{TimeoutSeconds: domain.TimeoutSecondsPtr(5)}
|
||||
if got, want := svc.effectiveTimeout(own), 5*time.Second; got != want {
|
||||
t.Errorf("per-job timeout = %s, want %s", got, want)
|
||||
}
|
||||
|
||||
// An explicit per-job 0 must beat a positive global default rather than be
|
||||
// mistaken for "unset".
|
||||
none := &domain.Job{TimeoutSeconds: domain.TimeoutSecondsPtr(0)}
|
||||
if got, want := svc.effectiveTimeout(none), time.Duration(0); got != want {
|
||||
t.Errorf("explicit per-job zero timeout = %s, want %s (no timeout)", got, want)
|
||||
}
|
||||
|
||||
svc.store.Config.DefaultTimeoutSeconds = 0
|
||||
if got, want := svc.effectiveTimeout(inherit), time.Duration(0); got != want {
|
||||
t.Errorf("inherited timeout with no global default = %s, want %s (no timeout)", got, want)
|
||||
|
||||
Reference in New Issue
Block a user