refactor(alertd): move the daemon into the bestool binary (K2) - #890
Conversation
Leave the checks and their machinery in bestool-alertd, and move the daemon that schedules them up into bestool, behind the existing `alertd` feature. A consumer that embeds the crate to harvest checks no longer pulls in the HTTP server, the backup registry, the Windows service registration, or the child-confinement code along with them. The crate loses its direct axum, tower-http, tokio-stream, sd-notify, win32job and windows-service dependencies, plus bestool-kopia, which it had stopped using. The daemon, its server, its tasks and its backups now live in crates/bestool/src/alertd/, with the clap CLI staying in actions/alertd.rs. `bestool tamanu doctor` keeps working with the daemon feature off. Since removing `run`, `DaemonConfig` and `BackgroundTask` from the public API is a major bump regardless, `doctor::` is flattened to the crate root in the same bump: `doctor::checks::all()` becomes `checks::all()`. VERSION, http_builder and http_client stay behind, so the outbound User-Agent remains bestool-alertd's version rather than the binary's. The postgres-less CI job now runs the whole lib test binary, since the module path it filtered on is gone and the crate no longer holds the HTTP server tests that the filter existed to exclude. No behaviour changes. Co-authored-by: Claude <noreply@anthropic.com>
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary (round 1) Local fix prompt (copy to your coding agent) |
The daemon moved into bestool, where bestool-postgres, bestool-tamanu and node-semver are optional deps; the alertd feature didn't enable them. It built before because the code lived in bestool-alertd, which depends on them unconditionally. A Tamanu-less host running the daemon alone is an explicitly supported configuration, and nothing in CI built it, so add a job that does and gate tests-pass on it. Co-authored-by: Claude <noreply@anthropic.com>
The field's own doc said "retained for redacted display" while the hand-written Debug printed the postgres URL verbatim, password and all, so any debug-logging of the config would leak it. The sibling device key was already wrapped for exactly this reason. Nothing reads the field to connect; it exists only for display. Co-authored-by: Claude <noreply@anthropic.com>
|
🦸 Review Hero (could not post inline comments — showing here instead)
[Bugs & Correctness]
[Design & Architecture]
[Design & Architecture] The move leaves
[Design & Architecture]
[Performance]
[Security]
|
|
🦸 Review Hero Summary (round 2) Below consensus threshold (4 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
The daemon opened a connection pool at startup and threaded it through DaemonConfig, InternalContext and TaskContext, and then nothing read it: the sweep opened its own connection with connect_one every tick. A daemon sweeping every minute paid for a fresh connect each time while the pool sat unused. The dead field was the visible end of an unfinished wire. perform_sweep now takes an optional pool. The daemon passes its own, so a sweep checks a connection out and returns it when the last check drops it. The one-shot doctor CLI has no pool and still opens its own via connect_one. CheckContext::db holds a SweepDb, either of those, deref'ing to the same client, so checks read it through ctx.db() without caring which — which also settles the as_ref/as_deref split at the call sites. db_connect still opens its own connection to measure connect latency, so a pool that can't hand one out cannot mask the database being down. A failed acquire warns and skips the DB-dependent checks, as a failed connect_one did, and with no pool at all the sweep falls back to connect_one every tick, so postgres is still not required for the daemon to start. Co-authored-by: Claude <noreply@anthropic.com>
Both call sites passed env!("CARGO_PKG_VERSION").to_string(), which is what
DaemonConfig::new already defaults to — the same value, resolved in the same
crate. The setter only existed because the config used to live in
bestool-alertd, which couldn't see the binary's version; the move settles
that.
Co-authored-by: Claude <noreply@anthropic.com>
The move left http_builder and http_client in the checks crate, where nothing called them: every check takes a client from its context, and the only callers were the daemon's own commands and startup. So the crate documented as the checks and nothing that schedules them still owned the daemon's client factory, and the daemon reached back across the seam for it. The clients are built in crate::alertd now. Only USER_AGENT stays behind, because the point of keeping it there is that it carries the checks crate's version rather than the binary's; a test in that crate holds the two together so rebuilding it from the wrong CARGO_PKG_VERSION can't pass. Co-authored-by: Claude <noreply@anthropic.com>
A sweep had two ways to open its database connection: the daemon's pool, or connect_one when there wasn't one. Keep only the pool. connect_one was itself a create_pool that took one connection and dropped the pool, so the doctor CLI building its own pool costs it nothing and puts both callers on the same path; the SweepDb enum that spanned the two ways is gone with the second. The pool moves from DaemonConfig to the doctor task. Building one needs the database up, so a daemon started while postgres is down cannot be handed one and has to build it later: the task builds its pool on the first sweep that reaches the database, keyed by URL so an in-place upgrade rebuilds it, and retries each tick until then. Startup no longer touches the database at all, which is a better fit for a daemon whose job includes alerting on that database being down. db_connect still opens its own connection with tokio_postgres::connect, so a pool that can't hand one out cannot mask an outage. The endpoint tests no longer need DATABASE_URL: they built a pool only to fill a field that no longer exists, and the endpoints they cover report on the daemon rather than the database. Co-authored-by: Claude <noreply@anthropic.com>
Left behind when InternalContext lost its pool and gained a Windows-only restart handle. The build that verified the previous commit read the fixed file from the working tree, but a stale fsmonitor entry meant git staged the old blob, so the commit went out without it. Co-authored-by: Claude <noreply@anthropic.com>
A sweep took one connection and shared it between every DB check. The checks already run concurrently, so sharing meant their queries pipelined onto a single backend: a sweep cost the sum of its database work rather than the longest piece of it. A pool that only ever hands out one connection is not doing anything. CheckContext now holds the pool and ctx.db() acquires per check, returning the connection when the check ends. The pool bounds how many run at once, so a check that has to wait simply starts later. The sweep keeps one connection of its own for setup — server kind, Tamanu version, server facts — and that acquire is where a single warning for an unreachable database comes from; db_connect still opens its own connection and is what reports the outage. grades_a_seeded_gap_against_central seeded its fixture in an uncommitted transaction and relied on the check sharing that connection to see it. It commits the seed and deletes it afterwards now, which is what a check reading through its own connection requires, and deletes the probe rows before seeding too so a run that dies before its cleanup can't poison the next. Co-authored-by: Claude <noreply@anthropic.com>
|
🦸 Review Hero Summary (round 3) Local fix prompt (copy to your coding agent) |
Committing the fixture was necessary once checks read through their own connection, but it left the test mutating whatever database answers at the central URL. CI has no tamanu-central database, so the only machines where this ran were developer and ops boxes with a live Tamanu — exactly where the writes do damage. The settings delete was not probe-scoped either: it removed an operator's configured value and the re-insert replaced it with 'true'. The test now reads the setting first and restores it verbatim, or removes the row only when it created one, and is gated behind BESTOOL_TEST_DESTRUCTIVE_DB so a reachable local Tamanu is no longer enough to trigger writes against it. Co-authored-by: Claude <noreply@anthropic.com>
Checks take a connection each and all run at once, but the pool kept mobc's default of ten. With twenty-two DB checks the rest queued, and an acquire that timed out returned None — which db_version, migrations, fhir_jobs, fhir_workers and sync_sessions report as a failed check. Pool contention would have raised a database-down alert. Sizing the pool to the fan-out means a failed acquire means what it did before: the database is unusable. max_idle stays well below max_open, so the burst lasts a sweep rather than holding a backend per check open between them. Co-authored-by: Claude <noreply@anthropic.com>
…s run It was acquired for detect_kind and current_version, then held until the server-facts query at the end — occupying a pool slot through the whole phase where every check is asking for one. It is dropped after the setup queries now, and the facts query takes a connection again once the checks are done. Co-authored-by: Claude <noreply@anthropic.com>
pool_for held the lock across create_pool, which talks to the database and can sit for the connect timeout when the host is unreachable. A concurrent sweep — /tasks/doctor/recompute runs independently of the tick — stalled for the whole of it, every tick, for as long as postgres was down. The lock is taken to read the cached pool and again to store the result. If two sweeps race, whichever pool is already cached for the URL wins, so a pool with live connections is never replaced. Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
|
🦸 Review Hero Summary (round 4) Below consensus threshold (5 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
The sweep handed every check the pool even when its own setup acquire had just failed, so all twenty-two DB checks repeated that failing acquire and each waited out the connect timeout in turn. The sweep's result — including the db_connect failure that is the point of the daemon when postgres is down — only lands once the slowest check returns, so an outage cost the setup acquire plus another full timeout per sweep, where before this branch a connect_one failed fast. The checks get the pool only once a connection has actually come out of it. Without one they skip immediately, as the card said they would, and the next tick tries again. Co-authored-by: Claude <noreply@anthropic.com>
Sizing the pool to the check fan-out fixed queueing by making the sweep burst twenty-odd backends a minute at the deployment's own database — the one the application is also connecting to. `bestool tamanu doctor` opens a second pool that can overlap with the daemon's, so against a cluster on the usual hundred connections, shared with Tamanu's pools, the healthcheck could have caused the outage it exists to report. Eight connections, and checks queue for one. They still run several at a time rather than pipelining onto a single connection, and their queries are short. Waiting is safe now that a database which is actually down leaves the checks with no pool at all rather than something to queue for. Idle connections now outlive the gap between sweeps, so a minute-by-minute daemon reuses them instead of reconnecting, and age out when it goes quiet. Co-authored-by: Claude <noreply@anthropic.com>
The failure branch cleared the cache entry for the URL it had just failed on, which is exactly the entry a concurrent sweep may have written after our own miss. A transient failure in a recompute could throw away the pool the tick had just built, and the next sweep would rebuild against a database that was reachable the whole time. Co-authored-by: Claude <noreply@anthropic.com>
Saving and restoring the setting read whatever was there on entry, which can be the test's own value from a run interrupted between forcing it on and cleaning up. The next run would then read 'true' as the deployment's prior value and faithfully restore it, making the change permanent. The test owns the setting for its duration or it doesn't run: a row already present is either the deployment's configuration, which isn't ours to edit, or that leftover, and the two can't be told apart. It declines and says how to clear it. Everything it touches it created. This also puts the test's doc comment back on the test, where the previous round's edit left it describing the restore helper instead. Co-authored-by: Claude <noreply@anthropic.com>
database_url was threaded in from build_config only to be hidden again: the doctor task resolves its own URL and builds its own pool, so nothing read it to connect, and its only consumer was a Debug impl that — because the field is wrapped in Redacted — printed a redaction marker. It carried information to nobody. binary_version was always this binary's own version once the config stopped crossing a crate boundary, the same shape as the with_binary_version setter removed last round. It is a const now, which the status line and the doctor task both read, so the two stop being independent copies of one value. Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
|
🦸 Review Hero (could not post inline comments — showing here instead)
[Bugs & Correctness]
[Bugs & Correctness] The pool is capped at 8 connections, but
[Performance] This check now holds one of only eight pool slots for the whole of a sequential per-resource loop: for every discovered resource it runs
[Performance] The pooled connection is held for the whole function body, well past the last query.
[Security] The seeded-gap test commits a real write to whatever database answers at the central URL (on an ops box, the live Tamanu), and its cleanup is not failure-safe. The
[Security]
[Performance]
|
|
🦸 Review Hero Summary (round 5) Below consensus threshold (1 unique issue not confirmed by majority)
Local fix prompt (copy to your coding agent) |
The pool has been sized up and back down across three rounds, because no single number satisfies both constraints: large enough that the checks never queue means bursting twenty-odd backends a minute at a production database, and small enough to be a safe budget means they do queue. The defect was on the other axis. A queue that times out is indistinguishable from a database that is unreachable — db() hands back None either way, and five checks report that as a failure — so contention, or simply a slow cluster, raised a database-down alert on a database that was up. The pool waits instead of giving up. A failed acquire now means what it says, and the size can be what it should be: a budget, not a slot per check. A busy sweep takes longer, which is the right trade for something whose job is to report whether the database is healthy — and a sweep that cannot take a connection at all still hands the checks no pool, so they skip at once rather than queueing for something that will not arrive. Co-authored-by: Claude <noreply@anthropic.com>
pg_tuning held a slot through a blocking memory refresh, a kernel probe and all its grading, having made its only query; sync_session_errors held one through the arithmetic after both of its. With a small shared pool that occupancy is what everything else queues behind. Co-authored-by: Claude <noreply@anthropic.com>
Every other statement in the test binds its values; the cleanup interpolated them into the SQL, which is the one shape you don't want to see near a DELETE even when both values are compile-time constants. Co-authored-by: Claude <noreply@anthropic.com>
|
🦸 Review Hero Summary (round 6) Below consensus threshold (5 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
create_pool asks for a password on the terminal when authentication fails and the URL carries none — which is exactly the deployed shape: the unit file documents peer auth over the socket with no password to keep in sync. Moving pool creation into the per-tick sweep put that prompt in the daemon's hot path. Under systemd it errors for want of a terminal, but `bestool alertd run` from a shell blocks on stdin: the sweep never finishes, nothing reaches canopy, and the watchdog restarts it ten minutes later. Broken database auth would silence the monitoring rather than be reported by it. The daemon asks for a pool that never prompts, so an auth failure comes back as an error the sweep can report. The interactive doctor keeps its prompt. Co-authored-by: Claude <noreply@anthropic.com>
Removing the acquire deadline last round stopped contention being misreported as a database outage, but left nothing bounding the sweep: a check stuck on a lock holds its slot, every queued check waits behind it, and the sweep never returns. Nothing is posted to canopy and the watchdog restarts the daemon into the same hang — silence, from the thing whose job is to report trouble. Three things have to hold together, and moving one number up and down kept trading one for another: the pool has to stay a small budget against a production database, ordinary queueing must not be reportable as an outage, and the sweep has to finish. A deadline far longer than any healthy sweep and still finite holds all three. With these slots and the checks' short queries ordinary queueing finishes in well under a second; two minutes means the database has stopped answering, which is worth reporting as such. Idle connections drop to two, so the footprint at rest is proportional to what a sweep actually leaves behind rather than to its widest moment. Co-authored-by: Claude <noreply@anthropic.com>
The setting was written before the probe patient, and the statements between them can fail — schema drift, a new NOT NULL column, a transient error. That window left materialisation switched on for the deployment's FHIR worker, which is the one thing this test is not allowed to do, and poisoned every later run because the guard then refuses. The patient goes first now. Everything before the setting can fail without consequence: the probe is a row only this test creates, and the next run's own delete clears it. The test pool is also built with the sweep's own settings, so the DB-backed tests exercise the pool the daemon actually uses. Co-authored-by: Claude <noreply@anthropic.com>
| max_idle: 2, | ||
| max_idle_lifetime: Some(std::time::Duration::from_secs(300)), | ||
| get_timeout: Some(std::time::Duration::from_secs(120)), | ||
| }; |
There was a problem hiding this comment.
[Bugs & Correctness] critical
get_timeout: None removes the only bound on connection establishment, not just on queueing. Nothing in bestool-postgres sets a connect_timeout on the tokio-postgres config (grep: no connect_timeout/keepalive anywhere in crates/postgres/src), so mobc's get_timeout was the sole deadline on pool.get() — and get() covers opening a new connection, not merely waiting for a free slot. Failure scenario: the postgres host becomes unreachable in a way that drops packets (firewall DROP, vanished VM, stalled TLS handshake) rather than refusing. Each acquire now blocks for the full OS SYN-retry window (~130s) or indefinitely instead of failing at 30s, and the sweep does these serially before any check runs: pool_for → create_pool_sized → check_pool → pool.get(), then perform_sweep's setup acquire, then up to 8 slot-filling connects with 22 checks queueing behind them with no deadline. The tick can easily exceed the 10-minute watchdog, so the daemon is killed and restarted before db_connect ever posts its FAIL — precisely the outage alertd exists to report (AGENTS.md: postgres must never be required for alertd to alert on the database being down). The reasoning in the doc comment ("a failed acquire means the database could not be connected to") depends on the acquire actually failing. Keep the unbounded wait for slot contention but bound the connect: set connect_timeout on the config in create_pool_sized (or give POOL_SIZE a generous-but-finite get_timeout and distinguish mobc::Error::Timeout from Error::Inner in CheckContext::db, so contention and unreachability stay separable).
| .await | ||
| .expect("enabling the resource should succeed"); | ||
|
|
||
| let check = super::run(ctx).await; |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
The cleanup isn't failure-safe, and the new "decline if the setting already exists" guard turns a single interrupted run into a permanent block. The setting is inserted first (line ~676), then the probe patient; if the patient INSERT fails (a NOT NULL column added upstream, a changed enum for sex, …) the expect panics and the setting row survives. Likewise, cleaned_up ?-chains the two deletes, so a failure deleting the patient skips the settings delete entirely. Either way the next run hits assert!(existing.is_none()) and the test can never run again until someone removes the row by hand on a live deployment. Insert the setting last (after the patient is seeded), and run both deletes independently — collect their results rather than short-circuiting — so the setting is always removed.
| ); | ||
| } | ||
| let Some(client) = ctx.db.as_ref() else { | ||
| let Some(client) = ctx.db().await else { |
There was a problem hiding this comment.
[Performance] suggestion
This check takes one pooled connection at line 202 and holds it through the whole per-resource loop: schema discovery, then for each of the 9 RESOURCES a possible has_any_row plus a measure gap/lag query — ~18 sequential round trips, several of them aggregate scans over large materialisation tables. Under the new model that slot is 1/8 of the sweep's entire budget, and since POOL_SIZE.get_timeout is None the other checks queue behind it with no deadline, so this single check sets the floor on sweep latency for every DB check that starts after it. The sibling checks changed in this PR (pg_tuning, sync_session_errors) already adopt the fix: drop the connection once the querying phase is done. Here the cheapest version is to scope the connection to the measurement loop and drop it before the grading/aggregation below, and ideally re-acquire per resource rather than holding one across all nine.
|
🦸 Review Hero Summary (round 7) Below consensus threshold (5 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
The acquire deadline covers opening a connection as well as waiting for a free one, and nothing set a connect timeout, so a host that drops packets rather than refusing absorbs the whole deadline on a single connect. Every tick pays it — the pool build, then the sweep's setup acquire — which delays the outage report the daemon exists to make, on the one occasion it matters most. The two waits are not the same question. A busy database deserves patience; an unreachable one deserves none. Connecting gets a short bound, so it fails fast and the queueing deadline keeps its generosity. Co-authored-by: Claude <noreply@anthropic.com>
The cleanup chained its two deletes, so a failure removing the probe patient skipped the settings delete entirely — leaving materialisation switched on for the deployment, and blocking every later run through the guard that refuses when the setting exists. Both deletes run now, independently of each other. The check also held its connection through grading, having made up to two aggregate queries per resource. It is the sweep's long pole, so that slot was what everything still queueing waited on; it goes back when the querying ends, as pg_tuning and sync_session_errors already do. Co-authored-by: Claude <noreply@anthropic.com>
passcod
changed the title
1 participant
🦸 Review Hero