From b6e63f0ced42392f7bf09506cd40cc37f3769996 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Sun, 25 Oct 2026 16:07:07 +0000 Subject: [PATCH 1/2] Fix NativeWatcher missing changes made straight after startup `NativeWatcher.startWatching` resolves as soon as `fs.watch` returns, but on macOS that can be before any changes are reported. libuv only signals another thread to (re)create the FSEvents stream, and the stream reports only changes made after it starts: https://github.com/libuv/libuv/blob/2b4b918d3381100854250c89d5159d4206daafb7/src/unix/fsevents.c#L362-L372 So a change made straight after startup can be silently missed, and since #2054 started non-Watchman backends before the crawl, so can one made during the crawl. On a slow host the gap is long enough to hit regularly - it's the remaining flake in the Native watcher integration tests, where the suite's first event never arrives. 14 of 297 macOS test jobs since #1974 hit it, across all three Node versions. This has `Watcher` confirm each root's watch before relying on it. Once every backend has started, it writes a health check file into each root and waits for it to be reported, as `checkHealth()` already does for the project root. If a watch can't be confirmed within 10s, Metro warns and carries on. Roots added later with `addRoot` are confirmed the same way. A backend opts in by returning `false` from a new static `isLiveOnStart()`, which only `NativeWatcher` does - Watchman subscribes from the crawl's clock, and `FallbackWatcher`'s watches are live as soon as `fs.watch` returns. It's done in `Watcher`, after all the backends have started, because `NativeWatcher`s for multiple roots share one FSEvents stream, which restarts as each root is added. This adds ~10ms to a watch-mode start on macOS without Watchman (145ms -> 155ms on our benchmark app). Changelog: ``` - **[Fix]**: Fix changes made immediately after startup sometimes being missed on macOS ``` Test plan: New `Watcher-test.js` cases cover confirming each root before the crawl, warning and carrying on when a watch can't be confirmed, confirming a root added later, and leaving backends that are live on start alone. The backend integration tests now wait for a cookie of their own, as `Watcher` does. To reproduce the race, I patched `NativeWatcher` to drop all events for its first 300ms. A `FileMap` in watch mode with two roots, writing a file to each as soon as `build()` resolves, sees neither change without this PR (3/3 runs) and both with it (3/3). The `metro-file-map` suite passes on macOS, Linux and Windows. Benchmark (AI-driven): the harness from #2104, 8 interleaved rounds on an M4 Pro Mac mini, n=32 warm watch-mode starts per variant (+6.7%, 95% CI +5.2% to +8.0%). --- packages/metro-file-map/src/Watcher.js | 60 ++++++++- .../src/__tests__/Watcher-test.js | 115 ++++++++++++++++++ .../src/watchers/AbstractWatcher.js | 9 ++ .../src/watchers/NativeWatcher.js | 7 ++ .../src/watchers/__tests__/helpers.js | 32 +++++ 5 files changed, 220 insertions(+), 3 deletions(-) diff --git a/packages/metro-file-map/src/Watcher.js b/packages/metro-file-map/src/Watcher.js index c1bd3ab6be..ddba02c2a9 100644 --- a/packages/metro-file-map/src/Watcher.js +++ b/packages/metro-file-map/src/Watcher.js @@ -42,6 +42,11 @@ const debug = debugModule('Metro:Watcher'); const MAX_WAIT_TIME = 240000; +// For a backend that isn't live as soon as it starts, how long to wait for each +// health check that confirms a watch, and for the watch overall. +const LIVE_CHECK_TIMEOUT = 200; +const MAX_LIVE_WAIT_TIME = 10000; + type InternalCrawlOptions = Readonly<{ previousState: CrawlerOptions['previousState'], roots: ReadonlyArray, @@ -92,6 +97,9 @@ export class Watcher extends EventEmitter { #bufferedEvents: Array = []; #closed: boolean = false; #createBackend: ?(root: Path) => Promise; + // Whether a started backend's watches need confirming before they are + // relied on. + #confirmWatches: boolean = false; readonly #instanceId: number; #nextHealthCheckId: number = 0; #onChange: ?(change: WatcherBackendChangeEvent) => void; @@ -364,6 +372,42 @@ export class Watcher extends EventEmitter { createWatcherBackend(root, watchmanClocks), ), ); + // Confirmed only once every backend has started, because starting one may + // restart the others' watches (NativeWatchers share one FSEvents stream). + this.#confirmWatches = !WatcherImpl.isLiveOnStart(); + if (this.#confirmWatches) { + await Promise.all( + this.#options.roots.map(root => this.#waitUntilLive(root)), + ); + } + } + + /** + * Check the health of a root's watch until it reports a change, so that + * changes made from now on aren't missed. Warns if it can't be confirmed. + */ + async #waitUntilLive(root: Path): Promise { + const deadline = Date.now() + MAX_LIVE_WAIT_TIME; + let result; + do { + result = await this.#checkHealth(root, LIVE_CHECK_TIMEOUT); + } while ( + result.type === 'timeout' && + Date.now() < deadline && + !this.#closed && + !this.#options.abortSignal.aborted + ); + debug('Confirming the watch of %s: %s', root, result.type); + if ( + result.type !== 'success' && + !this.#closed && + !this.#options.abortSignal.aborted + ) { + this.#options.console.warn( + `metro-file-map: Could not confirm that ${root} is being watched ` + + `(${result.type}). Changes made during startup may be missed.`, + ); + } } /** @@ -379,8 +423,11 @@ export class Watcher extends EventEmitter { // `close()` may have been called while the backend was starting. if (this.#closed) { await backend.stopWatching(); - } else { - this.#backends.push(backend); + return; + } + this.#backends.push(backend); + if (this.#confirmWatches) { + await this.#waitUntilLive(root); } } @@ -399,6 +446,13 @@ export class Watcher extends EventEmitter { } async checkHealth(timeout: number): Promise { + return this.#checkHealth(this.#options.rootDir, timeout); + } + + /** + * Write a health check file into `dir`, and wait for a backend to report it. + */ + async #checkHealth(dir: Path, timeout: number): Promise { const healthCheckId = this.#nextHealthCheckId++; if (healthCheckId === Number.MAX_SAFE_INTEGER) { this.#nextHealthCheckId = 0; @@ -412,7 +466,7 @@ export class Watcher extends EventEmitter { this.#instanceId + '-' + healthCheckId; - const healthCheckPath = path.join(this.#options.rootDir, basename); + const healthCheckPath = path.join(dir, basename); let result: ?HealthCheckResult; const timeoutPromise = new Promise(resolve => setTimeout(resolve, timeout), diff --git a/packages/metro-file-map/src/__tests__/Watcher-test.js b/packages/metro-file-map/src/__tests__/Watcher-test.js index bb76b56dc5..cda12f91eb 100644 --- a/packages/metro-file-map/src/__tests__/Watcher-test.js +++ b/packages/metro-file-map/src/__tests__/Watcher-test.js @@ -20,6 +20,7 @@ import type { import TreeFS from '../lib/TreeFS'; import {Watcher} from '../Watcher'; +import * as fs from 'node:fs'; import * as path from 'node:path'; const mockWatchmanCrawl = jest.fn<[CrawlerOptions], Promise>(); @@ -40,6 +41,7 @@ type MockBackend = { emit: (event: WatcherBackendChangeEvent) => void, }; const mockStartedBackends: Array = []; +let mockNativeLiveOnStart = true; function mockBackendClass(kind: MockBackend['kind']) { return class { #root: string; @@ -48,6 +50,9 @@ function mockBackendClass(kind: MockBackend['kind']) { static isSupported(): boolean { return true; } + static isLiveOnStart(): boolean { + return kind !== 'native' || mockNativeLiveOnStart; + } constructor(root: string, opts: {clocks?: ?Map, ...}) { this.#root = root; this.#clocks = opts.clocks; @@ -64,6 +69,9 @@ function mockBackendClass(kind: MockBackend['kind']) { }); } async stopWatching() {} + getPauseReason(): ?string { + return null; + } }; } jest.mock('../watchers/NativeWatcher', () => ({ @@ -133,6 +141,7 @@ function createWatcher( beforeEach(() => { mockStartedBackends.length = 0; + mockNativeLiveOnStart = true; mockWatchmanCrawl.mockReset(); mockNodeCrawl.mockReset(); jest.spyOn(console, 'warn').mockImplementation(() => {}); @@ -273,6 +282,112 @@ describe('Watcher backend choice in watch mode', () => { }); }); +describe('Watcher with a backend that is not live on start', () => { + const healthCheckFilePrefix = '.metro-file-map-health-check'; + let writtenFiles: Array; + // Health check files are reported only once this many have been written. + let liveAfterWrites: number; + + beforeEach(() => { + // Health checks time out, and backends report changes asynchronously + jest.useRealTimers(); + mockNativeLiveOnStart = false; + writtenFiles = []; + liveAfterWrites = 1; + jest + .spyOn(fs.promises, 'writeFile') + .mockImplementation(async (filePath: string) => { + writtenFiles.push(filePath); + if (writtenFiles.length < liveAfterWrites) { + return; + } + const backend = mockStartedBackends.find( + b => b.root === path.dirname(filePath), + ); + // As a backend reports a change after the file is written + setImmediate(() => + backend?.emit({ + event: 'touch', + relativePath: path.basename(filePath), + root: backend.root, + metadata: {modifiedTime: 1, size: 0, type: 'f'}, + }), + ); + }); + jest.spyOn(fs.promises, 'unlink').mockImplementation(async () => {}); + }); + + afterEach(() => { + jest.useFakeTimers(); + }); + + test('confirms the watch of each root before the crawl', async () => { + liveAfterWrites = 2; + let writesBeforeCrawl = 0; + mockNodeCrawl.mockImplementation(async () => { + writesBeforeCrawl = writtenFiles.length; + return crawlResult([]); + }); + const watcher = createWatcher(false, null, true); + const warn = jest.spyOn(console, 'warn'); + + await watcher.crawl(); + + // The first health check went unreported, the second was reported. + expect(writesBeforeCrawl).toBe(2); + expect(writtenFiles.map(f => path.dirname(f))).toEqual([rootDir, rootDir]); + expect( + writtenFiles.every(f => + path.basename(f).startsWith(healthCheckFilePrefix), + ), + ).toBe(true); + expect(warn).not.toHaveBeenCalled(); + }); + + test('warns, and carries on, if a watch is never confirmed', async () => { + liveAfterWrites = Infinity; + mockNodeCrawl.mockImplementation(async () => crawlResult([])); + jest.useFakeTimers(); + const watcher = createWatcher(false, null, true); + const warn = jest.spyOn(console, 'warn'); + + const crawled = watcher.crawl(); + // $FlowFixMe[prop-missing] - in Jest 29.5+, missing from flow-typed's defs + await jest.advanceTimersByTimeAsync(11000); + await crawled; + + expect(mockNodeCrawl).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining( + `Could not confirm that ${rootDir} is being watched`, + ), + ); + }); + + test('confirms the watch of a root added later', async () => { + mockNodeCrawl.mockImplementation(async () => crawlResult([])); + const watcher = createWatcher(false, null, true); + await watcher.crawl(); + await watcher.watch(() => {}); + writtenFiles = []; + + const addedRoot = path.join(path.sep, 'other'); + await watcher.watchRoot(addedRoot); + + expect(writtenFiles.map(f => path.dirname(f))).toEqual([addedRoot]); + }); + + test('a backend that is live on start is not checked', async () => { + mockNativeLiveOnStart = true; + mockNodeCrawl.mockImplementation(async () => crawlResult([])); + const watcher = createWatcher(false, null, true); + + await watcher.crawl(); + + expect(writtenFiles).toEqual([]); + }); +}); + describe('Watcher crawl option', () => { const stubCrawler = (canonicalPaths: ReadonlyArray): Crawler => diff --git a/packages/metro-file-map/src/watchers/AbstractWatcher.js b/packages/metro-file-map/src/watchers/AbstractWatcher.js index 26fce73d8b..0f50bc52dd 100644 --- a/packages/metro-file-map/src/watchers/AbstractWatcher.js +++ b/packages/metro-file-map/src/watchers/AbstractWatcher.js @@ -32,6 +32,15 @@ export class AbstractWatcher implements WatcherBackend { #emitter: EventEmitter = new EventEmitter(); + /** + * Whether a watch reports every change made after `startWatching` resolves. + * If not, `Watcher` confirms each watch with a change of its own before + * relying on it. + */ + static isLiveOnStart(): boolean { + return true; + } + constructor(dir: string, opts: WatcherBackendOptions) { const {ignored, included} = opts; this.ignored = ignored; diff --git a/packages/metro-file-map/src/watchers/NativeWatcher.js b/packages/metro-file-map/src/watchers/NativeWatcher.js index f2203794ab..669f9fb32d 100644 --- a/packages/metro-file-map/src/watchers/NativeWatcher.js +++ b/packages/metro-file-map/src/watchers/NativeWatcher.js @@ -59,6 +59,13 @@ export default class NativeWatcher extends AbstractWatcher { return platform() === 'darwin'; } + // fs.watch can return before any changes are reported: libuv starts the + // FSEvents stream on another thread, and the stream reports only changes + // made after it starts. + static isLiveOnStart(): boolean { + return false; + } + constructor( dir: string, opts: Readonly<{ diff --git a/packages/metro-file-map/src/watchers/__tests__/helpers.js b/packages/metro-file-map/src/watchers/__tests__/helpers.js index d0a5d752ab..5a0e76b88a 100644 --- a/packages/metro-file-map/src/watchers/__tests__/helpers.js +++ b/packages/metro-file-map/src/watchers/__tests__/helpers.js @@ -109,6 +109,38 @@ export const startWatching = async ( await watcherInstance.startWatching(); + // As Metro's Watcher does, confirm a watch that isn't live on start with a + // change of its own - a cookie file, matched by the `cookie-*` prefix. + // Watchers emit in order, so once the latest is reported, none of the + // earlier ones remain in flight to leak into a test. + if (!Watcher.isLiveOnStart()) { + const deadline = Date.now() + 10000; + for (let i = 1; ; i++) { + const cookie = `cookie-live-${i}`; + let unsubscribe: () => void = () => {}; + const observed = new Promise(resolve => { + unsubscribe = watcherInstance.onFileEvent(change => { + if (change.relativePath === cookie) { + resolve(true); + } + }); + }); + await writeFile(join(watchRoot, cookie), ''); + const live = await Promise.race([ + observed, + new Promise(resolve => setTimeout(() => resolve(false), 200)), + ]); + unsubscribe(); + if (live) { + break; + } + invariant( + Date.now() < deadline, + `${watcherName} watch never became live`, + ); + } + } + const eventHelpers: EventHelpers = { nextEvent: afterFn => Promise.all([ From 09a4004a8fd2ebaa7dae5076b5725cf94d8eea5f Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Mon, 26 Oct 2026 11:08:55 +0000 Subject: [PATCH 2/2] Treat FallbackWatcher as not live on start on darwin --- packages/metro-file-map/src/watchers/FallbackWatcher.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/metro-file-map/src/watchers/FallbackWatcher.js b/packages/metro-file-map/src/watchers/FallbackWatcher.js index 6d7e4f0f93..dba8ac4b47 100644 --- a/packages/metro-file-map/src/watchers/FallbackWatcher.js +++ b/packages/metro-file-map/src/watchers/FallbackWatcher.js @@ -57,6 +57,11 @@ export default class FallbackWatcher extends AbstractWatcher { readonly #watched: {[key: string]: FSWatcher, __proto__: null} = Object.create(null); + // Not live on darwin, though FallbackWatcher isn't currently used there. + static isLiveOnStart(): boolean { + return platform !== 'darwin'; + } + async startWatching(): Promise { this.#watchdir(this.root);