[workers-shared] Retry null KV reads when serving static assets#14649
[workers-shared] Retry null KV reads when serving static assets#14649MattieTK wants to merge 1 commit into
Conversation
A KV read for a static asset can fail two ways: the read throws (a storage-layer error), or it returns null. A null can occur in the brief window after a deploy where an asset referenced by the manifest has not yet propagated to the KV storage provider being read from. Previously only the throw path was retried with backoff; a null did a single immediate re-read and then gave up, surfacing an intermittent 500. Apply the same jittered exponential backoff retry loop to the null path so propagation has a chance to catch up before we return. The backoff is kept small with full jitter because a single page load can fan out into hundreds of asset requests, so long per-request stalls would hurt page load and synchronised retries could stampede a provider that is still catching up. Retried reads keep the short cache TTL so a transient miss is not cached long-term.
🦋 Changeset detectedLatest commit: 13af4c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
workers-devprod
requested review from
a team and
penalosa
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Summary: reworks getAssetWithMetadataFromKV to retry null KV reads (not just errored reads) with a small jittered exponential backoff, defaulting to 3 retries.
One real (latent) issue found around the cache-TTL guarantee when retries is 0; everything else looks good.
Issues
- (medium)
retries = 0regresses the "never cache a null for a year" guarantee. In the previous implementation anullvalue always triggered a re-read with the short (60s) TTL, regardless of the retry count, to avoid caching a transient 404 for a year. In the new loop, whenretries === 0, attempt 0 is not a retry (so the read usesLONG_CACHE_TTL) andattempt < retries(0 < 0) is false, so thenullresult read with the 1-year TTL is returned and cached. The comment on lines 63-66 acknowledges this only holds "with the default retries (>= 1)", leaving a footgun for any caller that passes0. Since the exported signature allowsretries = 0, worth guarding against caching a long-TTL null. See suggestion below.
| // always means the value has not yet propagated to the KV storage | ||
| // provider we read from (the eventual-consistency window right after | ||
| // a deploy). Retry with backoff to give propagation time to complete. | ||
| if (attempt < retries) { |
There was a problem hiding this comment.
When retries === 0, this branch is skipped on attempt 0 and the null asset read with LONG_CACHE_TTL on line 66 is returned/cached for a year. The old code always avoided caching a null long-term. To preserve that guarantee without penalising the successful-hit fast path, re-read once with the short TTL when a long-TTL read returned null and no retries remain:
| if (attempt < retries) { | |
| if (attempt < retries) { | |
| continue; | |
| } | |
| // Out of retries. If this null came from a long-TTL read (e.g. | |
| // retries === 0), re-read once with the short TTL so a transient | |
| // null is never cached for the long TTL. | |
| if (!isRetry) { | |
| return assetsKVNamespace.getWithMetadata<AssetMetadata>(assetKey, { | |
| type: "stream", | |
| cacheTtl: SHORT_CACHE_TTL, | |
| }); | |
| } | |
| return asset; |
|
Review posted successfully to PR #14649. I reviewed the PR and posted a single batch review with one actionable issue: Issue (medium): The The retry-on-null logic, backoff/jitter, short-TTL usage on retries, sentry logging, and test coverage otherwise look correct. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
I would be interested in pulling KV into reviewing this. I don't think we should need retry at all. We do have guarantee that assets are written to at least one store, and we have some special guarantees that KV will attempt to reach from BOTH before returning a 404.. at least last I looked 😓 |
3 participants
Retry KV reads that return
nullwhen serving static assets, not just reads that error.Immediately after a deploy there is a brief window where an asset referenced by the manifest may not yet have propagated to every KV storage provider, so a read can return
nulleven though the asset exists. Previously this surfaced as an intermittent500.There are two failure routes in
getAssetWithMetadataFromKV:null(asset not yet propagated). Previously did a single immediate re-read and then gave up. Now retried with the same jittered exponential backoff before returning.The backoff is deliberately small with full jitter (roughly 0–50ms, 0–100ms, 0–200ms) because a single page load can fan out into hundreds of asset requests: long per-request stalls would hurt page load, and jitter avoids synchronised retries stampeding a provider that is still catching up. Retried reads continue to use the short cache TTL so a transient miss is never cached long-term.
A picture of a cute animal (not mandatory, but encouraged)