fix(readiness): bound DB ping and unset Infisical in test-integration (PMS-685) #466

Closed

nrupard wants to merge 2 commits from fix/PMS-685-readiness-timeout into main

pull from: fix/PMS-685-readiness-timeout

merge into: psa-systems:main

psa-systems:main

psa-systems:mokosh-client-login

psa-systems:feat/PMS-676-bootstrap-admin-unverified-login

psa-systems:feat/PMS-621-file-based-seed-data-io

psa-systems:fix/PMS-592-chromium-spa-login-credential-race

Conversation 1 Commits 2 Files changed 2 +34 -6

nrupard commented 2026-07-24 20:50:36 +02:00

Owner

Copy link

What

Make the /api/v1/ready readiness probe robust and its integration test deterministic: bound the DB ping with a forgiving 3-second timeout, and run just test-integration with INFISICAL_BASE_URL unset so the local harness honors the test's documented "Infisical unconfigured" precondition and faithfully mirrors CI.

Why

The spec attributed the intermittent readiness 503 to a hard 1s DB-ping timeout, but there is no DB-ping timeout in the code; the DB ping was unbounded (only the pool's 30s acquire timeout applied). The actual cause of the failing ready_returns_ok_when_db_reachable_and_infisical_unconfigured test is environmental: the dev compose server service sets INFISICAL_BASE_URL=http://infisical:8080, but Infisical is an opt-in profile that is not running during tests, so probe_infisical() times out after ~1s (the observed ~1002ms latency) and /ready returns 503 with infisical != "skipped". CI (integration.yml) runs the suite directly on the runner with no INFISICAL_BASE_URL, so the probe is skipped and the test passes there; only the local dev-container run diverged from CI. Separately, an unbounded readiness DB ping is genuinely the wrong shape for a probe: a hung database would let /ready block up to 30s rather than returning a prompt 503, so a bounded-but-forgiving 3s timeout is a real robustness improvement in line with the spec's proposed approach.

Changes

• src/api/router.rs: wrap the readiness DB ping in tokio::time::timeout(Duration::from_secs(3), db.health_check()); on elapse, synthesize an AppError::Database so the existing 503 / JSON-breakdown / Cache-Control: no-store path is unchanged. The 3s bound is documented inline on both the ready_check doc comment and the timeout site.
• justfile: test-integration now runs cargo test via env -u INFISICAL_BASE_URL, unsetting the dev-container-only var so the suite matches CI and honors readiness.rs's documented precondition. This touches only the test harness, not the production probe: a configured-but-unreachable Infisical still 503s /ready by design.

Tests

• just pre-commit (mirrors CI check.yml: fmt, clippy --all-targets -D warnings, check --all-targets, unit + doc tests): PASSED, exit 0. Unit tests 293 passed / 0 failed; doc tests 2 passed / 0 failed / 3 ignored.
• tests/readiness.rs via the Postgres-backed docker harness with INFISICAL_BASE_URL unset (as the updated test-integration recipe now does), run 2x at --test-threads=4 and 1x at --test-threads=1: all three runs PASSED, 4 passed / 0 failed each, exit 0. Covers ready_returns_ok_when_db_reachable_and_infisical_unconfigured (now 200), ready_returns_503_when_db_pool_closed (still 503 when the DB is truly down), plus the cache-control and liveness cases.
• Note: a pre-existing local-only pollution (83 stale _sqlx_test_* databases owned by postgres while the dev harness connects as mokosh_migrator) had to be cleared first, by reassigning their ownership to mokosh_migrator so sqlx's #[sqlx::test] cleanup could manage them. CI is unaffected (it starts a fresh Postgres and connects as the superuser role).

## What Make the `/api/v1/ready` readiness probe robust and its integration test deterministic: bound the DB ping with a forgiving 3-second timeout, and run `just test-integration` with `INFISICAL_BASE_URL` unset so the local harness honors the test's documented "Infisical unconfigured" precondition and faithfully mirrors CI. ## Why The spec attributed the intermittent readiness 503 to a hard 1s DB-ping timeout, but there is no DB-ping timeout in the code; the DB ping was unbounded (only the pool's 30s acquire timeout applied). The actual cause of the failing `ready_returns_ok_when_db_reachable_and_infisical_unconfigured` test is environmental: the dev compose `server` service sets `INFISICAL_BASE_URL=http://infisical:8080`, but Infisical is an opt-in profile that is not running during tests, so `probe_infisical()` times out after ~1s (the observed ~1002ms latency) and `/ready` returns 503 with `infisical` != "skipped". CI (integration.yml) runs the suite directly on the runner with no `INFISICAL_BASE_URL`, so the probe is skipped and the test passes there; only the local dev-container run diverged from CI. Separately, an unbounded readiness DB ping is genuinely the wrong shape for a probe: a hung database would let `/ready` block up to 30s rather than returning a prompt 503, so a bounded-but-forgiving 3s timeout is a real robustness improvement in line with the spec's proposed approach. ## Changes - `src/api/router.rs`: wrap the readiness DB ping in `tokio::time::timeout(Duration::from_secs(3), db.health_check())`; on elapse, synthesize an `AppError::Database` so the existing 503 / JSON-breakdown / `Cache-Control: no-store` path is unchanged. The 3s bound is documented inline on both the `ready_check` doc comment and the timeout site. - `justfile`: `test-integration` now runs `cargo test` via `env -u INFISICAL_BASE_URL`, unsetting the dev-container-only var so the suite matches CI and honors readiness.rs's documented precondition. This touches only the test harness, not the production probe: a configured-but-unreachable Infisical still 503s `/ready` by design. ## Tests - `just pre-commit` (mirrors CI check.yml: fmt, clippy --all-targets -D warnings, check --all-targets, unit + doc tests): PASSED, exit 0. Unit tests 293 passed / 0 failed; doc tests 2 passed / 0 failed / 3 ignored. - `tests/readiness.rs` via the Postgres-backed docker harness with `INFISICAL_BASE_URL` unset (as the updated `test-integration` recipe now does), run 2x at `--test-threads=4` and 1x at `--test-threads=1`: all three runs PASSED, 4 passed / 0 failed each, exit 0. Covers `ready_returns_ok_when_db_reachable_and_infisical_unconfigured` (now 200), `ready_returns_503_when_db_pool_closed` (still 503 when the DB is truly down), plus the cache-control and liveness cases. - Note: a pre-existing local-only pollution (83 stale `_sqlx_test_*` databases owned by `postgres` while the dev harness connects as `mokosh_migrator`) had to be cleared first, by reassigning their ownership to `mokosh_migrator` so sqlx's `#[sqlx::test]` cleanup could manage them. CI is unaffected (it starts a fresh Postgres and connects as the superuser role).

nrupard added 1 commit 2026-07-24 20:50:36 +02:00

fix(readiness): bound DB ping and unset Infisical in test-integration ... 4b89bc51d4

Two changes so /api/v1/ready is robust and tests/readiness.rs passes deterministically.

1) Bound the DB ping. ready_check called db.health_check() (a bare SELECT 1) with no timeout of its own, so the ping was bounded only by the pool's 30s acquire timeout. That is the wrong shape for a readiness probe: a genuinely hung database would let /ready block for up to 30s instead of returning a prompt 503, so an orchestrator could time out the whole probe and needlessly drain or restart a healthy instance. Wrap the ping in tokio::time::timeout with a forgiving 3s bound: cold-pool / under-load latency still reads ready, a hung DB fails the probe after 3s, and a closed/unreachable pool still errors immediately, so a truly-down DB is still reported 503. On elapse the handler synthesizes an AppError::Database so the existing 503 / JSON-breakdown / no-store path is unchanged. The bound is documented inline.

