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 <cursoragent@cursor.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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) из комментариев
|
||||
+1
-3
@@ -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. |
|
||||
|
||||
|
||||
Reference in New Issue
Block a user