Repository navigation
fix(cli): skip identical writes and emit atomically - #133
Simen Bekkhus (SimenB) wants to merge 10 commits into
Conversation
Every emit path truncate-wrote in place and rewrote output even when the bytes were identical. A watch consumer that reads emitted files on change could see a partially written file, and a bulk mtime-only touch of the sources triggered a rebuild storm downstream for no content change. Compiled output, sourcemaps, dts files and copied files now skip the write when the destination already matches, and otherwise go through a temp file next to the destination followed by a rename.
🦋 Changeset detectedLatest commit: c1d2209 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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: 8424eb81ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Windows has no POSIX permission bits, so the mode always reads back as 0o666 and the assertion can never hold there.
8424eb8 to
210f42a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 210f42ace8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Handles unique temp names per write, keeps the destination's mode and symlinks, and removes the temp file when a write fails. Copies stay hand-rolled: they can be larger than the 2 GiB a buffer holds, so they compare size, mode and then chunked contents before going through copyFile. Content comparison is now byte-based, since decoding to UTF-8 makes malformed bytes compare equal to U+FFFD.
|
Claude again 😀 Good catches, all of them reproduce. Rather than hand-roll the fixes I switched the write path to
Pinned to Copies stay hand-rolled, since Content comparison is byte-based everywhere now. Decoding to UTF-8 made a malformed byte compare equal to There are tests for each of these. Happy to drop the dependency and hand-roll it instead if you'd rather not add one — say the word. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6378bf3a6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
|
|
||
| await Promise.all([ | ||
| writeFile(destFile, sourceCode, { mode }), | ||
| writeFileAtomicallyIfChanged(destFile, sourceCode, { mode }), |
There was a problem hiding this comment.
When --out-dir recompiles an existing output whose permissions were changed independently—for example, an executable output set to 0755 while its source is 0644—passing { mode } makes write-file-atomic create the replacement with the source mode and resets the output permissions. The previous fs.writeFile call used mode only when creating a file and preserved an existing inode's permissions. Fresh evidence beyond the earlier permissions comment is that these production calls now explicitly supply the source mode, while the new preservation test only exercises the helper without options.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The behaviour is as you describe — destination at 0755, source at 0644, output comes out 0644 — but it is intended.
outputResult stats the source and passes its mode on purpose: dir mode mirrors source permissions into the output directory. Previously that mode applied only when the file was created, so the result depended on whether the output already existed. It is now consistent either way.
Dropping the option to keep the destination's bits would break the intent in the other direction: a 0755 source would no longer produce a 0755 output on first create.
This also contradicts the earlier comment asking to preserve the destination's mode. Both cannot hold, and the current split is deliberate: dir mode mirrors the source, --out-file keeps the destination's mode.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebf88f18ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| if (await hasContent(filename, content)) { | ||
| return; | ||
| } | ||
|
|
||
| await writeFileAtomic(filename, content, { ...options, fsync: false }); |
There was a problem hiding this comment.
When two unsequenced watch callbacks produce the same new bytes while the destination still contains old bytes, both hasContent calls can return false before either write begins, so both calls replace the destination with identical content and emit redundant filesystem changes. The atomic writer cannot protect this check because it occurs before entering the write operation, defeating the advertised unchanged-write suppression and potentially triggering duplicate downstream rebuilds; serialize the entire compare-and-write sequence per destination.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, but leaving it. Sampling the destination inode shows 3 distinct inodes for two concurrent calls with identical new bytes, against 2 when the same two calls run in sequence — so the compare is per call, not per destination, exactly as you describe.
Declining because the trade is poor here. The content still ends up correct, nothing errors and no temp file leaks; the cost is one extra rename, so a downstream watcher fires twice instead of once. It is also not a regression: before this PR both compiles wrote unconditionally, so that case already produced two writes.
Closing the window means a per-destination promise chain, which puts shared mutable state and entry lifetime management into a util module to save one write in an already rare case. If the duplicate work matters, the better fix is upstream: serialise per-file compiles in the watch handler in dir.ts. That avoids the duplicate compile too, which costs far more than the rename. Out of scope for this PR.
…lure Opening the source and the destination together left the source handle open when the destination open rejected, which Node 26 turns into an uncaught error once the handle is collected. Open them one at a time. Copies also renamed over a symlinked destination rather than following it, unlike copyFile before it and unlike the generated output path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1421d5bfa9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| if ( | ||
| srcStats.size !== destStats.size || | ||
| srcStats.mode !== destStats.mode |
There was a problem hiding this comment.
On privileged POSIX builds, if the source's uid or gid changes while its bytes and mode remain identical, this check returns true and leaves the destination with stale ownership. The previous unconditional fs.copyFile propagated source ownership in this situation, and the new temp-copy path still does so whenever it runs, so include uid and gid in the metadata comparison before skipping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise does not hold: fs.copyFile never propagated source ownership.
Copying root-owned /etc/hosts as uid 501 produces a destination owned by 501, not 0 — the new file belongs to the effective user. And when the destination already exists, copyFile truncates it in place, so the existing inode keeps whatever owner it had.
So there is no ownership propagation for the skip to lose, and comparing uid and gid would only cause copies that change nothing. Leaving this as is.
| // Copies can be arbitrarily large binaries, so they go through copyFile | ||
| // rather than a buffer. The temporary file must live next to the | ||
| // destination: rename is only atomic within a single filesystem. | ||
| const tmpDest = `${target}.${process.pid}.${++copies}.tmp`; |
There was a problem hiding this comment.
When a copied asset's destination basename is at or near the filesystem's per-component limit (255 bytes on common Linux filesystems), appending the PID, counter, and .tmp makes this otherwise valid path exceed the limit, so copyFile fails with ENAMETOOLONG; the previous direct copy to dest succeeded. Create the temporary file under a short independent basename in dirname(target) rather than extending the destination basename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed for copies: a 253-character basename made the direct copy succeed and the helper fail with ENAMETOOLONG. Temporary copies now use a short name in the destination's directory rather than extending the destination's own name.
Worth stating plainly: the generated-output path still has this limit. write-file-atomic builds its temporary name by appending to the filename, so a 253-character basename fails there too. Closing that means patching upstream or dropping its temp naming, neither of which belongs in this PR. Filed under known limitation rather than fixed.
A dangling destination symlink now resolves through its link text, so the copy creates the target instead of replacing the link. Temporary copies use a short name in the destination's directory. The previous name extended the destination's own, which fails when that name already fills a path component. An --out-file target that is not a regular file, such as a FIFO or /dev/null, is written directly: reading it can block, and renaming over it would replace the special file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 414d2d185b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
|
|
||
| function isRegularFile(filename: string) { | ||
| try { | ||
| return statSync(filename).isFile(); |
There was a problem hiding this comment.
When --out-file names one link of a hard-linked file, this check classifies it as an ordinary regular file and the subsequent atomic rename gives that pathname a new inode. Any other hard links therefore retain the stale output, whereas the previous writeFileSync updated their shared inode; detect stat.nlink > 1 and retain a direct write for such destinations if hard-linked consumers must continue observing updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and it applies to the copy path as well, not only --out-file. Against a destination with two hard links: the old writeFileSync and copyFile both left the other link reading the new content with nlink still 2, while the rename-based write leaves it on the old content with nlink 1.
Leaving it, because this is what atomic replacement is rather than an oversight in the implementation. A new inode cannot be swapped into place and a shared inode updated in place at the same time — every rename-based writer breaks hard links, write-file-atomic included, which is the behaviour npm itself ships.
Falling back to a direct write when nlink > 1 would trade that away silently: the destinations someone cared enough about to hard-link would be exactly the ones that lose the torn-read protection this PR exists to provide. If hard-linked consumers do need in-place updates, that belongs behind a documented opt-out rather than an implicit heuristic, and I would rather see a real report of that setup before adding one.
write-file-atomic follows a symlinked destination, but falls back to the link itself when the target does not exist yet, which replaces the link. Resolve the link text first, as the copy path already does, so a dangling link keeps pointing at a file the write creates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 789a6155a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| `.swc-${process.pid}-${++copies}.tmp` | ||
| ); | ||
| try { | ||
| await promises.copyFile(src, tmpDest); |
There was a problem hiding this comment.
When the output directory is writable by another user or process, the predictable PID/counter basename lets it pre-create tmpDest as a symlink before this call. Because copyFile is invoked without an exclusive flag, it follows that link and overwrites its target with the source bytes; the subsequent rename then moves the symlink to dest. Open the temporary destination exclusively (for example with COPYFILE_EXCL) and retry with an unpredictable unique basename.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. The temporary name exists to avoid clashes, not to defend a boundary, and it already does that: handleCopy runs on the main thread, so the counter comes from a single module instance, and pids are unique per machine. write-file-atomic folds in threadId for the same reason on the generated-output side. There is no collision for COPYFILE_EXCL to prevent.
What is left is the symlink case, and that needs an output directory writable by someone other than the build. Anyone who can create files there can already overwrite the emitted output directly; the only gain is writing outside the directory through a planted link, which assumes a shared-writable -d target that is not how the CLI is used.
COPYFILE_EXCL without a retry loop also costs something real: a temporary file left behind by a killed run whose pid was later reused is silently replaced today, and would become a hard EEXIST failure. That trades a case needing a hostile local user for one needing only a crash.
Worth noting for completeness: the generated-output path has the same shape upstream and this PR cannot change it. write-file-atomic derives its temporary name from sha1(__filename, pid, threadId, counter) and opens it with 'w', which follows symlinks. Hardening only the copy path would not close the class it describes.
An output directory that allows no new files cannot host the temporary file, even when the output itself is writable, so an EACCES from the atomic write falls back to writing the destination directly. Every other error still propagates.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9d0eb65b6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Resolving a dangling destination stopped after one link, so a chain such as out.js -> current.js -> missing.js replaced the intermediate link rather than creating the target. Walk the chain instead, and report ELOOP once it stops ending anywhere, as a plain write did. The asynchronous writer also needs the direct write its synchronous counterpart already does for a FIFO or device, which --out-dir can reach just as easily as --out-file.
|
I'll stop following Codex down the symlink rabbit hole for now unless a maintainer gives signal they'd like me to. What I wanted to solve was FS churn on unchanged files and writing partial files so other watchers are not needlessly triggered. While the symlink concerns are real, the diff has doubled to handle it, and it seems like an edge case rather than a real problem. But if a maintainer thinks I should continue chasing them, say the word 👍 |
This branch has not been deployed
1 participant
Caveat: Authored by Claude Code. I have verified this works in my real project without churn.
Skip identical writes and emit atomically
Every emit path in
@swc/clitruncate-writes in place (writeFile,writeFileSync,copyFile), and a recompile rewrites the output even when the bytes are identical. Both hurt watch-mode consumers:TypeError, and every dependent module stays broken until the next rebuild.src/swc/util.tsgains three helpers used by every emit site:writeFileAtomicallyIfChanged(async, honours{ mode })writeFileAtomicallyIfChangedSynccopyFileAtomicallyIfChangedEach one reads the destination first and returns without writing when the content already matches — string compare for compiled output, sourcemaps and dts, buffer compare for copies, which may be binary. Any read error,
ENOENTincluded, falls through to writing. The write goes to<dest>.<pid>.tmpnext to the destination, thenrenames over it, so a reader sees the old file or the new file, never a partial one. The temp file must share the destination's directory:renameis only atomic within one filesystem.Call sites:
src/swc/compile.tsoutputResult— code, sourcemap and dts. The hot path for dir-mode and watch.src/swc/util.tsoutputFile— code and map, for--out-file.src/swc/dir.tshandleCopy—--copy-files.The same logic has run in production downstream for a while as a patch against the compiled
lib/of 0.8.1: a full-tree touch storm produces zero output writes, real edits land, no temp files leak.Verification
tscclean and 44 tests pass inpackages/cli, including a newsrc/swc/__tests__/atomicWrite.test.tsthat runs against a real temp directory: identical content leaves both mtime and inode untouched, changed content lands,{ mode }applies, binary copies skip and update correctly, and no*.tmpfile survives any case.Manual check with
--copy-fileson a scratch tree: aftertouchon every source, all outputs kept their original mtime and inode. Editing one.tsand one.txtupdated exactly those two outputs and left the rest alone.--out-filemode behaves the same.