fix(ci/deploy-dev): smoke-test gates on /health/ready, not /api/info - #174
Merged
Conversation
Promote: staging -> develop
* feat(api): add GET /api/history endpoint (#153) (#162) * feat(api): add GET /api/history endpoint (#153) Adds paginated per-address transaction history reading from the account_history table populated by the migration-0008 trigger. * /api/history?address=<hex>&limit=<n>&offset=<n>, behind 'always' status (no feature gate). * Reuses the same hex decode + 32-byte length rules /api/balance applies; rejects missing address, invalid hex, limit outside [1, 200], negative offset with 400. * Newest-first ORDER BY changed_at DESC; offset beyond total returns empty items with the unfiltered total so the caller can detect end-of-list. * LEFT JOIN observed_inscriptions + pending_inscriptions on triggering_commit_txid so block_height + status surface once a future caller threads zkcoins.account_commit_txid through the upsert. Today both joined columns are NULL and txid/block_height remain null on the wire. * counterparty + memo intentionally null in v1: the current schema does not store the recipient address per-mutation and has no memo column. Out-of-scope for this PR: app UI changes, OpenAPI export, WebSocket push, the Zod schema in zk-coins/app. * README Features table entry added. * fix(api/history): round-2 review fixes — SQL-side filter, pending default, 422, one DB call Six fixes from the two independent reviews of #153: 1. Push `source IN ('mint','send','receive')` into both the page query and the filtered total so pagination is correct. The post-fetch `filter_map` stays as a defense-in-depth safety net but no longer actually drops rows. Closes a bug where `total` over-counted hidden rows and pages came back smaller than the requested `limit`. 2. Status default flips from `confirmed` to `pending`. A DB-committed `account_history` row only proves a server-side state change, not an on-chain confirmation. The new mapping: * pending_inscriptions.status='complete' -> 'confirmed' * pending_inscriptions.status='failed' -> 'failed' * pending_inscriptions.status IN ('constructed', 'commit_broadcast','reveal_broadcast') -> 'pending' * no pending row + observed_inscriptions.block_height IS NOT NULL -> 'confirmed' * no pending row + no observed row -> 'pending' The match goes through a new `PendingInscriptionStatus` enum so the `match` is exhaustive — a future schema state addition fails to compile (no `_ => "pending"` catch-all). 3. Collapse `count_account_history` + `list_account_history` into one round-trip via a CTE: one filtered-count CTE cross-joined to the LIMIT/OFFSET page CTE. The handler now has a single DB error branch, closing the dead-arm coverage gap the two-call layout left behind. The empty-page case still returns the real total (sentinel row) so the caller can drive pagination without a second query. 4. Switch all input-validation status codes from 400 to 422 to match `/api/balance` and the rest of the read surface. Framework-level 400 (axum Query rejection on non-integer limit) stays. 5. A non-null `prev_data` blob that fails to bincode-decode no longer silently collapses to `prev_balance = 0` (which would fabricate the full new balance as the delta). The row is dropped with a warn log. 6. TODOs for the deferred work reference the follow-up issues: * #159 — thread `zkcoins.account_commit_txid` GUC * #160 — capture counterparty_address per row (zk-coins/app#145 covers the typed-client wiring on the other repo.) * fix(api/history): hoist blob.len() out of tracing::warn! for coverage The tracing macro lazily evaluates its arguments based on the active log level, so under the default-off test subscriber blob.len() is never executed. Coverage Gate flagged it as the only uncovered line in the node + shared scope (99.97% lines, 1 missed). Pre-compute blob_len in a let binding so the line runs regardless of tracing config. * docs: clarify operator language — api.zkcoins.app runs at zkcoins.app, not DFX (#158) Three spots in README.md leaked the DFX-as-operator framing: 1. Trust Model table row was "Yes — DFX runs the hosted node". The hosted node at api.zkcoins.app is operated by zkcoins.app (one of hopefully many such service providers). DFX is the underlying hosting / financial-services layer — invisible to wallet integrators and consumers of this README. 2. Configuration table description for ESPLORA_URL said "PRD: ... (DFX Mainnet stack)". Replaced with "On the api.zkcoins.app stack: PRD ..., DEV ...". Same meaning, correct attribution. 3. ESPLORA_WS_URL description analog — "on the DFX mempool/backend stack" → "self-hosted mempool/backend sidecar" (the relevant detail is "self-hosted vs external", not "whose hosting"). Memory: reference_zkcoins_org_structure documents the three-layer separation (zkCoins protocol / zkcoins.app service provider / DFX infra) so this distinction does not get muddled again. No code change. Pre-push sanity: fmt clean, clippy clean. * perf(bootstrap): warmup prover in background; gate /health/ready on prover_warm (#154) * perf(bootstrap): warmup prover in a background task; gate /health/ready on prover_warm PR #147 paid the ~7 s Plonky2 cold-prove tax synchronously between load_from_pg and TcpListener::bind, which pushed API offline time per deploy from ~14 s (circuit build alone) to ~21 s (circuit build + cold prove). The user constraint is explicit: API must be reachable as soon as possible. PR #147 was closed for failing that constraint. This shape moves the warmup off the bootstrap-critical path: 1. TcpListener::bind returns at ~0.1 s. axum::serve starts draining connections — /health (liveness) is 200, /api/* paths return correct answers (a /api/mint or /api/send during the warmup window pays the ~7 s cold tax, but it serves correctly). 2. tokio::task::spawn_blocking launches AccountNode::warmup_prover on the blocking pool so the CPU-bound prove does not starve the tokio worker that owns axum::serve. 3. After ~21 s the warmup task flips the new prover_warm Arc<AtomicBool> to true. /health/ready transitions from 503 with {"status":"starting","prover":"warming","failures":["prover"]} to 200 with {"status":"ready","prover":"ready"}. A load balancer / Kuma monitor keyed on /health/ready holds traffic on the previous-generation pod through the warmup window; /health (liveness) is unaffected so container restart loops are not triggered. ZKCOINS_SKIP_BOOTSTRAP_WARMUP=1 skips the background task entirely (smoke tests in runtime_tests.rs). Three architecture decisions, codified in MIGRATION_RESEARCH.md §7.25: - spawn_blocking over tokio::spawn — Plonky2 prove is CPU-bound and would starve the tokio worker dispatching HTTP requests. - Arc<AtomicBool> over Arc<RwLock<bool>> — flag is write-once + read-many, AtomicBool::store is a single instruction. - std::process::exit(1) over panic!() — a panic inside spawn_blocking only surfaces when the JoinHandle is awaited (it deliberately is not), so a bare panic would leave the node serving 503 forever. exit(1) crash-loops the container at the same severity as PR #147's synchronous expect(). CONTRIBUTING.md gains a Bootstrap timing section + a row for the new env var. AccountNode::warmup_prover + the warmup_prover_completes_successfully test were adapted from PR #147 with the return type switched to anyhow::Result for the runtime call site. A new router test asserts /health/ready returns 503 with the warming-tag payload when prover_warm is false. * docs(runtime): correct warmup-task scanner-ordering comment Reviewer caught: the prior comment claimed the scanner spawns AFTER start_rest_node returns. That is factually wrong — main.rs runs start_rest_node + run_scanner_ws concurrently via tokio::spawn. The correctness conclusion still holds because the scanner locks `state`, not `account_node`. Rewrite the comment to name the right invariant and the right contender (a user request that lands during the ~7 s warmup window). * test(coverage): drop unused .map_err closure in warmup_prover The previous shape `.map(|_| ()).map_err(|e| anyhow!("...{e}"))` left two never-called closures in the happy-path test, costing the 100% function + 100% line coverage gate. `?` propagation matches what `prove_initial` already returns (`anyhow::Result<Proof>`) and is covered by the same single test. --------- Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
Promote: staging -> develop
Post-deploy smoke test was checking `/api/info` for HTTP 200, but after #154 the node binds the HTTP listener BEFORE the Plonky2 prover warmup completes — `/api/info` returns 200 within seconds while `/health/ready` stays at `{"ready":false,"prover":"warming"}` for the 10-30 s warmup window. Downstream jobs (API E2E preflight against `/health/ready` + `/health/publisher`) raced the warmup: the E2E job picked the runner up ~4 s after the deploy job reported success, hit `/health/ready` once, got back the warming snapshot, and failed with `::error::/health/ready not ready` — observed empirically on Release PR #166's run https://github.com/zk-coins/node/actions/runs/26793933906/job/78986599030. Switch the smoke loop to `/health/ready` + a `jq '.ready == true'` assertion, keeping the 30-attempt × 10-s budget (~5 min) so a genuine bootstrap stall still surfaces with the same timeout behaviour. The deploy job now only reports success once the node is actually ready for traffic, which removes the race the E2E preflight was tripping over. `/api/info` is no longer a deploy-success signal. The E2E preflight retains its explicit `/health/ready` + publisher-wallet gate as a sanity check (still a single shot — it relies on the smoke test having already enforced readiness). The deploy job runs on `ubuntu-24.04-arm` where `jq` is part of the default GitHub-hosted image; no install step needed here.
TaprootFreak
marked this pull request as ready for review
June 2, 2026 07:45
TaprootFreak
added a commit
that referenced
this pull request
Jun 2, 2026
…174) (#175) Post-deploy smoke test was checking `/api/info` for HTTP 200, but after #154 the node binds the HTTP listener BEFORE the Plonky2 prover warmup completes — `/api/info` returns 200 within seconds while `/health/ready` stays at `{"ready":false,"prover":"warming"}` for the 10-30 s warmup window. Downstream jobs (API E2E preflight against `/health/ready` + `/health/publisher`) raced the warmup: the E2E job picked the runner up ~4 s after the deploy job reported success, hit `/health/ready` once, got back the warming snapshot, and failed with `::error::/health/ready not ready` — observed empirically on Release PR #166's run https://github.com/zk-coins/node/actions/runs/26793933906/job/78986599030. Switch the smoke loop to `/health/ready` + a `jq '.ready == true'` assertion, keeping the 30-attempt × 10-s budget (~5 min) so a genuine bootstrap stall still surfaces with the same timeout behaviour. The deploy job now only reports success once the node is actually ready for traffic, which removes the race the E2E preflight was tripping over. `/api/info` is no longer a deploy-success signal. The E2E preflight retains its explicit `/health/ready` + publisher-wallet gate as a sanity check (still a single shot — it relies on the smoke test having already enforced readiness). The deploy job runs on `ubuntu-24.04-arm` where `jq` is part of the default GitHub-hosted image; no install step needed here. Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Release PR #166's E2E job failed with:
Run: https://github.com/zk-coins/node/actions/runs/26793933906/job/78986599030
Root cause
Post-deploy smoke test was checking `/api/info` for HTTP 200, but after #154 the node binds the HTTP listener BEFORE the Plonky2 prover warmup completes. `/api/info` returns 200 within seconds while `/health/ready` stays at `{"ready":false,"prover":"warming"}` for the 10-30 s warmup window.
Downstream jobs (API E2E preflight against `/health/ready` + `/health/publisher`) raced the warmup: the E2E job picked the runner up ~4 s after the deploy job reported success, hit `/health/ready` once, got back the warming snapshot, and failed.
Fix
Switch the smoke loop's target from `/api/info` to `/health/ready` + a `jq '.ready == true'` assertion. Same 30-attempt × 10-s budget (~5 min) so a genuine bootstrap stall still surfaces with the same timeout behaviour.
The deploy job runs on `ubuntu-24.04-arm` where `jq` is part of the default GitHub-hosted image; no install step needed.
Test plan
Out of scope