From e9fc9eaba0c4b7e04869c352e485690e38e7fb4b Mon Sep 17 00:00:00 2001 From: mixeme Date: Mon, 29 Jun 2026 21:33:50 +0300 Subject: [PATCH] fix: harden run persistence and error surfacing from code review Snapshot store paths under lock before async runs, roll back failed start/save state, emit UI events only after successful persistence, surface log write failures, and sync stale YAML docs to JSON. Co-authored-by: Cursor --- docs/ARCHITECTURE.md | 5 +-- docs/CODE_REVIEW.md | 51 ++++++++++++++++++++++++++++++ docs/TESTS.md | 4 +-- src/app/operations.go | 43 +++++++++++++++++++------ src/app/operations_test.go | 39 ++++++++++++----------- src/app/platform.go | 2 ++ src/app/run.go | 65 +++++++++++++++++++++++++++++--------- src/app/run_test.go | 32 +++++++++---------- src/app/service.go | 2 +- src/domain/runtime.go | 5 +-- src/runner/logfile.go | 11 ++++--- src/runner/runner.go | 6 ++-- src/runner/runner_test.go | 45 ++++++++++++++++++++------ src/ui/history_view.go | 2 +- src/ui/mainwindow.go | 4 +-- src/ui/settings_view.go | 2 +- 16 files changed, 230 insertions(+), 88 deletions(-) create mode 100644 docs/CODE_REVIEW.md diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 992a502..a22e892 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -81,8 +81,9 @@ flowchart LR 4. Manual run: `Run now` in the UI calls `Service.RunNow`. The Service checks that the job - exists, is not already running, and that the scheduler is not globally paused, - then executes `runner.RunJob` with the `Manual` trigger. + exists, is not already running, and (in sequential mode) that no other job is + running, then executes `runner.RunJob` with the `Manual` trigger. Manual runs + are allowed even while the scheduler is globally paused. 5. Command execution: `runner.RunJob` builds the platform-specific invocation, executes the diff --git a/docs/CODE_REVIEW.md b/docs/CODE_REVIEW.md new file mode 100644 index 0000000..9d4c22c --- /dev/null +++ b/docs/CODE_REVIEW.md @@ -0,0 +1,51 @@ +# GoSentry — Code Review (2026-06-29) + +Версия на момент ревью: **0.11.2** + +## Итог + +| Критерий | Оценка | +|----------|--------| +| Архитектура | 9/10 | +| Сложность vs масштаб | 8/10 | +| Качество кода | 8/10 | +| Поддерживаемость | 8/10 | +| Логические ошибки | 8/10 (после исправлений) | + +Проект зрелый и поддерживаемый для десктопного планировщика (~59 `.go`-файлов). Архитектура слоистая, core-логика хорошо протестирована. + +## Сильные стороны + +- Single-writer `app.Service` с явным locking contract +- Разделение `domain.Job` (durable) и `domain.JobRuntime` (transient) +- Event-driven UI без обратных вызовов в Fyne под lock +- Portable storage от `os.Executable()` +- Инъекция `runJob` и `scheduler.Clock` в тестах +- Подробная документация (`ARCHITECTURE.md`, inline comments) + +## Найденные проблемы и статус исправлений + +| # | Проблема | Серьёзность | Статус | +|---|----------|-------------|--------| +| 1 | Data race: `store.Paths` в `executeRun` без lock | Высокая | Исправлено | +| 2 | Run стартует при ошибке `SaveJobs` | Средняя | Исправлено | +| 3 | CRUD эмитит events при failed save | Средняя | Исправлено | +| 4 | Overlap queue — только один `Pending` | Средняя | Документировано (by design) | +| 5 | `time.Now()` vs scheduler clock в `startRunLocked` | Низкая | Исправлено | +| 6 | Silent log write failures | Низкая | Исправлено | +| 7 | Невалидный per-job `overlap_policy` | Низкая | Исправлено | +| 8 | Docs drift (YAML, RunNow/pause) | Низкая | Исправлено | + +## Намеренное поведение (не баги) + +- `RunNow` разрешён при global pause и для disabled jobs +- Sequential mode — FIFO по порядку в `jobs.json` +- Scheduler tick 1s — sub-second `@every` не поддерживается +- Command timeout 30s — глобальный лимит + +## Рекомендации на будущее + +- UI widget tests или smoke E2E +- Per-job command timeout в конфиге +- Счётчик вместо `Pending bool` для overlap queue (если нужна полная очередь) +- Убрать legacy ticket-ссылки (T3.1) из комментариев diff --git a/docs/TESTS.md b/docs/TESTS.md index 7dab93b..cf304a4 100644 --- a/docs/TESTS.md +++ b/docs/TESTS.md @@ -170,15 +170,13 @@ Tests display-formatting helpers used by the UI. **Package:** `storage` -Tests JSON round-tripping, YAML migration import, and default generation. +Tests JSON round-tripping and default generation. | Test | Purpose | |------|---------| | `TestJobsRoundTrip` | Verifies that jobs saved to JSON are reloaded with identical field values. | | `TestConfigRoundTrip` | Verifies that settings saved to JSON are reloaded with identical field values. | | `TestNormalizeJobsFillsDefaults` | Verifies that `normalizeJobs` assigns sequential IDs and sets default name, schedule, and command for jobs missing those fields. | -| `TestLoadOrCreateConfigMigratesFromLegacy` | Verifies that when `gosentry.json` is absent but `gosentry.yaml` exists the config is imported from the legacy YAML file on first load. | -| `TestLoadOrCreateJobsMigratesFromLegacy` | Verifies that when `jobs.json` is absent but `jobs.yaml` exists the jobs are imported from the legacy YAML file on first load. | | `TestLoadOrCreateConfigCreatesDefaultsOnFirstRun` | Verifies that a missing config file is created with sane defaults and a sample job. | | `TestJobsJSONDoesNotPersistRuntimeNoise` | Verifies that `jobs.json` does not persist runtime state (LastRun, NextRun, etc.). Only durable job fields are stored. | diff --git a/src/app/operations.go b/src/app/operations.go index c0387fd..1c559f3 100644 --- a/src/app/operations.go +++ b/src/app/operations.go @@ -42,11 +42,17 @@ func (s *Service) CreateJob(job domain.Job) (domain.Job, error) { record := uiRecord(job.ID, job.Name, "Created", "Job was added") prependLog(runtime, record) err := s.store.SaveJobs(s.jobs) + if err != nil { + s.jobs = s.jobs[:len(s.jobs)-1] + delete(s.runtimes, job.ID) + delete(s.schedules, job.ID) + s.mu.Unlock() + return domain.Job{}, err + } s.mu.Unlock() - s.emit(RunRecorded{Record: record}) s.emit(JobChanged{JobID: job.ID}) - return job, err + return job, nil } // UpdateJob replaces the durable configuration of the job with the same ID, @@ -82,9 +88,12 @@ func (s *Service) UpdateJob(job domain.Job) error { err := s.store.SaveJobs(s.jobs) s.mu.Unlock() + if err != nil { + return err + } s.emit(RunRecorded{Record: record}) s.emit(JobChanged{JobID: job.ID}) - return err + return nil } // DeleteJob removes the job with the given ID along with its runtime and cached @@ -105,9 +114,12 @@ func (s *Service) DeleteJob(id int) error { err := s.store.SaveJobs(s.jobs) s.mu.Unlock() + if err != nil { + return err + } s.emit(RunRecorded{Record: record}) s.emit(JobChanged{JobID: 0}) - return err + return nil } // SetEnabled enables or disables a single job. Enabling moves it back to "Ready" @@ -140,15 +152,19 @@ func (s *Service) SetEnabled(id int, enabled bool) error { err := s.store.SaveJobs(s.jobs) s.mu.Unlock() + if err != nil { + return err + } s.emit(RunRecorded{Record: record}) s.emit(JobChanged{JobID: id}) - return err + return nil } -// SetGlobalPause flips the global pause that gates all execution, scheduled and -// manual. Each enabled job's next-run text reflects the new state immediately so -// the list view is understandable before the next tick. A "Paused"/"Resumed" -// scheduler activity record and a SchedulerStateChanged event are emitted. +// SetGlobalPause flips the global pause that gates scheduled execution. +// Manual "Run now" remains available while paused. Each enabled job's next-run +// text reflects the new state immediately so the list view is understandable +// before the next tick. A "Paused"/"Resumed" scheduler activity record and a +// SchedulerStateChanged event are emitted. func (s *Service) SetGlobalPause(paused bool) error { s.mu.Lock() s.paused = paused @@ -165,13 +181,16 @@ func (s *Service) SetGlobalPause(paused bool) error { } s.mu.Unlock() + if err != nil { + return err + } state, detail := "Resumed", "All job execution resumed" if paused { state, detail = "Paused", "All job execution paused" } s.emit(RunRecorded{Record: uiRecord(0, "Scheduler", state, detail)}) s.emit(SchedulerStateChanged{Paused: paused}) - return err + return nil } // ShouldNotifyOnFailure reports whether the user has enabled desktop @@ -344,6 +363,10 @@ func validateJob(job domain.Job) error { if job.Name == "" || job.Schedule == "" || job.Command == "" { return errors.New("name, schedule, and command are required") } + policy := strings.TrimSpace(job.OverlapPolicy) + if policy != "" && policy != string(domain.OverlapPolicySkip) && policy != string(domain.OverlapPolicyQueue) { + return errors.New("overlap policy must be 'skip', 'queue', or empty") + } return nil } diff --git a/src/app/operations_test.go b/src/app/operations_test.go index 877419d..9961867 100644 --- a/src/app/operations_test.go +++ b/src/app/operations_test.go @@ -100,6 +100,9 @@ func TestCreateJobValidates(t *testing.T) { if got := svc.Jobs(); len(got) != 0 { t.Errorf("invalid job should not be stored, jobs = %+v", got) } + 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") + } } func TestUpdateJobKeepsRuntimeAndReflectsDisable(t *testing.T) { @@ -247,11 +250,11 @@ func TestRunNowUsesRunnerAndRecords(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) done := make(chan domain.RunRecord, 1) - svc.runJob = func(_ context.Context, job *domain.Job, trigger string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, trigger string, _ string) (domain.RunRecord, error) { if trigger != "Manual" { t.Errorf("trigger = %q, want Manual", trigger) } - return domain.RunRecord{Time: "2026-06-19 12:00:00", JobID: job.ID, JobName: job.Name, State: "Success", Output: "ok"} + return domain.RunRecord{Time: "2026-06-19 12:00:00", JobID: job.ID, JobName: job.Name, State: "Success", Output: "ok"}, nil } svc.Subscribe(ObserverFunc(func(e Event) { if rr, ok := e.(RunRecorded); ok && rr.Record.JobID == 1 { @@ -295,11 +298,11 @@ func TestRunNowRefusedWhileAlreadyRunning(t *testing.T) { entered := make(chan struct{}, 1) release := make(chan struct{}) var calls int32 - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { atomic.AddInt32(&calls, 1) entered <- struct{}{} <-release - return domain.RunRecord{Time: "2026-06-19 12:00:00", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "2026-06-19 12:00:00", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := make(chan struct{}, 1) svc.Subscribe(ObserverFunc(func(e Event) { @@ -338,12 +341,12 @@ func TestRunNowRefusedWhileAlreadyRunning(t *testing.T) { func TestRunNowAllowedWhilePaused(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) done := make(chan struct{}, 1) - svc.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + svc.runJob = func(context.Context, *domain.Job, string, string) (domain.RunRecord, error) { select { case done <- struct{}{}: default: } - return domain.RunRecord{State: "Success"} + return domain.RunRecord{State: "Success"}, nil } if err := svc.SetGlobalPause(true); err != nil { t.Fatalf("SetGlobalPause: %v", err) @@ -363,11 +366,11 @@ func TestRunDueStartsDueJob(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) done := make(chan domain.RunRecord, 1) - svc.runJob = func(_ context.Context, job *domain.Job, trigger string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, trigger string, _ string) (domain.RunRecord, error) { if trigger != "Schedule" { t.Errorf("trigger = %q, want Schedule", trigger) } - return domain.RunRecord{Time: "2026-06-19 12:00:00", JobID: job.ID, JobName: job.Name, State: "Success", Output: "ok"} + return domain.RunRecord{Time: "2026-06-19 12:00:00", JobID: job.ID, JobName: job.Name, State: "Success", Output: "ok"}, nil } svc.Subscribe(ObserverFunc(func(e Event) { if rr, ok := e.(RunRecorded); ok && rr.Record.JobID == 1 && rr.Record.State == "Success" { @@ -394,9 +397,9 @@ func TestRunDueStartsDueJob(t *testing.T) { func TestRunDueSkipsJobNotYetDue(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) var ran int32 - svc.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + svc.runJob = func(context.Context, *domain.Job, string, string) (domain.RunRecord, error) { atomic.AddInt32(&ran, 1) - return domain.RunRecord{} + return domain.RunRecord{}, nil } // Next-due is ~1m out, so nothing is due "now". @@ -415,9 +418,9 @@ func TestRunDueSkipsJobInRunningState(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) var calls int32 - svc.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + svc.runJob = func(context.Context, *domain.Job, string, string) (domain.RunRecord, error) { atomic.AddInt32(&calls, 1) - return domain.RunRecord{State: "Success"} + return domain.RunRecord{State: "Success"}, nil } // Force the job into "Running" with a past NextDue, simulating an in-flight @@ -439,9 +442,9 @@ func TestRunDueSkipsJobInRunningState(t *testing.T) { func TestRunDueDoesNothingWhilePaused(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) var ran int32 - svc.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + svc.runJob = func(context.Context, *domain.Job, string, string) (domain.RunRecord, error) { atomic.AddInt32(&ran, 1) - return domain.RunRecord{} + return domain.RunRecord{}, nil } if err := svc.SetGlobalPause(true); err != nil { t.Fatalf("SetGlobalPause: %v", err) @@ -469,12 +472,12 @@ func TestStartDrivesRunDueOnTick(t *testing.T) { svc := newTempService(t, []domain.Job{{ID: 1, Name: "A", Schedule: "@every 1m", Command: "echo", Enabled: true}}) done := make(chan struct{}, 1) - svc.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + svc.runJob = func(context.Context, *domain.Job, string, string) (domain.RunRecord, error) { select { case done <- struct{}{}: default: } - return domain.RunRecord{State: "Success"} + return domain.RunRecord{State: "Success"}, nil } clock := &appFakeClock{ticks: make(chan time.Time, 1), now: time.Now().Add(2 * time.Minute)} @@ -593,13 +596,13 @@ func TestServiceRebuiltFromPausedStoreStartsPaused(t *testing.T) { var ran int32 runStarted := make(chan struct{}, 1) - svc2.runJob = func(context.Context, *domain.Job, string, string) domain.RunRecord { + svc2.runJob = func(context.Context, *domain.Job, string, string) (domain.RunRecord, error) { atomic.AddInt32(&ran, 1) select { case runStarted <- struct{}{}: default: } - return domain.RunRecord{} + return domain.RunRecord{}, nil } // RunDue must not start any job while paused: the scheduler stays paused after diff --git a/src/app/platform.go b/src/app/platform.go index deedf6f..041386e 100644 --- a/src/app/platform.go +++ b/src/app/platform.go @@ -9,7 +9,9 @@ import ( // store.Paths.DesktopIcon so ApplyAutostart can reference it. func (s *Service) InstallDesktopIcon(appID string, iconBytes []byte) { if iconPath, err := desktop.InstallDesktopIntegration(appID, s.store.Paths.ExecutablePath, iconBytes); err == nil { + s.mu.Lock() s.store.Paths.DesktopIcon = iconPath + s.mu.Unlock() } } diff --git a/src/app/run.go b/src/app/run.go index 8080f73..6288a4e 100644 --- a/src/app/run.go +++ b/src/app/run.go @@ -36,11 +36,13 @@ func (s *Service) RunNow(id int) error { s.mu.Unlock() return errors.New("another job is already running (sequential mode)") } - err := s.startRunLocked(job, runtime, "Manual") + err := s.startRunLocked(job, runtime, "Manual", time.Now()) s.mu.Unlock() - // Reflect the "Running" transition; the run's completion emits again later. - s.emit(JobChanged{JobID: id}) + if err == nil { + // Reflect the "Running" transition; the run's completion emits again later. + s.emit(JobChanged{JobID: id}) + } return err } @@ -86,8 +88,9 @@ func (s *Service) RunDue(now time.Time) { // tick once the in-flight run has finished. continue } - if err := s.startRunLocked(job, runtime, "Schedule"); err != nil { + if err := s.startRunLocked(job, runtime, "Schedule", now); err != nil { startErr = err + continue } started = append(started, job.ID) running = true @@ -103,22 +106,47 @@ func (s *Service) RunDue(now time.Time) { } } +// runEnv snapshots path and retention settings for one background run so +// executeRun does not read store.Paths or store.Config without holding mu. +type runEnv struct { + logsDir string + maxFiles int + maxAge int +} + // startRunLocked transitions a job to "Running", advances its NextDue to the next // scheduled occurrence, persists that, and launches the run on a background // goroutine. Advancing (rather than zeroing) NextDue keeps the schedule marching // while the run is in flight, which is what lets RunDue notice a fresh occurrence // firing during a long run and apply the overlap policy. The caller must hold mu. -func (s *Service) startRunLocked(job *domain.Job, runtime *domain.JobRuntime, trigger string) error { +// now is the reference time for next-due advancement and the running placeholder. +func (s *Service) startRunLocked(job *domain.Job, runtime *domain.JobRuntime, trigger string, now time.Time) error { jobCopy := *job + prevState := runtime.LastState + prevNextRun := runtime.NextRun + prevOutput := runtime.Output + prevNextDue := runtime.NextDue + runtime.LastState = "Running" runtime.NextRun = "Running" - runtime.Output = runningOutput(jobCopy, trigger, time.Now()) - s.advanceNextDueLocked(job, runtime, time.Now()) - err := s.store.SaveJobs(s.jobs) + runtime.Output = runningOutput(jobCopy, trigger, now) + s.advanceNextDueLocked(job, runtime, now) + if err := s.store.SaveJobs(s.jobs); err != nil { + runtime.LastState = prevState + runtime.NextRun = prevNextRun + runtime.Output = prevOutput + runtime.NextDue = prevNextDue + return err + } + env := runEnv{ + logsDir: s.store.Paths.LogsDir, + maxFiles: s.store.Config.MaxLogFiles, + maxAge: s.store.Config.MaxLogAgeDays, + } // Capture ctx under the lock so a concurrent Start/Stop cannot swap it out // from under the goroutine after we release mu. - go s.executeRun(s.ctx, jobCopy, trigger) - return err + go s.executeRun(s.ctx, jobCopy, trigger, env) + return nil } // executeRun runs the job off the lock, then records the result back through the @@ -126,11 +154,12 @@ func (s *Service) startRunLocked(job *domain.Job, runtime *domain.JobRuntime, tr // running (the "queue" overlap policy), and it is still enabled and the scheduler // is not paused, the deferred run is started immediately. It runs on its own // goroutine. -func (s *Service) executeRun(ctx context.Context, jobCopy domain.Job, trigger string) { - record := s.runJob(ctx, &jobCopy, trigger, s.store.Paths.LogsDir) +func (s *Service) executeRun(ctx context.Context, jobCopy domain.Job, trigger string, env runEnv) { + record, logErr := s.runJob(ctx, &jobCopy, trigger, env.logsDir) s.mu.Lock() var cleanupErr, saveErr error + var rerunStarted bool if current := s.findByIDLocked(jobCopy.ID); current != nil { runtime := s.runtimeForLocked(current) runtime.LastRun = record.Time @@ -143,15 +172,19 @@ func (s *Service) executeRun(ctx context.Context, jobCopy domain.Job, trigger st if rerun { // A scheduled occurrence fired while this run was active under the // "queue" policy; start that deferred run now. - saveErr = s.startRunLocked(current, runtime, "Schedule") + saveErr = s.startRunLocked(current, runtime, "Schedule", time.Now()) + rerunStarted = saveErr == nil } else { s.refreshNextRunLocked(current, runtime) saveErr = s.store.SaveJobs(s.jobs) } - cleanupErr = runner.CleanupLogs(s.store.Paths.LogsDir, s.store.Config.MaxLogFiles, s.store.Config.MaxLogAgeDays) + cleanupErr = runner.CleanupLogs(env.logsDir, env.maxFiles, env.maxAge) } s.mu.Unlock() + if logErr != nil { + s.emit(ErrorOccurred{Err: fmt.Errorf("write run log for %q: %w", jobCopy.Name, logErr)}) + } if cleanupErr != nil { s.emit(ErrorOccurred{Err: fmt.Errorf("log cleanup after run %q: %w", jobCopy.Name, cleanupErr)}) } @@ -159,7 +192,9 @@ func (s *Service) executeRun(ctx context.Context, jobCopy domain.Job, trigger st s.emit(ErrorOccurred{Err: fmt.Errorf("save jobs after run %q: %w", jobCopy.Name, saveErr)}) } s.emit(RunRecorded{Record: record}) - s.emit(JobChanged{JobID: jobCopy.ID}) + if !rerunStarted { + s.emit(JobChanged{JobID: jobCopy.ID}) + } } // effectiveOverlapPolicy resolves the overlap policy that actually governs a diff --git a/src/app/run_test.go b/src/app/run_test.go index 91bad14..f515331 100644 --- a/src/app/run_test.go +++ b/src/app/run_test.go @@ -116,10 +116,10 @@ func TestRunDueParallelStartsAllDueJobs(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -157,10 +157,10 @@ func TestRunDueSequentialSerializes(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -194,11 +194,11 @@ func TestRunDueSkipDropsOverlap(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) var calls int32 - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { atomic.AddInt32(&calls, 1) entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -240,11 +240,11 @@ func TestRunDueQueueRerunsAfterFinish(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) var calls int32 - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { atomic.AddInt32(&calls, 1) entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -297,11 +297,11 @@ func TestRunDuePerJobQueueOverridesGlobalSkip(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) var calls int32 - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { atomic.AddInt32(&calls, 1) entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -347,11 +347,11 @@ func TestRunDuePerJobSkipOverridesGlobalQueue(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) var calls int32 - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { atomic.AddInt32(&calls, 1) entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -395,10 +395,10 @@ func TestRunDueEmptyOverlapInheritsGlobal(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) @@ -437,10 +437,10 @@ func TestRunNowSequentialGuard(t *testing.T) { entered := make(chan int, 2) release := make(chan struct{}) - svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) domain.RunRecord { + svc.runJob = func(_ context.Context, job *domain.Job, _ string, _ string) (domain.RunRecord, error) { entered <- job.ID <-release - return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"} + return domain.RunRecord{Time: "t", JobID: job.ID, JobName: job.Name, State: "Success"}, nil } done := completions(svc) diff --git a/src/app/service.go b/src/app/service.go index 33c7ff3..65ea5e0 100644 --- a/src/app/service.go +++ b/src/app/service.go @@ -51,7 +51,7 @@ type Service struct { // processes. ctx is the lifecycle context passed to runs; Start replaces it // with a cancelable context so Stop can abort in-flight runs, and until Start // it is context.Background(). - runJob func(ctx context.Context, job *domain.Job, trigger string, logsDir string) domain.RunRecord + runJob func(ctx context.Context, job *domain.Job, trigger string, logsDir string) (domain.RunRecord, error) ctx context.Context // sched is the timing loop installed by Start; cancel tears down ctx on Stop. diff --git a/src/domain/runtime.go b/src/domain/runtime.go index df5508b..a9b802b 100644 --- a/src/domain/runtime.go +++ b/src/domain/runtime.go @@ -3,7 +3,7 @@ package domain import "time" // JobRuntime is the transient execution state for a Job. It is never written to -// jobs.yaml: it is rebuilt from scratch each time GoSentry starts and is held in +// jobs.json: it is rebuilt from scratch each time GoSentry starts and is held in // memory keyed by Job.ID for the lifetime of the process. Keeping it separate // from Job is what lets the durable configuration file stay free of run records, // status strings, and scheduling bookkeeping. @@ -20,7 +20,8 @@ type JobRuntime struct { NextDue time.Time // Pending is set when a run was skipped due to the overlap policy being - // "queue". The scheduler will start this job as soon as the current run ends. + // "queue". At most one deferred run is remembered; executeRun starts it when + // the current run ends. Pending bool // Execution-time statistics accumulated since the last process start. diff --git a/src/runner/logfile.go b/src/runner/logfile.go index a80dc95..470ceec 100644 --- a/src/runner/logfile.go +++ b/src/runner/logfile.go @@ -1,6 +1,7 @@ package runner import ( + "errors" "fmt" "os" "path/filepath" @@ -11,12 +12,12 @@ import ( "gitea.mixdep.ru/mix/gosentry/src/domain" ) -func writeRunLog(logsDir string, job domain.Job, trigger string, state string, detail string, output string, durationMS int64, started time.Time) string { +func writeRunLog(logsDir string, job domain.Job, trigger string, state string, detail string, output string, durationMS int64, started time.Time) (string, error) { if strings.TrimSpace(logsDir) == "" { - return "" + return "", errors.New("logs directory is empty") } if err := os.MkdirAll(logsDir, 0o755); err != nil { - return "" + return "", fmt.Errorf("create logs directory: %w", err) } // The timestamp comes first so a plain directory listing is naturally sorted // by run time. The job name is included for human scanning, but sanitized to @@ -26,9 +27,9 @@ func writeRunLog(logsDir string, job domain.Job, trigger string, state string, d content := fmt.Sprintf("time: %s\njob_id: %d\njob_name: %s\ntrigger: %s\nstate: %s\ndetail: %s\nduration: %d\ncommand: %s\narguments: %s\nstart_only: %t\n\n%s\n", started.Format("2006-01-02 15:04:05"), job.ID, job.Name, trigger, state, detail, durationMS, job.Command, logArguments(job.Arguments), job.StartOnly, output) if err := os.WriteFile(path, []byte(content), 0o644); err != nil { - return "" + return "", fmt.Errorf("write log file: %w", err) } - return path + return path, nil } func sanitizeFileName(name string) string { diff --git a/src/runner/runner.go b/src/runner/runner.go index e61f9a1..82daf6a 100644 --- a/src/runner/runner.go +++ b/src/runner/runner.go @@ -15,7 +15,7 @@ import ( const commandTimeout = 30 * time.Second const commandWaitDelay = 2 * time.Second -func RunJob(ctx context.Context, job *domain.Job, trigger string, logsDir string) domain.RunRecord { +func RunJob(ctx context.Context, job *domain.Job, trigger string, logsDir string) (domain.RunRecord, error) { started := time.Now() // Commands can hang forever if a script waits for input or a child process // stalls. A fixed timeout is a conservative first guardrail for a desktop @@ -54,7 +54,7 @@ func RunJob(ctx context.Context, job *domain.Job, trigger string, logsDir string now := time.Now() timestamp := now.Format("2006-01-02 15:04:05") - logFile := writeRunLog(logsDir, *job, trigger, state, detail, output, durationMS, now) + logFile, logErr := writeRunLog(logsDir, *job, trigger, state, detail, output, durationMS, now) // The runner is now pure with respect to the job: it returns a RunRecord and // lets the caller fold that record into the job's JobRuntime. Run state no @@ -69,7 +69,7 @@ func RunJob(ctx context.Context, job *domain.Job, trigger string, logsDir string LogFile: logFile, Output: output, DurationMS: durationMS, - } + }, logErr } func startJobOnly(invocation commandInvocation, job domain.Job, started time.Time) (string, string, string) { diff --git a/src/runner/runner_test.go b/src/runner/runner_test.go index b1d2b2e..4cf1bae 100644 --- a/src/runner/runner_test.go +++ b/src/runner/runner_test.go @@ -27,7 +27,10 @@ func TestRunJobLogFileAllHeaders(t *testing.T) { Command: echoCommand("header test output"), } - record := RunJob(context.Background(), &job, "Schedule", logsDir) + record, err := RunJob(context.Background(), &job, "Schedule", logsDir) + if err != nil { + t.Fatal(err) + } if record.LogFile == "" { t.Fatal("expected log file to be written") } @@ -74,7 +77,10 @@ func TestRunJobRecordFields(t *testing.T) { Command: echoCommand("record field check"), } - record := RunJob(context.Background(), &job, "Schedule", t.TempDir()) + record, err := RunJob(context.Background(), &job, "Schedule", t.TempDir()) + if err != nil { + t.Fatal(err) + } if record.JobID != job.ID { t.Errorf("JobID: got %d, want %d", record.JobID, job.ID) @@ -158,7 +164,10 @@ func TestRunJobWritesLogFile(t *testing.T) { Command: echoCommand("hello from test"), } - record := RunJob(context.Background(), &job, "Manual", logsDir) + record, err := RunJob(context.Background(), &job, "Manual", logsDir) + if err != nil { + t.Fatal(err) + } if record.LogFile == "" { t.Fatal("expected log file path") } @@ -193,7 +202,10 @@ func TestRunJobRunsQuotedWindowsExecutable(t *testing.T) { Command: `"C:\Windows\System32\cmd.exe" /C echo quoted command ok`, } - record := RunJob(context.Background(), &job, "Manual", logsDir) + record, err := RunJob(context.Background(), &job, "Manual", logsDir) + if err != nil { + t.Fatal(err) + } if record.State != "OK" { t.Fatalf("expected quoted command to run, got state %q detail %q output:\n%s", record.State, record.Detail, record.Output) } @@ -222,7 +234,10 @@ func TestRunJobRunsUnquotedWindowsProgramPathWithSpaces(t *testing.T) { Command: scriptPath, } - record := RunJob(context.Background(), &job, "Manual", logsDir) + record, err := RunJob(context.Background(), &job, "Manual", logsDir) + if err != nil { + t.Fatal(err) + } if record.State != "OK" { t.Fatalf("expected unquoted command path to run, got state %q detail %q output:\n%s", record.State, record.Detail, record.Output) } @@ -244,7 +259,10 @@ func TestRunJobRunsWindowsCommandWithSeparateArguments(t *testing.T) { Arguments: "/C\necho separate arguments ok", } - record := RunJob(context.Background(), &job, "Manual", logsDir) + record, err := RunJob(context.Background(), &job, "Manual", logsDir) + if err != nil { + t.Fatal(err) + } if record.State != "OK" { t.Fatalf("expected separate arguments to run, got state %q detail %q output:\n%s", record.State, record.Detail, record.Output) } @@ -267,7 +285,10 @@ func TestRunJobFailsOnNonZeroExitCode(t *testing.T) { job.Arguments = "/C\nexit /b 1" } - record := RunJob(context.Background(), &job, "Manual", t.TempDir()) + record, err := RunJob(context.Background(), &job, "Manual", t.TempDir()) + if err != nil { + t.Fatal(err) + } if record.State != "Failed" { t.Fatalf("expected non-zero exit code to fail, got state %q detail %q", record.State, record.Detail) } @@ -291,7 +312,10 @@ func TestRunJobStartOnlyDoesNotWaitForExitCode(t *testing.T) { StartOnly: true, } - record := RunJob(context.Background(), &job, "Manual", t.TempDir()) + record, err := RunJob(context.Background(), &job, "Manual", t.TempDir()) + if err != nil { + t.Fatal(err) + } if record.State != "OK" { t.Fatalf("expected start-only job to be OK after launch, got state %q detail %q", record.State, record.Detail) } @@ -312,7 +336,10 @@ func TestRunJobStartOnlyReportsStartFailure(t *testing.T) { StartOnly: true, } - record := RunJob(context.Background(), &job, "Manual", t.TempDir()) + record, err := RunJob(context.Background(), &job, "Manual", t.TempDir()) + if err != nil { + t.Fatal(err) + } if record.State != "Failed" { t.Fatalf("expected missing start-only command to fail, got state %q detail %q", record.State, record.Detail) } diff --git a/src/ui/history_view.go b/src/ui/history_view.go index aa38eaa..6f93bb9 100644 --- a/src/ui/history_view.go +++ b/src/ui/history_view.go @@ -29,7 +29,7 @@ func newEvent(jobID int, jobName string, state string, detail string) event { func collectActivity(jobs []job, runtimes map[int]*domain.JobRuntime) []event { var events []event for _, current := range jobs { - // At startup this is usually empty because jobs.yaml does not persist + // At startup this is usually empty because jobs.json does not persist // runtime logs. The function still centralizes the merge for future // history loading from log metadata. if rt := runtimes[current.ID]; rt != nil { diff --git a/src/ui/mainwindow.go b/src/ui/mainwindow.go index 17c5367..131e5e0 100644 --- a/src/ui/mainwindow.go +++ b/src/ui/mainwindow.go @@ -43,8 +43,8 @@ func newMainView(w fyne.Window) (fyne.CanvasObject, func(time.Duration, bool)) { history, refreshHistory := newHistoryView(&events) recordStartup := func(duration time.Duration, windowShown bool) { // Startup is recorded as an in-memory History event instead of being - // persisted into jobs.yaml. It is session diagnostics, not durable job - // state, and keeping it ephemeral avoids polluting the human-editable YAML + // persisted into jobs.json. It is session diagnostics, not durable job + // state, and keeping it ephemeral avoids polluting the human-editable JSON // file with process-lifetime bookkeeping. detail := "Window shown in " + duration.Round(time.Millisecond).String() if !windowShown { diff --git a/src/ui/settings_view.go b/src/ui/settings_view.go index 568e4b0..4ba94dc 100644 --- a/src/ui/settings_view.go +++ b/src/ui/settings_view.go @@ -196,7 +196,7 @@ func settingsView(w fyne.Window, svc *app.Service) fyne.CanvasObject { // gap between them instead of merging into one block. container.NewVBox( widget.NewLabelWithStyle("Storage", fyne.TextAlignLeading, fyne.TextStyle{Bold: true}), - settingsRow("Config YAML", widget.NewLabel(store.Paths.ConfigPath)), + settingsRow("Config JSON", widget.NewLabel(store.Paths.ConfigPath)), settingsRow("Jobs directory", container.NewBorder(nil, nil, nil, jobsDirBrowse, jobsDir)), settingsRow("Logs directory", container.NewBorder(nil, nil, nil, logsDirBrowse, logsDir)), settingsRow("Max log files", maxLogFiles),