Unify periodic work behind a single scheduler

[Aloento]

Summary

Replaces the checker's own goroutine/ticker and the notification worker's internal sweep ticker with a single scheduler that owns all periodic tasks. Each task round runs under one advisory lock taken at the scheduler level, so only one replica executes a given task at a time.

Changes

• internal/scheduler (new): Task/Register/Run/Stop. Stop cancels the schedule and waits for in-flight tasks with a bounded deadline. Lock keys live here in the SD3 reserved range: scan=9001,
  otify_sweep=9002,
  etention=9003.
• checker: removed Run/Shutdown, the internal WithAdvisoryLock wrap, and scanLockKey. Check(ctx) is now the body of the scan task (2min); the scan task calls Publisher().Notify() afterwards to wake the worker (idempotent).
• worker: removed the internal ime.NewTicker and the per-sweep retention call. Run only handles the Notify signal and drains once at startup, so rows orphaned by a crashed replica are recovered without waiting for the first 5min sweep.
  unRetention is exported as RunRetention (24h task).
• app: no longer starts or cancels the worker; exposes Worker() so main owns the lifecycle (single owner, no double start/stop).
• main: starts the scheduler and worker; on SIGTERM: sched.Stop (waits for an in-flight scan) -> wait for the worker to finish its in-flight drain -> s.Shutdown (HTTP + metrics + DB.Close()). The pool is closed last, after all scheduled work has finished.

Verification

• go build ./..., go vet ./... - clean
• go test ./internal/... - all pass, including new scheduler unit tests (fake WithAdvisoryLock via a narrow locker interface, so no Docker needed): task fires on interval, ErrLockBusy skips the round without error, Stop waits for an in-flight task, a failing task does not stop the schedule
• ests/ integration suite: compiles (go test -c ./tests/); not run locally (no Docker)
• golangci-lint: not usable on this machine (Go 1.27 export-data mismatch in the installed binary, pre-existing, affects all packages)

Rollback

Revert to the PR 1 shape: checker goroutine + worker internal ticker.

[ecosquad-autoreview]

Summary

Replaces the checker goroutine + worker ticker with a single scheduler that owns all periodic tasks under one advisory lock per task. The design is sound and the lifecycle in cmd/main.go is largely correct (scheduler stopped before pool close, worker drained before pool close, lock keys centralized in internal/scheduler). Two issues in the shutdown path are critical enough to block merging.

Critical

1. Check(ctx) ignores its context — the shutdown deadline is unenforceable

internal/checker/checker.go L31–48: Check(ctx context.Context) spawns two goroutines that call CheckMaintenance() / CheckInfoEvents(), neither of which accepts a context; maintenance.go uses context.Background() for WithTx (L99–108) and GetMaintenances/GetIncident are context-free. So:

• The ctx passed by the scheduler (internal/scheduler/scheduler.go L69, WithAdvisoryLock(ctx, …)) is never observable inside the scan. Scheduler.Stop (L86–99) waits up to taskStopTimeout = 30 s (cmd/main.go L23) for in-flight tasks, but a running scan cannot be signalled to stop.
• Worse: when Stop times out, main proceeds to s.Shutdown(shutdownCtx) (cmd/main.go L88–91), which calls a.DB.Close(). The still-running scan goroutines then hit sql: database is closed — the exact thing the comment at cmd/main.go L72–74 ("the pool must stay open until in-flight tasks finish") claims to guarantee. The bounded wait converts what used to be a hang into a corrupted-pool shutdown.

Fix: thread ctx through CheckMaintenance(ctx)/CheckInfoEvents(ctx) into the DB calls (GetMaintenances(ctx), GetIncident(ctx), WithTx(ctx, …)), and have Check return early / stop spawning work when ctx.Err() != nil. As an extra safety net, in Scheduler.Stop treat a timeout as fatal (or keep waiting unbounded) rather than returning while tasks are still running, so the pool is never closed under in-flight tasks.

Warning

2. Scan error swallowed

