feat: add database healthcheck callback - #39
Conversation
|
@goshander pls use commit message conventions |
goshander
changed the title
| topologyMode: 'primary-replica', | ||
| connections: this.connections.map((connection) => ({ | ||
| host: connection.host, | ||
| primary: connection.primary, |
There was a problem hiding this comment.
[P2] Preserve an unknown role for failed checks
When a primary becomes unreachable, checkDatabase sets primary = false, and this public snapshot emits it as a non-primary host. In primary-replica mode, consumers have no state other than false, so availability metrics will misclassify an unreachable primary as a replica during the outage. Please model the role as primary | replica | unknown (or expose a separate last-known role) instead of collapsing a failed role check to false.
AI generated
There was a problem hiding this comment.
Fixed in 7e244d1. The callback snapshot now exposes role as primary, replica, or unknown; unavailable hosts use unknown. The existing status log keeps its primary boolean for backward compatibility.
| }), | ||
| ); | ||
| Promise.all(checkups).then(() => { | ||
| const status = this.getHealthcheckStatus(); |
There was a problem hiding this comment.
[P2] Build the snapshot from one health-check cycle
Health-check cycles can overlap when healthcheckInterval is shorter than a check. Each cycle mutates the shared this.connections, while getHealthcheckStatus() reads that shared state only after its own Promise.all completes. A slower older cycle can therefore publish a mixture of its own results and values written by a newer cycle. Please serialize cycles or build the snapshot from per-cycle local results.
AI generated
There was a problem hiding this comment.
The callback intentionally mirrors the existing Database current status semantics and snapshots shared current connection state rather than cycle-local results. I added a short comment to make this explicit. Serializing healthcheck cycles would change existing dispatcher behavior and is better handled separately.
| connections: status.connections, | ||
| }, | ||
| }); | ||
| this.notifyHealthcheck(status); |
There was a problem hiding this comment.
[P2] Do not notify after terminate() has completed
terminate() clears the interval but does not cancel or await an already-running health check. Its Promise.all can finish after teardown and call onHealthcheck after terminate() has resolved, when the consumer may already have closed its exporter or registry. Please track the lifecycle and either await in-flight work or suppress notifications once termination starts.
AI generated
There was a problem hiding this comment.
Fixed in 7e244d1. Termination now marks the dispatcher before clearing its timer, prevents initialization from starting a healthcheck after termination, and suppresses callback notification from an in-flight check.
| } | ||
|
|
||
| try { | ||
| Promise.resolve(this.onHealthcheck(status)).catch((error) => { |
There was a problem hiding this comment.
[P2] Bound concurrent async callbacks
PGHealthcheckHandler may return a Promise, but that Promise is only given a rejection handler; the next interval starts another callback regardless of whether the previous one is still pending. A slow or stuck metrics exporter can therefore accumulate unbounded concurrent operations. Please serialize, coalesce, or otherwise bound callback execution while keeping database routing independent.
AI generated
There was a problem hiding this comment.
Fixed in 7e244d1 by making the callback synchronous and removing Promise handling. Callback exceptions remain isolated from database routing.
2 participants
Summary
onHealthcheckcallback throughinitDBprimary-replicaandproxytopology modesunknownrolesuppressStatusLogsDatabase current statuslog payloadThis API lets consumers publish host availability, role, and latency metrics without parsing log records or running a second healthcheck loop.
Tests
npm run buildnpm run test:unitnpm run lintThe full
npm testcommand completed the package and demo builds plus unit tests, but the integration suite could not start locally because/var/run/docker.sockis unavailable. CI is expected to run that suite.