Skip to content

Make the checker safe for multi-replica deployments - #44

Merged
Aloento merged 2 commits into
mainfrom
feat/multi-replica-safety
Oct 4, 2026
Merged

Aloento merged 2 commits into
mainfrom
feat/multi-replica-safety

Conversation

@Aloento

@Aloento Aloento commented Oct 4, 2026

Copy link
Copy Markdown
Member

Summary

Makes the backend correct under replicas > 1 by removing all in-process scheduling state from the checker. This is PR 1 of the cluster-readiness plan (issues #1-#3, #13-#14).

Changes

  • Remove in-memory scan cursors (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 in processMaintenance / calculateInfoStatusHistory, so no new Transition* methods are introduced.
  • Drop the now-unused after parameter from db.GetMaintenances, db.GetInfoEvents and db.getEventsByType (the IDGTE branch is gone).
  • Add 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 new ErrLockBusy sentinel when the lock is held.
  • Wrap the whole scan round in the advisory lock (key 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 in Check() so it can move to the unified scheduler in PR 4.
  • Checker reuses the app's DB pool and notification publisher instead of opening its own pool and publisher. checker.New now takes *db.DB and *notification.Publisher; Close/Shutdown no longer close the pool. The manual SetNotify wiring in main is removed (the app already wires its publisher to the worker).
  • Fix the checker.Shutdown race (Replace Keycloak authentication with Zitadel OIDC #13): Run/Shutdown now use a cancel function and a done channel, so repeated shutdowns no longer panic and there is no race with Run's <-done.

Verification

  • go build ./... - pass
  • go vet ./... - pass
  • go test ./internal/... - pass
  • golangci-lint run - only a pre-existing goimports issue in internal/event/event.go that is present on main (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/...). New WithAdvisoryLock tests (mutual exclusion, concurrent holders, distinct keys, release-after-cancel) are in tests/advisory_lock_test.go and will run in CI.

No manifest changes; replicas stays at 1.

- 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.
ecosquad-autoreview[bot]

This comment was marked as outdated.

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>
@Aloento

Aloento commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

@ecosquad-autoreview review

@ecosquad-autoreview ecosquad-autoreview Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_conns is small (e.g. 1, or max_open_conns_per_pod * pods tuned 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 on db.sql.Conn/the pool — a live-while-deadlock with the held connection.
  • Fix/suggestion: consider (a) bounding the lock-hold window with a context.WithTimeout so 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 on scanLockKey/WithAdvisoryLock noting 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.go this ordering is unlikely (go ch.Run() then a signal-driven ch.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 an atomic.Bool/started flag) so Shutdown always cancels a valid context regardless of whether Run has begun, and have Run register on that shared context.

Suggestions

  • context.Background() for the lock round — internal/checker/checker.go:44: Check() passes a fresh context.Background(), so an in-flight scan cannot be interrupted during shutdown; Shutdown will block on <-ch.done until 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.
  • New no longer returns an error — cmd/main.go:32 drops the error check accordingly. Fine now that the constructor can't fail, but be aware the failure surface (DB/publisher construction) now lives entirely in app.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.WithoutCancel so it releases even after ctx cancellation (internal/db/lock.go:38-41).
  • ErrLockBusy sentinel + errors.Is handling and the debug skip path are correct (internal/checker/checker.go:47-52, internal/db/errors.go).
  • Removal of lastMntID/lastInfoID and slices.Min is consistent across checker.go, info.go, and the DB layer; GetMaintenances/GetInfoEvents/getEventsByType no longer take the after param (internal/db/info.go, maintenances.go, event_types.go).
  • Shutdown double-call safety via the closed done channel is fine (second call's <-ch.done returns immediately); the cancel()/close(ch.done) ordering in Run is 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.

@Aloento
Aloento merged commit a319d29 into main Oct 4, 2026
14 checks passed
@Aloento
Aloento deleted the feat/multi-replica-safety branch October 4, 2026 17:03
Aloento added a commit that referenced this pull request Oct 4, 2026
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant