Skip to content

fix(prerender): write files atomically to avoid torn output - #4488

Open
sobol-sudo wants to merge 2 commits into
nitrojs:mainfrom
sobol-sudo:fix/atomic-prerender-write
Open

fix(prerender): write files atomically to avoid torn output#4488
sobol-sudo wants to merge 2 commits into
nitrojs:mainfrom
sobol-sudo:fix/atomic-prerender-write

Conversation

@sobol-sudo

Copy link
Copy Markdown

🔗 Linked issue

Partially resolves #4487 — this covers the "not written atomically" half. The dedup half is left out deliberately, see below.

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Two prerender routes can resolve to the same output file (for example /other and /other/index.html via autoSubfolderIndex). With prerender.concurrency > 1 their writes overlap, and because writeFile truncates 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. rename is 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:

torn: 172/200
sizes seen: 300b x198, 256b x2

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

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

test/unit/fs.test.ts covers the regression along with the basic write behaviour. The concurrency test fails on main immediately (run 0) and passes with the change. Locally after pnpm build: tsc --noEmit clean, test/unit 178 passed / 2 skipped, oxlint and oxfmt --check clean.

@sobol-sudo
sobol-sudo requested a review from pi0 as a code owner July 28, 2026 14:57
@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

@sobol-sudo is attempting to deploy a commit to the Nitro Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: eb0cf008-46a7-4817-8939-a83d015bae43

📥 Commits

Reviewing files that changed from the base of the PR and between bba7350 and 607a830.

📒 Files selected for processing (2)
  • src/utils/fs.ts
  • test/unit/fs.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/utils/fs.ts
  • test/unit/fs.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

writeFile now writes to a uniquely named sibling temporary file before renaming it into place, cleans up failed temporary writes, and adds tests for nested paths, overwrites, cleanup, and concurrent writers.

Changes

Atomic write behavior

Layer / File(s) Summary
Temporary-file write and replacement
src/utils/fs.ts
writeFile uses PID- and counter-based temporary filenames, renames completed files into place, and removes temporary files when operations fail.
Write behavior and concurrency tests
test/unit/fs.test.ts
Tests cover nested directory creation, overwriting, temporary-file cleanup, and complete-file results from concurrent writes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 607a8

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)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR satisfies the atomic-write requirement in issue #4487, including same-directory temporary files, rename-based replacement, cleanup on failure, and regression tests. It does not satisfy the issu… Implement or separately link the route-deduplication change, or split the issue so this PR is evaluated only against the atomic-write requirement.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title uses the conventional commit format and accurately describes the atomic file-write fix.
Description check ✅ Passed The description clearly explains the concurrent-write corruption, the temporary-file and rename solution, test coverage, and intentionally excluded deduplication work.
Out of Scope Changes check ✅ Passed The production change and tests are directly related to preventing torn prerender output and cleaning up temporary files. No unrelated code changes are identified.
Full details: Linked Issues check

Explanation

The PR satisfies the atomic-write requirement in issue #4487, including same-directory temporary files, rename-based replacement, cleanup on failure, and regression tests. It does not satisfy the issue's separate requirement to deduplicate routes by resolved output filename.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/utils/fs.ts (1)

54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove 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 win

Cover 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

📥 Commits

Reviewing files that changed from the base of the PR and between 77b77ff and 0551607.

📒 Files selected for processing (2)
  • src/utils/fs.ts
  • test/unit/fs.test.ts

@pi0x
pi0x force-pushed the fix/atomic-prerender-write branch from 0551607 to 607a830 Compare September 7, 2026 22:12
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

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.

@pi0x

pi0x commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, and for the very clear write-up.

Rebase

I rebased the branch onto the latest main and force-pushed it. There were no conflicts. The diff is still exactly your two files.

Review

I checked the fix and it looks correct.

  • The temp file is created next to the target file, so the rename stays inside one filesystem and is atomic.
  • The temp name uses the pid plus a counter, so two writers in the same process, or in two processes, cannot pick the same name.
  • The failed write is cleaned up and the error is re-thrown, so callers still see it.

I also confirmed the regression test really is a regression test. I put the old non-atomic writeFile back and ran test/unit/fs.test.ts: the concurrency test fails right away. With your change all tests pass.

What I added

One small commit on top: a test for the cleanup path. Your tests covered the happy path, but nothing covered the rm in the catch block. The new test makes the rename fail (the target path is an existing directory) and then checks that no .tmp file is left behind.

Notes for the maintainers, not things you need to change

  1. There is a second, almost identical writeFile in src/presets/_utils/fs.ts that is still not atomic. It is not used by the prerenderer, and presets write their config files one at a time, so this fix does not need it. But the two copies now behave differently.
  2. Replacing a file by rename gives the new file default permissions instead of keeping the old file's mode. For prerender output this does not matter.
  3. If the build is killed in the middle of a write, a *.tmp file can be left in publicDir. That is a fair trade for not shipping a corrupted page, but it is worth knowing.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working prerender v3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prerender: two routes resolving to the same output file are neither deduped nor written atomically

3 participants