Repository navigation
Make the checker safe for multi-replica deployments - #44
Conversation
- Drop the in-memory scan cursors (lastMntID/lastInfoID) and the active-event bookkeeping; every round now scans from the start, with idempotency coming from the existing status derivation. - Remove the now-unused after parameter from GetMaintenances, GetInfoEvents and getEventsByType. - Add db.WithAdvisoryLock: a non-blocking session-level advisory lock acquired and released on the same dedicated connection, returning the new ErrLockBusy sentinel when the lock is held. - Wrap the whole scan round in the advisory lock (key 9001, SD3 reserved range); a busy lock skips the round with a debug log. - The checker now reuses the app's database pool and notification publisher instead of opening its own pool and publisher; the manual wiring in main is removed. - Fix the checker shutdown race: Run/Shutdown now use a cancel function and a done channel, so repeated shutdowns no longer panic.
The new tests broke the check job: govet flagged the inner loop counter shadowing the outer one, and testifylint required require.ErrorIs for the error assertion. Both loops now use bare for-range so there is no counter to shadow, and the assertion uses require. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@ecosquad-autoreview review |
There was a problem hiding this comment.
Summary
This PR makes the checker correct under replicas > 1 by removing in-process scan cursors / active-event bookkeeping and wrapping each scan round in a non-blocking session-level Postgres advisory lock. It also reuses the app's DB pool and notification publisher, and fixes the Run/Shutdown lifecycle race. The overall design is sound: idempotency via status derivation, a dedicated-connection lock held/released on the same session, and context.WithoutCancel for the unlock. CI is green (build, go-test, go-test-acc, lint, CodeQL).
No correctness bugs that block merge. The following are worth addressing.
Warnings
1. The scan holds a dedicated pool connection for the entire round — internal/db/lock.go:24
WithAdvisoryLock calls db.sql.Conn(ctx) and keeps that single connection (and the advisory lock) held for the full duration of runScan(). The scan issues its own queries (GetMaintenances, GetInfoEvents, per-maintenance GetIncident, WithTx) against the same pool. So during every scan round the pool has one fewer connection available, and the lock + connection are held for the whole sweep.
- Why it matters: if
max_open_connsis small (e.g. 1, ormax_open_conns_per_pod * podstuned tightly), holding one connection for the scan duration while the rest of the app also needs connections can drive the pool toward exhaustion, and if the pool has zero spare capacity the scan's own queries block ondb.sql.Conn/the pool — a live-while-deadlock with the held connection. - Fix/suggestion: consider (a) bounding the lock-hold window with a
context.WithTimeoutso a runaway scan can't pin a connection indefinitely, and/or (b) documenting/ensuring the pool size leaves at least one spare connection for the locked scan. At minimum, add a comment onscanLockKey/WithAdvisoryLocknoting the connection-occupancy cost so a future pool re-tune doesn't trip over it.
2. Shutdown is a no-op if called before Run starts — internal/checker/checker.go:110-117
Shutdown reads ch.cancel under the lock and returns immediately if it's nil. Run only sets ch.cancel partway into its body (after the log line and context.WithCancel). If Shutdown happens to run before Run has assigned ch.cancel, it returns without stopping anything, and Run then proceeds to loop forever with no remaining way to cancel it.
- In
cmd/main.gothis ordering is unlikely (go ch.Run()then a signal-drivench.Shutdown()), so it's not a live bug today — but it contradicts the documented "safe to call without Run having started" and the "safe to call multiple times" guarantee. - Fix: make the lifecycle deterministic, e.g. create and store the cancel in
New(or use anatomic.Bool/started flag) soShutdownalways cancels a valid context regardless of whetherRunhas begun, and haveRunregister on that shared context.
Suggestions
context.Background()for the lock round —internal/checker/checker.go:44:Check()passes a freshcontext.Background(), so an in-flight scan cannot be interrupted during shutdown;Shutdownwill block on<-ch.doneuntil the whole sweep finishes. This is acceptable for a 2-minute scan, but pairing it with the timeout in Warning #1 would make shutdown bounded.Newno longer returns an error —cmd/main.go:32drops the error check accordingly. Fine now that the constructor can't fail, but be aware the failure surface (DB/publisher construction) now lives entirely inapp.New; nothing actionable, just noting the contract change is coherent.
Verified as correct
- Advisory lock acquired and released on the same dedicated connection (session-scoped semantics), with unlock via
context.WithoutCancelso it releases even afterctxcancellation (internal/db/lock.go:38-41). ErrLockBusysentinel +errors.Ishandling and the debug skip path are correct (internal/checker/checker.go:47-52,internal/db/errors.go).- Removal of
lastMntID/lastInfoIDandslices.Minis consistent acrosschecker.go,info.go, and the DB layer;GetMaintenances/GetInfoEvents/getEventsByTypeno longer take theafterparam (internal/db/info.go,maintenances.go,event_types.go). Shutdowndouble-call safety via the closeddonechannel is fine (second call's<-ch.donereturns immediately); thecancel()/close(ch.done)ordering inRunis race-free for the common path.
No manifest changes (replicas stays at 1), consistent with the description.
I could not run the test suite locally (no Docker/testcontainers in this environment); the new tests/advisory_lock_test.go is reported to run in CI and the relevant CI jobs (go-test-acc, go-test) are green.
* Make the checker safe for multi-replica deployments - Drop the in-memory scan cursors (lastMntID/lastInfoID) and the active-event bookkeeping; every round now scans from the start, with idempotency coming from the existing status derivation. - Remove the now-unused after parameter from GetMaintenances, GetInfoEvents and getEventsByType. - Add db.WithAdvisoryLock: a non-blocking session-level advisory lock acquired and released on the same dedicated connection, returning the new ErrLockBusy sentinel when the lock is held. - Wrap the whole scan round in the advisory lock (key 9001, SD3 reserved range); a busy lock skips the round with a debug log. - The checker now reuses the app's database pool and notification publisher instead of opening its own pool and publisher; the manual wiring in main is removed. - Fix the checker shutdown race: Run/Shutdown now use a cancel function and a done channel, so repeated shutdowns no longer panic. * fix(tests): clear the lint failures in the advisory lock tests The new tests broke the check job: govet flagged the inner loop counter shadowing the outer one, and testifylint required require.ErrorIs for the error assertion. Both loops now use bare for-range so there is no counter to shadow, and the assertion uses require.
Summary
Makes the backend correct under
replicas > 1by removing all in-process scheduling state from the checker. This is PR 1 of the cluster-readiness plan (issues #1-#3, #13-#14).Changes
lastMntID/lastInfoID) and the active-event bookkeeping (activeMaintenances/activeInfoEvents,trackActiveMaintenance,slices.Min). Every round now scans from the start; idempotency comes from the existing status derivation inprocessMaintenance/calculateInfoStatusHistory, so no newTransition*methods are introduced.afterparameter fromdb.GetMaintenances,db.GetInfoEventsanddb.getEventsByType(theIDGTEbranch is gone).db.WithAdvisoryLock(internal/db/lock.go): a non-blocking session-level advisory lock (pg_try_advisory_lock) acquired and released on the same dedicated connection, since advisory locks are session-scoped. The unlock runs even when the context is cancelled (context.WithoutCancel), and the connection is returned to the pool afterwards. Returns the newErrLockBusysentinel when the lock is held.9001, SD3 reserved range 9000-9099). A busy lock skips the round with a debug log. The lock is acquired at a single call site inCheck()so it can move to the unified scheduler in PR 4.checker.Newnow takes*db.DBand*notification.Publisher;Close/Shutdownno longer close the pool. The manualSetNotifywiring inmainis removed (the app already wires its publisher to the worker).checker.Shutdownrace (Replace Keycloak authentication with Zitadel OIDC #13):Run/Shutdownnow use a cancel function and a done channel, so repeated shutdowns no longer panic and there is no race withRun's<-done.Verification
go build ./...- passgo vet ./...- passgo test ./internal/...- passgolangci-lint run- only a pre-existinggoimportsissue ininternal/event/event.gothat is present onmain(unrelated to this change)tests/integration suite requires Docker/testcontainers, which is not available in this environment; the package compiles (go test -run NONE ./tests/...). NewWithAdvisoryLocktests (mutual exclusion, concurrent holders, distinct keys, release-after-cancel) are intests/advisory_lock_test.goand will run in CI.No manifest changes;
replicasstays at 1.