fix: retry the startup verifying-key fetch instead of panicking#192
Conversation
Cryptify fetched the IBS verifying key from pg-pkg once at startup and panicked if the PKG was unreachable or the response unparsable, creating a hard restart-order dependency (cryptify -> pg-pkg -> irma server): a brief PKG unavailability window during a rolling deploy took cryptify down with it. The fetch now retries transient failures with exponential backoff (500ms..10s) for a 120s budget, mirroring the existing /api-key/validate retry pattern, and runs the blocking minreq call via spawn_blocking. If the budget is exhausted the process still exits with a clear error so a misconfigured pkg_url does not fail silently. Fixes encryption4all/postguard#235
|
Thanks for the PR! I'm on it — kicking off a review of the retry/backoff logic in |
There was a problem hiding this comment.
Reviewed the startup verifying-key retry fix. Clean, well-scoped, and properly tested — approving.
- Correctness:
try_fetch_verifying_keyretries transient failures (unreachable / non-2xx / unparsable body) with exponential backoff against a 120s deadline, and the blockingminreqcall is correctly moved off the async workers viaspawn_blocking. Exhausting the budget still panics with a clear message, so a genuinely misconfiguredpkg_urldoesn't fail silently. Mirrors the existing/api-key/validateretry pattern and its constants style. - Tests: two committed regression tests (
verifying_key_fetch_survives_transient_pkg_outage,verifying_key_fetch_gives_up_after_budget) exercise both the recover-after-outage and give-up-after-budget paths via a thread-based flaky server. Meets the fix-needs-a-regression-test bar. - Rules sweep (Rust fmt/clippy, tests-on-fixes, conventional PR title, closing-keyword/cross-repo link format): all clean. Title is conventional (
fix:) and thepr-title.ymlcheck applies;Fixes encryption4all/postguard#235uses the correct fully-qualified cross-repo form.
One non-blocking observability nit inline.
| Ok(response) => match response.json::<Parameters<VerifyingKey>>() { | ||
| Ok(vk) => return Some(vk), | ||
| Err(e) => log::warn!( | ||
| "Failed to parse verification key from {pkg_params_url}: {e} — will retry" |
There was a problem hiding this comment.
Non-2xx PKG responses (e.g. a 503 during a rolling deploy — the exact transient case this PR targets) land in the JSON-parse-failure branch, because minreq returns Ok(response) for any completed HTTP exchange regardless of status. So an operator debugging a slow startup sees Failed to parse verification key ... rather than a status/reachability message. The sibling validate() retry loop (~L300) explicitly distinguishes returned status {} from parse failures; checking response.status_code here would give the same clarity. Behaviour is correct (it does retry — verified by verifying_key_fetch_survives_transient_pkg_outage); purely an observability nit.
|
Approved #192 👍 The retry/backoff fix looks solid — nice mirroring of the existing One small non-blocking nit left inline: a 503 response body currently falls into the JSON-parse-failure log branch, so the log reads "Failed to parse verification key" instead of surfacing the actual status. Checking |
|
Addressed, /dobby: non-2xx responses are now logged as |
|
Thanks for the update! I'm on it — kicking off a review of the PR now to check the fix and the test coverage, and I'll follow up with a consolidated review shortly. |
There was a problem hiding this comment.
Reviewed the addressed change on HEAD d005245.
The non-2xx branch in try_fetch_verifying_key now surfaces a status problem (PKG at … returned status {} — will retry) before attempting a parse, mirroring the validate() retry loop's wording — so an operator watching a slow startup sees the real reachability/status issue instead of a misleading parse-failure. The match ordering (non-2xx guard → parse → transport error) is correct, the deadline/backoff loop terminates cleanly (verified against verifying_key_fetch_gives_up_after_budget), and the 503 path is now exercised by the flaky-server regression test.
Full quality suite is green (cargo fmt --all -- --check, clippy --all-targets -- -D warnings, and 149/149 tests including both new regression tests). No actionable findings — good to merge.
|
Reviewed the update — the retry loop now logs |
Fixes encryption4all/postguard#235 — same startup-fragility class as encryption4all/postguard#233 (PKG side), found via the e2e harness.
Before: the startup fetch of
/v2/sign/parameterspanicked on the first failure (src/main.rsinrocket()), so cryptify's boot hard-depended on the PKG already being up — a brief PKG unavailability window during a rolling deploy took cryptify down.After:
try_fetch_verifying_keyretries transient failures (unreachable, non-2xx, unparsable body) with exponential backoff (500ms → 10s) for a 120s budget, mirroring the existing/api-key/validateretry pattern and its constants style. The blockingminreqcall now runs viaspawn_blockinginstead of on the async worker. If the budget is exhausted the process still exits with a clear error, so a genuinely misconfiguredpkg_urldoesn't fail silently.Regression tests (thread-based flaky
/v2/sign/parametersserver):verifying_key_fetch_survives_transient_pkg_outage— two 503s then a valid key → succeeds, ≥3 attempts observedverifying_key_fetch_gives_up_after_budget— permanent failure → returnsNoneafter the budgetcargo clippy --all-targetsclean (CI runs-D warnings); full suite 149 passed.