Add cluster-health observability callbacks to leader and task handler

[jonyoder]

What's Changed

Adds three optional, nil-able push callbacks so a consumer (Posit Package Manager) can emit cluster-health metrics without polling leader internals. All are no-ops when unset, so there is no behavior change for existing callers.

• PgxLeaderConfig.OnIntegrityResult func(error) — fired after every cluster integrity evaluation (nil = healthy).
• PgxLeaderConfig.OnPingResult func(success bool, t time.Time) — fired after every leader ping attempt. pingNodes was refactored into pingNodes + sendPings so the callback fires once with the final outcome, outside pingSuccessLock.
• GenericTaskHandlerConfig.OnTaskRun func(name string) — fired when a scheduled task runs, only after the verify gate passes (i.e. genuine leader-gated execution).

Callbacks run on the leader/task-handler goroutine and are documented as "must not block or panic."

Note: pingNodes now sets pingSuccess=false on a (practically impossible) JSON marshal failure, where previously it left the prior value; this is more correct and has no real-world impact.

Tests

Postgres-free unit tests for each callback (integrity healthy/unhealthy, ping success/failure, task-run on a verify-gated scheduled run). Supports the cluster-observability work in rstudio/package-manager#17889.

[jonyoder]

Self-review notes from a local pass (Claude Code). The main thing worth addressing before merge is the goroutine doc contract; the rest are minor.

[jonyoder]

The "It runs on the lead() goroutine" claim is inaccurate for OnPingResult, and the inaccuracy understates concurrency. The periodic path dispatches go p.pingNodes(ctx) (leader.go:205), so this fires on a transient per-tick goroutine — only the startup ping at leader.go:174 runs on lead(). Net effect: OnPingResult (ping goroutine) and OnIntegrityResult (lead goroutine, sweep tick) run concurrently with each other. The current wording invites a consumer to assume serialized execution and skip synchronization on shared callback state. Suggest rewording both leader callbacks to promise what is actually guaranteed, e.g. "May be called concurrently with the other callbacks and across invocations; must be non-blocking, panic-free, and safe for concurrent use."

[jonyoder]

Agreed and fixed in 5de2848. You're right that only the startup ping runs on lead(); periodic pings dispatch go p.pingNodes (leader.go:205), so OnPingResult runs on transient per-tick goroutines and can overlap both itself and OnIntegrityResult. Reworded both leader callbacks to drop the lead()-goroutine claim and require non-blocking, panic-free, and safe-for-concurrent-use. (PPM's consumers only do goroutine-safe Prometheus ops, so no consumer change needed.)

[jonyoder]

Same concurrency-contract issue here: Handle() launches go h.runScheduled(...) per scheduled task, so with N scheduled tasks there is no single "task handler's goroutine" and OnTaskRun can fire concurrently across tasks. Recommend rewording to state it may be called concurrently and must be safe for concurrent use.

[jonyoder]

Fixed in 5de2848 — reworded OnTaskRun to note that Handle() launches a goroutine per scheduled task, so it may be called concurrently across tasks and must be safe for concurrent use.

[jonyoder]

Minor test gap: the callback receives a t time.Time that nothing asserts on. Consider injecting leader.now (the test relies on the timeNow() fallback today) and asserting the timestamp is passed through, since that arg is part of the callback contract and is otherwise untested.

[jonyoder]

Fixed in 5de2848 — TestPingNodesFiresPingCallback now injects leader.now (a fixed clock) and asserts the timestamp is passed through to the callback on both the success and failure paths.

[CDRayn]

Should this also check if err is not nil to avoid unnecessary reporting of errors that are actually nil?

[jonyoder]

Good question — this is intentional. OnIntegrityResult is a result hook, not a failure hook: it reports the outcome of every integrity check, where nil means healthy (as noted in its doc comment). That mirrors OnPingResult, which likewise fires on every ping with success bool (both true and false), so the two leader callbacks stay consistent.

Firing on nil also lets a consumer observe healthy checks, not just failures — e.g. recording a "last successful integrity check" timestamp or resetting state on recovery. Guarding with err != nil here would turn it into a failure-only hook and foreclose that.

The current consumer (PPM) treats the healthy case as a no-op (if err != nil { ClusterIntegrityCheckFailed() }), so a nil result never increments anything, and the per-sweep nil call is negligible. Happy to revisit if you'd prefer a narrower failure-only hook, but I'd lean toward keeping it symmetric with OnPingResult.

[CDRayn]

Overall this looks good. I just had a single question about checking an error for nil before reporting the error.