cmd/main.go L44–48: the registered scan task ignores the return value of ch.Check(ctx) and always returns nil, so the scheduler's task failed log (scheduler.go L73) can never fire for the scan, and Publisher().Notify() is called even when the scan failed. Make Check return an error and let the task propagate it.

3. First scan delayed by one full interval

The old checker also fired on its first tick after 2 min, so no regression, but worth confirming this is intended: time.NewTicker means nothing runs at t=0. The worker does an immediate drain at startup (internal/notification/worker.go L78–80), so the asymmetry is defensible — but if a scan at startup is desired, run Fn once before the ticker loop in scheduler.Run/runTask.

Suggestion

• internal/scheduler/scheduler.go L54: Run stores s.cancel without synchronization; fine as-is since it's called once before any Stop, but a comment noting the single-call contract would prevent a future misuse (second Run/Stop race).
• CI is still queued (go-test, build, golangci-lint) — the author's local go build/go test claims are plausible but unverified by CI at review time; please confirm the checks pass before merging.

Verdict

Request changes: the context-propagation gap (issue 1) means the shutdown path can close the database pool while a scan is in flight, which is a data-integrity risk on every slow-scan SIGTERM. Fix that (and ideally issue 2) and this is good to approve.

[ecosquad-autoreview]

Summary

This PR replaces the checker's own goroutine/ticker and the notification worker's internal sweep ticker with a single internal/scheduler that owns all periodic tasks, taking one advisory lock per task round at the scheduler level. The lifecycle is re-owned by main (scheduler + worker start/stop), with a bounded shutdown that stops the scheduler, waits for the worker drain, then closes the pool last.

Overall the design is clean and the test coverage in internal/scheduler/scheduler_test.go is good (interval firing, busy-lock skip, Stop-waiting-for-in-flight, failing-task resilience). I have one warning-level concern about shutdown boundedness and a few smaller suggestions. The CI check job is failing, which must be investigated before merge.

Findings

Critical

None found.

Warning

1. CI check job is failing (no file — pipeline status).
   The PR description says go build, go vet, and go test ./internal/... are clean, and the build/go-test/go-test-acc/CodeQL checks all pass, but the check job reports failure. Since I cannot run builds/tests in this environment, I cannot confirm it is unrelated. Fix: before merging, confirm the check failure is not caused by this PR (e.g. a lint/golden-file/new-test expectation). If it is, address it; if it is a pre-existing flake, note it and re-run.

2. Scan task may exceed the bounded stop deadline if CheckMaintenance/CheckInfoEvents ignore ctx.
   internal/scheduler/scheduler.go runTask runs the round under the signal-derived ctx, and cmd/main.go sched.Stop(stopCtx) waits up to taskStopTimeout (30s) for in-flight tasks. internal/checker/checker.go Check(ctx) only checks ctx.Err() at entry; it then spawns CheckMaintenance and CheckInfoEvents without passing the context. If those do network/DB work with context.Background() (the previous code wrapped the scan in WithAdvisoryLock(context.Background(), …)), a slow round cannot be interrupted by shutdown. Stop will then log "timed out waiting for scheduled tasks" and s.Shutdown proceeds to DB.Close() while scan goroutines may still hold connections — the exact scenario the PR comment says it must avoid ("a scan round holds a dedicated connection, so the pool must stay open until in-flight tasks finish").
   Fix: thread ctx (or at least a cancellable context) into CheckMaintenance/CheckInfoEvents so an in-flight scan is bounded during shutdown, or add a comment + a hard cap confirming those calls are short and context-respecting. Verify the 30s stop deadline is comfortably longer than the worst-case scan time.

Suggestion

3. Scheduler.cancel is stored as mutable state set in Run (internal/scheduler/scheduler.go).
   Run writes s.cancel and Stop reads it. This is safe in the current main flow (single goroutine, Run before Stop, happens-before), but it is a data race if Run/Stop were ever called concurrently. Consider making Stop tolerate a nil cancel (already done) and documenting that Run must not be called concurrently with Stop, or guarding with a mutex.

