fix(oidc-provider): never block a caller on another's credential renewal by alukach · Pull Request #133 · developmentseed/multistore

alukach · 2026-07-28T05:37:11Z

What I'm changing

CredentialCache::get_or_fetch takes a per-key futures::lock::Mutex before the freshness check, so concurrent callers for a key await whoever is minting — including callers whose cached credential is perfectly fresh, since the fast path takes the same lock.

On Cloudflare Workers — which multistore-cf-workers exists to serve — awaiting that lock is fatal rather than slow. Per Cloudflare's docs, the runtime cancels a request once "all the code associated with the request has executed and no events are left in the event loop", and a future parked on an in-memory waker has no pending I/O of its own. It is killed immediately, not when the lock holder finishes. Sharing the in-flight future instead of the lock is no escape either: that trips Cannot perform I/O on behalf of a different request.

This is not theoretical. It took down federated reads on data.source.coop, where nearly all open data sits behind one connection — one role ARN, therefore one lock. A five-minute wrangler tail caught 167 cancelled requests, all of them on that single key. 149 died at 5–8ms wall / 1–2ms CPU: warm requests whose credentials were already cached, killed purely for touching the lock while a sibling renewed.

The downstream mitigation is source-cooperative/data.source.coop#197, which replaces AwsBackendAuth with a local middleware. This PR is the upstream fix that lets that fork be deleted.

The design is not a Workers accommodation. Serving stale-but-valid credentials is what removes the need for a lock at all — latecomers never need the claimant's result, so there is nothing to wait for. Native consumers get the same win: a renewal no longer stalls every concurrent caller for the length of a token round-trip.

Also fixes a cache-key defect found while reviewing the above: 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.

How I did it

crates/oidc-provider/src/cache.rs — replaced the per-key Arc<AsyncMutex<Option<..>>> slot map with HashMap<String, Entry> behind a plain std::sync::Mutex that is only ever held for a get or an insert, never across an .await. Entry carries the credential plus renewing_since.
- New private claim() returns Action::Serve | Action::Fetch under that lock. Fresh → Serve. Inside the refresh lead with a live claim → Serve (the credential has not expired). Otherwise take the claim → Fetch.
- MIN_SERVE_SECS (5s) floors the serve branch. Without it a non-claimant could receive a credential with milliseconds left, and the backend request it signs would land after expiry — ExpiredToken reaching the client as a misleading AccessDenied rather than a retryable server-side error.
- CLAIM_TIMEOUT_SECS (30s) reaps an abandoned claim. A claimant whose request is cancelled between claiming and storing would otherwise pin the key as renewing until the credential expired, at which point every caller stampedes at once — the burst the single-flight exists to prevent.
- A failed fetch clears the claim so the next caller retries rather than coasting on an unrenewed credential.
- Added scoped_key(scope, subject), joining on U+001F (unrepresentable in both a role ARN and an OIDC subject, so no escaping is needed).
crates/oidc-provider/src/lib.rs — get_credentials now passes cache::scoped_key(cache_key, subject). Fixing it here rather than at the AwsBackendAuth call site also covers third-party callers that pass a bare role ARN. Public signatures are unchanged.
crates/oidc-provider/Cargo.toml — dropped the futures dependency; the AsyncMutex was its only use.
docs/architecture/caching.md — rewrote the single-flight bullet, added a Why the cache never blocks section documenting the Workers constraint for other integrators, and noted the cold-key trade-off and the subject scoping.

Considered and rejected

Single-flighting a cold key too. There is nothing valid to serve concurrent callers, so collapsing the burst means making them wait — and the only way to wait on Workers without being cancelled is polling 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 to one burst per process (or isolate) cold start, against a token endpoint provisioned for it. The recurring event, renewal, is collapsed.

Downstream: how to adapt

