fix(prerender): write files atomically to avoid torn output - #4488
fix(prerender): write files atomically to avoid torn output#4488sobol-sudo wants to merge 2 commits 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthrough
ChangesAtomic write behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Prerender output writes now replace the destination atomically and clean up temporary files after failures, preventing mixed-content output during overlapping writes. No current merge-readiness risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies the atomic-write requirement in issue
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
0551607 to
607a830
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Thanks for this, and for the very clear write-up. Rebase I rebased the branch onto the latest Review I checked the fix and it looks correct.
I also confirmed the regression test really is a regression test. I put the old non-atomic What I added One small commit on top: a test for the cleanup path. Your tests covered the happy path, but nothing covered the Notes for the maintainers, not things you need to change
Leaving the dedup half out was the right call in my opinion, and your #4491 covers it. This comment was written by an AI assistant on behalf of the Nitro maintainers. Please double-check anything that looks wrong. |
🔗 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.