2) Make just test-integration honor readiness.rs's documented "Infisical unconfigured" precondition. The spec attributed the intermittent readiness 503 to a hard 1s DB-ping timeout, but there is no DB-ping timeout in the code. The real cause of the failing test is environmental: the dev compose `server` service sets INFISICAL_BASE_URL=http://infisical:8080, but Infisical is an opt-in profile that is not running during tests, so probe_infisical() times out after ~1s and /ready 503s. CI (integration.yml) runs the suite directly on the runner with no INFISICAL_BASE_URL set, so the probe is skipped and the test passes there; only the local dev-container run diverged. readiness.rs documents that the harness "does not configure Infisical", so the recipe now runs cargo test with `env -u INFISICAL_BASE_URL`, restoring that precondition and making just test-integration a faithful mirror of CI. This changes only the test harness, not the production probe: a configured-but-unreachable Infisical still 503s /ready by design.

#PMS-685

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

David added 1 commit 2026-08-01 00:58:23 +02:00

Merge branch 'main' into fix/PMS-685-readiness-timeout 2386c86456

David commented 2026-08-01 01:07:12 +02:00

Owner

Copy link

Closing as superseded by #469, which was merged as 6dce6ab and ships PMS-685 on main.

Both PRs implement the same issue. Where they overlap, #469's approach is the one that landed:

