FileStore: write cache entries atomically by frankleng · Pull Request #2040 · react/metro

frankleng · 2026-10-08T01:45:09Z

Summary

Refs #1961.

FileStore.set rewrites each cache entry in place with fs.promises.writeFile, which truncates the file on open and then writes it in 512 KiB chunks. A concurrent get of the same key can therefore see an empty, truncated or zero-prefixed file. Since #2122 these are cache misses rather than a TypeError: dependencies is not iterable crash, but each miss still throws away a valid entry and causes a re-transform.

This shows up in practice with expo export in 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. Before #2122, this failed the export in 8 of 16 runs with metro-cache 0.84.5. @expo/metro-config extends this FileStore.

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. A torn binary entry has no JSON parse to catch it and is currently returned as a truncated hit, which this also prevents. If the write or rename fails, the temporary file is removed and the error is rethrown as before (the existing ENOENT retry that creates the directory still applies). The on-disk format is unchanged, so existing caches stay readable.

On Windows, renaming over a file fails while another writer or reader has it open. EPERM, EACCES and EBUSY from the rename count as success if the target exists, because keys are content-addressed and the existing entry is equivalent.

The extra rename makes a write of a new key about 1.4x-1.8x slower across macOS, Linux and Windows (tens of microseconds, or about 90µs on Windows). Cold builds are up to about 2% slower for a single build, and up to about 3% with four builds racing on one store. Full results are in the review.

Changelog: [Fix] FileStore writes cache entries atomically, so concurrent reads no longer miss valid entries or see partial writes

Test 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 is a miss. 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 behind after concurrent writes of one key.
Rename conflicts keep the existing entry, and are rethrown if there is no existing entry or if the same code comes from the write.

yarn jest packages/metro-cache    # 7 suites, 65 tests passed
yarn flow check                   # no errors

Also a stress test on real disks, Node 24: 8 concurrent set + get loops on one 2 MB key, 1,200 of each per side, against main including #2122:

	Failed sets (`main` -> this PR)	Cache misses from torn reads (`main` -> this PR)
macOS (APFS)	0 -> 0	540 -> 0
Linux (ext4)	0 -> 0	386 -> 0
Windows (NTFS)	0 -> 0	3 -> 0

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.

Copilot

🟡 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

Cleanup rm can mask the original write or rename error · New

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 rename into place; remove the temp file and rethrow on failure.
Import node:crypto to 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/metro-cache/src/stores/FileStore.js	Replaces in-place `writeFile` with temp-file write + `rename`, with cleanup on error
packages/metro-cache/src/stores/__tests__/FileStore-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.

robhogan · 2026-10-08T07:16:03Z

