[codex] Avoid blocking route cache misses on cache writes#1548
[codex] Avoid blocking route cache misses on cache writes#1548auggernaut wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 1bc2791 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
auggernaut
changed the title
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approach
The change is sound and fits EmDash's conventions. Cache writes on a route-cache MISS are exactly the kind of maintenance/best-effort work that AGENTS.md says shouldn't block TTFB ("Defer bookkeeping … Maintenance writes don't need to block TTFB"), and moving cache.put() behind waitUntil() is the idiomatic Cloudflare pattern for that. It's also consistent in intent with the existing SWR revalidation path in the same file, which already defers its background cache.put() via waitUntil(). So this is the right change solving the right problem.
What I checked
runtime.tsMISS path (line 278):waitUntil(cache.put(cacheKey, toStore))correctly does not block the returnedmissresponse. Body/stream semantics are safe:toStoreis built fromresponse.clone()whilemissreadsresponse.body, so the background write and the client response consume independent tee'd streams — no double-read.- Sibling SWR path (lines 243–257): traced for consistency. It defers
cache.put()the same way but wraps it in atry/catch("Non-fatal — next request will retry"). invalidate(): stillawaitscache.delete()— correct, since explicit invalidation callers expect completion; not a candidate for deferral.- New test (
cache-runtime.test.ts): genuinely verifies the behavior, not false confidence.cache.putis mocked to never resolve, and the test asserts the response still returns with theMISSheader and body whilewaitUntilis called once with a promise. Under the oldawait cache.put(...)this test would hang, so it would catch a regression. Thecloudflare:workers/cachesmocks line up with what the runtime imports. - Changeset: present, targets
@emdash-cms/cloudflare(patch), leads with a present-tense verb and states the observable effect — consistent with existing changesets in the repo.
Headline
Implementation is correct and the test is solid. One issue: the MISS path hands cache.put() to waitUntil() without a try/catch, unlike the SWR path directly above it and unlike AGENTS.md's rule that deferred work be wrapped in try/catch. A rejecting cache.put becomes an unhandled rejection in the waitUntil promise (noisy error reports in production, even though the visitor's response is already sent). The fix is trivial and mirrors the SWR path.
| if (toStore) { | ||
| await cache.put(cacheKey, toStore); | ||
| waitUntil(cache.put(cacheKey, toStore)); | ||
| } |
There was a problem hiding this comment.
[needs fixing] The MISS path schedules the write with waitUntil(cache.put(cacheKey, toStore)) but — unlike the SWR revalidation path directly above (lines 243–257, which wraps cache.put in a try { … } catch { // Non-fatal — next request will retry }) — it has no error handling. AGENTS.md requires deferred work to be wrapped in try/catch ("Wrap your function body in try/catch"). If cache.put rejects (storage error, aborted write), the promise handed to waitUntil rejects unhandled, surfacing as an unhandled-rejection error report in production even though the visitor's response has already been returned. The same "non-fatal, next request retries" rationale the SWR path documents applies identically here. Mirror the SWR path:
| if (toStore) { | |
| await cache.put(cacheKey, toStore); | |
| waitUntil(cache.put(cacheKey, toStore)); | |
| } | |
| if (toStore) { | |
| waitUntil( | |
| (async () => { | |
| try { | |
| await cache.put(cacheKey, toStore); | |
| } catch { | |
| // Non-fatal — next request will retry | |
| } | |
| })(), | |
| ); | |
| } |
github-actions
Bot
added
review/awaiting-author
1 participant
What does this PR do?
Schedules Cloudflare route-cache MISS writes with
waitUntil()instead of awaitingcache.put()before returning the rendered response.The Cloudflare cache runtime was rendering fresh responses on route-cache misses, then waiting for Cache API storage before sending the page. If Cloudflare cache storage was slow or stalled in a colo, an otherwise valid page response could hang. Cache writes are best-effort optimization work, so they should not block visitor responses.
Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
pnpm --filter emdash buildpnpm --filter @emdash-cms/cloudflare testpnpm --filter @emdash-cms/cloudflare typecheckpnpm --filter @emdash-cms/cloudflare build