fix(builders): skip generated writes that would not change the file - #3454
fix(builders): skip generated writes that would not change the file#3454josuediazflores wants to merge 1 commit into
Conversation
|
@josuediazflores is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| * signal must override this to `false`, otherwise skipping the write also | ||
| * suppresses the signal. | ||
| */ | ||
| protected get skipsUnchangedGeneratedWrites(): boolean { |
There was a problem hiding this comment.
AI Review: Note
This getter only governs writeGeneratedFile. Three call sites added in this same change bypass it by calling the free writeFileIfChanged directly: createManifest's manifest.json, its diagnostics copy, and createWebhookBundle's unbundled path.
Nitro reaches all three (createManifest + createWebhookBundle({ bundle: false }) in its #buildOnce), so those writes are skipped for nitro even though it returns false here. Nothing breaks today, since the mtime signal the opt-out protects is on workflows.mjs, and the webhook route content is a static string with no user code in its graph. But the getter reads like a per-target kill switch and isn't one, so the next mtime-signal consumer added on any other generated file will silently not be covered.
Either route those writes through a method that consults the flag, or narrow the doc comment to say it governs writeGeneratedFile only.
| if (!bundle) { | ||
| // For Next.js, just write the unbundled file | ||
| await writeFile(outfile, routeContent); | ||
| await writeFileIfChanged(outfile, routeContent); |
There was a problem hiding this comment.
AI Review: Nit
The comment above this line ("For Next.js, just write the unbundled file") is now misleading: nitro's #buildOnce also calls createWebhookBundle({ bundle: false }), so this branch is not Next-only. Worth dropping the framework name while you're in here.
| * `<buildDir>/workflow`, which no dev server watches, so skipping the write | ||
| * would not have saved a recompile here. | ||
| */ | ||
| protected override get skipsUnchangedGeneratedWrites(): boolean { |
There was a problem hiding this comment.
AI Review: Note
I could not reproduce the staleness this opt-out is protecting against, so flagging it in case the premise is narrower than the comment implies.
I flipped this getter to true, rebuilt, and ran three consecutive step-body-only edits against workbench/nitro-v3 dev. Fresh step code was served every time. Across each rebuild workflows.mjs kept a constant sha but got a new inode and mtime, so the dev handler's version key kept moving and the steps.mjs re-import kept firing.
Instrumenting writeGeneratedFile showed only a skip for that path on those rebuilds, while a directory watcher caught a workflows.mjs.<uuid>.tmp -> rename each time. I could not attribute that write, most likely a second builder instance whose stderr I was not capturing, so I am not claiming the opt-out is unnecessary and I would keep it: preserving the write is the conservative side and it costs nitro nothing.
Two asks: which workbench did you see the stale step code on (nitro, nitro-v2, or nitro-v3)? And if this is meant to be load-bearing, it is worth a test that fails when the getter returns true. The unit test added here asserts the mechanism (opted-out targets still rewrite) rather than the consequence, so as written nothing stops someone from deleting this override later as apparent dead weight.
| export { isValidBuildTarget, validBuildTargets } from './types.js'; | ||
| export { VercelBuildOutputAPIBuilder } from './vercel-build-output-api.js'; | ||
| export { resolveWorkflowAliasRelativePath } from './workflow-alias.js'; | ||
| export { hasSameContent, writeFileIfChanged } from './write-if-changed.js'; |
There was a problem hiding this comment.
AI Review: Note
Now that this is exported, the same identical-rewrite churn is still present in the Vite-based integrations, which write their generated routes into the watched app directory with plain writeFile:
packages/astro/src/builder.ts:93(workflows route),:172(webhook route),:57(.gitignore)packages/sveltekit/src/builder.ts:113(workflows route),:196(webhook route),:73and:134(.gitignore)
Same bug class as #3451, and those builders don't set config.watch, so they can't be distinguished from a production build by that flag. Not asking for it in this PR, the Next fix stands on its own, just worth a follow-up issue so it doesn't get lost.
| '@workflow/nitro': patch | ||
| --- | ||
|
|
||
| Keep rewriting the generated bundles even when unchanged, since the dev handler cache-busts `steps.mjs` off the mtime of `workflows.mjs` and a step-body-only edit would otherwise serve stale code. |
There was a problem hiding this comment.
AI Review: Nit
Changeset messages land in the user-facing changelog, so internals like the mtime cache-busting read oddly there. Something closer to the user-visible effect, e.g. "Keep regenerating dev bundles so step edits are picked up without restarting the dev server", would fit better. Same for the manifest-order one, which is fine as-is but leans a little on "concurrent discovery order".
|
AI Review: Note All 7 GitHub Actions workflows are sitting at Ran locally in the meantime, all green: typecheck 14/14, |
🦋 Changeset detectedLatest commit: 41e719f 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 |
CI triage: 53 of the 57 red checks are fork-permission fallout, the other 4 reproduce on
|
| Lane | Test | Error |
|---|---|---|
| Local Dev (turbopack, stable node) | dev.test.ts > should follow Next flow-route HMR rebuild rules for body-only changes |
expected 5 to be 1 |
| Local Dev (webpack, stable node) | same | expected 5 to be 1 |
| Local Dev (webpack, canary quickjs) | same | 300s timeout, hmrFuzzAddedStep never in manifest |
| E2E Windows (node) | dev.test.ts > should rebuild on adding workflow file |
120s timeout, newWorkflowFile never generated |
On main, predating this run:
expected 5 to be 1, same test: 31533009514, 31643295401, 31636132289. Alsoexpected 4 to be 1in 31636826001.hmrFuzzAddedSteptimeout: webpack canary lanes hit it in 8 of the last 14mainruns.- Windows
should rebuild on adding workflow file: byte-identical failure in 31752021817, which started 25 minutes before this run.
All 14 most recent Tests runs on main are red, and three Local Dev failures in one run is within the range main already shows.
Caveat: the flaky assertion is expectHmrLogCounts, which counts exactly the rebuild signals this PR changes, so matching a known flake is strong evidence rather than proof.
Green here: Unit Tests on ubuntu and windows (including the new write-if-changed.test.ts), Biome, Vitest Plugin Tests, Build Shared E2E Packages, and ~100 other E2E lanes including all Local Prod and Local Postgres.
Ask: authorize the Vercel deployments, or re-run just the three E2E Local Dev lanes on this SHA to confirm the flake.
|
@josuediazflores Could you sign your commits? Our policy requires it before merging. Easiest is to just ask your agent to squash + force push this branch content as a new signed commit |
96147a8 to
037284f
Compare
The builder rewrote its generated files on every rebuild regardless of content. Those files live inside the directory the dev server watches, so a byte-identical rewrite still bumped the mtime and, via the temp-file + rename, the inode. Webpack saw a change to a file in its module graph and ran a second compilation round for a rebuild that produced nothing new. Compare before writing, and skip when the bytes already match. Measured on workbench/nextjs-webpack, a no-op rebuild goes from two compilation rounds to one. An edit that genuinely changes the bundle is unaffected: the content differs, so the write happens as before. Covers the paths that bypassed the atomic write helper as well: the manifest and its diagnostics copy, the unbundled webhook route, and the generated .gitignore / config.json / public manifest in the Next builder. Two things fell out of this: Nitro opts out. Its dev handler cache-busts the dynamic import of steps.mjs off the mtime of workflows.mjs, so a step-body-only edit leaves workflows.mjs byte-identical and skipping that write would freeze the version key and serve the previous step code until restart. It loses nothing by opting out, since it generates into <buildDir>/workflow, which no dev server watches. manifest.json is now emitted in sorted order. Entry discovery runs concurrently, so its key order varied between otherwise identical builds and it was rewritten every time regardless of the guard. Signed-off-by: josuediazflores <152042011+josuediazflores@users.noreply.github.com>
037284f to
41e719f
Compare
2 participants
Description
Fixes #3451.
The builder rewrote its generated files on every rebuild regardless of content. Those files live inside the directory the dev server watches, so a byte-identical rewrite still bumped the mtime and, via the temp-file + rename, the inode. Webpack saw a change to a file in its module graph and ran a second compilation round for a rebuild that produced nothing new.
Tapping
compiler.hooks.watchRuninworkbench/nextjs-webpackand readingmodifiedFiles, a no-optouchof a workflow file onmaingives:This compares before writing and skips when the bytes already match, which removes round 2. An edit that genuinely changes the bundle is unaffected: the content differs, the write happens, and round 2 is legitimate work.
Covers the paths that bypassed
writeGeneratedFiletoo: the manifest and its diagnostics copy, the unbundled webhook route, and the generated.gitignore/config.json/ publicmanifest.jsonin the Next builder.Two things fell out of implementing it, both worth calling out:
Nitro opts out. Its dev handler cache-busts the dynamic import of
steps.mjsoff the mtime ofworkflows.mjs(packages/nitro/src/index.ts:496, v2 branch at:520). A step-body-only edit leavesworkflows.mjsbyte-identical, so guarding that write freezes the version key and the handler keeps serving the previous step code until restart. I hit this with the first version of the patch. The unconditional rewrite was accidentally load-bearing. Nitro loses nothing by opting out, since it generates into<buildDir>/workflow, which no dev server watches.grep -rn "mtimeMs" packages/says it is the only such consumer.manifest.jsonis now emitted in sorted order. Entry discovery runs concurrently, so its key order varied between otherwise identical builds and it was rewritten every time regardless of the guard. Two consecutivenext build --webpackruns produced deep-equal manifests with different key order instepsandworkflows. This is separable from the rest if you would rather not take it.Also worth noting: #2809 proposed this same fix in its second bullet ("skip the write when generated file content is unchanged"). Only its first bullet shipped, in #2813.
How did you test your changes?
Unit tests. New
packages/builders/src/write-if-changed.test.ts, 16 tests. Includes a regression test thatwriteGeneratedFileleaves mtime and inode untouched on identical content, and one asserting the opted-out path still rewrites. Both were verified to fail when the guard is removed.Workbench, Next (webpack). Two consecutive
pnpm buildruns inworkbench/nextjs-webpackwith no source change. Before:manifest.jsonchanged sha every run andflow/route.jschurned its inode. After: every generated file identical in sha, mtime, and inode.Workbench, Next dev. The
watchRunmeasurement above, run with and without the patch on the same app, verifyingbase-builder.jsgenuinely lacked the guard in the control. Steady state goes from 2 nodejs rounds per no-op edit to 1.Workbench, Nitro. Two consecutive
pnpm buildruns inworkbench/nitro-v3confirmworkflows.mjsstill bumps its mtime while its sha stays constant, i.e. the opt-out preserves the pre-existing behavior the dev handler depends on.Suites.
typecheck41/41. Unit tests across builders/next/nitro/core: 2309 passing. 15 pre-existing failures (14 inpackages/core/src/vm/uint8array-base64.test.ts, 1 innode-module-esbuild-plugin.test.ts) reproduce identically on a clean tree; they look like Node 25 vs the base64 polyfill and are unrelated.PR Checklist - Required to merge
pnpm changesetwas run to create a changelog for this PRgit commit --signoffon your commits)@vercel/workflowin a comment once the PR is ready, and the above checklist is complete