+      await fs.promises.writeFile(tempPath, content);
+      await fs.promises.rename(tempPath, filePath);
+    } catch (err) {
+      await fs.promises.rm(tempPath, {force: true});


@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.

robhogan

Thanks for this @frankleng, and sorry it's taken a while to get back.

Predictably there's some perf cost to this which I needed to measure, but it's negligible except under contrived conditions (details below).

The only real issue I found with this was on Windows, and I've pushed a small fix for that. Renaming over an existing file fails with EPERM while another writer or reader has it open, which is the race this PR is fixing otherwise. We can treat EPERM/EACCES/EBUSY from the rename as success if the target exists - keys are content-addressed, so losing the race is safe.

Benchmark

AI-driven benchmark of main vs this PR rebased on main, in interleaved pairs on Node 24, with medians and 95% CIs. The extra rename adds a roughly fixed cost per write, and the builds barely notice because transforms don't wait for cache writes.

Per write (15 pairs)	macOS	Linux	Windows
Typical: new keys, real entry size mix, 8 concurrent	37 -> 68µs (1.82x, CI 1.79-1.87)	55 -> 75µs (1.37x, CI 1.35-1.41)	155 -> 244µs (1.63x, CI 1.32-2.00)
Worst: overwriting existing keys, 8 concurrent	28 -> 77µs (2.74x, CI 2.70-2.86)	65 -> 84µs (1.28x, CI 1.22-1.32)	109 -> 269µs (2.43x, CI 2.39-2.60)
Worst: 1 KB entries, sequential	77 -> 139µs (1.82x, CI 1.81-1.84)	98 -> 154µs (1.45x, CI 1.41-1.51)	388 -> 661µs (1.71x, CI 1.68-1.82)

Cold build	macOS	Linux	Windows
Typical: single build, default transformer	1264 -> 1299ms (+2.0%, CI 1.7-3.1)	10115 -> 10125ms (+0.2%, CI -0.1-1.1)	5359 -> 5452ms (+1.0%, CI 0.6-2.8)
Worst: 4 parallel builds sharing one store and racing on keys	4597 -> 4723ms (+3.3%, CI -1.2-5.4)	39891 -> 40329ms (+1.1%, CI 0.7-1.8)	19865 -> 20223ms (+1.3%, CI 0.0-4.2)

Overwrites should be rare, because a cache hit skips the write. The build fixture is 1,500 real CommonJS files (4 MB) with very little transform work, so a real app should see a smaller percentage. Single builds had 30 pairs (20 on Windows) and parallel builds had 5.

Stress test

8 concurrent set + get loops on one 2 MB key, 1,200 of each per side, with the Windows fix:

	Failed sets (`main` -> this PR)	Cache misses from torn reads (`main` -> this PR)
macOS	0 -> 0	540 -> 0
Linux	0 -> 0	386 -> 0
Windows	0 -> 0 (1,131 before the fix)	3 -> 0

The parallel builds on Windows also went from 47 failed writes in about 30,000 before the fix to none after it.

# Conflicts: # packages/metro-cache/src/stores/FileStore.js # packages/metro-cache/src/stores/__tests__/FileStore-test.js

frankleng · 2026-11-05T23:19:47Z

Thanks for this @frankleng, and sorry it's taken a while to get back.

Predictably there's some perf cost to this which I needed to measure, but it's negligible except under contrived conditions (details below).

The only real issue I found with this was on Windows, and I've pushed a small fix for that. Renaming over an existing file fails with EPERM while another writer or reader has it open, which is the race this PR is fixing otherwise. We can treat EPERM/EACCES/EBUSY from the rename as success if the target exists - keys are content-addressed, so losing the race is safe.

Benchmark

AI-driven benchmark of main vs this PR rebased on main, in interleaved pairs on Node 24, with medians and 95% CIs. The extra rename adds a roughly fixed cost per write, and the builds barely notice because transforms don't wait for cache writes.

Per write (15 pairs) macOS Linux Windows

Typical: new keys, real entry size mix, 8 concurrent 37 -> 68µs (1.82x, CI 1.79-1.87) 55 -> 75µs (1.37x, CI 1.35-1.41) 155 -> 244µs (1.63x, CI 1.32-2.00)

Worst: overwriting existing keys, 8 concurrent 28 -> 77µs (2.74x, CI 2.70-2.86) 65 -> 84µs (1.28x, CI 1.22-1.32) 109 -> 269µs (2.43x, CI 2.39-2.60)

Worst: 1 KB entries, sequential 77 -> 139µs (1.82x, CI 1.81-1.84) 98 -> 154µs (1.45x, CI 1.41-1.51) 388 -> 661µs (1.71x, CI 1.68-1.82)

Cold build macOS Linux Windows

Typical: single build, default transformer 1264 -> 1299ms (+2.0%, CI 1.7-3.1) 10115 -> 10125ms (+0.2%, CI -0.1-1.1) 5359 -> 5452ms (+1.0%, CI 0.6-2.8)

Worst: 4 parallel builds sharing one store and racing on keys 4597 -> 4723ms (+3.3%, CI -1.2-5.4) 39891 -> 40329ms (+1.1%, CI 0.7-1.8) 19865 -> 20223ms (+1.3%, CI 0.0-4.2)

Overwrites should be rare, because a cache hit skips the write. The build fixture is 1,500 real CommonJS files (4 MB) with very little transform work, so a real app should see a smaller percentage. Single builds had 30 pairs (20 on Windows) and parallel builds had 5.

Stress test

8 concurrent set + get loops on one 2 MB key, 1,200 of each per side, with the Windows fix:

Failed sets (main -> this PR) Cache misses from torn reads (main -> this PR)

macOS 0 -> 0 540 -> 0

Linux 0 -> 0 386 -> 0

Windows 0 -> 0 (1,131 before the fix) 3 -> 0

The parallel builds on Windows also went from 47 failed writes in about 30,000 before the fix to none after it.

Great. Thanks for looking into this

@robhogan

Summary: Publish Metro 0.88.0 from `main`. This is a draft until we choose the cut. These breaking PRs are still open, and can land first or wait for the next minor: - #2186 (`globalPrefix` for all module system globals) - #2158 (`/symbolicate` stops reading `req.rawBody`) - #2157 (remove `functionMap` from the Babel transformer result) - #1990 (keep the file extension in async bundle paths, approved and held for 0.88) - #1985 and #2156 (drafts, decoded source maps) Approved non-breaking PRs that could also make it: #2185, #2040, #2034 and #2150 (which turns a missing watch folder into an immediate startup error, rather than a 240s hang). I'll rebase this and extend the notes as they land. Changelog: [Internal] Test plan: ``` node scripts/updateVersion.js 0.88.0 sl diff --stat ``` Only the 17 `packages/*/package.json` manifests change, and every `0.87.3` reference is replaced. Notable breakage in third parties, for the notes or a heads-up before React Native adopts this: - `@rnx-kit/metro-config` 2.2.6 adds `path.dirname(resolver.emptyModulePath)` as a watch folder when there are none, which is now `metro:runtime/modules`, so the dev server never serves a bundle for a single-package app. - `@tamagui/metro-plugin` 3.x refuses `metro-resolver` outside 0.84-0.87, and then imports the removed `metro-resolver/private/createDefaultContext`. - `@rnx-kit/metro-resolver-symlinks` calls the removed `context.redirectModulePath`. End to end (AI-driven), with the packed tarballs against 0.87.3 from npm, in a React Native 0.89.0-rc.0 template app with a few popular dependencies: dev and release bundles, source maps and assets for iOS and Android are byte-identical, except where #2174 changes resolution. `rxjs/operators` now resolves to the same ESM build as `rxjs`, dropping 190 duplicate modules, and `require('nanoid/non-secure')` gets nanoid's CJS build. Release builds reach the first screen on a Galaxy A16 and the iOS simulator (built before #2184 landed, which leaves the prelude and module system byte-identical). The dev server passes the same checks on both, with and without Watchman (edits, added and deleted files, HMR, `/symbolicate`, assets and lazy `import()`), and Fast Refresh works on the simulator. Draft release notes (from the 14 commits in v0.87.3...main - internal-only changes omitted): ``` - **[Breaking]**: Resolver: Read `"exports"` from the package root, and apply `"browser"` redirections to the file it resolves to, by default. Set `resolver.unstable_resolveExportsFromPackageRoot` and `resolver.unstable_applyBrowserSpecAfterExports` to `false` for the previous behavior (#2174 by @robhogan) - **[Breaking]**: Default `transformer.asyncRequireModulePath` and `resolver.emptyModulePath` to `metro:runtime/...`. A custom `resolver.resolveRequest` must delegate `metro:` specifiers it doesn't handle to `context.resolveRequest`, and code that reads either option as a file path must resolve it instead (#2180 by @robhogan) - **[Breaking]**: Resolve the module system via `metro:runtime/polyfills/require`. A custom `resolver.resolveRequest` must delegate `metro:` specifiers it doesn't handle to `context.resolveRequest` (#2184 by @robhogan) - **[Breaking]**: Remove the deprecated `redirectModulePath` from `ResolutionContext`. A custom resolver should drop any call to it and delegate to `context.resolveRequest` or `resolve`, which already apply `"browser"` redirections and return `{type: 'empty'}` for `false` (#2181 by @robhogan) - **[Breaking]**: Remove the deprecated `doesFileExist` from `ResolutionContext`, and the `DoesFileExist` type from `metro-resolver` (use `fileSystemLookup`) (#2181 by @robhogan) - **[Breaking]**: `ResolutionContext.isESMImport` is required (#2182 by @robhogan) - **[Breaking]**: Source and JSON files containing the reserved string `_$$_METRO_`, including in comments and string literals, now fail to transform (#2155 by @robhogan) - **[Breaking]**: Remove the deprecated, always-`true` `hot` option from `getTransformOptions` (#2159 by @robhogan) - **[Feature]**: Custom serializers receive the reserved string as `options.reservedString` (#2155 by @robhogan) - **[Fix]**: Fall back to file-based resolution, instead of throwing a `TypeError`, when a package's `"exports"` field is not a string, array or object (#2187 by @kwy404) - **[Fix]**: Fix relative symlinks on a different Windows drive than the project root (#2161 by @ramya-2196) - **[Performance]**: ~5-10% faster resolution (#2181 by @robhogan) > NOTE: Experimental features are not covered by semver and can change at any time. - **[Experimental]**: Remove `unstable_incrementalResolution` from `ResolutionContext`, and `metro-resolver/private/createDefaultContext` (#2181 by @robhogan) **Full Changelog**: v0.87.3...v0.88.0 ```

@robhogan

Summary: Publish Metro 0.88.0 from `main`. This is a draft until we choose the cut. These breaking PRs are still open, and can land first or wait for the next minor: - #2158 (`/symbolicate` stops reading `req.rawBody`) - #2157 (remove `functionMap` from the Babel transformer result) - #1990 (keep the file extension in async bundle paths, approved and held for 0.88) - #1985 and #2156 (drafts, decoded source maps) Approved non-breaking PRs that could also make it: #2185, #2040, #2034 and #2150 (which turns a missing watch folder into an immediate startup error, rather than a 240s hang). I'll rebase this and extend the notes as they land. Changelog: [Internal] Test plan: ``` node scripts/updateVersion.js 0.88.0 sl diff --stat ``` Only the 17 `packages/*/package.json` manifests change, and every `0.87.3` reference is replaced. Notable breakage in third parties, for the notes or a heads-up before React Native adopts this: - `@rnx-kit/metro-config` 2.2.6 adds `path.dirname(resolver.emptyModulePath)` as a watch folder when there are none, which is now `metro:runtime/modules`, so the dev server never serves a bundle for a single-package app. - `@tamagui/metro-plugin` 3.x refuses `metro-resolver` outside 0.84-0.87, and then imports the removed `metro-resolver/private/createDefaultContext`. - `@rnx-kit/metro-resolver-symlinks` calls the removed `context.redirectModulePath`. End to end (AI-driven), with the packed tarballs against 0.87.3 from npm, in a React Native 0.89.0-rc.0 template app with a few popular dependencies: dev and release bundles, source maps and assets for iOS and Android are byte-identical, except where #2174 changes resolution. `rxjs/operators` now resolves to the same ESM build as `rxjs`, dropping 190 duplicate modules, and `require('nanoid/non-secure')` gets nanoid's CJS build. With #2186, only the module system and `asyncRequire` differ besides those. Release builds reach the first screen on a Galaxy A16 and the iOS simulator. The dev server passes the same checks on both, with and without Watchman (edits, added and deleted files, HMR, `/symbolicate`, assets and lazy `import()`), and Fast Refresh works on the simulator. Draft release notes (from the 15 commits in v0.87.3...main - internal-only changes omitted): ``` - **[Breaking]**: Resolver: Read `"exports"` from the package root, and apply `"browser"` redirections to the file it resolves to, by default. Set `resolver.unstable_resolveExportsFromPackageRoot` and `resolver.unstable_applyBrowserSpecAfterExports` to `false` for the previous behavior (#2174 by @robhogan) - **[Breaking]**: Default `transformer.asyncRequireModulePath` and `resolver.emptyModulePath` to `metro:runtime/...`. A custom `resolver.resolveRequest` must delegate `metro:` specifiers it doesn't handle to `context.resolveRequest`, and code that reads either option as a file path must resolve it instead (#2180 by @robhogan) - **[Breaking]**: Resolve the module system via `metro:runtime/polyfills/require`. A custom `resolver.resolveRequest` must delegate `metro:` specifiers it doesn't handle to `context.resolveRequest` (#2184 by @robhogan) - **[Breaking]**: `transformer.globalPrefix` now also prefixes `__r`, `__c`, `__registerSegment` and `__accept`, and the default run statement. Code that calls these by name must apply the prefix (#2186 by @robhogan) - **[Breaking]**: Remove the deprecated `redirectModulePath` from `ResolutionContext`. A custom resolver should drop any call to it and delegate to `context.resolveRequest` or `resolve`, which already apply `"browser"` redirections and return `{type: 'empty'}` for `false` (#2181 by @robhogan) - **[Breaking]**: Remove the deprecated `doesFileExist` from `ResolutionContext`, and the `DoesFileExist` type from `metro-resolver` (use `fileSystemLookup`) (#2181 by @robhogan) - **[Breaking]**: `ResolutionContext.isESMImport` is required (#2182 by @robhogan) - **[Breaking]**: Source and JSON files containing the reserved string `_$$_METRO_`, including in comments and string literals, now fail to transform (#2155 by @robhogan) - **[Breaking]**: Remove the deprecated, always-`true` `hot` option from `getTransformOptions` (#2159 by @robhogan) - **[Feature]**: Custom serializers receive the reserved string as `options.reservedString` (#2155 by @robhogan) - **[Fix]**: Fall back to file-based resolution, instead of throwing a `TypeError`, when a package's `"exports"` field is not a string, array or object (#2187 by @kwy404) - **[Fix]**: Fix relative symlinks on a different Windows drive than the project root (#2161 by @ramya-2196) - **[Performance]**: ~5-10% faster resolution (#2181 by @robhogan) > NOTE: Experimental features are not covered by semver and can change at any time. - **[Experimental]**: Remove `unstable_incrementalResolution` from `ResolutionContext`, and `metro-resolver/private/createDefaultContext` (#2181 by @robhogan) **Full Changelog**: v0.87.3...v0.88.0 ```

Copilot AI balanced review requested due to automatic review settings October 8, 2026 01:45

frankleng mentioned this pull request Oct 8, 2026

dependencies is not iterable / Cannot read property 'reduce' of undefined: FileStore serves zero-prefixed cache files as hits #1961

Closed

meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 8, 2026

Copilot started reviewing on behalf of frankleng October 8, 2026 01:45

View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 8, 2026

maximilliangrand mentioned this pull request Oct 26, 2026

Treat zero-filled FileStore cache entries as misses #2122

Merged

Handle rename conflicts in FileStore on Windows

1985653

robhogan approved these changes Nov 4, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into filestore-atomic-write

7d60114

# Conflicts: # packages/metro-cache/src/stores/FileStore.js # packages/metro-cache/src/stores/__tests__/FileStore-test.js

robhogan mentioned this pull request Nov 6, 2026

Publish 0.88.0 #2192

Merged

Navigation Menu

Repository navigation

FileStore: write cache entries atomically - #2040

FileStore: write cache entries atomically#2040
frankleng wants to merge 3 commits into
react:mainfrom
frankleng:filestore-atomic-write

frankleng commented Oct 8, 2026 •

edited by robhogan

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

robhogan Oct 8, 2026

Uh oh!

robhogan left a comment •

edited

Loading

Uh oh!

frankleng commented Nov 5, 2026

Benchmark

Stress test

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

frankleng commented Oct 8, 2026 • edited by robhogan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

robhogan Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

robhogan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Benchmark

Stress test

Uh oh!

frankleng commented Nov 5, 2026

Benchmark

Stress test

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

frankleng commented Oct 8, 2026 •

edited by robhogan

Loading

robhogan left a comment •

edited

Loading