No API changes. get_or_fetch and get_credentials keep their signatures; upgrading is a version bump.
If you run on Workers and see hung and would never generate a response on federated requests, this is your fix. Upgrade; no code change needed.
If you forked AwsBackendAuth to work around it (as data.source.coop#197 did), you can delete the fork after upgrading and go back to MaybeOidcAuth::Enabled(Box::new(AwsBackendAuth::new(provider))).
Expect slightly more mints, in two bounded ways. Cold keys are no longer single-flighted (see above), and entries are now scoped by subject — so a deployment that pointed several connections at one role ARN previously shared credentials across those subjects and now mints per subject. That sharing was the bug. If either matters for your token endpoint's quota, layer an L2 tier inside the fetch closure (Cache API or KV) as docs/architecture/caching.md describes.
Workers consumers should keep the provider in module scope, unchanged advice — the cache is per-isolate and does nothing if rebuilt per request.

Test plan

🤖 Generated with Claude Code

`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) <noreply@anthropic.com>

claude · 2026-07-28T05:37:31Z

Claude finished @alukach's task in 3m 53s —— View job

✅ No blocking issues — safe to merge.

Reviewed the diff (fdcff53...HEAD, matching the PR's stated 5-file/467+/94- change set) covering crates/oidc-provider/src/cache.rs, lib.rs, Cargo.toml, Cargo.lock, and docs/architecture/caching.md.

Correctness

The new claim() state machine was traced through all branches (fresh serve, in-lead claim/serve, stale-claim reclaim, near-expiry overtake, cold miss, failed-fetch claim release) and matches the documented design — no path holds the std::sync::Mutex across an .await, satisfying the Workers cancellation constraint this PR exists to fix. The MIN_SERVE_SECS/CLAIM_TIMEOUT_SECS interplay is sound: a credential inside the serve floor is never handed out regardless of claim state, and a stale or completed claim is always retaken correctly.

One benign race worth being aware of (not a bug): if caller A holds a claim and is overtaken by caller B inside the MIN_SERVE_SECS window (both now fetching), and A's fetch fails first, A's error handler unconditionally clears renewing_since — which can clear B's newer claim and let a third caller pile on. This only amplifies the already-documented "several fetches run at once" trade-off in that 5s window and doesn't affect correctness or security.

scoped_key's U+001F join could theoretically collide if scope/subject contained that byte, but both values trace back to operator-supplied BucketConfig options (oidc_role_arn/oidc_subject in backend_auth.rs), not request-controlled input, so this isn't a real trust-boundary issue.

Cargo.lock/Cargo.toml changes are consistent (removed futures only where its sole use — the AsyncMutex — was deleted; still present elsewhere in the workspace). Docs accurately reflect the new behavior, including the new "Why the cache never blocks" section and updated anchors.

Note on verification

This sandbox's cargo/git fetch invocations require interactive approval that isn't available here, so I could not run cargo test -p multistore-oidc-provider, cargo clippy, or cargo check --target wasm32-unknown-unknown myself. Review is based on static reading of the diff and full cache.rs/lib.rs logic tracing. The PR body's own test plan (39 passing tests including the new concurrency-ordering test) looks appropriately thorough for this change — no additional test gaps identified.

No Simplify (ponytail) findings — the new Action/Entry types, the two new consts, and scoped_key are all minimal and directly justified by the concurrency/security requirements; nothing speculative or reinvented.

💰 Estimated review cost: $1.13 · 3m53s · 30 turns

github-actions · 2026-07-28T05:38:04Z

📖 Docs preview deployed to https://multistore-docs-pr-133.development-seed.workers.dev

Date: 2026-07-28T06:04:03Z
Commit: 9a114e2

github-actions · 2026-07-28T05:39:42Z

🚀 Latest commit deployed to https://multistore-proxy-pr-133.development-seed.workers.dev

Date: 2026-07-28T06:04:03Z
Commit: 9a114e2

…aims Two defects in the non-blocking renewal added in 203e693, found in review. A non-claimant could be served a credential with milliseconds of life left: the serve branch only required `expiration > now`. The backend request it signs still has to be sent and answered, so it could land after expiry — S3 answers `ExpiredToken`, which reaches the caller as a misleading `AccessDenied` rather than a retryable server-side error. Reachable whenever renewals keep failing (STS throttling): each attempt releases the claim, the next request claims and fails too, and everyone else keeps serving an ever-older credential for the whole 60s lead. `MIN_SERVE_SECS` now floors it at 5s; below that a request mints its own. A claim was also never reaped. `renewing` was cleared only by `store` or `release`, so a claimant whose request died between claiming and either — a cancelled or evicted request — pinned the key as renewing until the credential expired, at which point every concurrent request exchanged at once: exactly the burst the claim exists to prevent. `renewing` becomes `renewing_since`, and a claim older than `CLAIM_TIMEOUT_SECS` (30s, comfortably above the 10s `STS_REQUEST_TIMEOUT`) can be re-taken. Also corrects `release`'s doc comment, which claimed retries stay serialized unconditionally. That holds only while the credential is still servable; below the floor there is nothing safe to serve, so concurrent requests each exchange — correct behaviour, but not what the comment said. Matches developmentseed/multistore#133, which upstreams this design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…overtake 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) <noreply@anthropic.com>

… overtake Mirrors developmentseed/multistore#133 (bcf3024), from review of that PR. "Bounded to one burst per isolate cold start" undersold the case that matters. One cold isolate's burst is small; a deploy or mass eviction cools every isolate at once, so the bursts coincide fleet-wide and reach STS together — where throttling becomes a 502 per request. Name that, and point at an L2 tier (#175) as the mitigation rather than a bigger in-memory cache, which is exactly what a deploy discards. Also state, in the module docs and at the call site, that inside MIN_SERVE_SECS a live claim is deliberately overtaken so several exchanges can run at once at the edge of expiry. Implied by the code but not written down; the existing test now asserts it rather than leaving it looking accidental. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ache 0.7.2 (developmentseed/multistore#133) is published, so the temporary `[patch.crates-io]` pin comes out and the dependencies move to the released version. Every multistore crate now resolves from crates.io with a checksum again; no git sources remain in the lock. The pinned rev and 0.7.2 are the same code, and the PR preview was A/B'd against `data.staging.source.coop` on it: 22 hung cancellations there over 320 concurrent federated requests, zero on the preview. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…197) ## What this does Bumps multistore `0.7.1` → `0.7.2`, which fixes production requests being cancelled with `The Workers runtime canceled this request because it detected that your Worker's code had hung and would never generate a response`. That is the whole diff. The fix itself is [developmentseed/multistore#133][up]; this PR started as a local reimplementation of it and was reduced to a dependency bump once that merged and released. [up]: developmentseed/multistore#133 ## The bug `multistore-oidc-provider`'s `CredentialCache` single-flighted concurrent misses on a role ARN behind a `futures::lock::Mutex`. On Cloudflare Workers, awaiting that lock is fatal rather than slow: per [Cloudflare's docs][err], the runtime cancels a request once "all the code associated with the request has executed and no events are left in the event loop", and a future parked on an in-memory waker has no pending I/O of its own. It is killed immediately, not when the lock holder finishes. The fast path took the same lock, so requests whose credentials were already cached died too. Nearly all of `data.source.coop`'s open data sits behind one connection — one role ARN, therefore one lock — so a single isolate renewing credentials took out every concurrent read on it. A five-minute `wrangler tail --status error` on production caught 167 cancellations, **all 167** on that one connection (`tge-labs/aef` 163, `protomaps/openstreetmap` 3, `ftw/global-data` 1). The timing split as the mechanism predicts: 149 at 5–8ms wall / 1–2ms CPU (warm requests, killed instantly with nothing pending) and 19 at 30–66ms / 21–26ms CPU (cold isolates — the lock holders' siblings). [err]: https://developers.cloudflare.com/workers/observability/errors/ ## Why it looked random The failures need a credential fetch in flight, so they cluster at isolate cold start and at renewal, and vanish once an isolate is warm. That is the "product won't load, works on reload" shape. ## Verification Two live Cloudflare deployments, same staging API, same product, same federated connection (`aws-opendata-eu-central-1`) — so the dependency version is the only variable. Control is `data.staging.source.coop`, running `main` on multistore 0.7.1. Four rounds of 80 concurrent GETs of `/alukach/euro-test/README.md` each, cache-busted: | round | `main` (0.7.1) | this PR (0.7.2) | |---|---|---| | 1 (cold) | 59× 200, **21× 500** | 80× 200 | | 2 | 80× 200 | 80× 200 | | 3 | 79× 200, **1× 500** | 80× 200 | | 4 | 80× 200 | 80× 200 | `wrangler tail` over that window: **staging logged 22 `hung and would never generate a response` cancellations, all on this product, at the same 5–8ms / 1–2ms signature. The preview logged zero events with exceptions across all 320 requests.** Also verified locally against the stub-API harness (50 concurrent federated reads: 49/50 cancelled on `main`, 0/50 on this branch), and 73 native tests, `cargo clippy --target wasm32-unknown-unknown -D warnings`, and the integration suite all pass. ## What changed upstream For anyone reading this later, 0.7.2 replaces the async lock with a cache that never blocks a caller: - Fresh credentials are served from a plain map under a sync lock that is never held across an `.await`. - **Renewals are single-flighted without anyone waiting**: inside the refresh lead the credential is still valid, so one caller claims the exchange and the rest keep serving what they have. That works precisely because latecomers never need the claimant's result — the thing a lock cannot give you here. - Two guards: a 5s floor so a credential that could expire in transit is never served (an expired one comes back from S3 as `ExpiredToken` and reaches the client as a misleading `AccessDenied`), and a 30s claim timeout so a claimant killed mid-exchange cannot pin the key as renewing until expiry. - Cache entries are now keyed by role ARN **and** subject. Keying on the ARN alone was unsound when two connections share a role: the trust policy conditions on the assertion's `sub`, so a connection that policy would reject could be served another's cached credentials. A cold key is deliberately *not* single-flighted — there is nothing valid to serve, so collapsing that burst would mean waiting. Worth knowing that a deploy or mass eviction cools every isolate at once, so those bursts coincide fleet-wide; the mitigation is an L2 tier (#175), not a larger in-memory cache, since that tier is exactly what a deploy discards. ## Relationship to #175 #175 adds an L2 Cloudflare-Cache-API tier for the STS response. It does **not** fix this on its own: it wraps `FetchHttpExchange::post_form`, which ran *inside* the old lock's critical section, so concurrent arrivals were still cancelled. It shortens the held window a lot, but a cold colo — L2 miss, highest concurrency — is exactly when it doesn't help. The two compose: 0.7.2 is L1, #175 is L2, and #175 keeps independent value by making the cold-key duplication mostly network-free. Its central constraint ("the only seam we control is `post_form`") is worth revisiting now that upstream owns the cache. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

alukach temporarily deployed to preview July 28, 2026 05:37 — with GitHub Actions Inactive

github-actions Bot added the fix label Jul 28, 2026

alukach mentioned this pull request Jul 28, 2026

fix(deps): bump multistore to 0.7.2 to stop federated reads hanging source-cooperative/data.source.coop#197

Merged

alukach marked this pull request as ready for review July 28, 2026 05:47

alukach temporarily deployed to preview July 28, 2026 06:04 — with GitHub Actions Inactive

alukach merged commit 11a5556 into main Jul 28, 2026
16 checks passed

alukach deleted the fix/credential-cache-nonblocking branch July 28, 2026 06:11

ds-release-bot Bot mentioned this pull request Jul 28, 2026

chore(main): release 0.7.2 #134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(oidc-provider): never block a caller on another's credential renewal - #133

fix(oidc-provider): never block a caller on another's credential renewal#133
alukach merged 2 commits into
mainfrom
fix/credential-cache-nonblocking

alukach commented Jul 28, 2026 •

edited

Loading

Uh oh!

claude Bot commented Jul 28, 2026 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

alukach commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I'm changing

How I did it

Considered and rejected

Downstream: how to adapt

Test plan

Uh oh!

claude Bot commented Jul 28, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Correctness

Note on verification

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

alukach commented Jul 28, 2026 •

edited

Loading

claude Bot commented Jul 28, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading