Fix ENOENT crash on clean builds with merge: true (#930)#931
Fix ENOENT crash on clean builds with merge: true (#930)#931
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
WalkthroughThe changes address issue Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Review of PR #931 – Fix ENOENT crash on clean builds with merge: trueThe fix is clearly necessary and the approach (seeding an empty manifest before the plugin instantiates) is the right one. A few things worth discussing before merging: 1. Import style inconsistencyThe file already uses ES module import syntax for its first two lines: import type { Config } from "../types"
import { getFilteredEnv } from "./envFilter"But the new additions use CommonJS const fs = require("fs")
const path = require("path")Every other file in the project that uses 2. No tests for the new functionThe PR description mentions "All 349 JS tests pass" but there is no new test covering
…would protect against regressions and document the intended behavior. 3. TOCTOU race condition (minor / low severity)The A simple mitigation is to wrap in a try/catch and ignore const ensureManifestExists = (manifestPath: string): void => {
if (!existsSync(manifestPath)) {
mkdirSync(dirname(manifestPath), { recursive: true })
try {
writeFileSync(manifestPath, "{}", { flag: "wx" }) // wx = exclusive create, fails if exists
} catch (err) {
if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err
}
}
}Using the 4. Side-effect placement inside
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
package/plugins/webpack.ts
Outdated
| const fs = require("fs") | ||
| const path = require("path") |
There was a problem hiding this comment.
These two lines use CommonJS require(), but every other file in the project that imports from "fs" uses ES module syntax (e.g. import { existsSync, mkdirSync, writeFileSync } from "fs"). The file already mixes both styles (the first two lines are ES import statements), but adding more require() calls deepens that inconsistency.
| const fs = require("fs") | |
| const path = require("path") | |
| import { existsSync, mkdirSync, writeFileSync } from "fs" | |
| import { dirname } from "path" |
Then update ensureManifestExists to use existsSync, mkdirSync, writeFileSync, and dirname directly.
package/plugins/webpack.ts
Outdated
| const ensureManifestExists = (manifestPath: string): void => { | ||
| if (!fs.existsSync(manifestPath)) { | ||
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) | ||
| fs.writeFileSync(manifestPath, "{}") | ||
| } | ||
| } |
There was a problem hiding this comment.
The existsSync → writeFileSync sequence has a small TOCTOU window: two webpack processes starting simultaneously could both pass the existsSync check and one could overwrite a real manifest written by the other. Using the "wx" (exclusive-create) flag makes the write atomic on POSIX and turns the race into a silent no-op:
| const ensureManifestExists = (manifestPath: string): void => { | |
| if (!fs.existsSync(manifestPath)) { | |
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) | |
| fs.writeFileSync(manifestPath, "{}") | |
| } | |
| } | |
| const ensureManifestExists = (manifestPath: string): void => { | |
| if (!fs.existsSync(manifestPath)) { | |
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) | |
| try { | |
| fs.writeFileSync(manifestPath, "{}", { flag: "wx" }) | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err | |
| } | |
| } | |
| } |
package/plugins/webpack.ts
Outdated
| const getPlugins = (): unknown[] => { | ||
| // TODO: Remove WebpackAssetsManifestConstructor workaround when dropping 'webpack-assets-manifest < 6.0.0' (Node >=20.10.0) support | ||
| const WebpackAssetsManifestConstructor = | ||
| "WebpackAssetsManifest" in WebpackAssetsManifest | ||
| ? WebpackAssetsManifest.WebpackAssetsManifest | ||
| : WebpackAssetsManifest | ||
|
|
||
| // webpack-assets-manifest v6 crashes with ENOENT when merge: true | ||
| // and the manifest file doesn't exist yet (clean builds, Docker, CI). | ||
| ensureManifestExists(config.manifestPath) |
There was a problem hiding this comment.
This call sits in the middle of getPlugins(), sandwiched between the constructor workaround and the plugins array. As a file-system side effect it's easy to miss here. Consider moving it to the very top of getPlugins() so the intent is immediately visible:
| const getPlugins = (): unknown[] => { | |
| // TODO: Remove WebpackAssetsManifestConstructor workaround when dropping 'webpack-assets-manifest < 6.0.0' (Node >=20.10.0) support | |
| const WebpackAssetsManifestConstructor = | |
| "WebpackAssetsManifest" in WebpackAssetsManifest | |
| ? WebpackAssetsManifest.WebpackAssetsManifest | |
| : WebpackAssetsManifest | |
| // webpack-assets-manifest v6 crashes with ENOENT when merge: true | |
| // and the manifest file doesn't exist yet (clean builds, Docker, CI). | |
| ensureManifestExists(config.manifestPath) | |
| const getPlugins = (): unknown[] => { | |
| // webpack-assets-manifest v6 crashes with ENOENT when merge: true | |
| // and the manifest file doesn't exist yet (clean builds, Docker, CI). | |
| ensureManifestExists(config.manifestPath) | |
| // TODO: Remove WebpackAssetsManifestConstructor workaround when dropping 'webpack-assets-manifest < 6.0.0' (Node >=20.10.0) support | |
| const WebpackAssetsManifestConstructor = | |
| "WebpackAssetsManifest" in WebpackAssetsManifest | |
| ? WebpackAssetsManifest.WebpackAssetsManifest | |
| : WebpackAssetsManifest | |
Greptile SummaryThis PR fixes an ENOENT crash that occurs during clean builds when The fix adds an Key changes:
The fix is minimal, webpack-specific (rspack uses a different plugin that doesn't have this issue), and preserves existing manifest content on subsequent builds since the file check prevents overwriting. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 4636955 |
package/plugins/webpack.ts
Outdated
| const fs = require("fs") | ||
| const path = require("path") |
There was a problem hiding this comment.
These two lines use CommonJS require() while the file's existing imports at the top use ES module syntax (import type …, import { … }). All other files in the project that use fs (e.g. package/configExporter/fileWriter.ts) follow the ES module convention. Please align with the rest of the file:
| const fs = require("fs") | |
| const path = require("path") | |
| import { existsSync, mkdirSync, writeFileSync } from "fs" | |
| import { dirname } from "path" |
package/plugins/webpack.ts
Outdated
| const ensureManifestExists = (manifestPath: string): void => { | ||
| if (!fs.existsSync(manifestPath)) { | ||
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) | ||
| fs.writeFileSync(manifestPath, "{}") | ||
| } | ||
| } |
There was a problem hiding this comment.
Two issues here:
1. TOCTOU race condition – the existsSync → writeFileSync sequence has a check-then-act window. Two simultaneous webpack processes (e.g. parallel CI shards sharing the same public directory) could both pass existsSync, and whichever writes second silently overwrites the other's in-progress manifest. Using the "wx" (exclusive-create) flag makes the write atomic on POSIX:
| const ensureManifestExists = (manifestPath: string): void => { | |
| if (!fs.existsSync(manifestPath)) { | |
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) | |
| fs.writeFileSync(manifestPath, "{}") | |
| } | |
| } | |
| const ensureManifestExists = (manifestPath: string): void => { | |
| if (!fs.existsSync(manifestPath)) { | |
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) | |
| try { | |
| fs.writeFileSync(manifestPath, "{}", { flag: "wx" }) | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err | |
| } | |
| } | |
| } |
2. Missing tests – there is no test covering the new function. A test that verifies (a) the file is created with {} when absent, (b) an existing file is left untouched, and (c) missing parent directories are created would protect against regressions and document intent. test/package/plugins/ would be the natural home.
package/plugins/webpack.ts
Outdated
|
|
||
| // webpack-assets-manifest v6 crashes with ENOENT when merge: true | ||
| // and the manifest file doesn't exist yet (clean builds, Docker, CI). | ||
| ensureManifestExists(config.manifestPath) |
There was a problem hiding this comment.
Minor placement nit: this side-effecting call sits in the middle of getPlugins(), between the constructor-resolution block and the plugin array. Moving it to the very top of the function (before any conditional logic) makes the side effect easier to spot when reading through the code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/plugins/webpack.ts (1)
14-18: Use atomic file creation to avoid TOCTOU edge cases.Line 15-17 can still race across concurrent processes (
existsSync+ write). Prefer atomic create (flag: "wx") and ignoreEEXISTso an existing manifest is never truncated.Suggested patch
const ensureManifestExists = (manifestPath: string): void => { - if (!fs.existsSync(manifestPath)) { - fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) - fs.writeFileSync(manifestPath, "{}") - } + fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) + try { + fs.writeFileSync(manifestPath, "{}", { flag: "wx" }) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/plugins/webpack.ts` around lines 14 - 18, The ensureManifestExists function currently uses existsSync + writeFileSync which can race; replace the non-atomic path with an atomic create: ensure the directory exists (keep fs.mkdirSync(path.dirname(manifestPath), { recursive: true })) then attempt to create the file atomically using a write/open with flag "wx" and handle the EEXIST error by ignoring it (so an existing manifest is not truncated) while propagating other errors; reference ensureManifestExists and the manifestPath handling when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package/plugins/webpack.ts`:
- Around line 14-18: The ensureManifestExists function currently uses existsSync
+ writeFileSync which can race; replace the non-atomic path with an atomic
create: ensure the directory exists (keep
fs.mkdirSync(path.dirname(manifestPath), { recursive: true })) then attempt to
create the file atomically using a write/open with flag "wx" and handle the
EEXIST error by ignoring it (so an existing manifest is not truncated) while
propagating other errors; reference ensureManifestExists and the manifestPath
handling when making this change.
ReviewThe fix correctly addresses the webpack-assets-manifest v6 ENOENT crash on clean builds. The approach of pre-seeding an empty manifest file is sound, and the use of the A few concerns worth addressing: Test quality: duplicated logicThe test file reimplements The straightforward fix is to export Brittle source-inspection testsTwo of the five tests read rspack path not covered
|
| if (err.code !== "EEXIST") throw err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a copy of the production logic rather than a call to the real function. If webpack.ts is changed but this copy isn't updated, all tests will pass while the live code is broken.
Consider exporting ensureManifestExists from webpack.ts (or extracting it to a small utility file) so this test file can import and exercise the real implementation directly.
| ) | ||
|
|
||
| expect(webpackSource).toContain('{ flag: "wx" }') | ||
| }) |
There was a problem hiding this comment.
This test reads the source file as a text string and checks for a specific literal. It will fail on cosmetic refactors (e.g. reordering options, adding a space) even when behaviour is unchanged, and it doesn't verify anything about runtime correctness.
A behavioral alternative: create a file, delete it, then call ensureManifestExists twice concurrently and assert both calls complete without error and the file contains {}.
| expect(webpackSource).toContain('import { dirname } from "path"') | ||
| expect(webpackSource).not.toContain('require("fs")') | ||
| expect(webpackSource).not.toContain('require("path")') | ||
| }) |
There was a problem hiding this comment.
Same concern as the previous source-inspection test — this asserts on import syntax rather than behavior. If the imports are reformatted or the function is moved to a utility module that uses require, this will fail despite the logic being identical. These two source-inspection tests can safely be dropped once the function is exported and tested directly.
| @@ -0,0 +1,51 @@ | |||
| const fs = require("fs") | |||
There was a problem hiding this comment.
The test file is placed under test/package/plugins/ but the source it tests lives at package/utils/ensureManifestExists.ts. To keep the test tree mirroring the source tree, this should be moved to test/package/utils/ensureManifestExists.test.js.
The existing neighbour test/package/plugins/envFiltering.test.js tests package/plugins/envFilter.ts, so that directory convention is intentional. This file just ended up in the wrong sub-folder.
| // exist yet. rspack.ts uses rspack-manifest-plugin without merge: true, | ||
| // so it is not affected. | ||
| const ensureManifestExists = (manifestPath: string): void => { | ||
| if (!existsSync(manifestPath)) { |
There was a problem hiding this comment.
The outer existsSync check is redundant and introduces a classic TOCTOU race window: between the existsSync call returning false and the subsequent writeFileSync, another concurrent build process could create the file.
The two inner operations are already safe without the guard:
mkdirSync(…, { recursive: true })is idempotent — it does not throw when the directory already exists.writeFileSync(…, { flag: "wx" })+ EEXIST-catch is idempotent — it skips writing if the file already exists.
Simplified version:
const ensureManifestExists = (manifestPath: string): void => {
mkdirSync(dirname(manifestPath), { recursive: true })
try {
writeFileSync(manifestPath, "{}", { flag: "wx" })
} catch (err) {
if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err
}
}This is strictly more correct (no TOCTOU) and slightly simpler. The performance difference of one extra syscall per build is immeasurable.
ReviewThe fix is well-motivated and the approach is sound. The root cause analysis (v6 of A few things to address before merging: Missing CHANGELOG entryThis is a user-visible bug fix (ENOENT crash on clean builds / after Structural / code notes (see inline comments)
Everything else looks good
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c69987f4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { moduleExists } = require("../utils/helpers") | ||
|
|
||
| const getPlugins = (): unknown[] => { | ||
| ensureManifestExists(config.manifestPath) |
There was a problem hiding this comment.
Calling ensureManifestExists(config.manifestPath) before compilation means a failed first compile still leaves a brand-new manifest file on disk, which makes MtimeStrategy#fresh? treat assets as up to date (manifest_mtime > latest_modified_timestamp) until a watched file changes. In the default development mtime flow, this can stop automatic recompilation after non-source-related failures (for example, transient tooling/environment errors), leaving an empty {} manifest and repeated missing-entry errors instead of retrying compilation on subsequent requests.
Useful? React with 👍 / 👎.
webpack-assets-manifest v6 crashes with ENOENT when merge: true and the manifest file doesn't exist yet (clean builds, Docker, CI). Ensure the manifest file and its parent directory exist before instantiating the plugin. Fixes #930 Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Use ES module imports for fs and path instead of require() - Fix TOCTOU race condition with wx exclusive-create flag - Move ensureManifestExists call to top of getPlugins() - Add tests for manifest creation, preservation, and directory handling Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Move ensureManifestExists from webpack.ts to package/utils/ensureManifestExists.ts so it can be exported and tested directly - Update test to import the real function instead of reimplementing it - Remove source-inspection tests (wx flag check, ES import check) that break on cosmetic refactors without testing runtime behavior - Add comment noting rspack is unaffected (no merge: true) Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Add CHANGELOG entry for ENOENT crash fix with webpack-assets-manifest v6
- Move test file from test/package/plugins/ to test/package/utils/ to
mirror source tree structure
- Remove redundant existsSync guard in ensureManifestExists to eliminate
TOCTOU race window; mkdirSync and writeFileSync({flag:"wx"}) are
already idempotent
Co-Authored-By: Claude Opus 4.6 <[email protected]>
justin808
force-pushed
the
jg/930-fix-sass-resolve
branch
from
9c69987 to
9fab462
Compare
ReviewThis is a clean, minimal fix for a real bug. The implementation is correct and well-tested. What the fix doesSeeds an empty Strengths
Minor observations
VerdictLGTM. The fix is correct, minimal, and well-tested. The |
1 participant
Summary
WebpackAssetsManifestENOENT crash whenmerge: trueand no manifest file exists (clean builds, Docker, CI, after clobber){}manifest file (and creates parent directories) before instantiating the pluginwebpack-assets-manifestv6 which lacks acatchblock inmaybeMerge()Details
Since v9.3.0, shakapacker configures
WebpackAssetsManifestwithmerge: true. On a clean build wherepublic/packs/manifest.jsondoesn't exist yet,webpack-assets-manifestv6'smaybeMerge()throws an unhandled ENOENT because it hastry/finallywith nocatch. v5 has an emptycatchthat swallows the error, but v6 removed it.The fix ensures the manifest file exists before the plugin is instantiated, so the merge read succeeds with
{}on clean builds.Fixes #930
Test plan
🤖 Generated with Claude Code
Note
Low Risk
Low risk change limited to build-time behavior: it pre-creates the webpack manifest file/directories to avoid an ENOENT crash when
webpack-assets-manifestruns withmerge: true.Overview
Prevents clean builds from crashing when
webpack-assets-manifestis configured withmerge: truebut the manifest file doesn’t exist yet.webpack.tsnow ensuresconfig.manifestPath(and its parent directory) exists by seeding an empty{}file before instantiatingWebpackAssetsManifest.Written by Cursor Bugbot for commit 27099f5. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests