From 24e70fa31d7c3b0e7642c41daccd1fc3d5e25d3e Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Tue, 29 Sep 2026 20:53:32 +0100 Subject: [PATCH] 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 events are delivered. 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 immediately after startup can be silently missed. 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 adds a `probe` option for watcher backends. `Watcher` passes one that writes a health check file to the backend's root and waits for it to be reported, and `NativeWatcher` probes until one is observed before resolving. If it can't confirm within 10s, Metro warns and carries on. Note that `NativeWatcher`s for multiple roots share one FSEvents stream, which restarts as each root is added, so the guarantee is for `Watcher.watch()` as a whole rather than each backend. Changelog: ``` - **[Fix]**: Fix changes made immediately after startup sometimes being missed on macOS ``` Test plan: New unit tests for the probe loop. The integration tests now pass their own probe. To reproduce the race locally, I patched `NativeWatcher` to drop all events for its first 300ms: - The Native integration tests fail as on CI without a probe ("detects a new, changed, deleted file" times out in its hook), and pass with this PR. - A `FileMap` in watch mode with two roots, writing a file to each as soon as `build()` resolves, sees neither change without a probe (3/3 runs) and both with this PR (3/3). - Dropping events indefinitely, `build()` resolves after 10s with a warning per root, and no health check files are left behind. --- packages/metro-file-map/src/Watcher.js | 24 ++++++++++- .../src/watchers/NativeWatcher.js | 22 ++++++++++ .../watchers/__tests__/NativeWatcher-test.js | 42 +++++++++++++++++++ .../src/watchers/__tests__/helpers.js | 28 ++++++++++++- .../metro-file-map/src/watchers/common.js | 14 +++++++ 5 files changed, 126 insertions(+), 4 deletions(-) diff --git a/packages/metro-file-map/src/Watcher.js b/packages/metro-file-map/src/Watcher.js index 96e83f02f2..5cf417aa78 100644 --- a/packages/metro-file-map/src/Watcher.js +++ b/packages/metro-file-map/src/Watcher.js @@ -18,7 +18,10 @@ import type { WatcherBackend, WatcherBackendChangeEvent, } from './flow-types'; -import type {WatcherOptions as WatcherBackendOptions} from './watchers/common'; +import type { + WatcherOptions as WatcherBackendOptions, + WatchProbeResult, +} from './watchers/common'; import nodeCrawl from './crawlers/node'; import watchmanCrawl from './crawlers/watchman'; @@ -220,6 +223,7 @@ export class Watcher extends EventEmitter { this.#activeWatcher = watcher; const createWatcherBackend = (root: Path): Promise => { + let probeResult: ?WatchProbeResult = null; const watcherOptions: WatcherBackendOptions = { dot: true, globs: [ @@ -231,6 +235,11 @@ export class Watcher extends EventEmitter { ...extensions.map(extension => '**/*.' + extension), ], ignored: ignorePatternForWatch, + probe: async timeoutMs => { + const {type} = await this.#checkHealth(root, timeoutMs); + probeResult = type === 'success' ? 'observed' : type; + return probeResult; + }, watchmanDeferStates: this.#options.watchmanDeferStates, }; const watcher: WatcherBackend = new WatcherImpl(root, watcherOptions); @@ -267,6 +276,13 @@ export class Watcher extends EventEmitter { }); await watcher.startWatching(); clearTimeout(rejectTimeout); + if (probeResult != null && probeResult !== 'observed') { + this.#options.console.warn( + `metro-file-map: Could not confirm that ${root} is being watched ` + + `(probe result: ${probeResult}). Changes made during startup ` + + 'may be missed.', + ); + } resolve(watcher); }); }; @@ -290,6 +306,10 @@ export class Watcher extends EventEmitter { } async checkHealth(timeout: number): Promise { + return this.#checkHealth(this.#options.rootDir, timeout); + } + + async #checkHealth(dir: string, timeout: number): Promise { const healthCheckId = this.#nextHealthCheckId++; if (healthCheckId === Number.MAX_SAFE_INTEGER) { this.#nextHealthCheckId = 0; @@ -303,7 +323,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/watchers/NativeWatcher.js b/packages/metro-file-map/src/watchers/NativeWatcher.js index 72dc3e8fc7..0faba6a25c 100644 --- a/packages/metro-file-map/src/watchers/NativeWatcher.js +++ b/packages/metro-file-map/src/watchers/NativeWatcher.js @@ -9,6 +9,7 @@ */ import type {WatcherBackendChangeEvent} from '../flow-types'; +import type {WatchProbe} from './common'; import type {FSWatcher} from 'node:fs'; import {AbstractWatcher} from './AbstractWatcher'; @@ -24,6 +25,10 @@ const TOUCH_EVENT = 'touch'; const DELETE_EVENT = 'delete'; const RECRAWL_EVENT = 'recrawl'; +// How long to wait for each probe, and for the watch to become live overall. +const PROBE_TIMEOUT_MS = 200; +const LIVE_TIMEOUT_MS = 10000; + /** * NativeWatcher uses Node's native fs.watch API with recursive: true. * @@ -46,6 +51,7 @@ const RECRAWL_EVENT = 'recrawl'; */ export default class NativeWatcher extends AbstractWatcher { #fsWatcher: ?FSWatcher; + readonly #probe: ?WatchProbe; /** * Promise chain to emit events in the order they were received. @@ -62,6 +68,7 @@ export default class NativeWatcher extends AbstractWatcher { ignored: ?RegExp, globs: ReadonlyArray, dot: boolean, + probe?: ?WatchProbe, ... }>, ) { @@ -69,6 +76,7 @@ export default class NativeWatcher extends AbstractWatcher { throw new Error('This watcher can only be used on macOS'); } super(dir, opts); + this.#probe = opts.probe; } async startWatching(): Promise { @@ -108,6 +116,20 @@ export default class NativeWatcher extends AbstractWatcher { ); debug('Watching %s', this.root); + + // fs.watch can return before any events are delivered: libuv starts the + // FSEvents stream on another thread, and the stream reports only changes + // made after it starts. Probe until a change is reported, so that changes + // made as soon as this resolves are not missed. + const probe = this.#probe; + if (probe != null) { + const deadline = Date.now() + LIVE_TIMEOUT_MS; + let result; + do { + result = await probe(PROBE_TIMEOUT_MS); + } while (result === 'timeout' && Date.now() < deadline); + debug('Probed watch of %s: %s', this.root, result); + } } /** diff --git a/packages/metro-file-map/src/watchers/__tests__/NativeWatcher-test.js b/packages/metro-file-map/src/watchers/__tests__/NativeWatcher-test.js index f2ef843d12..aea59528c2 100644 --- a/packages/metro-file-map/src/watchers/__tests__/NativeWatcher-test.js +++ b/packages/metro-file-map/src/watchers/__tests__/NativeWatcher-test.js @@ -10,6 +10,7 @@ */ import type {WatcherBackendChangeEvent} from '../../flow-types'; +import type {WatchProbeResult} from '../common'; import NativeWatcher from '../NativeWatcher'; import fs from 'node:fs'; @@ -161,4 +162,45 @@ describe('NativeWatcher', () => { await flush(); expect(order).toEqual(['touch:first.js', 'error:EACCES', 'touch:third.js']); }); + + test.each([ + [['observed'], 1], + [['timeout', 'timeout', 'observed'], 3], + [['timeout', 'error', 'observed'], 2], + ])( + 'startWatching probes until the watch is live: %j', + async (results, expectedCalls) => { + const probe = jest.fn<[number], Promise>(); + for (const result of results) { + probe.mockResolvedValueOnce(result); + } + const probed = new NativeWatcher(ROOT, { + dot: true, + globs: [], + ignored: null, + probe, + }); + await probed.startWatching(); + expect(probe).toHaveBeenCalledTimes(expectedCalls); + await probed.stopWatching(); + }, + ); + + test('startWatching stops probing after an overall timeout', async () => { + let now = 0; + jest.spyOn(Date, 'now').mockImplementation(() => now); + const probe = jest.fn<[number], Promise>(async () => { + now += 4000; + return 'timeout'; + }); + const probed = new NativeWatcher(ROOT, { + dot: true, + globs: [], + ignored: null, + probe, + }); + await probed.startWatching(); + expect(probe).toHaveBeenCalledTimes(3); + await probed.stopWatching(); + }); }); diff --git a/packages/metro-file-map/src/watchers/__tests__/helpers.js b/packages/metro-file-map/src/watchers/__tests__/helpers.js index d0a5d752ab..d45dba9397 100644 --- a/packages/metro-file-map/src/watchers/__tests__/helpers.js +++ b/packages/metro-file-map/src/watchers/__tests__/helpers.js @@ -10,7 +10,7 @@ */ import type {ChangeEventMetadata} from '../../flow-types'; -import type {WatcherOptions} from '../common'; +import type {WatcherOptions, WatchProbe} from '../common'; import FallbackWatcher from '../FallbackWatcher'; import NativeWatcher from '../NativeWatcher'; @@ -105,7 +105,31 @@ export const startWatching = async ( }>) => { const Watcher = WATCHERS[watcherName]; invariant(Watcher != null, `Watcher ${watcherName} is not supported`); - const watcherInstance = new Watcher(watchRoot, opts); + // Probe with cookie files (matched by the `cookie-*` glob), as Metro does + // with health check files. Watchers emit in order, so once the latest is + // observed, none of the earlier ones remain in flight to leak into a test. + let probeCount = 0; + const probe: WatchProbe = async timeoutMs => { + const cookie = `cookie-probe-${++probeCount}`; + let unsubscribe: () => void = () => {}; + const observed = new Promise<'observed'>(resolve => { + unsubscribe = watcherInstance.onFileEvent(change => { + if (change.relativePath === cookie) { + resolve('observed'); + } + }); + }); + await writeFile(join(watchRoot, cookie), ''); + const result = await Promise.race([ + observed, + new Promise<'timeout'>(resolve => + setTimeout(() => resolve('timeout'), timeoutMs), + ), + ]); + unsubscribe(); + return result; + }; + const watcherInstance = new Watcher(watchRoot, {...opts, probe}); await watcherInstance.startWatching(); diff --git a/packages/metro-file-map/src/watchers/common.js b/packages/metro-file-map/src/watchers/common.js index 04826e7d5b..e3b322d54f 100644 --- a/packages/metro-file-map/src/watchers/common.js +++ b/packages/metro-file-map/src/watchers/common.js @@ -29,10 +29,24 @@ export const TOUCH_EVENT = 'touch'; export const RECRAWL_EVENT = 'recrawl'; export const ALL_EVENT = 'all'; +/** + * Writes a file under the watched root and resolves with whether the backend + * reported it within `timeoutMs`, or 'error' if it could not be written. + */ +export type WatchProbe = (timeoutMs: number) => Promise; +export type WatchProbeResult = 'observed' | 'timeout' | 'error'; + export type WatcherOptions = Readonly<{ globs: ReadonlyArray, dot: boolean, ignored: ?RegExp, + /** + * Used by backends whose watch may not be live as soon as it is set up, to + * wait until it is before startWatching resolves. NativeWatcher backends in + * one process share an FSEvents stream, which restarts as each is added, so + * a watch is only guaranteed live once every backend has started. + */ + probe?: ?WatchProbe, watchmanDeferStates: ReadonlyArray, watchman?: unknown, watchmanPath?: string,