4. Worker wait reuses the same stopCtx deadline as the scheduler wait (cmd/main.go).
   sched.Stop(stopCtx) may consume the full 30s, after which the worker wait on <-stopCtx.Done() immediately times out and logs a warning even though the worker would have finished moments later. Consider a fresh context.WithTimeout for the worker wait, or a combined total budget, so the two phases don't share a single countdown.

5. s.Publisher().Notify() inside the scan task (cmd/main.go).
   This is safe today (non-blocking buffered signal; Publisher.Notify is nil-guarded), but it couples the scheduler task to the app publisher. It works when notifications are disabled because the scan task still registers unconditionally. No change strictly required — just be aware that a future change to Notify blocking could stall the scan round.

Notes

• Lock key 9001 correctly moves from checker.scanLockKey to scheduler.KeyScan; 9002/9003 are new and documented in the SD3 reserved range.
• Removing the worker's internal ticker and driving Drain via the scheduler's notify_sweep (5min) plus the startup drain is a reasonable simplification; the advisory lock around Drain is harmless since claim/lease already makes it idempotent across replicas.
• Rollback path is clearly described.

Verdict

request_changes — primarily because the check CI job is failing and needs to be confirmed as unrelated, and the scan task's lack of context propagation may defeat the bounded-shutdown guarantee the PR is trying to establish. Both are quick to resolve.

[Aloento]

@ecosquad-autoreview review

[ecosquad-autoreview]

Summary

Clean refactor that centralizes the checker and notification worker's periodic work into a new internal/scheduler that owns the advisory lock per task round. The lifecycle is moved out of App and into main (single owner), the pool is closed last, and Stop/waitWorker gate the close on in-flight work. The logic reads correctly and the new scheduler unit tests cover the interesting cases (interval fire, ErrLockBusy skip, Stop waits for in-flight, errors don't kill the schedule). Unit tests (go-test) are green in CI; the integration suite and build were still running, so I could not confirm those.

Overall I would merge this. No correctness or security bugs found. A few notes below.

Findings

Suggestion — redundant Notify() in the scan task

cmd/main.go:70 calls s.Publisher().Notify() after ch.Check(ctx). But the scan halves already wake the worker themselves: internal/checker/maintenance.go:118 calls ch.notifier.Notify() after each commit, and CheckInfoEvents does the same. Since Worker.Notify is a non-blocking buffered(1) send, the extra call is harmless and idempotent, but it's dead weight. Either drop it (the worker is already woken by the commit path) or keep it with a one-line comment that it's an intentional safety net for the case where a scan round commits no rows — as written the PR description calls it "idempotent" but the code comment doesn't explain why it's needed.

Suggestion — Scheduler.Run double-call is only documented, not guarded

internal/scheduler/scheduler.go:50-60 — Run overwrites s.cancel and adds goroutines every call, so a second call leaks the first set of goroutines and cancels the wrong context. It's documented ("must be called at most once") and main only calls it once, so this is fine in practice, but a sync.Once (or a running flag returning early) would make the contract self-enforcing rather than relying on the caller.

Suggestion — Check ignores ctx mid-round (documented, worth a follow-up)

internal/checker/checker.go:28 — Check only checks ctx.Err() before starting; the two scan goroutines (CheckMaintenance/CheckInfoEvents) don't take a context at all. This is explicitly documented and is safe with the new shutdown sequence (the pool is only closed after Stop waits for the scan to return), so it's not a bug. But it means a shutdown can be blocked for the full duration of a slow scan (bounded only by taskStopTimeout=30s, after which the pool is left open and the process exits). Consider threading ctx through the two scans in a follow-up so cancellation is actually observed.

Suggestion — scheduler and worker waits are sequential (up to 60s)

cmd/main.go shutdown runs stopScheduler(sched) (up to 30s) and then waitWorker(workerDone, logger) (up to 30s) back-to-back. The comment says the two use "independent deadlines" — they do, but because they're sequential the worst-case shutdown is ~60s, which can exceed the pod termination grace period if both the scan and the worker drain are slow. If the grace period is <60s consider running the two waits concurrently (each with its own 30s deadline) or shortening the individual timeouts. Minor, and only a problem if both stall simultaneously.

No changes requested.