From 414c96e83d6ecaa423acd64d3c8ceef7bbe14b1f Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Mon, 7 Sep 2026 14:02:11 +0100 Subject: [PATCH] NativeWatcher: Emit events in the order fs.watch reported them **Problem** Each raw `fs.watch` callback was dispatched fire-and-forget, and every handler awaits an `lstat` before deciding what to emit. Those calls complete in whatever order the libuv thread pool finishes them, so the order we emitted in could differ from the order the OS reported. Over a recursive delete of `app/moved-in/file.js`, the raw callback order was correct in 250/250 runs but emission diverged in 5/250 - most often `delete app` overtaking `delete app/moved-in`. **Fix** `#handleEvent` now resolves a raw event into the event to emit rather than emitting it directly, and the `fs.watch` callback appends the result to a promise chain. The `lstat` calls still start immediately and run concurrently - only emission is sequenced - so there is no throughput cost. Also fixes a missing call in the constructor's `isSupported` guard, which made the check always truthy. **Impact** No user-facing bug is known to result from the old behaviour. `NativeWatcher` is the default backend on macOS when Watchman is unavailable, but each handler stats the path at handling time rather than trusting the event, so a reordered pair still tends to settle on the correct final state. The demonstrated effect is intermittent failure of the watcher integration tests, where a straggling deletion arriving after the one the test waited on desynchronised every later assertion. Ordering is worth guaranteeing regardless - `WatcherBackend` consumers reasonably assume events on a path and its ancestors arrive as the OS reported them, and `recrawl` in particular asks the file map to re-crawl a subtree that later events may still refer to. **Test plan** 250/250 emissions correctly ordered after the change, against 245/250 before. `packages/metro-file-map/src/watchers/__tests__/integration-test.js` passed 50/50 runs with this change alone and the tests unmodified, against a 5-10% baseline failure rate on macOS. --- .../src/watchers/NativeWatcher.js | 59 ++++++++++++++----- 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/packages/metro-file-map/src/watchers/NativeWatcher.js b/packages/metro-file-map/src/watchers/NativeWatcher.js index 6d35e18b9a..294ca2cd21 100644 --- a/packages/metro-file-map/src/watchers/NativeWatcher.js +++ b/packages/metro-file-map/src/watchers/NativeWatcher.js @@ -8,6 +8,7 @@ * @format */ +import type {WatcherBackendChangeEvent} from '../flow-types'; import type {FSWatcher} from 'node:fs'; import {AbstractWatcher} from './AbstractWatcher'; @@ -46,6 +47,11 @@ const RECRAWL_EVENT = 'recrawl'; export default class NativeWatcher extends AbstractWatcher { #fsWatcher: ?FSWatcher; + /** + * Promise chain to emit events in the order they were received. + */ + #emitQueue: Promise = Promise.resolve(); + static isSupported(): boolean { return platform() === 'darwin'; } @@ -59,7 +65,7 @@ export default class NativeWatcher extends AbstractWatcher { ... }>, ) { - if (!NativeWatcher.isSupported) { + if (!NativeWatcher.isSupported()) { throw new Error('This watcher can only be used on macOS'); } super(dir, opts); @@ -76,9 +82,25 @@ export default class NativeWatcher extends AbstractWatcher { recursive: true, }, (event, relativePath) => { - this._handleEvent(event, relativePath).catch(error => { - this.emitError(error); - }); + // Start handling immediately so that stats are gathered concurrently + // and as close as possible to the event, but emit in arrival order. + const settled = this.#handleEvent(event, relativePath).then( + change => ({change, error: null}), + (error: Error) => ({change: null, error}), + ); + this.#emitQueue = this.#emitQueue + .then(async () => { + const {change, error} = await settled; + if (error != null) { + this.emitError(error); + } else if (change != null) { + this.emitFileEvent(change); + } + }) + .catch(error => { + // Only reached if emitting threw + this.emitError(error); + }); }, ); @@ -95,7 +117,14 @@ export default class NativeWatcher extends AbstractWatcher { } } - async _handleEvent(event: string, relativePath: string) { + /** + * Resolve a raw `fs.watch` event into the event to emit for it, or `null` if + * it should be dropped. + */ + async #handleEvent( + event: string, + relativePath: string, + ): Promise> { const absolutePath = path.resolve(this.root, relativePath); if (this.doIgnore(relativePath)) { debug( @@ -104,7 +133,7 @@ export default class NativeWatcher extends AbstractWatcher { relativePath, this.root, ); - return; + return null; } debug( 'Handling event "%s" on %s (root: %s)', @@ -119,11 +148,11 @@ export default class NativeWatcher extends AbstractWatcher { // Ignore files of an unrecognized type if (!type) { - return; + return null; } if (!includedByGlob(type, this.globs, this.dot, relativePath)) { - return; + return null; } // For directory "rename" events, notify that we need a recrawl since we @@ -136,14 +165,13 @@ export default class NativeWatcher extends AbstractWatcher { 'Directory rename detected on %s, requesting recrawl', relativePath, ); - this.emitFileEvent({ + return { event: RECRAWL_EVENT, relativePath, - }); - return; + }; } - this.emitFileEvent({ + return { event: TOUCH_EVENT, relativePath, metadata: { @@ -151,14 +179,13 @@ export default class NativeWatcher extends AbstractWatcher { modifiedTime: stat.mtime.getTime(), size: stat.size, }, - }); + }; } catch (error) { if (error?.code !== 'ENOENT') { - this.emitError(error); - return; + throw error; } - this.emitFileEvent({event: DELETE_EVENT, relativePath}); + return {event: DELETE_EVENT, relativePath}; } } }