fix(cloudflare): prewarm the uploaded Worker version safely#2593
fix(cloudflare): prewarm the uploaded Worker version safely#2593NathanDrake2406 wants to merge 16 commits into
Conversation
commit: |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
NathanDrake2406
changed the title
NathanDrake2406
changed the title
NathanDrake2406
changed the title
…fore promoting Workers Cache is keyed per Worker version by default. `--experimental-warm-cdn-cache` issued warmup requests against production before confirming which version answered them, so a request routed to the still-live previous version silently warmed the wrong cache partition. The deploy then promoted the new version anyway, reporting success while the cache stayed cold for real traffic. The fix enforces one invariant: a warmup only counts as successful when the response proves it came from the uploaded Worker version. - `vinext init` provisions a `version_metadata` binding under a fixed name (VINEXT_VERSION_METADATA); the Worker entry reads the current version ID from it and stamps every response with `x-vinext-worker-version`. - Deploy stages the uploaded version at 0% traffic, warms build-discovered paths through a `Cloudflare-Workers-Version-Overrides` header, and rejects any response whose version header doesn't match the uploaded version — without following redirects, so the canonical cache key is what gets verified. - Promotion only happens after warmup completes; in strict mode a failed or unverified warmup aborts before promotion, leaving the previous version at 100% and the new one staged at 0% — already the safe state, so no compensating mutation is issued. In non-strict mode the deploy promotes anyway but does not claim the cache was warmed. - Static exports (`output: "export"`) skip verified warmup entirely: Cloudflare Assets serves them without invoking the Worker, so there's no version header to verify against. Scope cut from the original draft: dropped the generic version-traffic reconciliation module (ambiguous-failure classification, auto-rollback, manual recovery command rendering), the configurable binding-name companion variable in favor of the fixed binding name the runtime always reads, and cross_version_cache config parsing/validation. None of those are required to enforce the core invariant, and each is a separable concern better reviewed on its own. Tests: tests/worker-version.test.ts, tests/cloudflare-cdn-warm.test.ts, tests/cloudflare-cdn-warm-deploy.test.ts, tests/deploy.test.ts, tests/init.test.ts, tests/deploy-prerender-config.test.ts.
NathanDrake2406
force-pushed
the
nathan/fix-workers-dev-cdn-warmup
branch
from
4c90d59 to
094c4a9
Compare
…TPR's config reader, and stop reporting unconfirmed warm-ups as plain success
cdn-warm-deployment.ts read Worker name, production host, and legacy_env
suffixing directly out of tpr.ts's WranglerConfig/env map. tpr.ts owns
TPR-specific config extraction; CDN warmup's env-fallback and
Worker-name-suffixing rules do not belong to that module, and the coupling
made tpr.ts a de facto shared deployment-semantics resolver.
Separately, when non-strict warmup exhausted its retries without ever
confirming the uploaded version served a response, deployWithCdnWarmup still
promoted and returned a bare URL string. deploy.ts printed a generic
"Deployed to: <url>" box identical to a fully-warmed deploy, so an operator
had no way to tell a confirmed warm-up from a silent fallback short of
scrolling back through mid-deploy warnings.
Added wrangler-deployment-target.ts to resolve {workerName, productionHost,
versionMetadataBinding} on top of the existing parseWranglerConfig output,
so cdn-warm-deployment.ts no longer touches TOML/env-map shape directly.
deployWithCdnWarmup now returns {url, warmed}; deploy.ts's closing summary
states plainly when a promoted version's cache was not confirmed
pre-warmed, and a console.warn now fires at the retry-exhausted fallback
site that previously logged nothing beyond per-path failures.
Also named the decisions that were previously inlined at effectful
call-sites (promotionPhaseFor, isFullyWarmed, pickDeployedUrl in
cdn-warm-deployment.ts; describeVersionMismatch in cdn-warm.ts) so they're
independently readable and testable from the wrangler/fetch calls around
them.
Added a test covering the previously-uncovered non-strict-promotes-without-
confirmed-warmup path. vp check clean; targeted suite 480/480.
…ing config mid-warm-up resolveCdnWarmupTargetUrl called resolveWranglerDeploymentTarget a second time inside warmAndPromote's warm attempt, reparsing wrangler.jsonc/.toml from disk even though validateCdnWarmupConfiguration had already resolved the same target moments earlier in the same deploy. The duplicate I/O made the WranglerDeploymentTarget abstraction ceremonial rather than real: two call sites could theoretically observe different config state despite describing one deploy. Deleted resolveCdnWarmupTargetUrl. deployWithCdnWarmup now resolves the target once and threads it through; the production-host fallback (`target.productionHost ? https://... : staged.deployedUrl`) is inlined where it's used. Migrated its direct tests in deploy.test.ts onto resolveWranglerDeploymentTarget's `.productionHost` field, which is what they were actually exercising — the deployedUrl fallback itself stays covered by the existing end-to-end warmAndPromote tests. Also split deployWithCdnWarmup into promoteWithoutWarmup and warmAndPromote: they are genuinely different deployment modes (no safe staging split vs. staged-warm-promote) with different guarantees, not just a long function that happened to have an early return. Removed isFullyWarmed — it restated `result.failed === 0` without adding a decision worth naming separately. vp check clean; targeted suite green (deploy.test.ts, cdn-warm-deploy, deploy-prerender-config, cdn-warm, version-deploy, worker-version, init).
getZeroPercentStagingTraffic didn't check the uploaded version ID against the currently-live one. If upload ever returned an ID matching the version already at 100%, it built a traffic split with the same version listed twice (v@100% v@0%) and handed that straight to `wrangler versions deploy`. Added the equality check so that case falls back to promoteWithoutWarmup like any other unsafe traffic shape, instead of reaching wrangler with a malformed split. Also stopped re-deriving previousVersionId from currentVersions[0] in deployWithCdnWarmup — that index is only safe because a non-null stagingTraffic already proved current has exactly one entry, and that proof lived in a different function. Read it from stagingTraffic[0] instead, the value that was actually checked.
Upstream moved TPR KV key construction to the shared key-space helper while this branch added Wrangler deployment-target parsing in the same import block. Keep both APIs so TPR uses the current cache key contract without dropping CDN warmup config support.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4926519547
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Generated environment-specific Wrangler configs flatten the selected environment, failed strict warmups leave zero-percent versions behind, and direct fallback promotion can lose trigger recovery context. That can reject a valid retry or obscure the deployed state after a trigger failure. The deployment flow assumed selected environments always remained nested and that every version in a retry status had positive traffic. Recognize generated configs through Wrangler's targetEnvironment marker, replace stale zero-percent versions when staging a fresh attempt, reuse post-promotion trigger diagnostics, and accept trailing TOML route comments when resolving the production host.
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
|
UnknownError: "Overloaded" |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: staged CDN warmup deployment
I reviewed the full diff and ran the affected test files locally — tests/worker-version.test.ts, tests/cloudflare-cdn-warm.test.ts, tests/cloudflare-cdn-warm-deploy.test.ts, tests/deploy.test.ts, tests/init.test.ts, and tests/deploy-prerender-config.test.ts all pass (474 tests).
The overall design is solid. The core insight — that a 2xx/3xx response alone can't prove the uploaded version served it, because Workers Cache partitions by version ID — is correct, and verifying via x-vinext-worker-version + Cloudflare-Workers-Version-Overrides is the right structural fix. The staging-at-0% / promote-after-warm ordering, the fail-closed strict mode, and the retry-after-stale-0%-version recovery are all handled well and well-tested. The Codex P1/P2 items appear genuinely addressed in e22bc00.
Dev/prod parity check: fetch-handler (the recommended default entry) resolves to app-router-entry/pages-router-entry via virtual:vinext-worker-entry, and custom workers that delegate to handler.fetch() inherit the stamp, so stampWorkerVersion coverage is complete for Worker-served responses. Good.
A few non-blocking observations below.
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the full diff and read the new/changed source (cdn-warm-deployment.ts, cdn-warm.ts, worker-version.ts, wrangler-deployment-target.ts, utils/toml.ts, tpr.ts, deploy wiring) and ran tests/worker-version.test.ts + tests/cloudflare-cdn-warm.test.ts locally (26 pass).
Overall: the design is correct and no blocking issues found. The core insight is sound: because Workers Cache partitions its key by invoked version ID (unless cross_version_cache), a plain 2xx/3xx cannot prove the uploaded version populated the cache. Verifying via the x-vinext-worker-version header + a Cloudflare-Workers-Version-Overrides override is the right structural fix, and the stage-at-0% / warm / promote / triggers-after-promotion ordering keeps the risky window in the already-safe state. Fail-closed strict mode, stale-0%-version retry recovery (getZeroPercentStagingTraffic), path-scoped-route refusal, and the static-export carve-out are all handled and well-tested.
Parity confirmed: stampWorkerVersion runs in both app-router-entry and pages-router-entry, and fetch-handler → virtual:vinext-worker-entry resolves to those, so all Worker-served responses are stamped. The Node prod-server correctly does not stamp (no version-metadata binding off-Cloudflare). The prior Codex/bigbonk rounds appear genuinely addressed.
A few small non-blocking observations below.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent review of the staged CDN-warmup deployment.
Overall: the design is correct and I found no blocking issues. I traced the core mechanism end-to-end and it holds up:
- The structural insight is sound: because Workers Cache partitions its key by invoked version ID (unless
cross_version_cache), a plain 2xx/3xx can't prove the uploaded version populated the cache. Verifying viax-vinext-worker-version+ aCloudflare-Workers-Version-Overridesoverride, withredirect: "manual"so a redirect is proven at its own cache key, is the right fix. - The stage-at-0% -> warm -> promote -> triggers-after-promotion ordering keeps the risky window in the already-safe state (previous version at 100%, new at 0%), and strict mode fails closed by leaving that split rather than issuing an undo mutation.
getZeroPercentStagingTrafficcorrectly rejects partial traffic splits and drops stale 0% leftovers so a fresh attempt retries from the safe state.- The path-scoped-route carve-out is correct: for
app.example.com/api/*,cleanDomainsetscustomDomain(sohasProductionRouteis true) whileroutePatternToWarmupHostreturns null (soproductionHostis undefined), andwarmAndPromotethen refuses to warm workers.dev on a different cache key. Good.
I also confirmed stampWorkerVersion parity: both app-router-entry and pages-router-entry stamp at the top-level fetch, and the recommended default fetch-handler resolves to those, so all Worker-served responses carry the header. The prior Codex P1/P2 and bigbonk items appear genuinely addressed.
One new, non-blocking finding below (a test-coverage regression), plus a small parser observation.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: staged CDN warmup deployment
I reviewed the full diff, read the new/changed source (cdn-warm-deployment.ts, cdn-warm.ts, worker-version.ts, wrangler-deployment-target.ts, utils/toml.ts, tpr.ts, deploy.ts, init-cloudflare.ts, entry stamping) and ran tests/worker-version.test.ts + tests/cloudflare-cdn-warm.test.ts locally (27 pass).
Overall: the design is correct and I found no blocking issues. I re-verified the core mechanism end-to-end and it holds up:
- The structural insight is right: because Workers Cache partitions its key by invoked version ID (unless
cross_version_cache), a plain 2xx/3xx cannot prove the uploaded version populated the cache. Verifying viax-vinext-worker-version+ aCloudflare-Workers-Version-Overridesoverride, withredirect: "manual"so a redirect is proven at its own cache key, is the correct fix. - The stage-at-0% -> warm -> promote -> triggers-after-promotion ordering keeps the risky window in the already-safe state (previous @100%, new @0%), and strict mode fails closed by leaving the split rather than mutating.
getZeroPercentStagingTrafficrejects partial splits, drops stale 0% leftovers, and fails closed when the upload is already the 100% version.- The path-scoped-route carve-out is correct:
hasProductionRoute && !productionHostrefuses to warm workers.dev on a different cache key. - The strict + missing-target path throws inside the
tryand is re-wrapped by the catch, so promotion is correctly skipped. stampWorkerVersionparity confirmed: bothapp-router-entryandpages-router-entrystamp at the top-levelfetch, andfetch-handlerresolves to those, so all Worker-served responses carry the header.
The prior Codex P1/P2 and /bigbonk rounds all appear genuinely addressed. A few non-blocking observations below.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: staged CDN warmup deployment
I read the full diff and the changed source (cdn-warm-deployment.ts, cdn-warm.ts, worker-version.ts, wrangler-deployment-target.ts, utils/toml.ts, tpr.ts, deploy.ts, init-cloudflare.ts, entry stamping) and ran tests/worker-version.test.ts + tests/cloudflare-cdn-warm.test.ts locally (27 pass).
Overall: the design is correct and I found no blocking issues. The structural insight holds: because Workers Cache partitions its key by invoked version ID (unless cross_version_cache), a plain 2xx/3xx cannot prove the uploaded version populated the cache. Verifying via x-vinext-worker-version + a Cloudflare-Workers-Version-Overrides override, with redirect: "manual" so a redirect is proven at its own cache key, is the right fix. The stage-at-0% → warm → promote → triggers-after-promotion ordering keeps the risky window in the already-safe state (previous @100%, new @0%), and strict mode fails closed by leaving the split rather than issuing an undo mutation. getZeroPercentStagingTraffic correctly rejects partial splits, drops stale 0% leftovers, and fails closed when the upload is already at 100%. The path-scoped-route carve-out (hasProductionRoute && !productionHost) is correct, and the custom_domains fallback now applies the same host-wide guard as routes.
Parity confirmed: stampWorkerVersion runs in both app-router-entry and pages-router-entry, and fetch-handler resolves to those, so all Worker-served responses carry the header. The prior Codex P1/P2 and the many /bigbonk rounds all appear genuinely addressed.
Two small, non-blocking observations below.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: staged CDN warmup deployment
I read the full diff and the new/changed source (cdn-warm-deployment.ts, cdn-warm.ts, worker-version.ts, wrangler-deployment-target.ts, utils/toml.ts, tpr.ts, deploy.ts, init-cloudflare.ts, entry stamping) and cross-checked the version-metadata binding shape and the version-override header format.
Overall: the design is correct and I found no blocking issues. The structural fix is right — because Workers Cache partitions its key by invoked version ID (unless cross_version_cache), a plain 2xx/3xx can't prove the uploaded version populated the cache, so verifying via x-vinext-worker-version + a Cloudflare-Workers-Version-Overrides override (with redirect: "manual" so a redirect proves at its own cache key) is the correct approach. The stage-at-0% -> warm -> promote -> triggers-after-promotion ordering keeps the risky window in the already-safe state, strict mode fails closed by leaving the split, and getZeroPercentStagingTraffic correctly rejects partial splits / drops stale 0% leftovers / fails closed when the upload is already 100%. stampWorkerVersion parity is complete: both app-router-entry and pages-router-entry stamp at the top-level fetch, and fetch-handler resolves to those. The .id field read from the version-metadata binding matches Cloudflare's binding shape.
The prior Codex P1/P2 and the many /bigbonk rounds all appear genuinely addressed. Everything below is non-blocking.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
/bigbonk review for issues |
Independent review: staged CDN warmup deploymentI reviewed the full diff and read the new/changed source ( Overall: the design is correct and I found no blocking issues. The structural insight is sound — because Workers Cache partitions its key by invoked version ID (unless Things I verified independently:
The prior Codex P1/P2 and the many
|
|
fatal: could not read Username for 'https://github.com': No such device or address |
Strict CDN warmup errors named the first failure but did not make the verified path count obvious after deployment wrapped the error with its safe-state message. This made truncated CI logs harder to diagnose.\n\nInclude the warmed path count alongside the failure count in the original error so the deployment wrapper preserves both values. Extend the strict staged-deployment regression test to assert the diagnostic and the existing safe-state guidance.
2 participants
Human note
I looked into this. the bug was that vinext couldn’t tell whether the new Worker version actually handled the warmup request or not. It treated any response 2xx, 3xx response as a success even if it was an old Worker version.
So this PR: stages the uploaded Worker at 0%, prewarms cache keys, then verifies the response using version metadata then promote to 100%
Overview
This fixes #2592 by prewarming the uploaded Worker version before it receives normal traffic. The deploy stages the upload at 0%, requests the real production cache keys with a verified version override, then promotes it to 100%.
Why
Cloudflare Workers Cache includes the invoked Worker version in its cache key by default. That gives each deployed version an isolated cache partition. If an override temporarily misses during deployment propagation, the request can only populate the old version cache; vinext detects the producer mismatch and retries. A timing-based post-promotion barrier cannot provide that structural guarantee.
What changed
cache.cross_version_cacheis enabled, because that deliberately shares cache entries across versions.Validation
vp checkvp run vinext#buildvp run cloudflare#buildThe deploy behavior tests observe the full command and request sequence, version override header, retry on producer mismatch, and absence of promotion after strict failure. TOML and JSONC compatibility remain ordinary parser tests.
References