feat(worker): health/readiness endpoints + awa health CLI probe#387
feat(worker): health/readiness endpoints + awa health CLI probe#387hardbyte wants to merge 2 commits into
awa health CLI probe#387Conversation
#368) - Minimal HTTP listener in awa-worker (AWA_HEALTH_ADDR or ClientBuilder::health_addr): GET /healthz answers 200 for as long as the runtime is up (including graceful drain); GET /readyz answers 200 only when Postgres is reachable, the schema is at least the binary's expected version, every claim loop is ticking, heartbeat + maintenance are alive, and shutdown has not started — else 503 with per-check JSON reasons. Hand-rolled over tokio's TcpListener: no new dependencies in awa-worker. Port 0 supported; bound address exposed via Client::health_listener_addr. - awa health CLI probe for probe-less environments: DB reachability, schema-vs-binary compatibility, storage state, fleet heartbeat counts; --json output; exit 0 ready / 1 not ready. - Tests: five integration tests on a dedicated awa_health_test database (baseline, 404/405, schema mismatch -> 503, database outage via ALLOW_CONNECTIONS false + backend termination -> 503 and recovery, shutting_down 503 during drain while /healthz stays 200), plus two CLI tests (ready JSON + exit codes, unreachable -> exit 1 with report). - Docs: deployment.md health-checks rewrite with k8s probe manifests, configuration.md AWA_HEALTH_ADDR, CHANGELOG entries.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds opt-in worker HTTP endpoints ( ChangesHealth probe feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
full-ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9167e572c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // deploy skew (older binary against newer schema); an older schema | ||
| // means `awa migrate` has not run and the runtime cannot work. | ||
| let schema_version = if postgres_connected { | ||
| awa_model::migrations::current_version(&self.pool) |
There was a problem hiding this comment.
In the rolling-deploy case this probe explicitly allows (database schema newer than this binary), awa_model::migrations::current_version is not a read-only lookup: the current migration helper treats a raw schema_version outside this binary's range with any row >= 6 as legacy and rewrites the table back to the normalized legacy version. An older worker can therefore corrupt schema metadata just because Kubernetes polls GET /readyz (the same pattern exists in awa-cli/src/health.rs) and then report the newer schema as incompatible instead of ready. Use a non-mutating max-version query for probes, or make current_version safe for future versions before calling it here.
Useful? React with 👍 / 👎.
| let listener = tokio::net::TcpListener::bind(addr).await.map_err(|err| { | ||
| awa_model::AwaError::Validation(format!( | ||
| "failed to bind health listener on {addr}: {err}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
When the configured health address is already in use, this ? returns an error after the completion batcher and cancel listener have already been spawned into self.service_handles. Callers see start() fail and normally will not call shutdown(), so the cancel listener can keep a DB connection/task alive even though startup supposedly aborted. Bind before starting background services, or cancel and join the already-spawned handles on this error path.
Useful? React with 👍 / 👎.
|
|
||
| // Simulate a binary that is newer than the schema: remove the newest | ||
| // schema_version row so the DB reports an older version. | ||
| sqlx::query("DELETE FROM awa.schema_version WHERE version = $1") |
There was a problem hiding this comment.
These tests all share the single awa_health_test database, and the Rust test harness runs tests in this integration-test binary in parallel by default. While this test deletes the global schema version row (and the outage test later disables connections to the same database), concurrently running baseline/drain tests can observe schema_compatible: false or lose DB access and time out. Serialize the fault-injection tests or create a separate database per test.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
awa-worker/src/client.rs (1)
1605-1639: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHealth-listener bind failure leaks already-spawned service tasks.
completion_batcher.spawn()and the cancel listener are pushed intoservice_handles(lines 1607-1610) before the fallibleTcpListener::bindfor the health listener (lines 1617-1621). If the bind fails (port in use, permission denied on a privileged port, etc.),start()returnsErrvia?, but nothing cancelsservice_cancelor awaitsservice_handles— those already-spawned background tasks (holding DB connections/LISTEN sockets) keep running detached for the life of the process, contradicting the stated intent that "Bind failures abort startup."This breaks the pattern the rest of
start()follows, where every fallible step up to this point runs before anything is spawned. Moving the health-listener bind earlier (beforecompletion_batcher/cancel_listenerare created) restores that invariant with no behavior change on the success path.♻️ Suggested reordering (conceptual — bind before any service spawns)
// Resolve/bind the health listener first — before anything else is // spawned — so a bind failure aborts start() with nothing to leak. let health_listener = match self.health_addr { Some(addr) => { let listener = tokio::net::TcpListener::bind(addr).await.map_err(|err| { awa_model::AwaError::Validation(format!( "failed to bind health listener on {addr}: {err}" )) })?; let bound = listener.local_addr().map_err(|err| { awa_model::AwaError::Validation(format!( "failed to resolve health listener address: {err}" )) })?; let _ = self.health_bound_addr.set(bound); Some(listener) } None => None, }; // ... existing completion_batcher / cancel_listener / service_handles setup ... if let Some(listener) = health_listener { let probe = HealthProbe { pool: self.pool.clone(), dispatcher_alive: self.dispatcher_alive.clone(), heartbeat_alive: self.heartbeat_alive.clone(), maintenance_alive: self.maintenance_alive.clone(), leader: self.leader.clone(), dispatch_cancel: self.dispatch_cancel.clone(), }; let cancel = self.service_cancel.clone(); service_handles.push(tokio::spawn(health_server::serve(listener, probe, cancel))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@awa-worker/src/client.rs` around lines 1605 - 1639, The health listener bind currently happens after completion_batcher.spawn() and the cancel listener are already added to service_handles, so a TcpListener::bind failure can leak already-spawned background tasks. Reorder the start() flow in client.rs so the health_addr bind and local_addr resolution happen before any service handles are spawned or pushed, and only create/push the completion_batcher, cancel listener, and health_server::serve task after the bind succeeds.
♻️ Duplicate comments (1)
awa/tests/health_endpoint_test.rs (1)
271-310: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDB-outage injection disables connections for the entire shared test database.
Same root cause as the schema-mismatch test (see comment above):
ALTER DATABASE awa_health_test WITH ALLOW_CONNECTIONS falsepluspg_terminate_backendon every backend for that database affects all connections toawa_health_test, not just this test's pool. If any other test in this file (e.g. the baseline or drain test) is running concurrently, its pool connections get killed and new connections refused, producing spurious failures unrelated to the intended scenario.Consider giving fault-injection tests (this one and the schema-mismatch test) their own dedicated per-test database, or serializing this file's tests, to make the suite deterministic regardless of test-runner parallelism.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@awa/tests/health_endpoint_test.rs` around lines 271 - 310, The DB-outage injection in test_readyz_flips_503_when_database_unreachable is mutating the shared awa_health_test database for all tests, which can kill unrelated pools and refuse new connections. Fix this by isolating the fault-injection tests (this one and the schema-mismatch test) from the rest of the file, either by giving each test its own dedicated database or by serializing the health_endpoint_test suite so only one test can run at a time. Keep the change localized around test_readyz_flips_503_when_database_unreachable and the related schema-mismatch test setup.
🧹 Nitpick comments (3)
awa-cli/src/health.rs (1)
56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSchema-compatibility logic duplicated with the worker's
/readyz.This
schema_compatiblederivation is identical toawa-worker/src/health_server.rs'sreadyz()(both computeschema_version.is_some_and(|v| v >= expected)), andpostgres_connectedviaSELECT 1is the same pattern too. Since both already depend onawa-model, consider hoisting this into a small shared helper (e.g.awa_model::migrations::schema_compatible(pool) -> (Option<i32>, bool)) to keep the two probes from drifting if the compatibility rule ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@awa-cli/src/health.rs` around lines 56 - 59, The schema-compatibility and postgres-connectivity checks are duplicated between `health.rs` and the worker’s `readyz()` in `health_server.rs`, so update the `schema_compatible`/`postgres_connected` logic to use a shared helper in `awa_model` instead of keeping two separate implementations. Move the common compatibility derivation (including the `SELECT 1` connectivity check) into a small reusable function such as `awa_model::migrations::schema_compatible(...)`, then have both health probes call it so the rule stays consistent if it changes later.awa-cli/tests/health_cli_test.rs (1)
56-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest doesn't exercise a real connect-timeout hang.
127.0.0.1:9is normally closed and yields an immediate connection refusal, so this doesn't actually validate that--connect-timeoutbounds wall-clock time against a genuinely unresponsive/black-holed host (e.g. a non-routable IP). Given the tool's cron/CI/systemd use case, a test that connects to something like10.255.255.1and asserts the command exits within a bounded window would give real confidence in the timeout behavior — this ties into the timeout gap flagged inmain.rs/health.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@awa-cli/tests/health_cli_test.rs` around lines 56 - 77, The current health CLI test uses a closed localhost port, so it only checks immediate connection refusal rather than a real connect-timeout. Update test_health_unreachable_database_exits_nonzero_with_report to target a black-holed/non-routable host and assert the Command::cargo_bin("awa") invocation completes within a bounded duration while still producing the expected JSON report. Keep the existing assertions on health output, but make the test exercise the actual timeout path in the health/main connection flow.awa-worker/src/health_server.rs (1)
66-90: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
/readyzDB check has no independent timeout.
sqlx::query("SELECT 1").execute(&self.pool)acquires from the same pool used by dispatchers/heartbeat/maintenance. If the pool is saturated under load, this call can block well beyond a "quick ping," and the only backstop is the bluntCONNECTION_DEADLINE(5s) inserve(), which just drops the TCP connection rather than returning a proper503. Infra probes then see a connection reset/timeout instead of a fast, informative503.Wrapping the check with a short explicit timeout (e.g. 1–2s) would let the probe fail fast with
postgres_connected: falseinstead of silently hanging until the outer deadline kills the socket.♻️ Proposed fix
- let postgres_connected = sqlx::query("SELECT 1").execute(&self.pool).await.is_ok(); + let postgres_connected = tokio::time::timeout( + Duration::from_secs(2), + sqlx::query("SELECT 1").execute(&self.pool), + ) + .await + .map(|result| result.is_ok()) + .unwrap_or(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@awa-worker/src/health_server.rs` around lines 66 - 90, The `/readyz` database ping in HealthProbe::readyz currently depends on sqlx::query("SELECT 1").execute(&self.pool) with no independent timeout, so it can block on pool saturation and only fail via the outer CONNECTION_DEADLINE in serve(). Add a short explicit timeout around the postgres connectivity check inside HealthProbe::readyz so it returns quickly with postgres_connected false and a proper readiness failure instead of hanging; keep the rest of the readiness logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@awa-cli/src/health.rs`:
- Around line 47-98: The health probe in probe currently runs several database
calls without any execution deadline, so it can hang even after the pool is
created. Wrap the probe flow in tokio::time::timeout, or apply per-query
timeouts around the SELECT 1, awa_model::migrations::current_version,
awa_model::storage::status, and the runtime count query, and on timeout return
an unreachable-style report with ready set to false. Keep the change localized
to probe and the HealthReport construction so the timeout behavior is consistent
for all probe steps.
In `@awa-cli/src/main.rs`:
- Around line 667-692: The health check flow in Commands::Health only times out
the pool connection, not the actual health::probe(&pool) work, so a stalled
database can still hang indefinitely. Wrap the probe call in a timeout or add
per-query deadlines inside health::probe so the whole health path has a bounded
duration, and keep the existing unreachable_report handling for connection
failures.
In `@awa-worker/src/client.rs`:
- Around line 949-960: The doc comment for Client::build is getting attached to
Client::health_addr because the new health_addr doc block is placed immediately
after the existing “Build the client.” comment. Move the health_addr
documentation so it only annotates health_addr, and ensure build retains its own
doc comment by separating the two doc blocks clearly around the health_addr and
build methods.
- Around line 2090-2098: Move the stray “Health check.” doc comment so it
directly documents `health_check()` instead of `health_listener_addr()`, and
keep the listener address docs attached to `health_listener_addr`. Update the
`Client` impl block around `health_listener_addr` and `health_check` so each
public method has the correct docstring immediately above its declaration.
In `@awa/tests/health_endpoint_test.rs`:
- Around line 157-184: The health endpoint tests are mutating shared database
state in test_readyz_flips_503_on_schema_mismatch, which can race with other
tests using awa_health_test and cause nondeterministic failures. Update the
tests in this file so the schema_version mutation is isolated or the
health-related tests are serialized, and ensure
test_readyz_flips_503_on_schema_mismatch restores the DB state in a way that
cannot affect concurrent tests.
In `@docs/deployment.md`:
- Around line 185-191: Update the readiness probe description in the deployment
docs to match `HealthProbe::readyz` and `ready` in `health_server.rs`: remove
`leader` from the list of failing checks because it is reported but does not
gate readiness. Keep the wording focused on the actual readiness inputs
(`postgres_connected`, `schema_compatible`, `poll_loop_alive`,
`heartbeat_alive`, `maintenance_alive`, `shutting_down`) so the `GET /readyz`
JSON body description reflects the code accurately.
---
Outside diff comments:
In `@awa-worker/src/client.rs`:
- Around line 1605-1639: The health listener bind currently happens after
completion_batcher.spawn() and the cancel listener are already added to
service_handles, so a TcpListener::bind failure can leak already-spawned
background tasks. Reorder the start() flow in client.rs so the health_addr bind
and local_addr resolution happen before any service handles are spawned or
pushed, and only create/push the completion_batcher, cancel listener, and
health_server::serve task after the bind succeeds.
---
Duplicate comments:
In `@awa/tests/health_endpoint_test.rs`:
- Around line 271-310: The DB-outage injection in
test_readyz_flips_503_when_database_unreachable is mutating the shared
awa_health_test database for all tests, which can kill unrelated pools and
refuse new connections. Fix this by isolating the fault-injection tests (this
one and the schema-mismatch test) from the rest of the file, either by giving
each test its own dedicated database or by serializing the health_endpoint_test
suite so only one test can run at a time. Keep the change localized around
test_readyz_flips_503_when_database_unreachable and the related schema-mismatch
test setup.
---
Nitpick comments:
In `@awa-cli/src/health.rs`:
- Around line 56-59: The schema-compatibility and postgres-connectivity checks
are duplicated between `health.rs` and the worker’s `readyz()` in
`health_server.rs`, so update the `schema_compatible`/`postgres_connected` logic
to use a shared helper in `awa_model` instead of keeping two separate
implementations. Move the common compatibility derivation (including the `SELECT
1` connectivity check) into a small reusable function such as
`awa_model::migrations::schema_compatible(...)`, then have both health probes
call it so the rule stays consistent if it changes later.
In `@awa-cli/tests/health_cli_test.rs`:
- Around line 56-77: The current health CLI test uses a closed localhost port,
so it only checks immediate connection refusal rather than a real
connect-timeout. Update
test_health_unreachable_database_exits_nonzero_with_report to target a
black-holed/non-routable host and assert the Command::cargo_bin("awa")
invocation completes within a bounded duration while still producing the
expected JSON report. Keep the existing assertions on health output, but make
the test exercise the actual timeout path in the health/main connection flow.
In `@awa-worker/src/health_server.rs`:
- Around line 66-90: The `/readyz` database ping in HealthProbe::readyz
currently depends on sqlx::query("SELECT 1").execute(&self.pool) with no
independent timeout, so it can block on pool saturation and only fail via the
outer CONNECTION_DEADLINE in serve(). Add a short explicit timeout around the
postgres connectivity check inside HealthProbe::readyz so it returns quickly
with postgres_connected false and a proper readiness failure instead of hanging;
keep the rest of the readiness logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26c3cc5f-0784-43b5-856c-7307d65aaf0b
📒 Files selected for processing (10)
CHANGELOG.mdawa-cli/src/health.rsawa-cli/src/main.rsawa-cli/tests/health_cli_test.rsawa-worker/src/client.rsawa-worker/src/health_server.rsawa-worker/src/lib.rsawa/tests/health_endpoint_test.rsdocs/configuration.mddocs/deployment.md
| pub async fn probe(pool: &PgPool) -> HealthReport { | ||
| let postgres_connected = sqlx::query("SELECT 1").execute(pool).await.is_ok(); | ||
|
|
||
| let schema_version = if postgres_connected { | ||
| awa_model::migrations::current_version(pool).await.ok() | ||
| } else { | ||
| None | ||
| }; | ||
| let expected_schema_version = awa_model::migrations::CURRENT_VERSION; | ||
| // A schema newer than the binary is the supported rolling-deploy skew; | ||
| // an older schema means `awa migrate` has not run. | ||
| let schema_compatible = | ||
| schema_version.is_some_and(|version| version >= expected_schema_version); | ||
|
|
||
| let (storage_state, active_engine) = if schema_compatible { | ||
| match awa_model::storage::status(pool).await { | ||
| Ok(status) => (Some(status.state), Some(status.active_engine)), | ||
| Err(_) => (None, None), | ||
| } | ||
| } else { | ||
| (None, None) | ||
| }; | ||
|
|
||
| let (live_runtimes, unhealthy_runtimes) = if schema_compatible { | ||
| sqlx::query_as::<_, (i64, i64)>( | ||
| r#" | ||
| SELECT | ||
| count(*)::bigint, | ||
| count(*) FILTER (WHERE NOT healthy)::bigint | ||
| FROM awa.runtime_instances | ||
| WHERE last_seen_at + make_interval(secs => 90) >= now() | ||
| "#, | ||
| ) | ||
| .fetch_one(pool) | ||
| .await | ||
| .unwrap_or((0, 0)) | ||
| } else { | ||
| (0, 0) | ||
| }; | ||
|
|
||
| HealthReport { | ||
| ready: postgres_connected && schema_compatible, | ||
| postgres_connected, | ||
| schema_version, | ||
| expected_schema_version, | ||
| schema_compatible, | ||
| storage_state, | ||
| active_engine, | ||
| live_runtimes, | ||
| unhealthy_runtimes, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== health.rs outline ==\n'
ast-grep outline awa-cli/src/health.rs --view expanded || true
printf '\n== health.rs relevant lines ==\n'
cat -n awa-cli/src/health.rs | sed -n '1,220p'
printf '\n== search for probe call sites and timeouts ==\n'
rg -n --hidden --glob '!target' 'probe\(' awa-cli . || true
rg -n --hidden --glob '!target' 'acquire_timeout|connect_timeout|tokio::time::timeout|timeout\(' awa-cli . || true
printf '\n== search for module docs mentioning bounded time ==\n'
rg -n --hidden --glob '!target' 'bounded|timeout|health|cron|systemd|CI' awa-cli/src . || trueRepository: hardbyte/awa
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== awa-cli/src/health.rs outline ==\n'
ast-grep outline awa-cli/src/health.rs --view expanded || true
printf '\n== awa-cli/src/health.rs lines 1-220 ==\n'
cat -n awa-cli/src/health.rs | sed -n '1,220p'
printf '\n== probe call sites in awa-cli ==\n'
rg -n --hidden --glob '!target' '\bprobe\(' awa-cli/src || true
printf '\n== health docs / module comments in awa-cli ==\n'
rg -n --hidden --glob '!target' 'bounded|timeout|health|probe' awa-cli/src || trueRepository: hardbyte/awa
Length of output: 8870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== awa-cli/src/main.rs around health command ==\n'
cat -n awa-cli/src/main.rs | sed -n '110,190p'
printf '\n== awa-cli/src/main.rs around pool setup and health dispatch ==\n'
cat -n awa-cli/src/main.rs | sed -n '650,690p'
printf '\n== search for any timeout wrapping around health::probe ==\n'
rg -n --hidden --glob '!target' 'health::probe|tokio::time::timeout\(|connect_timeout' awa-cli/src/main.rs awa-cli/src/health.rs || trueRepository: hardbyte/awa
Length of output: 6416
Bound awa health with a probe timeout. connect_timeout only covers pool establishment; once health::probe() starts, SELECT 1, current_version, storage::status, and the runtime count query can still block forever if Postgres stalls after accepting the connection. Wrap the probe path in tokio::time::timeout (or add per-query timeouts) and return unreachable_report()/ready: false on expiry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@awa-cli/src/health.rs` around lines 47 - 98, The health probe in probe
currently runs several database calls without any execution deadline, so it can
hang even after the pool is created. Wrap the probe flow in
tokio::time::timeout, or apply per-query timeouts around the SELECT 1,
awa_model::migrations::current_version, awa_model::storage::status, and the
runtime count query, and on timeout return an unreachable-style report with
ready set to false. Keep the change localized to probe and the HealthReport
construction so the timeout behavior is consistent for all probe steps.
| Commands::Health { | ||
| json, | ||
| connect_timeout, | ||
| } => { | ||
| let db_url = require_pool(&cli.database_url)?; | ||
| let pool_result = PgPoolOptions::new() | ||
| .max_connections(1) | ||
| .acquire_timeout(Duration::from_secs(connect_timeout)) | ||
| .connect(&db_url) | ||
| .await; | ||
|
|
||
| let report = match pool_result { | ||
| Ok(pool) => health::probe(&pool).await, | ||
| Err(_) => health::unreachable_report(), | ||
| }; | ||
|
|
||
| if json { | ||
| println!("{}", serde_json::to_string_pretty(&report)?); | ||
| } else { | ||
| println!("{}", health::render_human(&report)); | ||
| } | ||
| if !report.ready { | ||
| std::process::exit(1); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant snippet in main.rs
sed -n '640,720p' awa-cli/src/main.rs
printf '\n--- health.rs locations ---\n'
rg -n "connect-timeout|probe|systemd|ExecStartPre|timeout" awa-cli/src/health.rs awa-cli/src -g '!target'
printf '\n--- health.rs outline ---\n'
ast-grep outline awa-cli/src/health.rs --view expanded || true
printf '\n--- health.rs selected content ---\n'
sed -n '1,260p' awa-cli/src/health.rsRepository: hardbyte/awa
Length of output: 10609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '640,720p' awa-cli/src/main.rs
printf '\n---\n'
sed -n '1,260p' awa-cli/src/health.rsRepository: hardbyte/awa
Length of output: 7803
Wrap the health probe in a timeout
acquire_timeout(...) only covers the initial connection. health::probe(&pool) still issues several queries with no deadline, so a database that accepts the connection and then stalls can block awa health indefinitely in ExecStartPre/CI. Add a timeout around the probe or enforce one per query.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@awa-cli/src/main.rs` around lines 667 - 692, The health check flow in
Commands::Health only times out the pool connection, not the actual
health::probe(&pool) work, so a stalled database can still hang indefinitely.
Wrap the probe call in a timeout or add per-query deadlines inside health::probe
so the whole health path has a bounded duration, and keep the existing
unreachable_report handling for connection failures.
| /// Build the client. | ||
| /// Serve `GET /healthz` and `GET /readyz` on this address while the | ||
| /// runtime is running (#368). Defaults to off; the `AWA_HEALTH_ADDR` | ||
| /// environment variable is honoured when this is not set. Port 0 binds | ||
| /// an ephemeral port — read it back with | ||
| /// [`Client::health_listener_addr`] after `start()`. | ||
| pub fn health_addr(mut self, addr: SocketAddr) -> Self { | ||
| self.health_addr = Some(addr); | ||
| self | ||
| } | ||
|
|
||
| pub fn build(self) -> Result<Client, BuildError> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment misattached: build() loses its docstring, health_addr() inherits the wrong one.
/// Build the client. (line 949, pre-existing) is immediately followed by the new health_addr doc block with no blank line, so both concatenate into a single doc comment attached to pub fn health_addr. The actual pub fn build() right below ends up with no doc comment at all.
🩹 Proposed fix
- /// Build the client.
/// Serve `GET /healthz` and `GET /readyz` on this address while the
/// runtime is running (`#368`). Defaults to off; the `AWA_HEALTH_ADDR`
/// environment variable is honoured when this is not set. Port 0 binds
/// an ephemeral port — read it back with
/// [`Client::health_listener_addr`] after `start()`.
pub fn health_addr(mut self, addr: SocketAddr) -> Self {
self.health_addr = Some(addr);
self
}
+ /// Build the client.
pub fn build(self) -> Result<Client, BuildError> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Build the client. | |
| /// Serve `GET /healthz` and `GET /readyz` on this address while the | |
| /// runtime is running (#368). Defaults to off; the `AWA_HEALTH_ADDR` | |
| /// environment variable is honoured when this is not set. Port 0 binds | |
| /// an ephemeral port — read it back with | |
| /// [`Client::health_listener_addr`] after `start()`. | |
| pub fn health_addr(mut self, addr: SocketAddr) -> Self { | |
| self.health_addr = Some(addr); | |
| self | |
| } | |
| pub fn build(self) -> Result<Client, BuildError> { | |
| /// Serve `GET /healthz` and `GET /readyz` on this address while the | |
| /// runtime is running (`#368`). Defaults to off; the `AWA_HEALTH_ADDR` | |
| /// environment variable is honoured when this is not set. Port 0 binds | |
| /// an ephemeral port — read it back with | |
| /// [`Client::health_listener_addr`] after `start()`. | |
| pub fn health_addr(mut self, addr: SocketAddr) -> Self { | |
| self.health_addr = Some(addr); | |
| self | |
| } | |
| /// Build the client. | |
| pub fn build(self) -> Result<Client, BuildError> { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@awa-worker/src/client.rs` around lines 949 - 960, The doc comment for
Client::build is getting attached to Client::health_addr because the new
health_addr doc block is placed immediately after the existing “Build the
client.” comment. Move the health_addr documentation so it only annotates
health_addr, and ensure build retains its own doc comment by separating the two
doc blocks clearly around the health_addr and build methods.
| /// Health check. | ||
| /// The address the health/readiness listener bound during `start()`. | ||
| /// `None` when the listener is disabled or `start()` has not run yet. | ||
| /// Useful with port 0 (ephemeral) configurations. | ||
| pub fn health_listener_addr(&self) -> Option<SocketAddr> { | ||
| self.health_bound_addr.get().copied() | ||
| } | ||
|
|
||
| pub async fn health_check(&self) -> HealthCheck { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Same doc-misattachment bug: health_check() loses its docstring.
/// Health check. (line 2090, pre-existing) now sits directly above the new health_listener_addr doc block, so it attaches to health_listener_addr instead of health_check, leaving pub async fn health_check undocumented.
🩹 Proposed fix
- /// Health check.
/// The address the health/readiness listener bound during `start()`.
/// `None` when the listener is disabled or `start()` has not run yet.
/// Useful with port 0 (ephemeral) configurations.
pub fn health_listener_addr(&self) -> Option<SocketAddr> {
self.health_bound_addr.get().copied()
}
+ /// Health check.
pub async fn health_check(&self) -> HealthCheck {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Health check. | |
| /// The address the health/readiness listener bound during `start()`. | |
| /// `None` when the listener is disabled or `start()` has not run yet. | |
| /// Useful with port 0 (ephemeral) configurations. | |
| pub fn health_listener_addr(&self) -> Option<SocketAddr> { | |
| self.health_bound_addr.get().copied() | |
| } | |
| pub async fn health_check(&self) -> HealthCheck { | |
| /// The address the health/readiness listener bound during `start()`. | |
| /// `None` when the listener is disabled or `start()` has not run yet. | |
| /// Useful with port 0 (ephemeral) configurations. | |
| pub fn health_listener_addr(&self) -> Option<SocketAddr> { | |
| self.health_bound_addr.get().copied() | |
| } | |
| /// Health check. | |
| pub async fn health_check(&self) -> HealthCheck { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@awa-worker/src/client.rs` around lines 2090 - 2098, Move the stray “Health
check.” doc comment so it directly documents `health_check()` instead of
`health_listener_addr()`, and keep the listener address docs attached to
`health_listener_addr`. Update the `Client` impl block around
`health_listener_addr` and `health_check` so each public method has the correct
docstring immediately above its declaration.
| - `GET /readyz` — readiness. `200` only when the worker can do useful work: | ||
| Postgres reachable, schema at least the binary's expected version, every | ||
| queue's claim loop ticking, heartbeat + maintenance services alive, and not | ||
| shutting down. Otherwise `503` with a JSON body naming each failing check | ||
| (`postgres_connected`, `schema_version`, `expected_schema_version`, | ||
| `schema_compatible`, `poll_loop_alive`, `heartbeat_alive`, | ||
| `maintenance_alive`, `shutting_down`, `leader`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
leader isn't actually a "failing check."
Per HealthProbe::readyz (awa-worker/src/health_server.rs), ready is computed from postgres_connected, schema_compatible, poll_loop_alive, heartbeat_alive, maintenance_alive, and !shutting_down — leader is reported but never gates readiness. Listing it alongside the other fields under "JSON body naming each failing check" could send an on-call engineer chasing leader: false as a cause of a 503 when it never is one.
✏️ Suggested wording
- shutting down. Otherwise `503` with a JSON body naming each failing check
- (`postgres_connected`, `schema_version`, `expected_schema_version`,
- `schema_compatible`, `poll_loop_alive`, `heartbeat_alive`,
- `maintenance_alive`, `shutting_down`, `leader`).
+ shutting down. Otherwise `503` with a JSON body naming each failing check
+ (`postgres_connected`, `schema_version`, `expected_schema_version`,
+ `schema_compatible`, `poll_loop_alive`, `heartbeat_alive`,
+ `maintenance_alive`, `shutting_down`). `leader` is also included in the
+ body for observability but never gates readiness.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `GET /readyz` — readiness. `200` only when the worker can do useful work: | |
| Postgres reachable, schema at least the binary's expected version, every | |
| queue's claim loop ticking, heartbeat + maintenance services alive, and not | |
| shutting down. Otherwise `503` with a JSON body naming each failing check | |
| (`postgres_connected`, `schema_version`, `expected_schema_version`, | |
| `schema_compatible`, `poll_loop_alive`, `heartbeat_alive`, | |
| `maintenance_alive`, `shutting_down`, `leader`). | |
| - `GET /readyz` — readiness. `200` only when the worker can do useful work: | |
| Postgres reachable, schema at least the binary's expected version, every | |
| queue's claim loop ticking, heartbeat + maintenance services alive, and not | |
| shutting down. Otherwise `503` with a JSON body naming each failing check | |
| (`postgres_connected`, `schema_version`, `expected_schema_version`, | |
| `schema_compatible`, `poll_loop_alive`, `heartbeat_alive`, | |
| `maintenance_alive`, `shutting_down`). `leader` is also included in the | |
| body for observability but never gates readiness. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/deployment.md` around lines 185 - 191, Update the readiness probe
description in the deployment docs to match `HealthProbe::readyz` and `ready` in
`health_server.rs`: remove `leader` from the list of failing checks because it
is reported but does not gate readiness. Keep the wording focused on the actual
readiness inputs (`postgres_connected`, `schema_compatible`, `poll_loop_alive`,
`heartbeat_alive`, `maintenance_alive`, `shutting_down`) so the `GET /readyz`
JSON body description reflects the code accurately.
…ners CI runs test binaries without --test-threads=1, so the five health tests raced on the shared awa_health_test database (schema_version deletion and ALLOW_CONNECTIONS fault injection are process-wide). Serialize with the same guard shape as migration_test — process-local mutex + Postgres advisory lock — and tolerate the duplicate-database creation race.
1 participant
Closes #368. Unblocks the probe requirements in #344 (Helm) and applies to the deployment shapes in #282/#372 (roadmap WS-1, PR #384).
Design
Worker listener. Opt-in via
AWA_HEALTH_ADDRorClientBuilder::health_addr(SocketAddr)(invalid env value is aBuildError; bind failure abortsstart()— a misconfigured probe must not fail silently). Two endpoints:GET /healthz— process liveness. Stays200through the graceful drain: a draining worker must not be killed by its liveness probe. The listener runs onservice_cancel, so it outlives dispatchers and dies with the services.GET /readyz—200only when the worker can do useful work: Postgres reachable, schema>=the binary'sCURRENT_VERSION(newer-schema skew is the supported rolling-deploy shape; older means unmigrated), every queue's claim loop ticking, heartbeat + maintenance alive, not shutting down. Otherwise503with a JSON body naming each failing check.Per the issue's constraint ("must not pull the UI stack into awa-worker") the listener is hand-rolled over
tokio::net::TcpListener— zero new dependencies inawa-worker. Bounded request size, per-connection deadline, connection-close responses, 404/405 for anything that isn'tGET /healthz|/readyz. Port 0 binds ephemeral andClient::health_listener_addr()reports the bound address.The readiness flags are the same atomics the runtime snapshot reporter already publishes to
awa.runtime_instances— no new state, just an HTTP view of it.awa healthCLI probe for probe-less environments (cron drivers, systemdExecStartPre, CI smoke): cluster-level readiness from the database alone — reachable, schema migrated for this binary, storage state + active engine, fleet heartbeat counts (90s window, matching the runtime-liveness gates).--jsonfor machines, exit0ready /1not ready. Fleet counts are informational: an idle cluster with zero workers is still "ready" for producers and migrations.Tests
Five integration tests on a dedicated
awa_health_testdatabase (fault injection must not race the shared suite):schema_compatible: false) and recovery after restorepostgres_connected: false) and recovery — injected viaALTER DATABASE … ALLOW_CONNECTIONS false+pg_terminate_backend, no containers neededshutting_down: truewhile an in-flight job drains, healthz stays 200, listener gone after shutdown completesAWA_HEALTH_ADDRis a build errorPlus two
awa healthCLI tests (ready report + exit codes; unreachable DB → exit 1 with a JSON report).Docs
docs/deployment.mdhealth-checks section rewritten (endpoint semantics, response schema, Kubernetes liveness/readiness manifest,awa healthexamples);docs/configuration.mdgainsAWA_HEALTH_ADDR; CHANGELOG entries under Unreleased.TLA+
No model changes: the endpoints are read-only observers of existing runtime state and touch no storage lifecycle semantics. (The transition-model suite was run and extended separately in #386 this cycle.)
Verification
cargo fmtclean;clippy --all-targets --all-features -D warningsclean;cargo build --workspaceclean.health_endpoint_test: 5/5 pass.health_cli_test: 2/2 pass.Follow-ups (out of scope here)
/healthz+/readyzintoawa serveandawa callbacks serve(their axum routers) so all four ADR-027 surfaces expose the same probes.Summary by CodeRabbit
New Features
/healthzand/readyzendpoints for liveness and readiness checks.awa healthcommand with JSON output and exit codes for readiness checks when no probe server is available.Bug Fixes