From 232f80e5175012b590993f81bb393701498b81ed Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 22:36:22 -0700 Subject: [PATCH 1/2] fix(oidc-provider): never block a caller on another's credential renewal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CredentialCache::get_or_fetch` took a per-key `futures::lock::Mutex` before the freshness check, so concurrent callers for a key awaited whoever was minting — including callers whose cached credential was perfectly fresh, since the fast path took the same lock. On Cloudflare Workers, which `multistore-cf-workers` exists to serve, awaiting that lock is fatal rather than slow. A request parked on an in-memory waker has no pending I/O of its own, so the runtime cancels it — "your Worker's code had hung and would never generate a response" — instead of waiting for the lock holder. A single isolate renewing one key therefore killed every concurrent request touching that key. Sharing the in-flight future instead of the lock is not an escape either; that trips "Cannot perform I/O on behalf of a different request". Serving stale-but-valid credentials removes the need for the lock. Inside the refresh lead the credential has not expired, so one caller claims the renewal and the rest keep using what they already have — single-flight without anyone waiting, because latecomers never need the claimant's result. Native consumers gain from the same change: a renewal no longer stalls every concurrent caller for the length of a token round-trip. Also fixes a cache-key defect. Entries were keyed on the caller-supplied scope alone (the role ARN), while `AwsBackendAuth` mints a distinct per-connection `sub`. Since an AWS role's trust policy conditions on that `sub`, a subject the policy would reject could be served a credential minted for one it accepts — succeeding where STS would have refused. `get_credentials` now scopes the entry by subject as well. Behaviour change: a cold key (absent or expired) is deliberately not single-flighted — there is nothing valid to serve, so collapsing the burst would mean waiting. The duplication is bounded to one burst per process or isolate cold start; the recurring event, renewal, is collapsed. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 21 +- crates/oidc-provider/Cargo.toml | 2 - crates/oidc-provider/src/cache.rs | 407 +++++++++++++++++++++++++----- crates/oidc-provider/src/lib.rs | 42 ++- docs/architecture/caching.md | 25 +- 5 files changed, 403 insertions(+), 94 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 765cbd5..983e46d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1314,7 +1314,7 @@ dependencies = [ [[package]] name = "multistore" -version = "0.7.0" +version = "0.7.1" dependencies = [ "async-trait", "base64", @@ -1341,7 +1341,7 @@ dependencies = [ [[package]] name = "multistore-cf-workers" -version = "0.7.0" +version = "0.7.1" dependencies = [ "async-trait", "bytes", @@ -1366,7 +1366,7 @@ dependencies = [ [[package]] name = "multistore-cf-workers-example" -version = "0.7.0" +version = "0.7.1" dependencies = [ "bytes", "console_error_panic_hook", @@ -1391,7 +1391,7 @@ dependencies = [ [[package]] name = "multistore-lambda" -version = "0.7.0" +version = "0.7.1" dependencies = [ "bytes", "http", @@ -1411,7 +1411,7 @@ dependencies = [ [[package]] name = "multistore-metering" -version = "0.7.0" +version = "0.7.1" dependencies = [ "bytes", "futures", @@ -1423,11 +1423,10 @@ dependencies = [ [[package]] name = "multistore-oidc-provider" -version = "0.7.0" +version = "0.7.1" dependencies = [ "base64", "chrono", - "futures", "multistore", "quick-xml 0.41.0", "rand 0.8.5", @@ -1443,7 +1442,7 @@ dependencies = [ [[package]] name = "multistore-path-mapping" -version = "0.7.0" +version = "0.7.1" dependencies = [ "multistore", "percent-encoding", @@ -1452,7 +1451,7 @@ dependencies = [ [[package]] name = "multistore-server" -version = "0.7.0" +version = "0.7.1" dependencies = [ "axum", "bytes", @@ -1474,7 +1473,7 @@ dependencies = [ [[package]] name = "multistore-static-config" -version = "0.7.0" +version = "0.7.1" dependencies = [ "chrono", "multistore", @@ -1486,7 +1485,7 @@ dependencies = [ [[package]] name = "multistore-sts" -version = "0.7.0" +version = "0.7.1" dependencies = [ "aes-gcm", "base64", diff --git a/crates/oidc-provider/Cargo.toml b/crates/oidc-provider/Cargo.toml index 82addee..3eeec9f 100644 --- a/crates/oidc-provider/Cargo.toml +++ b/crates/oidc-provider/Cargo.toml @@ -22,8 +22,6 @@ rsa.workspace = true sha2.workspace = true tracing.workspace = true uuid.workspace = true -# Per-key async lock for single-flight credential refresh (see `cache.rs`). -futures.workspace = true [dev-dependencies] tokio = { workspace = true, features = ["rt", "macros"] } diff --git a/crates/oidc-provider/src/cache.rs b/crates/oidc-provider/src/cache.rs index 6dcb5ee..c519c9c 100644 --- a/crates/oidc-provider/src/cache.rs +++ b/crates/oidc-provider/src/cache.rs @@ -1,4 +1,4 @@ -//! Credential cache with single-flight refresh. +//! Credential cache with non-blocking single-flight refresh. //! //! Caches [`BackendCredentials`] by key so the proxy doesn't re-mint and //! re-exchange on every request. Beyond a plain TTL cache it: @@ -8,9 +8,36 @@ //! - **proactively refreshes** — once a value is within [`REFRESH_LEAD_SECS`] //! of expiry, the next access re-mints it, so a credential is never handed //! out about to expire mid-request, and -//! - **single-flights** — while one caller is minting for a key, concurrent -//! callers for that *same* key await the in-flight result instead of each -//! launching their own exchange. A cold-cache burst collapses to one STS call. +//! - **single-flights renewals** — one caller claims the renewal and the rest +//! keep serving the credential they already have, which has not expired. +//! +//! # Why no lock +//! +//! Renewal is single-flighted *without any caller ever waiting on another*. The +//! map sits behind a `std::sync::Mutex` that is only ever held for a get or an +//! insert and never across an `.await`. +//! +//! This is a hard requirement, not a preference. On Cloudflare Workers — which +//! `multistore-cf-workers` exists to serve — a request parked on an in-memory +//! waker has no pending I/O of its own, and the runtime cancels it with *"your +//! Worker's code had hung and would never generate a response"* rather than +//! waiting for whoever holds the lock. An earlier revision of this cache took a +//! per-key `futures::lock::Mutex` on *both* the hit and miss paths, so a single +//! isolate renewing one key killed every concurrent request touching that key. +//! +//! Serving stale-but-valid credentials is what makes the lock unnecessary: +//! latecomers never need the claimant's result, so there is nothing to wait for. +//! +//! # What is deliberately not single-flighted +//! +//! A **cold** key — absent or already expired — has nothing usable to serve, so +//! concurrent callers each run their own `fetch`. Collapsing those would mean +//! waiting, and the only way to wait on Workers without being cancelled is to +//! poll a real timer — which costs about as long as the exchange it avoids, +//! while adding a spin loop and a dependency on the claimant surviving. The +//! duplication is bounded (one burst per process start, or per isolate cold +//! start) and upstream token endpoints are provisioned for it; the recurring +//! event, renewal, *is* single-flighted. //! //! The fetch happens through a caller-supplied closure ([`get_or_fetch`]), so //! the cache never needs to know how credentials are minted, and a runtime can @@ -19,10 +46,9 @@ use std::collections::HashMap; use std::future::Future; -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, MutexGuard}; -use chrono::{Duration, Utc}; -use futures::lock::Mutex as AsyncMutex; +use chrono::{DateTime, Duration, Utc}; use crate::BackendCredentials; @@ -30,42 +56,80 @@ use crate::BackendCredentials; /// so it is never handed out about to expire mid-request. const REFRESH_LEAD_SECS: i64 = 60; -/// One async-locked slot per key. The per-key [`AsyncMutex`] is what serializes -/// (single-flights) refreshes; the value is shared via `Arc`. -type Slot = Arc>>>; +/// Never serve a credential with less than this much life left, even while a +/// renewal is in flight: the backend request it signs still has to reach the +/// store and complete. Below this the caller mints its own rather than sign +/// with something that may expire in transit — an expired credential comes back +/// from the store as `ExpiredToken`, which surfaces to the client as a +/// misleading `AccessDenied` instead of a retryable server-side error. +const MIN_SERVE_SECS: i64 = 5; -/// Thread-safe credential cache with proactive refresh and single-flight. +/// Treat a renewal claim older than this as abandoned and let another caller +/// take it. Without this, a claimant that never completes — a cancelled or +/// evicted request — leaves the key marked as renewing until the credential +/// expires, at which point every caller stampedes at once. +const CLAIM_TIMEOUT_SECS: i64 = 30; + +/// Compose the cache key for a credential minted for `subject` against `scope` +/// (e.g. an IAM role ARN). /// -/// `Clone` shares the same underlying store (the slot map is behind an `Arc`), +/// The subject is part of a credential's identity, not merely a label on it: an +/// AWS role's trust policy conditions on the assertion's `sub`, so two subjects +/// assuming the same role are **not** interchangeable. Keying on the scope alone +/// let a subject the trust policy would reject be served a credential minted for +/// one it accepts — succeeding where the token endpoint would have refused. +pub(crate) fn scoped_key(scope: &str, subject: &str) -> String { + // U+001F (unit separator) cannot occur in a role ARN or an OIDC subject, so + // the join is unambiguous without escaping either half. + format!("{scope}\u{1f}{subject}") +} + +/// A cached credential and the renewal claim on it, if any. +struct Entry { + creds: Arc, + /// When a caller claimed the renewal of `creds`. `None` when no renewal is + /// in flight; see [`CLAIM_TIMEOUT_SECS`] for how a stuck claim is reaped. + renewing_since: Option>, +} + +/// What a caller should do for a key, decided under the map lock. +enum Action { + /// Use this credential as-is; no `fetch` needed. + Serve(Arc), + /// Nothing usable, or this caller took the renewal claim: run `fetch`. + Fetch, +} + +/// Thread-safe credential cache with proactive, non-blocking refresh. +/// +/// `Clone` shares the same underlying store (the entry map is behind an `Arc`), /// so a cloned [`OidcCredentialProvider`](crate::OidcCredentialProvider) keeps /// hitting the same cache — letting a runtime hold the provider in a /// shared/`static` slot and reuse it across requests instead of re-minting and /// re-exchanging every time. #[derive(Clone, Default)] pub struct CredentialCache { - /// One slot per key. The outer `Mutex` only guards insertion into the map - /// and is never held across an `.await`; the per-key [`AsyncMutex`] inside - /// each [`Slot`] is what single-flights refreshes. - slots: Arc>>, + /// The `Mutex` guards map reads and writes only, and is never held across + /// an `.await` — see the module docs for why that matters. + entries: Arc>>, } impl CredentialCache { /// Create an empty credential cache. pub fn new() -> Self { Self { - slots: Arc::new(Mutex::new(HashMap::new())), + entries: Arc::new(Mutex::new(HashMap::new())), } } - /// Return cached credentials for `key` if still fresh, otherwise run `fetch` - /// (single-flighted) to obtain and cache new ones. - /// - /// A cached value is fresh while `now < expiration - REFRESH_LEAD_SECS`. + /// Return cached credentials for `key`, running `fetch` only when this + /// caller is the one that must. /// - /// Single-flight: while one caller is running `fetch` for a key, concurrent - /// callers for that same key block on the per-key lock; when it releases - /// they observe the freshly-cached value and return it without calling their - /// own `fetch`. + /// A cached value is served outright while fresh (`now < expiration - + /// REFRESH_LEAD_SECS`). Inside the refresh lead the first caller claims the + /// renewal and runs `fetch`; concurrent callers keep receiving the cached + /// value — which has not expired — instead of waiting on that claim. No + /// caller ever blocks on another. pub async fn get_or_fetch( &self, key: &str, @@ -75,35 +139,69 @@ impl CredentialCache { F: FnOnce() -> Fut, Fut: Future, E>>, { - let slot = self.slot(key); - let mut guard = slot.lock().await; - - if let Some(creds) = guard.as_ref() { - if is_fresh(creds) { - return Ok(creds.clone()); + if let Action::Serve(creds) = self.claim(key) { + return Ok(creds); + } + match fetch().await { + Ok(creds) => { + self.entries().insert( + key.to_string(), + Entry { + creds: creds.clone(), + renewing_since: None, + }, + ); + Ok(creds) + } + Err(e) => { + // Drop the claim so the next caller retries rather than serving + // a credential nobody is renewing until it expires. + if let Some(entry) = self.entries().get_mut(key) { + entry.renewing_since = None; + } + Err(e) } } + } - let fresh = fetch().await?; - *guard = Some(fresh.clone()); - Ok(fresh) + /// Decide what a caller should do for `key`, taking the renewal claim when + /// this caller is the one that will run `fetch`. + /// + /// Not a lock: the returned [`Action`] never depends on another caller + /// making progress. + fn claim(&self, key: &str) -> Action { + let now = Utc::now(); + let mut entries = self.entries(); + let Some(entry) = entries.get_mut(key) else { + return Action::Fetch; + }; + + if entry.creds.expiration > now + Duration::seconds(REFRESH_LEAD_SECS) { + return Action::Serve(entry.creds.clone()); + } + + let claimed = entry + .renewing_since + .is_some_and(|at| now < at + Duration::seconds(CLAIM_TIMEOUT_SECS)); + if claimed && entry.creds.expiration > now + Duration::seconds(MIN_SERVE_SECS) { + return Action::Serve(entry.creds.clone()); + } + + // Either we are first into the refresh lead, the previous claim went + // stale, or the credential is too close to expiry to hand out. Take the + // claim and mint. When there is nothing usable left this gates no one — + // concurrent callers have no credential to serve either. + entry.renewing_since = Some(now); + Action::Fetch } - fn slot(&self, key: &str) -> Slot { - self.slots + fn entries(&self) -> MutexGuard<'_, HashMap> { + self.entries .lock() .expect("credential cache mutex poisoned") - .entry(key.to_string()) - .or_insert_with(|| Arc::new(AsyncMutex::new(None))) - .clone() } } -/// A credential is fresh while it is more than [`REFRESH_LEAD_SECS`] from expiry. -fn is_fresh(creds: &BackendCredentials) -> bool { - creds.expiration > Utc::now() + Duration::seconds(REFRESH_LEAD_SECS) -} - #[cfg(test)] mod tests { use super::*; @@ -186,42 +284,211 @@ mod tests { assert!(fetched); } + /// A caller must never wait on another caller's in-flight renewal while the + /// cached credential is still usable. + /// + /// Waiting is not merely slow. On Cloudflare Workers a request parked on an + /// in-memory waker has no pending I/O of its own, and the runtime cancels it + /// outright ("your Worker's code had hung and would never generate a + /// response"), so this ordering is a correctness requirement there. #[tokio::test] - async fn single_flights_concurrent_fetches() { - let cache = Arc::new(CredentialCache::new()); - let calls = Arc::new(AtomicUsize::new(0)); - - let one = { - let cache = cache.clone(); - let calls = calls.clone(); - async move { - cache - .get_or_fetch("k", || async { - calls.fetch_add(1, Ordering::SeqCst); - // Yield while holding the per-key lock so the sibling - // future contends for it — exercising single-flight. + async fn serves_a_usable_credential_without_waiting_for_a_renewal() { + let cache = CredentialCache::new(); + // Seed a credential inside the refresh lead: due for renewal, still valid. + cache + .get_or_fetch("k", || async { Ok::<_, ()>(creds(30)) }) + .await + .unwrap(); + + let order = Mutex::new(Vec::new()); + + let renewing = async { + cache + .get_or_fetch("k", || async { + // Stay in flight long enough for the sibling to arrive. + for _ in 0..8 { tokio::task::yield_now().await; - Ok::<_, ()>(creds(600)) - }) - .await - } + } + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + order.lock().unwrap().push("renewal"); }; - let two = { - let cache = cache.clone(); - let calls = calls.clone(); - async move { + let serving = async { + cache + .get_or_fetch::<_, _, ()>("k", || async { + panic!("must not mint while a usable credential is cached") + }) + .await + .unwrap(); + order.lock().unwrap().push("served"); + }; + + tokio::join!(renewing, serving); + + assert_eq!( + order.lock().unwrap().first().copied(), + Some("served"), + "the cached credential should be served immediately, not after the renewal completes" + ); + } + + /// Renewal — the recurring event — collapses to a single exchange no matter + /// how many callers arrive during it. + #[tokio::test] + async fn single_flights_a_renewal() { + let cache = CredentialCache::new(); + cache + .get_or_fetch("k", || async { Ok::<_, ()>(creds(30)) }) + .await + .unwrap(); + + let calls = AtomicUsize::new(0); + let renewing = async { + cache + .get_or_fetch("k", || async { + calls.fetch_add(1, Ordering::SeqCst); + for _ in 0..8 { + tokio::task::yield_now().await; + } + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + }; + let others = async { + for _ in 0..10 { cache .get_or_fetch("k", || async { calls.fetch_add(1, Ordering::SeqCst); - Ok::<_, ()>(creds(600)) + Ok::<_, ()>(creds(3600)) }) .await + .unwrap(); + tokio::task::yield_now().await; } }; - let (a, b) = tokio::join!(one, two); - a.unwrap(); - b.unwrap(); - assert_eq!(calls.load(Ordering::SeqCst), 1, "fetch should run once"); + tokio::join!(renewing, others); + assert_eq!( + calls.load(Ordering::SeqCst), + 1, + "only the claimant should mint during a renewal" + ); + } + + /// A credential too close to expiry is never served, even while a renewal is + /// claimed: signing with it risks the backend rejecting an expired token + /// mid-flight, which reaches the client as a misleading `AccessDenied`. + #[tokio::test] + async fn does_not_serve_a_credential_about_to_expire() { + let cache = CredentialCache::new(); + cache + .get_or_fetch("k", || async { Ok::<_, ()>(creds(MIN_SERVE_SECS - 1)) }) + .await + .unwrap(); + // Claims the renewal, and leaves the near-expired credential cached. + cache + .get_or_fetch("k", || async { + Err::, _>("STS is throttling") + }) + .await + .unwrap_err(); + + let mut fetched = false; + cache + .get_or_fetch("k", || async { + fetched = true; + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + assert!( + fetched, + "must mint rather than serve a credential about to expire" + ); + } + + /// A claimant that never completes (a cancelled or evicted request) must not + /// keep the key marked as renewing forever. + #[tokio::test] + async fn reclaims_an_abandoned_renewal() { + let cache = CredentialCache::new(); + cache + .get_or_fetch("k", || async { Ok::<_, ()>(creds(30)) }) + .await + .unwrap(); + + // Simulate a claim taken by a request that then disappeared. + cache.entries().get_mut("k").unwrap().renewing_since = + Some(Utc::now() - Duration::seconds(CLAIM_TIMEOUT_SECS + 1)); + + let mut fetched = false; + cache + .get_or_fetch("k", || async { + fetched = true; + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + assert!(fetched, "a stale claim should be re-takeable"); + } + + /// A failed exchange releases the claim, so the next caller retries instead + /// of serving an unrenewed credential until it expires. + #[tokio::test] + async fn a_failed_renewal_releases_the_claim() { + let cache = CredentialCache::new(); + cache + .get_or_fetch("k", || async { Ok::<_, ()>(creds(30)) }) + .await + .unwrap(); + cache + .get_or_fetch("k", || async { Err::, _>("boom") }) + .await + .unwrap_err(); + + let mut fetched = false; + cache + .get_or_fetch("k", || async { + fetched = true; + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + assert!(fetched, "the claim should have been released on failure"); + } + + /// Documented trade-off: a cold key has nothing usable to serve, so + /// concurrent callers each mint rather than wait on one another. See the + /// module docs for why waiting is not an option. + #[tokio::test] + async fn cold_misses_are_not_single_flighted() { + let cache = CredentialCache::new(); + let calls = AtomicUsize::new(0); + + let one = async { + cache + .get_or_fetch("cold", || async { + calls.fetch_add(1, Ordering::SeqCst); + tokio::task::yield_now().await; + Ok::<_, ()>(creds(600)) + }) + .await + .unwrap(); + }; + let two = async { + cache + .get_or_fetch("cold", || async { + calls.fetch_add(1, Ordering::SeqCst); + Ok::<_, ()>(creds(600)) + }) + .await + .unwrap(); + }; + + tokio::join!(one, two); + assert_eq!(calls.load(Ordering::SeqCst), 2); } } diff --git a/crates/oidc-provider/src/lib.rs b/crates/oidc-provider/src/lib.rs index 2555063..d531f6f 100644 --- a/crates/oidc-provider/src/lib.rs +++ b/crates/oidc-provider/src/lib.rs @@ -89,12 +89,15 @@ impl OidcCredentialProvider { /// Get credentials for a backend, using cached values when available. /// /// `exchange` describes how to trade the self-signed JWT for cloud - /// credentials (AWS, Azure, GCP). `cache_key` identifies the backend for - /// caching purposes (e.g. the role ARN). + /// credentials (AWS, Azure, GCP). `cache_key` identifies the backend (e.g. + /// the role ARN); the entry is additionally scoped by `subject`, since a + /// credential minted for one subject is not interchangeable with another's + /// against the same backend — see [`cache::scoped_key`]. /// - /// Concurrent calls for the same `cache_key` are single-flighted: only one - /// JWT mint + exchange runs, and the rest await its result. A cached value - /// is reused until it nears expiry, then proactively re-minted. + /// A cached value is reused until it nears expiry, then renewed. Renewals + /// are single-flighted, but **no caller ever waits on another**: while one + /// mints, the rest keep receiving the cached credential, which has not + /// expired. See [`cache`] for why blocking is not an option. pub async fn get_credentials>( &self, cache_key: &str, @@ -103,7 +106,7 @@ impl OidcCredentialProvider { extra_claims: &[(&str, &str)], ) -> Result, OidcProviderError> { self.cache - .get_or_fetch(cache_key, || async { + .get_or_fetch(&cache::scoped_key(cache_key, subject), || async { // Cache miss (or due for refresh): mint a JWT and exchange it. let token = self.signer @@ -335,6 +338,33 @@ mod tests { assert_eq!(http.calls(), 2); } + /// Two subjects assuming the same backend must not share a cache entry: the + /// role's trust policy conditions on the assertion's `sub`, so a subject it + /// would reject must not be handed a credential minted for one it accepts. + #[tokio::test] + async fn different_subjects_on_one_backend_make_separate_calls() { + let http = MockHttp::new(); + let provider = OidcCredentialProvider::new( + test_signer(), + http.clone(), + "https://issuer.example.com".into(), + "sts.amazonaws.com".into(), + ); + + let exchange = exchange::aws::AwsExchange::new("arn:aws:iam::123:role/Test".into()); + + provider + .get_credentials("role-a", &exchange, "scv1:conn:one", &[]) + .await + .unwrap(); + provider + .get_credentials("role-a", &exchange, "scv1:conn:two", &[]) + .await + .unwrap(); + + assert_eq!(http.calls(), 2); + } + #[test] fn signed_jwt_is_verifiable_via_jwks_public_key() { use base64::Engine; diff --git a/docs/architecture/caching.md b/docs/architecture/caching.md index 6b219fc..bbfca63 100644 --- a/docs/architecture/caching.md +++ b/docs/architecture/caching.md @@ -28,7 +28,22 @@ It gives you three behaviours: - **Serve-while-fresh** — a cached value is returned directly while it is comfortably valid. - **Proactive refresh** — once a value is within its *refresh lead* (60s) of expiry, the next access re-mints it, so a credential is never handed out about to expire mid-request. -- **Single-flight** — while one caller is minting for a key, concurrent callers for that *same* key await the in-flight result instead of each launching their own mint. This collapses a cold-cache burst into a single upstream call. +- **Non-blocking single-flight renewal** — while one caller is renewing a key, concurrent callers keep receiving the cached credential, which has not expired. One mint runs, and **no caller ever waits on another**. + +The entry is keyed by the credential's full identity — the backend scope (e.g. the role ARN) *and* the subject the assertion is minted for. Those are not interchangeable: an AWS role's trust policy conditions on the assertion's `sub`, so a subject the policy would reject must never be served a credential minted for one it accepts. + +> [!IMPORTANT] +> A **cold** key — absent or already expired — is deliberately *not* single-flighted: concurrent callers each mint. There is nothing valid to serve them, so collapsing the burst would mean making them wait, which is [not an option on Workers](#why-the-cache-never-blocks). The duplication is bounded (one burst per process start, or per isolate cold start) while the recurring event — renewal — is collapsed. + +### Why the cache never blocks + +The cache holds a plain `std::sync::Mutex` for map reads and writes and never holds it across an `.await`. That is a correctness requirement, not a style preference. + +On Cloudflare Workers, a request parked on an in-memory waker has no pending I/O of its own. The runtime detects that "all the code associated with the request has executed and no events are left in the event loop" and **cancels the request** — surfacing as `The Workers runtime canceled this request because it detected that your Worker's code had hung and would never generate a response` — rather than waiting for whoever holds the lock. Sharing the in-flight future instead of the lock does not help either: that trips `Cannot perform I/O on behalf of a different request`. + +An earlier revision of this cache took a per-key `futures::lock::Mutex` on both the hit *and* miss paths. On Workers a single isolate renewing one key therefore killed every concurrent request touching that key — including requests whose credentials were already cached, since the fast path took the same lock. Serving stale-but-valid credentials is what removes the need for the lock: latecomers never need the claimant's result, so there is nothing to wait for. + +Native runtimes benefit from the same design — a renewal no longer stalls every concurrent caller for the duration of an STS round-trip. Because the cache calls *your* fetch closure on a miss, you can layer additional cache tiers (e.g. the Cloudflare Cache API) *inside* the closure without the cache ever depending on a runtime — see [Layering an external tier](#layering-an-external-tier). @@ -41,14 +56,14 @@ A credential cache is only as useful as the lifetime of the thing holding it. Th | Tier | Scope | Survives | Use for | |------|-------|----------|---------| -| In-memory (`CredentialCache`) | Per-process (native) / **per-isolate** (Workers) | While the process/isolate is warm | The default; single-flight + proactive refresh | +| In-memory (`CredentialCache`) | Per-process (native) / **per-isolate** (Workers) | While the process/isolate is warm | The default; non-blocking renewal single-flight + proactive refresh | | Cloudflare Cache API | **Per-colo** (data center) | Isolate cold starts within a colo | Sharing mints across isolates in one location | | Workers KV | Global, eventually consistent | Everything (≈seconds to propagate) | Cross-colo sharing of short-lived creds | | Durable Objects | Global, single owner per key | Everything | True cross-isolate single-flight | ### Native (server) runtime -The server runtime is a long-lived multi-threaded process. Construct the provider (and thus its `CredentialCache`) **once at startup** and share it across requests. The in-memory cache is then global to the process: one mint per credential lifetime, and single-flight collapses concurrent requests. This is the simple, fully-effective case. +The server runtime is a long-lived multi-threaded process. Construct the provider (and thus its `CredentialCache`) **once at startup** and share it across requests. The in-memory cache is then global to the process: one mint per credential lifetime, and renewals collapse to a single mint without stalling concurrent requests. This is the simple, fully-effective case. ### Cloudflare Workers runtime @@ -56,7 +71,7 @@ Workers run in V8 **isolates**, not per-request containers. Global/module-scope - The cache is **per-isolate**, and Cloudflare runs many isolates across many colos. With _N_ live isolates you get up to _N_ independent mints per credential lifetime, not one. - Isolates cold-start empty and are evicted under memory pressure or idle. -- Single-flight only collapses concurrency *within* one isolate. +- Renewal single-flight only collapses concurrency *within* one isolate. Even so, this is a large win: a warm isolate serving thousands of requests for the same bucket reuses one credential instead of minting per request. To get *any* cross-request benefit, hoist the provider into module scope (e.g. a `OnceCell`) rather than rebuilding it inside the `fetch` handler. @@ -68,7 +83,7 @@ The Cloudflare Cache API is **colo-local**: shared across all isolates in one da ```text request - └─ L1: in-memory CredentialCache (per-isolate, single-flight, proactive refresh) + └─ L1: in-memory CredentialCache (per-isolate, non-blocking renewal, proactive refresh) └─ on miss, the fetch closure does: L2: Cache API (colo-local, shared across isolates in the colo) └─ on miss, origin: STS / token exchange (mint) From bcf302433b3c51b296fb44a70ea322ad7be3a220 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 23:04:00 -0700 Subject: [PATCH 2/2] docs(oidc-provider): size the cold-key burst and pin the serve-floor overtake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review raised two properties of the non-blocking cache that were understated. The cold-key burst was described as "bounded (one burst per process start, or per isolate cold start)", which undersells the case that matters. A single cold start mints once per concurrent caller on one isolate and is small; a deploy or mass eviction cools every isolate at once, so those bursts coincide across the fleet and reach the token endpoint together. Say so, name the failure mode (STS throttling → `StsError` → a 502 for every caller in the burst), and point at the L2 tier as the mitigation — a larger in-memory cache cannot help, since that tier is exactly what a deploy discards. Inside `MIN_SERVE_SECS` a live claim is also deliberately overtaken, so several fetches can run at once right at the edge of expiry. That was implied by the code but not stated. It is the intended trade — nothing safe is left to serve, and the alternatives are making the caller wait or failing a request that could have succeeded — so document it and pin it with a test rather than leave it looking accidental. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) --- crates/oidc-provider/src/cache.rs | 80 +++++++++++++++++++++++++++---- docs/architecture/caching.md | 6 ++- 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/crates/oidc-provider/src/cache.rs b/crates/oidc-provider/src/cache.rs index c519c9c..74c1859 100644 --- a/crates/oidc-provider/src/cache.rs +++ b/crates/oidc-provider/src/cache.rs @@ -31,13 +31,21 @@ //! # What is deliberately not single-flighted //! //! A **cold** key — absent or already expired — has nothing usable to serve, so -//! concurrent callers each run their own `fetch`. Collapsing those would mean -//! waiting, and the only way to wait on Workers without being cancelled is to -//! poll a real timer — which costs about as long as the exchange it avoids, -//! while adding a spin loop and a dependency on the claimant surviving. The -//! duplication is bounded (one burst per process start, or per isolate cold -//! start) and upstream token endpoints are provisioned for it; the recurring -//! event, renewal, *is* single-flighted. +//! concurrent callers each run their own `fetch`, with no cap beyond how many +//! arrive before the first one stores. Collapsing those would mean waiting, and +//! the only way to wait on Workers without being cancelled is to poll a real +//! timer — which costs about as long as the exchange it avoids, while adding a +//! spin loop and a dependency on the claimant surviving. The recurring event, +//! renewal, *is* single-flighted. +//! +//! Size that burst before relying on it. A single cold start mints once per +//! concurrent caller on that one process or isolate, which is small. A **deploy +//! or mass eviction is different**: it cools every isolate at once, so the +//! bursts coincide across the whole fleet and land on the token endpoint +//! together. If yours is rate-limited (AWS STS throttling surfaces here as +//! `StsError` → a 502 for every caller in the burst), layer an L2 tier inside +//! the `fetch` closure so the second and later isolates read a mint instead of +//! performing one. See `docs/architecture/caching.md`. //! //! The fetch happens through a caller-supplied closure ([`get_or_fetch`]), so //! the cache never needs to know how credentials are minted, and a runtime can @@ -188,9 +196,16 @@ impl CredentialCache { } // Either we are first into the refresh lead, the previous claim went - // stale, or the credential is too close to expiry to hand out. Take the - // claim and mint. When there is nothing usable left this gates no one — - // concurrent callers have no credential to serve either. + // stale, or the credential is now inside `MIN_SERVE_SECS` and too close + // to expiry to hand out. + // + // That last case deliberately ignores a live claim, so in the final + // `MIN_SERVE_SECS` a claimant can be overtaken and several fetches run + // at once, each overwriting `renewing_since`. That is the intended + // trade: there is nothing safe left to serve, so the alternatives are + // making callers wait (which the module docs rule out) or failing a + // request that could have succeeded. Duplication is bounded to the + // callers arriving inside that window. entry.renewing_since = Some(now); Action::Fetch } @@ -410,6 +425,51 @@ mod tests { ); } + /// Inside `MIN_SERVE_SECS` a live claim is deliberately overtaken: the + /// cached credential is too close to expiry to hand out, so a caller + /// arriving during the renewal mints its own rather than being served + /// something that may die in transit. Bounded duplication, chosen over + /// making the caller wait or failing a request that could have succeeded. + #[tokio::test] + async fn overtakes_a_claim_when_the_credential_is_about_to_expire() { + let cache = CredentialCache::new(); + cache + .get_or_fetch("k", || async { Ok::<_, ()>(creds(MIN_SERVE_SECS - 1)) }) + .await + .unwrap(); + + let calls = AtomicUsize::new(0); + let claimant = async { + cache + .get_or_fetch("k", || async { + calls.fetch_add(1, Ordering::SeqCst); + for _ in 0..8 { + tokio::task::yield_now().await; + } + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + }; + let overtaker = async { + cache + .get_or_fetch("k", || async { + calls.fetch_add(1, Ordering::SeqCst); + Ok::<_, ()>(creds(3600)) + }) + .await + .unwrap(); + }; + + tokio::join!(claimant, overtaker); + assert_eq!( + calls.load(Ordering::SeqCst), + 2, + "a credential inside the serve floor must not be handed to a caller \ + arriving during a live renewal" + ); + } + /// A claimant that never completes (a cancelled or evicted request) must not /// keep the key marked as renewing forever. #[tokio::test] diff --git a/docs/architecture/caching.md b/docs/architecture/caching.md index bbfca63..5f58d2d 100644 --- a/docs/architecture/caching.md +++ b/docs/architecture/caching.md @@ -33,7 +33,11 @@ It gives you three behaviours: The entry is keyed by the credential's full identity — the backend scope (e.g. the role ARN) *and* the subject the assertion is minted for. Those are not interchangeable: an AWS role's trust policy conditions on the assertion's `sub`, so a subject the policy would reject must never be served a credential minted for one it accepts. > [!IMPORTANT] -> A **cold** key — absent or already expired — is deliberately *not* single-flighted: concurrent callers each mint. There is nothing valid to serve them, so collapsing the burst would mean making them wait, which is [not an option on Workers](#why-the-cache-never-blocks). The duplication is bounded (one burst per process start, or per isolate cold start) while the recurring event — renewal — is collapsed. +> A **cold** key — absent or already expired — is deliberately *not* single-flighted: concurrent callers each mint, with no cap beyond how many arrive before the first one stores. There is nothing valid to serve them, so collapsing the burst would mean making them wait, which is [not an option on Workers](#why-the-cache-never-blocks). The recurring event — renewal — is collapsed. + +Size that burst before you rely on it. One cold start mints once per concurrent caller on that single process or isolate, which is small. A **deploy or mass eviction is the case to plan for**: it cools every isolate at once, so those bursts coincide across the fleet and arrive at the token endpoint together. AWS STS throttling surfaces as an `StsError` and becomes a 502 for every caller in the burst. If your endpoint is rate-limited, [layer an L2 tier](#layering-an-external-tier) so the second and later isolates read a mint rather than performing one — that is the mitigation, not a larger in-memory cache, since the in-memory tier is exactly what a deploy discards. + +There is one more, smaller duplication: in the final few seconds before expiry (`MIN_SERVE_SECS`) a live renewal claim is ignored, because the cached credential is by then too close to expiry to hand out safely. Callers arriving in that window each mint. Bounded, and preferable to the alternatives — waiting, or failing a request that could have succeeded. ### Why the cache never blocks