• DB ping bound. This PR wraps the ping inline in ready_check with tokio::time::timeout(Duration::from_secs(3), db.health_check()). #469 extracts the same 3s bound into a READY_DB_PING_TIMEOUT const and a bounded_db_ping() helper that takes the ping future rather than the pool, which makes the timeout path unit-testable on tokio's paused clock. It ships three such tests (slow_but_answering_db_ping_is_ok, hung_db_ping_times_out, failing_db_ping_surfaces_the_error) that this PR has no equivalent for.
• Infisical env leak. This PR unsets INFISICAL_BASE_URL in the justfile test-integration recipe. #469 fixes it inside tests/readiness.rs with an unconfigure_infisical() helper each /ready case calls, so the suite's stated precondition holds no matter what the surrounding shell or harness exports, not only under that one recipe. INFISICAL_BASE_URL is read by exactly one test file, so the recipe-level unset adds nothing on top.

This branch also never merged past 43587d2 (Release v0.9.0), so it predates #469 and now conflicts with main on src/api/router.rs. Rebasing it would resolve to an empty diff.

The diagnosis in this PR's description was correct and matches what #469 concluded independently: there was no 1s DB-ping timeout in the code, and the observed ~1002ms latency was the Infisical probe against a host that the opt-in infisical compose profile never started.

One thing neither PR addressed, now filed separately as PMS-707: compose.dev.yml sets INFISICAL_BASE_URL unconditionally on the server service while infisical is profile-gated, so a plain just dev stack reports /api/v1/ready 503 permanently. That is a dev-stack config defect rather than a probe defect, so it is out of scope for PMS-685.

The fix/PMS-685-readiness-timeout branch is being deleted alongside this close.

Closing as superseded by #469, which was merged as `6dce6ab` and ships PMS-685 on `main`. Both PRs implement the same issue. Where they overlap, #469's approach is the one that landed: - **DB ping bound.** This PR wraps the ping inline in `ready_check` with `tokio::time::timeout(Duration::from_secs(3), db.health_check())`. #469 extracts the same 3s bound into a `READY_DB_PING_TIMEOUT` const and a `bounded_db_ping()` helper that takes the ping *future* rather than the pool, which makes the timeout path unit-testable on tokio's paused clock. It ships three such tests (`slow_but_answering_db_ping_is_ok`, `hung_db_ping_times_out`, `failing_db_ping_surfaces_the_error`) that this PR has no equivalent for. - **Infisical env leak.** This PR unsets `INFISICAL_BASE_URL` in the `justfile` `test-integration` recipe. #469 fixes it inside `tests/readiness.rs` with an `unconfigure_infisical()` helper each `/ready` case calls, so the suite's stated precondition holds no matter what the surrounding shell or harness exports, not only under that one recipe. `INFISICAL_BASE_URL` is read by exactly one test file, so the recipe-level unset adds nothing on top. This branch also never merged past `43587d2` (Release v0.9.0), so it predates #469 and now conflicts with `main` on `src/api/router.rs`. Rebasing it would resolve to an empty diff. The diagnosis in this PR's description was correct and matches what #469 concluded independently: there was no 1s DB-ping timeout in the code, and the observed ~1002ms latency was the Infisical probe against a host that the opt-in `infisical` compose profile never started. One thing neither PR addressed, now filed separately as PMS-707: `compose.dev.yml` sets `INFISICAL_BASE_URL` unconditionally on the `server` service while `infisical` is profile-gated, so a plain `just dev` stack reports `/api/v1/ready` 503 permanently. That is a dev-stack config defect rather than a probe defect, so it is out of scope for PMS-685. The `fix/PMS-685-readiness-timeout` branch is being deleted alongside this close.

David closed this pull request 2026-08-01 01:07:36 +02:00

All checks were successful

Hide checks

E2E / Playwright against staging (pull_request) Successful in 1m22s

Required

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 5m28s

Required

Details

Create release / Gate (release-branch merges only) (pull_request) Successful in 1s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

Integration / integration tests (pull_request) Successful in 13m43s

Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

psa-systems/mokosh-server!466

No description provided.