Skip to content

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

Open
sobol-sudo wants to merge 1 commit 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 1 commit 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

📝 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

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses the atomic-write half of #4487, but it intentionally leaves the deduplication requirement unresolved. Add resolved-output deduplication or narrow the linked issue scope to the atomic-write fix only.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows conventional commits and accurately summarizes the atomic write fix.
Description check ✅ Passed The description is clearly related to the atomic write corruption fix and matches the code changes.
Out of Scope Changes check ✅ Passed The changes stay focused on safe file writes and regression tests, with no unrelated scope introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 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

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

Labels

None yet

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

1 participant