feat(cloudflare): warm prerendered paths before deploy#2481
Conversation
|
/bigbonk review for issues |
commit: |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — warm prerendered paths before deploy
Reviewed the new --warm-cdn-cache path end-to-end (deploy.ts, cdn-warm.ts, version-deploy.ts, prerender-manifest.ts) plus tests. The code is clean, well-factored, and well-tested. A few issues worth resolving before merge, ordered by severity.
1. (Blocking question) Does warming the version preview URL actually populate the production CDN cache?
deployWithCdnWarmup warms paths against upload.previewUrl (the <version-prefix>-<worker>.workers.dev preview URL), then runs versions deploy … @100%. The CDN adapter (packages/cloudflare/src/cache/cdn-adapter.runtime.ts) relies on Cloudflare's edge cache (ctx.cache), which is keyed by request URL including hostname/zone.
The version preview URL lives on the workers.dev zone; a production custom domain (and even the stable <worker>.workers.dev URL) is a different hostname/zone. Cache entries populated against the preview hostname are not guaranteed to be served for production requests. If that's the case, this feature warms a cache namespace that production traffic never reads, and the whole flow is a no-op for the intended benefit.
Please confirm (ideally with a real deploy against a custom domain) that a page warmed via the preview URL is served as a CDN HIT on the production hostname. If it isn't, warming needs to target the production URL after versions deploy (or use the promoted stable URL), not the preview URL. This is the core correctness question for the PR.
2. Only /404 is filtered as an error document — /500 (and /_error) also land in the manifest
isErrorDocumentRoute in prerender-manifest.ts only excludes /404:
function isErrorDocumentRoute(pathname: string, route: PrerenderManifestRoute): boolean {
return pathname === "/404" || route.route === "/404";
}But /500 is a normal static page (it is not in BLOCKED_PAGES, which is only _document/_app/_error — see shims/constants.ts:119 and STATIC_STATUS_PAGES = ["/500"] at :169). The Pages Router prerender loop renders it with status: "rendered" (build/prerender.ts:752+ skips only /404), so /500 will be selected and warmed.
A direct GET to /500 returns 200 (so it won't fail strict mode), but warming the error/status page is wasteful and semantically wrong — you're caching the error page at the edge. Suggest extending the filter to cover /500 and /_error as well, mirroring the routePattern !== "/_error" && routePattern !== "/500" carve-outs the server already uses.
3. Warm paths are read twice
deploy() reads readPrerenderWarmPaths(...) at deploy.ts:539 to decide whether to warm, then warmCdnCacheFromPrerender() reads them again at cdn-warm.ts:157. Redundant file/JSON parse and, more importantly, two places that must stay in sync (same options threading). Consider reading once in deploy() and passing the resolved paths into warmCdnCacheFromPrerender (or having deployWithCdnWarmup return early), so the "should we warm?" decision and the actual warm use the exact same list.
4. getWarmPathsFromPrerenderManifest drops includeErrorDocuments
export function getWarmPathsFromPrerenderManifest(
manifest: PrerenderManifest,
options?: { includeFallbackShells?: boolean }, // no includeErrorDocuments
): string[] {
return getPrerenderedConcretePaths(manifest, options);
}The underlying getPrerenderedConcretePaths supports includeErrorDocuments, but this public wrapper narrows the options type so callers can't pass it. Either forward the full PrerenderedPathSelectionOptions or document why error docs are intentionally non-configurable here.
5. 3xx responses count as a successful warm
warmOnePath uses redirect: "manual" and treats response.status < 400 as ok. A prerendered path that responds with a redirect (e.g. trailing-slash normalization) is recorded as "warmed" even though the actual destination page was never fetched/cached. Probably acceptable, but worth a comment, or follow the redirect once for the target you actually want warmed.
Nits
fetchWithTimeoutsendsUser-Agent: vinext-cloudflare-cdn-warm/0— the/0version suffix reads like a placeholder; either drop it or wire the real package version.warmOnePathretries network exceptions up toretriesbut does not distinguish abort-from-timeout vs. other errors; fine, just noting the abort message ("The operation was aborted") is what surfaces infailures.
Items 1 and 2 are the ones I'd want resolved before merge; 3–5 are quality improvements.
|
Review posted to PR #2481. Summary of what I found: Review postedThe Blocking:
Quality: Plus two nits (placeholder I verified concerns against the codebase: confirmed |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for 24126e6)
Re-reviewed after the follow-up commit 24126e6 ("warm production CDN for uploaded versions"). That commit resolves the substantive issues from the first pass, and switches the design to the correct Cloudflare "smoke-test" pattern (upload version → stage new version at 0% → warm the production URL with Cloudflare-Workers-Version-Overrides → promote to 100%). Verified against the Cloudflare version-overrides docs, which document exactly this flow.
Resolved since the last review:
- #1 (cache scope) — warmup now targets the production URL (custom domain, else the promoted/stable
workers.devURL) via a 0% staged version + version-override header, instead of the version preview hostname. This is the right fix for the hostname/zone-scoped edge cache. - #2 (
/500+/_error) —isErrorDocumentRoutenow excludes/404,/500, and/_error(prerender-manifest.ts:54). Covered by tests. - #3 (double read) — paths are read once in
deploy()and threaded intodeployWithCdnWarmup(root, warmPaths, …). No more duplicate read. - #4 (
includeErrorDocuments) —getWarmPathsFromPrerenderManifestnow forwards the fullPrerenderedPathSelectionOptions. - #5 (3xx warming) —
fetchWithTimeoutnow usesredirect: "follow", so redirect targets get warmed.
Tests pass (304 in the 5 PR-affected files) and vp check is clean. Remaining items below, ordered by severity.
1. (Blocking) --env / --name deploys build the version-override header with the wrong Worker name
buildVersionOverrideHeaders(wranglerConfig?.name, versionId) (deploy.ts:382) keys the header off the top-level name from parseWranglerConfig, which only reads the top-level name field (tpr.ts:191, and extractFromTOML at :273). But the pre-traffic warmup path only runs for --env/preview/--name deploys as well as production, and:
- With
--env staging, Cloudflare deploys the script as<name>-stagingby default (or whateverenv.staging.nameis). The override header still saysname="…", so the version override silently does not apply — per the docs, an unmatched/invalid override falls back to routing by traffic percentage. Since the new version is staged at 0%, the warmup requests hit the old 100% version. Warming still returns 200 (old version serves fine), so even--warm-cdn-strictwon't catch it — the new version's cache is never warmed, and the feature is a silent no-op for env deploys. --name my-apphas the same problem: it setsinfo.projectNamebut never reachesparseWranglerConfig, so the header uses the configname, not the actual deployed name.
Fix: resolve the effective Worker name the same way Wrangler does for the target env (top-level name + env suffix, or env.<name>.name), and use options.name when provided. Consider verifying the override actually applied (e.g. version-metadata binding echoed in a response header) before trusting the warm, since a mismatch fails open.
2. (Design limitation, please document) Edge warming is per-colo, not global
The CDN adapter serves from Cloudflare's edge cache (ctx.cache / caches.default), which is per-data-center, not a global store. A warmup run from a single CI runner only reaches the handful of colos that terminate those requests, so --warm-cdn-cache warms a small subset of edge locations, not the whole network. That's still useful (it primes the origin render + the nearest colos) but the naming/help text ("warm prerendered paths through the production URL") implies broader coverage than it delivers. Suggest a one-line caveat in deploy-help.ts and/or the option doc comment so users don't expect a globally-warm cache.
3. (Please verify on a real deploy) Does a version-override request populate the cache that normal traffic reads?
The whole flow assumes a response produced while serving the 0% version (selected via Cloudflare-Workers-Version-Overrides) lands in the same URL-keyed edge cache entry that a subsequent normal request (no override header) will HIT. That's plausible — the override is a routing header, not part of the cache key — but I couldn't find explicit Cloudflare documentation confirming the override header is excluded from the cache key and doesn't force a bypass/Vary. This is the core correctness assumption of the pre-promotion warm; please confirm with a real custom-domain deploy that a path warmed via the 0% override serves as a HIT (or UPDATING) on the production hostname without the header. If it doesn't, the pre-promotion warm is wasted and only the post-promotion fallback path (warmedBeforePromotion === false) actually helps.
Nits
- ReDoS (CodeQL #239):
parseWorkersDevUrlregex/https:\/\/[^\s"'<>]+\.workers\.dev[^\s"'<>]*/(version-deploy.ts:65) is polynomial-backtracking on inputs likehttps://+ long non-matching runs. Input is trusted Wrangler stdout, so not exploitable, but it's a trivial fix to make the character class exclude.//boundaries or anchor the host, which would also silence the scanner. getZeroPercentStagingTrafficreturns[current, {versionId, percentage:0}]without guarding the case where the freshly-uploadedversionIdequals the current 100% version (would produce av@100% v@0%pair). Unlikely in practice, but a quick equality check would be defensive.- User-Agent is now
vinext-cloudflare-cdn-warm(dropped the placeholder/0— good).
Item 1 is the one I'd block on; 2 and 3 are verification/documentation asks before this is relied on in production.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head ( New finding I'd block on:
Verification/documentation asks: Plus nits: the CodeQL-flagged polynomial regex in |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for 7ed95fd)
Re-reviewed after the new follow-up commit 7ed95fd ("resolve Worker names for CDN warmup"), which landed on top of the 24126e6 head my previous review covered. This commit is a targeted fix for the one blocking issue from last time, and it holds up.
Resolved since the last review
#1 (blocking) — wrong Worker name in the version-override header for --env / --name. Fixed by resolveWorkerNameForVersionOverride (deploy.ts:503) and threaded through deployWithCdnWarmup → buildVersionOverrideHeaders (deploy.ts:391–392). I verified the resolution logic against Wrangler's own config normalization in workers-sdk (validation-helpers.ts inheritableInWranglerEnvironments / appendEnvName / notAllowedInNamedServiceEnvironment), and the three branches match Wrangler exactly:
| Case | Wrangler deployed name | vinext resolves |
|---|---|---|
--name X |
X |
X ✓ |
legacy env, env.X.name set |
env.X.name verbatim |
env.X.name ✓ |
| legacy env, no per-env name | <name>-<env> |
`${name}-${env}` ✓ |
service env (legacy_env:false) |
top-level name (per-env name is a hard error in Wrangler) |
config.name ✓ |
Covered end-to-end by tests/cloudflare-cdn-warm-deploy.test.ts (production, --env staging with custom env name + env custom domain, explicit --name) and unit-tested in tests/deploy.test.ts resolveWorkerNameForVersionOverride.
Nit — getZeroPercentStagingTraffic equality guard. Now guards the "uploaded version already deployed at 100%" case (deploy.ts:491–493), falling back to the post-promotion warm instead of staging a v@100% v@0% self-pair. Tested.
Nit — CodeQL #239 (polynomial regex). parseWorkersDevUrl was rewritten as a manual whitespace tokenizer + new URL() validation (workers-dev-url.ts), dropping the backtracking regex entirely. Regression-tested with a 2048-char input (cloudflare-version-deploy.test.ts:124).
All 324 tests in the affected files pass and vp check is clean on the changed files.
Remaining (verification/documentation, not blockers)
A. (Please verify on a real deploy) Does a version-override request populate the cache normal traffic reads? This is still the core unproven assumption, unchanged by this commit. The CDN adapter (cache/cdn-adapter.runtime.ts) is a pure edge-cache design: origin get() returns null, set() is a no-op, and caching happens entirely via CDN-Cache-Control on the response, keyed by URL. The pre-promotion warm sends Cloudflare-Workers-Version-Overrides to route to the 0% version; the flow assumes the response it produces lands in the same URL-keyed edge entry a later header-less request will HIT. That's plausible (override is a routing header, not part of the cache key), but I still couldn't find Cloudflare docs confirming the header is excluded from the cache key and doesn't force a bypass/Vary. Please confirm with a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header. If not, only the post-promotion fallback (warmedBeforePromotion === false) actually helps.
B. (Documented — good) Per-colo warming. The help text now carries the caveat that warmup only primes the data centers the warm run reaches (deploy-help.ts:46–47). Matches the edge-cache reality.
Nits (non-blocking)
parseWranglerVersionUploadOutputuses a recursivefindStringByKeyover the JSON for["id","version_id","versionId"]and["preview_url","previewUrl","url"](version-deploy.ts:44–54). A recursive first-match could latch onto an unrelated nestedid/urlif Wrangler's--jsonshape changes; theparseVersionIdUUID-regex fallback covers version IDs, but a stray nestedurlcould beat the realpreview_url. Low risk given current output shape — worth anchoring to the known top-level keys if you want to be defensive.- Strict-mode failure between staging and promotion leaves the deployment in the staged
old@100% / new@0%state with triggers not applied. Production is unaffected (old version still serves), so this is a safe failure, but a one-line note in the thrown error that the staged version was left in place (and how to clean it up) would help operators.
Net: the blocking issue from the previous pass is genuinely fixed and matches Wrangler's naming semantics. The only thing gating "rely on this in production" is the real-deploy confirmation in A.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head The blocking issue from last review is genuinely fixed. The wrong-Worker-name bug for
I initially suspected the Also resolved: the Verification: 324 tests pass in the affected files, Remaining (non-blocking): the core unproven assumption — that a version-override (0%-staged) request actually populates the URL-keyed edge cache that later header-less production traffic reads — still needs a real custom-domain deploy to confirm. Per-colo warming is now documented in the help text. Two nits: the recursive JSON key search in |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for e158661)
Re-reviewed after the new follow-up commit e158661 ("harden CDN warmup diagnostics"), which landed on top of the 7ed95fd head my previous review covered. It's a focused commit that closes out the two non-blocking nits from last pass — nothing new to block on.
Resolved since the last review
Nit A — recursive JSON key search could latch onto a nested id/url. Fixed. findStringByKey (arbitrary-depth recursion) is replaced by findStringInRecord anchored to a fixed set of top-level containers: the root object, .version, and .result (version-deploy.ts:64–80). A stray diagnostics.id / diagnostics.url can no longer beat the real version_id / preview_url. Covered by the new test tests/cloudflare-version-deploy.test.ts:141 ("does not treat unrelated nested JSON IDs and URLs as upload metadata"), and the existing .version-nested case (:145) still passes, so the legitimate one-level nesting Wrangler uses is preserved. The parseVersionId / parseWorkersDevUrl regex fallbacks remain as a backstop.
Nit B — strict-mode failure between staging and promotion leaves a staged version. Fixed. Both pre-promotion strict paths now attach an operator-facing cleanup note:
- the strict warm failure wraps the underlying error via
withStagedVersionCleanupNoteand preserves it ascause(deploy.ts:404–406, 542–547); - the "no production URL/Worker name" strict case appends
getStagedVersionCleanupNote()(deploy.ts:408–413).
The note correctly explains the version may remain at 0% with the previous version still serving 100%, and how to promote/clean it up. I confirmed the placement is right: both wrapped sites run before the 100% promotion (line 421), so the staged-at-0% description is accurate. The post-promotion fallback warm (warmedBeforePromotion === false, line 429) intentionally does not get this note, which is correct — by that point the version is already live at 100%, so there's nothing staged to clean up. Tested by tests/cloudflare-cdn-warm-deploy.test.ts:225 ("explains staged version cleanup when strict pre-promotion warmup fails").
Verification
vp checkclean on all changed source files.- 326 tests pass across the affected files (
tests/deploy.test.ts,tests/cloudflare-cdn-warm.test.ts,tests/cloudflare-version-deploy.test.ts,tests/cloudflare-cdn-warm-deploy.test.ts,tests/pregenerated-concrete-paths.test.ts,tests/deploy-prerender-config.test.ts).
Remaining (unchanged — verification, not a blocker)
A. (Please verify on a real deploy) Does a version-override request populate the cache normal traffic reads? This is still the one open item, unchanged by this commit. The CDN adapter (cache/cdn-adapter.runtime.ts) is a pure edge-cache design keyed by URL, and the pre-promotion warm sends Cloudflare-Workers-Version-Overrides to route to the 0% version. The flow assumes the response produced under that override lands in the same URL-keyed edge entry a later header-less request will HIT. That's plausible (the override is a routing header, not part of the cache key), but I still can't find Cloudflare docs confirming the header is excluded from the cache key and doesn't force a bypass/Vary. Please confirm with a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header. If it doesn't, only the post-promotion fallback path actually helps. Everything else about the design is sound and well-tested.
Net: no blocking issues. The two remaining nits from the last pass are cleanly resolved and tested. The only thing gating "rely on this in production" is the real-deploy confirmation in A.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head Resolved since last review:
Verification: No blocking issues. The only remaining item — unchanged by this commit — is the real-deploy confirmation that a version-override (0%-staged) request actually populates the URL-keyed edge cache that later header-less production traffic reads. That's a verification ask, not a code defect; the design and tests are sound. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for d8a1060f)
Re-reviewed after the new follow-up commit d8a1060f ("harden CDN warmup deploy flow"), which landed on top of the e158661 head my previous review covered. It makes two changes, both of which check out.
Resolved / new since last review
1. Trigger ordering (deploy.ts). runWranglerTriggersDeploy was previously called unconditionally at the very end (after promotion). It's now hoisted into an idempotent applyTriggers() closure and invoked before warmup in the staged branch and before the post-promotion fallback warm. This is the right fix: for a first-ever deploy the custom domain/route isn't bound until triggers deploy runs, so warming https://<custom-domain>/… would 404 (or fall through) unless triggers are applied first. I traced every exit path:
- Staged branch (single 100% version):
stage(old@100% new@0%)→applyTriggers()(wrapped, throwswithStagedVersionCleanupNoteon failure) → warm viaCloudflare-Workers-Version-Overrides→promote(new@100%). Triggers bind the domain to the deployment (percentage split), so production keeps servingold@100%between trigger-apply and promotion — safe. - Fallback branch (
warmedBeforePromotion === false):promote(new@100%)→applyTriggers()(no-op if already applied) → warm. Correct — by this point the new version is already live. - The
triggersAppliedguard makes the secondapplyTriggers()a genuine no-op, so triggers run exactly once.
Verified by the two new/updated tests in cloudflare-cdn-warm-deploy.test.ts that assert the exact event order (stage → triggers → fetch → promote for the staged path; promote → triggers → fetch for the fallback path) and that a trigger failure after staging surfaces the "may remain staged at 0%" cleanup note without issuing any warm fetches.
2. JSONC trailing-comma handling (tpr.ts). parseWranglerConfig now runs stripJsonTrailingCommas(stripJsonComments(content)) before JSON.parse. Wrangler treats wrangler.jsonc/wrangler.json as JSONC (comments and trailing commas), so the prior comments-only pass would throw on a valid config with trailing commas and silently continue to the next candidate file (returning null → wrong/absent Worker name/custom-domain resolution, which directly feeds the version-override header and warmup target). I exercised the new stripper against the tricky cases — trailing-comma-like sequences inside strings ("x,}"), comment-like text inside strings ("/* not a comment */",), escaped quotes before a trailing comma ("he said \"hi\"",), nested trailing commas, and a comment immediately preceding a trailing comma — and all parse correctly. The two-pass string-state tracking is consistent between the two functions.
Verification
vp checkclean ondeploy.ts,tpr.ts,version-deploy.ts,cdn-warm.ts.- 329 tests pass across the affected files (
deploy.test.ts280,cloudflare-cdn-warm-deploy.test.ts,cloudflare-version-deploy.test.ts,cloudflare-cdn-warm.test.ts,pregenerated-concrete-paths.test.ts,deploy-prerender-config.test.ts).
Nits (non-blocking)
applyTriggers()in the fallback branch isn't wrapped with the cleanup note (deploy.ts:439). Minor asymmetry: in the staged branch a trigger failure is annotated withwithStagedVersionCleanupNote, but in the fallback branchpromote(new@100%)has already succeeded whenapplyTriggers()runs, so a trigger failure there leaves the new version live at 100% with stale routes/cron and a bare error. Not a staged-version situation (so the existing note wouldn't apply verbatim), but a one-line hint that promotion succeeded and only trigger application failed would help operators know the code is already serving and only routes/cron need reapplying.- JSONC parsing is now a hand-rolled two-pass (comments, then trailing commas). It's correct and matches the pre-existing
stripJsonCommentsstyle, so no change requested — just flagging that these two passes must stay in sync (same string/escape state machine) if either is touched later.
Remaining (unchanged — verification, not a blocker)
Does a version-override (0%-staged) request populate the URL-keyed edge cache that later header-less production traffic reads? Still the one open correctness assumption, untouched by this commit. The CDN adapter (cache/cdn-adapter.runtime.ts) is a pure edge-cache design keyed by URL; the pre-promotion warm sends Cloudflare-Workers-Version-Overrides to route to the 0% version and assumes the resulting response lands in the same URL-keyed entry a later no-header request will HIT. Plausible (the override is a routing header, not part of the cache key), but please confirm on a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header. If not, only the post-promotion fallback path actually helps.
Net: no blocking issues. The trigger-ordering hoist is correct and the JSONC trailing-comma fix closes a real config-parse gap that would have broken Worker-name/custom-domain resolution for common wrangler.jsonc files. The only thing gating "rely on this in production" remains the real-deploy cache confirmation above.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head Resolved / new since last review:
Verification: No blocking issues. Two nits: (1) |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for 3d3e3cd)
Re-reviewed after the new follow-up commit 3d3e3cd ("clarify CDN warmup trigger failures"), which landed on top of the d8a1060f head the prior review covered. It closes out the two non-blocking nits from last pass, and both check out.
Resolved since the last review
Nit 1 — fallback-branch trigger failure was unannotated (deploy.ts:439). Fixed. The post-promotion applyTriggers() in the warmedBeforePromotion === false branch is now wrapped and rethrown via withPromotedVersionTriggerNote (deploy.ts:438–443, 572–580). I traced every fallback entry point (stagingTraffic === null, or staged-but-no-URL/headers-and-not-strict) — in all of them runWranglerVersionDeploy(…@100%) at deploy.ts:433 has already run before applyTriggers(), so the "may already be promoted to 100%, but triggers/routes may not be updated" wording is accurate (arguably it's definitely promoted, not just "may", but that's a harmless conservative phrasing). The note preserves the original error as cause. Covered by the new test cloudflare-cdn-warm-deploy.test.ts:374 ("explains promoted version state when fallback trigger deployment fails"), which drives the fallback path via a non-single-100% status, promotes, then fails triggers, and asserts both the note and that no warm fetch fired.
Nit 2 — hand-rolled two-pass JSONC parsing had to stay in sync. Fixed by merging the two passes. stripJsonComments + stripJsonTrailingCommas are collapsed into a single stripJsonCommentsAndTrailingCommas (tpr.ts:126) with a small isJsonTrailingComma lookahead helper (tpr.ts:199). This removes the duplicate string/escape state machine — there's now one comment/string tracker, and the trailing-comma decision re-uses a comment-aware lookahead so a comma followed by a comment-then-}/] is still correctly stripped. I re-ran the tricky cases against the merged implementation (comma-like/comment-like text inside strings, escaped quotes before a trailing comma, nested trailing commas, a comment between a comma and its closing bracket, and a comma followed by a comment then a real key which must be preserved) — all parse correctly.
Verification
vp checkclean ondeploy.ts,tpr.ts,cdn-warm.ts,version-deploy.ts.- 330 tests pass across the six affected files (
deploy.test.ts,cloudflare-cdn-warm-deploy.test.ts,cloudflare-version-deploy.test.ts,cloudflare-cdn-warm.test.ts,pregenerated-concrete-paths.test.ts,deploy-prerender-config.test.ts).
Remaining (unchanged — verification, not a blocker)
Does a version-override (0%-staged) request populate the URL-keyed edge cache that later header-less production traffic reads? Still the one open correctness assumption, untouched by this commit. The CDN adapter (cache/cdn-adapter.runtime.ts) is a pure edge-cache design keyed by URL; the pre-promotion warm sends Cloudflare-Workers-Version-Overrides to route to the 0% version and assumes the resulting response lands in the same URL-keyed entry a later no-header request will HIT. Plausible (the override is a routing header, not part of the cache key), but please confirm on a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header. If not, only the post-promotion fallback path actually helps.
Net: no blocking issues. Both nits from the previous pass are cleanly resolved, the JSONC merge removes a genuine dual-maintenance hazard, and the new fallback trigger annotation gives operators an accurate recovery hint. The only thing gating "rely on this in production" remains the real-deploy cache confirmation above.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head Resolved since last review:
Verification: No blocking issues. The only remaining item is the unchanged real-deploy confirmation that a 0%-staged version-override request populates the URL-keyed edge cache that later header-less production traffic reads — a verification ask, not a code defect. The design and tests are sound. |
|
/bigbonk review for issues |
|
Review posted to PR #2481. SummaryI re-reviewed against the current head Blocking issue from last pass is genuinely fixed. The
Contract confirmed sound. Plain One minor code nit (non-blocking): the refactor now gates Verification: |
|
Addressed the remaining nit from the last Bonk pass: Cloudflare deploy now emits the supplemental path manifest for warm deploys and for non-skip explicit prerender builds only. That means route-root metadata is always loaded when the manifest is emitted, while /bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for 6d1df98)
Re-reviewed after the new follow-up commit 6d1df98 ("avoid stale skip-build path manifests"), which lands on top of the a89176e head my previous review covered. It's a tight, correct fix for the exact nit I raised last pass (the dropped routeRootConfig), plus a deliberate tightening of the manifest-emit gate.
The change (deploy.ts)
Two edits:
shouldEmitPrerenderPathManifestnarrowed fromwarmCdnCache || prerenderDecisiontowarmCdnCache || (!skipBuild && prerenderDecision).routeRootConfigis now loaded unconditionally whenever the manifest is emitted (theshouldLoadRouteRootConfiggate is gone).
Resolved since the last review — nit #1 (dropped routeRootConfig)
Fixed correctly. Previously, in the --prerender-all-only (no --warm-cdn-cache) path, shouldLoadRouteRootConfig was false, so routeRootConfig was passed as null and discovery could scan the wrong dirs for a custom route root. Now routeRootConfig always loads when a manifest is emitted, so the manifest is accurate whenever it's actually written. This matches the PR description's stated intent: "route-root metadata is always loaded when the manifest is emitted."
The emit-gate tightening checks out
I traced the full truth table across warmCdnCache / skipBuild / prerenderDecision:
| warmCdnCache | skipBuild | prerenderDecision | emits manifest? | loads config? | prerender runs? |
|---|---|---|---|---|---|
| T | F | any | yes | yes | if prerenderDecision |
| T | T | any | yes | yes | if prerenderDecision |
| F | F | T | yes | yes | yes |
| F | F | F | no | no | no |
| F | T | T | no | no | yes |
| F | T | F | no | no | no |
--skip-build --warm-cdn-cache(rows 1–2, warm) still emits + loads config — thea89176eskip-build fix I verified last pass is preserved. Confirmed bydeploy-prerender-config.test.ts:288.--skip-build --prerender-all(row 5) no longer emits. I confirmed this is a genuinely safe no-op, not a regression:vinext-prerender-paths.jsonis consumed only byreadPrerenderWarmPaths, which is called only underif (options.warmCdnCache)(deploy.ts:898). With--skip-buildand no--warm-cdn-cache, the manifest was never read — so previously it wrote a manifest from a stale dist that nothing consumed, and now it correctly skips that work (and skips loading Vite config for it). That's exactly the "existing contract" described in the PR body, and it matches the commit title. Step 6a'srunPrerendergate (line 849) is untouched, so prerender still runs in that case as before.
Verification
vp check packages/cloudflare/src/deploy.tsclean (format + lint + types).shouldLoadVinextPrerenderConfig(line 810) is still live at line 811 after removing theshouldLoadRouteRootConfigreference — no dead variable.- Tests pass:
deploy-prerender-config.test.ts+deploy.test.ts+cloudflare-cdn-warm.test.ts(316),app-router-production-build.test.ts+prerender-paths.test.ts+cloudflare-cdn-warm-deploy.test.ts(24).
Note (optional, non-blocking)
There's no test asserting the new narrowed behavior — i.e. that --skip-build --prerender-all (without --warm-cdn-cache) now does not emit vinext-prerender-paths.json. The existing suite covers --skip-build --warm-cdn-cache (emits) and plain build (doesn't emit), but not this specific row that the commit changed. A one-line assertion would lock in the intended contract and guard against a future merge silently widening the gate again (the same class of accidental regression the 4c56a37 merge introduced). Not required for merge.
Remaining (unchanged — verification, not a blocker)
Does a version-override (0%-staged) request populate the URL-keyed edge cache that later header-less production traffic reads? Still the one open correctness assumption, untouched by this commit. Please confirm on a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header.
Net: no blocking issues. The dropped-routeRootConfig nit is cleanly fixed, and the emit-gate tightening is a correct, deliberate contract (skip-build only emits when warming, which is the only path that consumes the manifest). The only suggestion is an optional test for the newly-narrowed no-emit row. The real-deploy cache confirmation remains the gate for relying on this in production.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head Resolved — the dropped Emit-gate tightening verified correct. I traced the full
Verification: No blocking issues. One optional suggestion: add a test asserting the newly-narrowed no-emit row ( |
|
Added the optional contract-lock test from the last Bonk pass: /bigbonk review for issues |
|
Fixed the workers-cache deploy issue from the attached real run: when fallback warmup is used and /bigbonk review for issues |
|
Updated the deploy logs so the version flow distinguishes staging at 0%, fallback promotion, and final warmed promotion. The real workers-cache run should now read as staging -> triggers -> warm -> promote instead of repeating the same “Deploying warmed” label. /bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for e0f2e3b)
Re-reviewed after three new follow-up commits on top of the 3d3e3cd/6d1df98 heads prior passes covered:
7436104("lock skip-build prerender manifest gate") — the optional contract-lock test I suggested last pass.796fb4f("warm fallback deploy URL from triggers") — the workers-cache real-run fix.e0f2e3b("clarify CDN warmup deploy logs") — distinct staging/promotion log labels.
All changes since the last review are confined to deploy.ts and version-deploy.ts (plus tests). vp check is clean and 328 tests pass across the four affected suites.
Resolved / new since last review
1. Fallback warmup now recovers a target URL from triggers deploy (the workers-cache fix). This is the real bug behind the attached run. When the current deployment is not a single 100% version (e.g. a mixed old@100% / other@0% state), getZeroPercentStagingTraffic returns null, so the flow takes the post-promotion fallback branch. In that branch runWranglerVersionDeploy(…@100%) frequently prints no workers.dev URL, so deployed.deployedUrl was null and warmup bailed with "no production URL could be inferred" (or failed under --warm-cdn-strict).
The fix captures the URL from wrangler triggers deploy output (runWranglerTriggersDeploy now returns { deployedUrl, output }) into triggersDeployedUrl, and both warm sites fall back to it: staged.deployedUrl ?? triggersDeployedUrl (staged branch, deploy.ts:551) and deployed.deployedUrl ?? triggersDeployedUrl (fallback branch, deploy.ts:598). I traced ordering on every path:
- Fallback branch:
promote(@100%)→applyTriggers()(populatestriggersDeployedUrl) →resolveCdnWarmupTargetUrl(…, deployed.deployedUrl ?? triggersDeployedUrl). The trigger URL is populated before it's read. ✓ - Staged branch:
stage(0%)→applyTriggers()→ target resolution. Same ordering. ✓ resolveCdnWarmupTargetUrlstill prefers a configuredcustomDomainfirst, sotriggersDeployedUrlis only a last-resort host and can't override a custom-domain warm target. ✓- The final return value adds
triggersDeployedUrlto the fallback chain so the "Deployed to:" line reflects the real URL. ✓
Covered by the new test cloudflare-cdn-warm-deploy.test.ts:258 ("uses the triggers deploy URL for post-promotion fallback warmup") which reproduces the mixed-version status, a URL-less promote, and asserts the exact upload → status → promote → triggers → fetch order against the triggers-parsed URL.
2. Distinct deploy log labels. runWranglerVersionDeploy takes a phase param ("stage" | "promote-warmed" | "promote-uploaded") so the version flow now reads Staging uploaded Worker version at 0%… → … → Promoting warmed Worker version… instead of repeating "Deploying warmed Worker version". This directly addresses the confusing repeated label in the workers-cache run. Unit-tested at cloudflare-version-deploy.test.ts:97.
3. Contract-lock test landed. deploy-prerender-config.test.ts:210 now asserts --skip-build --prerender-all (without --warm-cdn-cache) writes no vinext-prerender-paths.json, locking the narrowed emit gate from 6d1df98 against a future merge silently re-widening it — exactly the class of regression the 4c56a37 merge introduced.
Verification
vp check packages/cloudflare/src/deploy.ts packages/cloudflare/src/version-deploy.ts— clean (format, lint, types).- All callers of the two changed signatures updated:
runWranglerTriggersDeploy(now returns a result) has one caller (applyTriggers);runWranglerVersionDeploy's newphaseparam precedesexecute, and all three source calls + three test calls pass args in the correct positions. - 328 tests pass across
cloudflare-cdn-warm-deploy,cloudflare-version-deploy,deploy,deploy-prerender-config.
Nits (non-blocking)
parseWorkersDevUrlontriggers deployoutput returns the first workers.dev token. For the fallback URL this is fine (it's only used when there's no custom domain and no promote URL), but if a future Wranglertriggers deployoutput prints multiple workers.dev URLs (e.g. a stale route plus the live one), the first-match could pick the wrong host. Low risk given current output shape and the custom-domain-first preference inresolveCdnWarmupTargetUrl; noting for awareness.- The staged-branch warm still reads
staged.deployedUrl ?? triggersDeployedUrl, but in the staged branch the 0%-staged version deploy typically doesn't print a live URL either, sotriggersDeployedUrlis the effective source there too — the two branches now share the same recovery logic, which is good, just implemented at two call sites that must stay in sync.
Remaining (unchanged — verification, not a blocker)
Does a version-override (0%-staged) request populate the URL-keyed edge cache that later header-less production traffic reads? Still the one open correctness assumption, untouched by these commits. The pre-promotion warm sends Cloudflare-Workers-Version-Overrides to route to the 0% version and assumes the resulting response lands in the same URL-keyed edge entry a later no-header request will HIT. Please confirm on a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header. (Note: the fallback path fixed in this pass warms after promotion, so it doesn't depend on this assumption — only the pre-promotion staged warm does.)
Net: no blocking issues. The 796fb4f fallback-URL fix resolves the real workers-cache deploy failure with correct ordering and a matching regression test; the log-label and contract-lock changes are clean. The only production gate remains the real-deploy edge-cache confirmation for the pre-promotion staged-warm path.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head Resolved / new since last review:
Verification: No blocking issues. Two nits (first-match |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — warm prerendered paths before deploy (updated for 09cc1bb)
Re-reviewed after five new follow-up commits on top of the e0f2e3b head prior passes covered:
be4635e("offer CDN warmup deploy script"),800b80d("update wrangler configs"),6c66db4("default CDN warmup to yes") — add the--warm-cdn-cacheinit/create-app option and default it to Yes for Workers Cache projects.101bcc6("explain first warm deploy failure") — detect the "Worker does not yet exist" upload error and rewrite it into an actionable message.09cc1bb("keep deploy errors concise") — the deploy CLI now printserror.messageinstead of the full error object.
All changes are confined to init/create-app plumbing plus version-deploy.ts / cli.ts error handling. vp check is clean on all six changed source files, and 170 tests pass across the four affected suites.
What I verified
1. Init / create-app wiring is correct.
--warm-cdn-cache/--no-warm-cdn-cacheparse via the new generalizedparseBooleanArg(shared with--prerender), tested for the=maybeerror path.- The prompt slots in after prerender and only when
cdnCache === "workers-cache";--warm-cdn-cachewith a non-Workers-Cache CDN throws--warm-cdn-cache requires --cdn-cache=workers-cache(tested). - The default-Yes fallback logic (
options.cloudflare?.warmCdnCache ?? options.cloudflare?.cdnCache === "workers-cache") parses correctly —===binds tighter than??, so it'swarmCdnCache ?? (cdnCache === "workers-cache"). Theinit.tsscript writer and thecreate-vinext-apptemplate use the identical expression, so the two paths stay in parity. - Existing
data-cachefixtures don't accidentally flip to warm; verified against the untouchedcloudflareInitOptionsfixture.
2. First-deploy error rewrite is sound and degrades safely. isMissingWorkerVersionUploadError runs errorText(error) over message + stdout/stderr/output (correctly filtering the null-first output array) and only rewrites the upload failure, which is the very first wrangler call — so nothing is staged/promoted yet, and there's no cleanup concern. If the Cloudflare API wording ever changes, detection just falls through to the raw error (safe). Unit-tested at cloudflare-version-deploy.test.ts:134.
Nits (non-blocking)
A. 09cc1bb (error.message only) drops stdout diagnostics for buffered version-deploy failures. The plain (non-warmup) deploy path streams wrangler stdout/stderr live via spawn (deploy.ts:481–489), so it's unaffected. But every warmup wrangler call (versions upload/deploy, deployments status, triggers deploy) goes through runWranglerCommand, which uses buffered execFileSync and only prints captured output on success — on failure it throws before the print. I confirmed execFileSync's error.message appends stderr but not stdout, so wrangler diagnostics it prints to stdout are now silently dropped in the concise CLI path. For a warmup deploy that fails on, say, a config/auth issue surfaced on stdout, the operator sees only Command failed: node …/wrangler … + stderr. The PR already has an errorText helper that reads stdout — consider surfacing captured stdout on failure in runWranglerCommand (print before rethrow) so the concise CLI message doesn't lose it.
B. Default-Yes warmup guarantees a first-deploy failure for brand-new Workers. resolveInitWarmCdnCache defaults to true in agent/non-interactive environments, so a fresh vinext init --platform=cloudflare scaffolds deploy:vinext = "vinext-cloudflare deploy --warm-cdn-cache". On a never-deployed Worker the first deploy will always hit the "Worker does not yet exist" path. That's exactly what 101bcc6 mitigates (clear "run once without --warm-cdn-cache first" message), so it's handled — but it's worth a one-line note in the generated README/help that the first deploy of a new Worker should omit the flag, rather than relying on users hitting the error and reading the remediation.
Remaining (unchanged — verification, not a blocker)
Does a version-override (0%-staged) request populate the URL-keyed edge cache that later header-less production traffic reads? Still the one open correctness assumption, untouched by these commits. Please confirm on a real custom-domain deploy that a path warmed via the 0% override serves as HIT/UPDATING on the production hostname without the header. (The post-promotion fallback path warms after promotion, so it doesn't depend on this assumption — only the pre-promotion staged warm does.)
Net: no blocking issues. The init/create-app wiring is clean, well-tested, and parity-consistent across both scaffolders, and the first-deploy error rewrite is a genuine UX improvement. The two nits are (A) restoring stdout in the concise error path for buffered warmup commands, and (B) a doc note about the first deploy of a new Worker. The real-deploy edge-cache confirmation remains the only gate for relying on the pre-promotion warm in production.
|
Review posted to PR #2481. SummaryI re-reviewed against the current head What I verified as correct:
Two non-blocking nits I flagged:
No blocking issues. The only remaining item is the longstanding real-deploy confirmation that a 0%-staged version-override request populates the URL-keyed edge cache normal traffic reads. |
2 participants
Summary
--warm-cdn-cachedeploy path that uploads a Worker version, warms prerendered paths through the version preview URL, then deploys that exact versionValidation
vp check packages/cloudflare/src/deploy.ts packages/cloudflare/src/cdn-warm.ts packages/cloudflare/src/version-deploy.ts packages/cloudflare/src/cli.ts packages/cloudflare/src/deploy-help.ts packages/vinext/src/server/prerender-manifest.ts tests/deploy.test.ts tests/deploy-prerender-config.test.ts tests/cloudflare-cdn-warm.test.ts tests/cloudflare-version-deploy.test.ts tests/pregenerated-concrete-paths.test.tsvp test run tests/deploy.test.ts tests/deploy-prerender-config.test.ts tests/cloudflare-cdn-warm.test.ts tests/cloudflare-version-deploy.test.ts tests/pregenerated-concrete-paths.test.ts