fix(prerender): write files atomically to avoid torn output - #4488
fix(prerender): write files atomically to avoid torn output#4488sobol-sudo wants to merge 1 commit into
Conversation
|
@sobol-sudo is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthrough
ChangesAtomic write behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/fs.ts (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove implementation-explaining comments.
The code is self-explanatory; these comments violate the source-file guideline. As per coding guidelines, “Do not add comments explaining what the line does unless prompted.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/fs.ts` around lines 54 - 56, Remove the implementation-explaining comment above the sibling temporary-file write and rename logic in the filesystem utility, leaving the underlying atomic write behavior unchanged.Source: Coding guidelines
test/unit/fs.test.ts (1)
26-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover cleanup after a failed replacement.
This only tests successful cleanup after
rename. Add a test that forces the post-write replacement to fail and verifies the sibling temporary file is removed, as required by the PR objective.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/fs.test.ts` around lines 26 - 30, Add a failure-path test alongside “leaves no temporary files behind” that forces the post-write replacement/rename to fail, then verifies the sibling temporary file is removed from dir. Reuse the existing filesystem helper and temporary-file naming behavior, and assert cleanup occurs after the write operation rejects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/utils/fs.ts`:
- Around line 54-56: Remove the implementation-explaining comment above the
sibling temporary-file write and rename logic in the filesystem utility, leaving
the underlying atomic write behavior unchanged.
In `@test/unit/fs.test.ts`:
- Around line 26-30: Add a failure-path test alongside “leaves no temporary
files behind” that forces the post-write replacement/rename to fail, then
verifies the sibling temporary file is removed from dir. Reuse the existing
filesystem helper and temporary-file naming behavior, and assert cleanup occurs
after the write operation rejects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 147c1344-e67e-4c9b-943d-388f2fe4c419
📒 Files selected for processing (2)
src/utils/fs.tstest/unit/fs.test.ts
🔗 Linked issue
Partially resolves #4487 — this covers the "not written atomically" half. The dedup half is left out deliberately, see below.
❓ Type of change
📚 Description
Two prerender routes can resolve to the same output file (for example
/otherand/other/index.htmlviaautoSubfolderIndex). Withprerender.concurrency > 1their writes overlap, and becausewriteFiletruncates and writes in place, the result can be a file torn between both payloads rather than either one of them.The interleaving is: writer A truncates, writer B truncates, B writes 256 bytes, A writes 300 bytes — leaving a 300 byte file whose first 256 bytes came from B and whose last 44 came from A. That is exactly the shape reported in #4487, where it corrupted roughly 1 in 10 production static builds of a Nuxt SPA and shipped silently.
This writes to a sibling temp file and renames it into place.
renameis atomic within a filesystem, and the temp file is created in the same directory as the destination, so overlapping writers now produce last-one-wins instead of a torn file. The temp file is removed if the write fails.Reproducing the current behaviour locally, 200 pairs of concurrent writes of a 300 byte and a 256 byte payload to one path:
With this change, 0/200.
I left the dedup half of #4487 out on purpose. Which of two colliding routes should win is a design call, and deduping needs the resolved
fileName, which is only known after the route has already been rendered — so it seemed better to leave that to you rather than guess. The atomic write stands on its own: it removes the corruption for any path that reaches this state, which is what the reporter suggested doing independently.📝 Checklist
test/unit/fs.test.tscovers the regression along with the basic write behaviour. The concurrency test fails onmainimmediately (run 0) and passes with the change. Locally afterpnpm build:tsc --noEmitclean,test/unit178 passed / 2 skipped,oxlintandoxfmt --checkclean.