diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1a12c3e..447b9c6 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -131,10 +131,21 @@ in flight increments `JobRuntime.PendingRuns`. When the current run finishes, ### Per-job command timeout -`domain.Job` carries a `TimeoutSeconds` field (`json:"timeout_seconds,omitempty"`), -following the same inherit pattern as the overlap policy. `0` means inherit the -global `Config.DefaultTimeoutSeconds` (default **0**, i.e. no timeout); a -positive value overrides it for that job alone. `app.Service.effectiveTimeout` +`domain.Job` carries a `TimeoutSeconds *int` field +(`json:"timeout_seconds,omitempty"`), following the same inherit pattern as the +overlap policy. It is a **pointer** because the setting has three states that +must stay distinguishable on disk: + +| `Job.TimeoutSeconds` | jobs.json | Meaning | +| --- | --- | --- | +| `nil` | field absent | inherit `Config.DefaultTimeoutSeconds` | +| `0` | `"timeout_seconds": 0` | no timeout, does **not** inherit | +| `> 0` | `"timeout_seconds": 45` | per-job limit in seconds | + +The global `Config.DefaultTimeoutSeconds` (default **0**, i.e. no timeout) is +written unconditionally — no `omitempty` — for the same reason: `0` there is a +deliberate choice, not a missing value, and `storage.loadOrCreateConfig` must not +normalize it away. `app.Service.effectiveTimeout` resolves the effective duration under `mu` and `startRunLocked` snapshots it into `runEnv.timeout`. `runner.RunJob(ctx, job, trigger, logsDir, timeout)` takes the resolved duration as an argument, so the runner stays ignorant of the global diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 46b6e80..48e38ef 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,24 @@ All notable GoSentry changes are recorded in this file. +## Unreleased + +**Timeouts: 0 now means "no timeout" at both levels.** + +- The global **Default timeout** in Settings now defaults to `0`, meaning jobs + run to completion with no deadline instead of being killed after 30s. +- A per-job timeout of `0` now also means "no timeout" and no longer inherits + the global default. Leaving the job's timeout **empty** is what inherits. + `Job.TimeoutSeconds` became `*int` so the three states — unset, explicit 0, + and a positive limit — stay distinguishable in `jobs.json`. +- Fixed: a global default of `0` did not survive a restart. `gosentry.json` was + loaded with `0` treated as a missing value and silently reset to 30s, so the + setting only held for the current session. `default_timeout_seconds` is now + written unconditionally 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. + ## 0.13.0 - 2026-07-26 **Branded GoSentry color theme; Cancel/Defaults buttons in Settings.** diff --git a/docs/STANDARDS.md b/docs/STANDARDS.md index 2d6fc7d..f77f40c 100644 --- a/docs/STANDARDS.md +++ b/docs/STANDARDS.md @@ -18,8 +18,9 @@ in [ARCHITECTURE.md](ARCHITECTURE.md); test conventions in [TESTS.md](TESTS.md). - Sequential mode runs jobs FIFO by order in `jobs.json`. - Scheduler tick is 1s — sub-second `@every` intervals are not supported. - Command timeout defaults to no timeout globally (`Config.DefaultTimeoutSeconds` - = 0) and is overridable per job (`Job.TimeoutSeconds`, 0 = inherit the global - default). + = 0) and is overridable per job (`Job.TimeoutSeconds *int`: unset = inherit the + global default, 0 = no timeout, positive = seconds). Neither zero may be + normalized away on load — 0 is a value, not a missing field. - **History tab is session-only.** `JobRuntime.Logs` exists only in memory for the current process. Log files on disk feed aggregate statistics via `SeedStats` only. See [ARCHITECTURE.md](ARCHITECTURE.md). diff --git a/src/app/format.go b/src/app/format.go index 06e81c6..739be56 100644 --- a/src/app/format.go +++ b/src/app/format.go @@ -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)" diff --git a/src/app/format_test.go b/src/app/format_test.go index 00c8751..613bd6f 100644 --- a/src/app/format_test.go +++ b/src/app/format_test.go @@ -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) } diff --git a/src/app/operations.go b/src/app/operations.go index 56fed0b..2769f79 100644 --- a/src/app/operations.go +++ b/src/app/operations.go @@ -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 } diff --git a/src/app/operations_test.go b/src/app/operations_test.go index 208c904..b9f60bb 100644 --- a/src/app/operations_test.go +++ b/src/app/operations_test.go @@ -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) { diff --git a/src/app/run.go b/src/app/run.go index 036bc37..b00817c 100644 --- a/src/app/run.go +++ b/src/app/run.go @@ -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 } diff --git a/src/app/run_test.go b/src/app/run_test.go index c0b7ecc..bbc2d51 100644 --- a/src/app/run_test.go +++ b/src/app/run_test.go @@ -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) diff --git a/src/domain/config.go b/src/domain/config.go index 853e7c9..2e66bd2 100644 --- a/src/domain/config.go +++ b/src/domain/config.go @@ -53,10 +53,11 @@ type Config struct { NotifyOnFailure bool `json:"notify_on_failure,omitempty"` ExecutionMode ExecutionMode `json:"execution_mode,omitempty"` OverlapPolicy OverlapPolicy `json:"overlap_policy,omitempty"` - // DefaultTimeoutSeconds is the run timeout applied to jobs that do not set - // their own Job.TimeoutSeconds. 0 (the default) means no timeout: such jobs - // run to completion however long that takes. - DefaultTimeoutSeconds int `json:"default_timeout_seconds,omitempty"` + // DefaultTimeoutSeconds is the run timeout applied to jobs that leave their + // own Job.TimeoutSeconds unset. 0 (the default) means no timeout: such jobs + // run to completion however long that takes. It is written even when 0 — + // omitempty would hide a deliberate choice from the hand-editable config. + DefaultTimeoutSeconds int `json:"default_timeout_seconds"` Paused bool `json:"paused,omitempty"` // Theme selects the visual appearance. Empty is treated as ThemeDefault so // configs written before this field existed keep the original look. diff --git a/src/domain/job.go b/src/domain/job.go index 220f149..062bde3 100644 --- a/src/domain/job.go +++ b/src/domain/job.go @@ -15,9 +15,19 @@ type Job struct { StartOnly bool `json:"start_only,omitempty"` Enabled bool `json:"enabled"` OverlapPolicy string `json:"overlap_policy,omitempty"` - // TimeoutSeconds bounds how long a run may take before it is killed. 0 means - // "inherit the global Config.DefaultTimeoutSeconds", mirroring OverlapPolicy: - // normalizeJobs must leave 0 untouched rather than backfilling the default. - // The inherited global default may itself be 0, meaning no timeout at all. - TimeoutSeconds int `json:"timeout_seconds,omitempty"` + // TimeoutSeconds bounds how long a run may take before it is killed. It is a + // pointer so the three states stay distinguishable on disk: absent (nil) + // means "inherit the global Config.DefaultTimeoutSeconds", mirroring + // OverlapPolicy's empty string; an explicit 0 means "no timeout" and does + // not inherit; a positive value is the per-job limit in seconds. The + // inherited global default may itself be 0, also meaning no timeout. + // normalizeJobs must leave nil untouched rather than backfilling a value. + TimeoutSeconds *int `json:"timeout_seconds,omitempty"` +} + +// TimeoutSecondsPtr returns a pointer suitable for Job.TimeoutSeconds. It exists +// because nil (inherit) and an explicit 0 (no timeout) are different states, so +// callers cannot just assign an int. +func TimeoutSecondsPtr(seconds int) *int { + return &seconds } diff --git a/src/storage/store.go b/src/storage/store.go index 8ee55f3..e9b38dd 100644 --- a/src/storage/store.go +++ b/src/storage/store.go @@ -102,9 +102,10 @@ func loadOrCreateConfig(paths Paths) (domain.Config, error) { if config.OverlapPolicy == "" { config.OverlapPolicy = domain.OverlapPolicySkip } - if config.DefaultTimeoutSeconds <= 0 { - config.DefaultTimeoutSeconds = 30 - } + // DefaultTimeoutSeconds is deliberately not normalized: 0 is a meaningful + // value ("no timeout"), not a missing one, so backfilling it here would make + // the setting impossible to persist. Negative values are rejected by + // app.validateConfig before they can be saved. if config.Theme == "" { config.Theme = domain.ThemeDefault } diff --git a/src/storage/store_test.go b/src/storage/store_test.go index aba884a..3913766 100644 --- a/src/storage/store_test.go +++ b/src/storage/store_test.go @@ -180,6 +180,63 @@ func TestLoadOrCreateConfigCreatesDefaultsOnFirstRun(t *testing.T) { } } +// TestLoadOrCreateConfigKeepsZeroTimeoutOnReload guards the "0 = no timeout" +// setting against being normalized away when an existing gosentry.json is read +// back. Loading must not treat 0 as a missing value. +func TestLoadOrCreateConfigKeepsZeroTimeoutOnReload(t *testing.T) { + dir := t.TempDir() + paths := Paths{ + AppDir: dir, + ConfigPath: filepath.Join(dir, ConfigFileName), + } + + // First call writes the defaults (DefaultTimeoutSeconds = 0) to disk. + if _, err := loadOrCreateConfig(paths); err != nil { + t.Fatal(err) + } + // Second call takes the "file exists" branch, where normalization runs. + reloaded, err := loadOrCreateConfig(paths) + if err != nil { + t.Fatal(err) + } + if reloaded.DefaultTimeoutSeconds != 0 { + t.Errorf("reloaded DefaultTimeoutSeconds = %d, want 0 (no timeout)", reloaded.DefaultTimeoutSeconds) + } +} + +// TestJobTimeoutRoundTripsThreeStates pins the on-disk encoding that keeps +// "inherit" and "no timeout" distinguishable: nil is omitted entirely, while an +// explicit 0 is written and read back as a set value. +func TestJobTimeoutRoundTripsThreeStates(t *testing.T) { + jobs := []domain.Job{ + {ID: 1, Name: "Inherit", TimeoutSeconds: nil}, + {ID: 2, Name: "No timeout", TimeoutSeconds: domain.TimeoutSecondsPtr(0)}, + {ID: 3, Name: "Own", TimeoutSeconds: domain.TimeoutSecondsPtr(45)}, + } + + data, err := json.Marshal(domain.JobsFile{Jobs: jobs}) + if err != nil { + t.Fatal(err) + } + if want := `"timeout_seconds":0`; !strings.Contains(string(data), want) { + t.Fatalf("explicit zero timeout should be written as %s:\n%s", want, data) + } + + var got domain.JobsFile + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.Jobs[0].TimeoutSeconds != nil { + t.Errorf("unset timeout should stay nil, got %d", *got.Jobs[0].TimeoutSeconds) + } + if got.Jobs[1].TimeoutSeconds == nil || *got.Jobs[1].TimeoutSeconds != 0 { + t.Errorf("explicit zero timeout should survive the round trip, got %v", got.Jobs[1].TimeoutSeconds) + } + if got.Jobs[2].TimeoutSeconds == nil || *got.Jobs[2].TimeoutSeconds != 45 { + t.Errorf("per-job timeout should survive the round trip, got %v", got.Jobs[2].TimeoutSeconds) + } +} + func TestJobsJSONDoesNotPersistRuntimeNoise(t *testing.T) { // Job carries only durable configuration; runtime state lives in // domain.JobRuntime and is never marshalled. This guards against a future diff --git a/src/ui/job_dialog.go b/src/ui/job_dialog.go index 59728e2..1eba1d4 100644 --- a/src/ui/job_dialog.go +++ b/src/ui/job_dialog.go @@ -55,9 +55,9 @@ func showJobDialog(w fyne.Window, title string, current job, onSave func(job)) { } overlapSelect.SetSelected(overlapSelected) timeoutEntry := widget.NewEntry() - timeoutEntry.SetPlaceHolder("Empty = use global default") - if current.TimeoutSeconds > 0 { - timeoutEntry.SetText(strconv.Itoa(current.TimeoutSeconds)) + timeoutEntry.SetPlaceHolder("Empty = global default, 0 = no timeout") + if current.TimeoutSeconds != nil { + timeoutEntry.SetText(strconv.Itoa(*current.TimeoutSeconds)) } form := dialog.NewForm( @@ -89,16 +89,17 @@ func showJobDialog(w fyne.Window, title string, current job, onSave func(job)) { dialog.ShowError(fmt.Errorf("invalid schedule: %w", err), w) return } - // An empty timeout inherits the global default (0); any entry must be a + // An empty timeout inherits the global default (nil); an explicit 0 + // means "no timeout" and does not inherit; anything else must be a // positive whole number of seconds. - timeoutSeconds := 0 + var timeoutSeconds *int if trimmed := strings.TrimSpace(timeoutEntry.Text); trimmed != "" { parsed, err := strconv.Atoi(trimmed) - if err != nil || parsed <= 0 { - dialog.ShowError(fmt.Errorf("timeout must be a positive number of seconds, or empty to use the global default"), w) + if err != nil || parsed < 0 { + dialog.ShowError(fmt.Errorf("timeout must be 0 (no timeout) or a positive number of seconds, or empty to use the global default"), w) return } - timeoutSeconds = parsed + timeoutSeconds = domain.TimeoutSecondsPtr(parsed) } current.Name = strings.TrimSpace(name.Text) current.Folder = strings.TrimSpace(folderEntry.Text)