fix(pkg): fetch the IRMA JWT verification key lazily instead of at startup

[rubenhensen]

Fixes #233 — found by the e2e harness (encryption4all/postguard-e2e) as a CI cold-start race.

Before: the Jwt auth middleware fetched {IRMA_SERVER}/publickey during worker init; if the IRMA server was briefly unreachable, every worker failed and the PKG exited (Error: IO error: can not start server service 0). The unbounded reqwest::get could also hang worker startup on a stalled connect.

After: AuthMethods::Jwt holds a JwtKeyCache that fetches the key (5s timeout) on the first JWT request:

• startup does no network I/O — no crash, no hang, no restart-order dependency on the IRMA server
• while the IRMA server is unreachable, JWT-authenticated requests return 503 (UpstreamError) and recover on their own once it's back

Regression test: test_jwt_auth_starts_when_irma_unreachable — middleware init against an unreachable IRMA URL must succeed, and the request must fail with 503 (previously init_service itself died).

Note: the key is still cached forever once fetched — refreshing it on rotation is #234, which I'll stack on this PR.

[dobby-coder]

Thanks for the detailed writeup! I'm on it — kicking off a review of PR #240 now, checking the JwtKeyCache changes and the regression test for correctness and rule-compliance. I'll follow up shortly with a consolidated review.

[dobby-coder]

Approving — clean, well-scoped fix.

Startup no longer does network I/O, so a briefly-unreachable IRMA server at boot no longer crashes or hangs the PKG; JWT requests degrade to 503 (UpstreamError) and self-recover once IRMA is back. The RefCell-based lazy cache is sound: no borrow is held across an .await (the condition Ref is dropped before fetch().await, and decode() is synchronous). Fixes #233, conventional PR title, and a committed regression test (test_jwt_auth_starts_when_irma_unreachable). Build/tests confirmed locally (cargo test -p pg-pkg 54/54, fmt + clippy clean).

One non-blocking nit inline; key rotation/refresh is already tracked in #234.

[dobby-coder]

Nit (non-blocking): while the IRMA server is unreachable, decode() re-runs fetch() — up to the full 5s timeout — on every JWT request, with no negative caching, backoff, or in-flight de-duplication. Under a burst of JWT traffic during an IRMA outage this can tie up each worker for ~5s per request. This is the intended self-recovery tradeoff, and key caching/refresh is deferred to #234, so just flagging in case a short negative-cache/backoff is wanted here too.

[dobby-coder]

Reviewed and approved 👍 — nice clean fix for the cold-start race. I checked that the RefCell-based lazy cache is sound (no borrow held across an .await), and the rule checks all pass (title, Fixes #233, regression test, fmt/clippy clean).

Left one non-blocking nit inline: during an IRMA outage, decode() re-runs the 5s-timeout fetch() on every request (no negative caching/backoff/in-flight dedup), which could tie up a worker ~5s per request under load. Sounds like a reasonable tradeoff for now, especially with key rotation/refresh already tracked in #234.

Full review: #240 (review)

[rubenhensen]

Addressed the nit, /dobby: failed key fetches are now negatively cached (FETCH_FAILURE_BACKOFF = 3s) — during an IRMA outage a burst of JWT traffic pays for one fetch timeout and everything else fails fast with 503, resuming automatically after the window. New regression test test_jwt_key_fetch_failure_is_negatively_cached asserts exactly one upstream fetch across a burst, and that fetching resumes after the backoff.

[dobby-coder]

Thanks for the update! I'm on it — kicking off a review of PR #240 now, focusing on the negative caching fix and the new test_jwt_key_fetch_failure_is_negatively_cached regression test.

[dobby-coder]

APPROVE — clean, well-scoped fix for the cold-start race in #233.

• Startup no longer does network I/O; the key is fetched lazily on the first JWT request with a 5s timeout, so an unreachable IRMA server degrades to 503 instead of killing the worker. ✔
• The follow-up negatively cache failed JWT key fetches commit correctly collapses the back-to-back (sequential) retry storm during an outage into one fetch per 3s window. ✔
• Both regression tests are committed; prior verification run: pg-pkg 55 tests pass (incl. both new tests), cargo fmt + build clean.
• Fixes #233 closing keyword present; title is conventional-commit compliant (checked by pr-title.yml).

One optional, non-blocking note inline on the negative-cache concurrency window. Good to merge.

[dobby-coder]

Non-blocking nit (carried over from the earlier review — the negative cache handles most of it): the "one upstream fetch per outage" guarantee holds for sequential requests and for anything arriving after the first fetch fails, but not for a truly concurrent burst during the very first in-flight fetch. last_failure is set only after self.fetch().await completes (line 292) while key stays None, so on a single-threaded worker, JWT requests that arrive while the first ~5s fetch is still awaiting each pass the key.is_none() gate (decode, line 305) and the backoff check here, and start their own fetch — a small thundering herd of in-flight timeouts, not one.

The new test awaits each decode sequentially, so it can't exercise this. It degrades safely to 503 regardless, so this is fine to leave — but if the stronger "exactly one fetch across a burst" guarantee is intended, marking an in-flight/pending state before awaiting fetch() would close the gap. Could also fold into #234 when you add key refresh.

[dobby-coder]

Nice, that closes it out — the negative-cache backoff is exactly what was needed for the retry-storm case, so I approved PR #240. Tests, fmt, and build are all clean.

Left one optional, non-blocking note: a truly concurrent burst during the very first in-flight fetch can still each start their own fetch (since last_failure is only set after that fetch completes) — but it degrades safely to 503 either way, so it's fine to merge as-is. Could fold an in-flight marker into #234 later if you want to close that gap too.

[rubenhensen]

Addressed the concurrency carry-over too, /dobby: fetches are now de-duplicated in flight on an async lock (futures::lock::Mutex) with a double-check — during a concurrent burst while the first fetch is still awaiting, one request does the network I/O and the rest wait, then observe either the fresh key or the fresh negative cache. One upstream fetch per burst, for both the sequential and the truly-concurrent case.

New regression test test_concurrent_jwt_requests_share_one_fetch: three concurrent decode()s against a slow (300ms) failing key server → all fail cleanly, exactly 1 upstream fetch observed (previously 3).

Also merged forward into #241, where ensure_key (first use, early-returns for waiters) and refresh (rotation, always re-fetches) share a locked fetch_and_record — 57/57 tests pass there.

[dobby-coder]

Thanks for the detailed update, @rubenhensen! I've picked this up and I'm reviewing the concurrency fix (async lock with double-check, the new test_concurrent_jwt_requests_share_one_fetch regression test) along with the forward-merge into #241. I'll follow up with a consolidated review shortly.

[dobby-coder]

Nice work — #240 is already merged (with two approving reviews), and the diff has exactly the de-dup fix you described: the futures::lock::Mutex-based fetch_lock with double-check, plus test_concurrent_jwt_requests_share_one_fetch confirming 1 upstream fetch across 3 concurrent decode()s. Nothing left to gate.

Only thing worth a follow-up (non-blocking): auth.rs's new tests only exercise garbage-PEM failure paths, so the happy-path caching behavior (fetch once, then serve from cache) has no positive-path regression test. I manually verified it works fine — just a coverage gap to pick up whenever #241 lands.

PR #240