Repository navigation
Conversation
FileStore.set rewrote each entry in place with fs.promises.writeFile, which truncates the file and then writes it in chunks. A concurrent reader of the same key could see a file whose head was still zeros, which get() returns as a binary (Buffer) entry, and the build then fails with "dependencies is not iterable". Write to a unique temporary file in the same directory and rename it into place, removing the temporary file if the write fails. The on-disk format is unchanged.
meta-cla
Bot
added
the
CLA Signed
There was a problem hiding this comment.
馃煛 Changes recommended
The atomic write and tests are correct, but the cleanup rm in the catch block can mask the original write/rename error if it rejects, which should be addressed before approval.
1 open finding
What changed in this PR
This PR makes FileStore.set atomic to fix a concurrency bug where overlapping writes of the same cache key (common with expo export bundling DOM components in parallel) could leave a truncated/zero-prefixed file. A concurrent reader would then interpret the leading 0x00 as a binary Buffer entry (causing TypeError: dependencies is not iterable) or read truncated JSON. Instead of calling writeFile in place (which truncates first), the code now writes to a unique temp file (<entry>.<pid>.<random>.tmp) and renames it into place, so readers always see either the previous or the new complete entry. The on-disk format is unchanged, and the existing ENOENT directory-creation retry still applies.
Changes:
- Write cache entries to a unique temp file and atomically
renameinto place; remove the temp file and rethrow on failure. - Import
node:cryptoto generate the temp-file suffix. - Add three tests covering atomic visibility to readers, no leftover temp files after concurrent writes, and temp-file cleanup on write failure.
| File | Description |
|---|---|
| packages/鈥媘etro-cache/鈥媠rc/鈥媠tores/鈥婩ileStore.js | Replaces in-place writeFile with temp-file write + rename, with cleanup on error |
| packages/鈥媘etro-cache/鈥媠rc/鈥媠tores/鈥媉_tests__/鈥婩ileStore-test.js | Adds regression tests for atomic reads, temp-file cleanup, and failed-write cleanup |
馃 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| await fs.promises.writeFile(tempPath, content); | ||
| await fs.promises.rename(tempPath, filePath); | ||
| } catch (err) { | ||
| await fs.promises.rm(tempPath, {force: true}); |
There was a problem hiding this comment.
@copilot Fix the code for all comments in this review comment.
When a review comment includes a suggested change, apply the suggestion exactly.
Do not make changes beyond what is described in the linked review comment.
facebook-github-tools
Bot
added
the
Shared with Meta
Summary
Refs #1961.
FileStore.setrewrites each cache entry in place withfs.promises.writeFile, which truncates the file on open and then writes it in 512 KiB chunks. When two writers of the same key overlap in one process, one can truncate the file while the other is between chunks, leaving a file that starts with zero bytes. A concurrentgetthen sees a leading0x00, returns the rest as a binaryBufferentry, andTransformerfails withTypeError: dependencies is not iterable. A reader can also see truncated JSON, which is a silent miss.This shows up in practice with
expo exportin a React Native app using Expo DOM components: it bundles every DOM component in parallel in one process, and they transform shared modules under the same keys. With metro-cache 0.84.5 the export failed in 8 of 16 runs, and the cached value was aBufferof 524,287 zero bytes.@expo/metro-configextends thisFileStore.This change writes each entry to a unique temporary file in the same directory (
<entry>.<pid>.<random>.tmp) and renames it into place, so a reader sees either the previous entry or the new one. If the write or rename fails, the temporary file is removed and the error is rethrown as before (the existingENOENTretry that creates the directory still applies). The on-disk format is unchanged, so existing caches stay readable.It does not address entries that are already corrupt on disk (for example after power loss on NTFS, as described in #1961); that still needs
getto treat an invalid entry as a miss.Changelog: [Fix]
FileStorewrites cache entries atomically, so concurrent readers never see a partially written entryTest plan
New tests in
packages/metro-cache/src/stores/__tests__/FileStore-test.js:never exposes a partially written entry to a reader: pauses a write after the file is truncated and its tail written, then reads the key. Without this change the read returns aBufferof zeros; with it, the read returns the previous entry.removes the temporary file when a write fails: without this change the failed write corrupts the existing entry; with it, the entry and directory are unchanged.leaves no temporary files behindafter concurrent writes of one key.Also a standalone stress test on real disk (ext4, Node 24): 8 concurrent
set+getloops on one key with a 2 MB value, 16 runs. Before: truncated JSON in 16/16 runs and a zero-prefixedBufferhit in 1/16. After: no bad reads and no leftover temporary files.