Repository navigation
Conversation
meta-cla
Bot
added
the
CLA Signed
Metro review: Changes suggestedRisk: medium · Commit: This PR routes watcher backend errors to diagnostics instead of crashing Metro, rejects failed starts with their original error, and retries Watchman with backoff; #2151 was closed in favor of it, and its author notes overlap with #2150 to coordinate at landing. All seven replied findings are fixed and hold up, and the PR's tests pass on macOS, Linux and Windows. One gap remains: when Findings
Verified
Not run: No benchmark: the change adds one error listener per backend plus error and reconnect paths, with no per-file or per-event cost. Not run: the full Automated review by Claude. Advisory only: it never approves or blocks, and a maintainer makes the call. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| MAX_WAIT_TIME, | ||
| ); | ||
|
|
||
| watcher.onError(error => { |
There was a problem hiding this comment.
should fix · A failed Watchman reconnect now leaves a permanently dead watcher with one log line
Logging is the right result for a one-off lstat failure, but for a Watchman client error it means watching has stopped for good, and nothing recovers or says so.
Failure case: Watchman goes away, so the client gets end and WatchmanWatcher.#init reconnects once. If that attempt fails (socket connect error, or watchman get-sockname can't spawn), fb-watchman emits error but not end, so no further retry happens. Before this PR the process exited and the user restarted it. Now Metro keeps running and serves stale bundles, because no file event ever arrives again. The only signal is one watchman watcher error line that doesn't say changes are no longer seen. The health check that could expose this defaults to enabled: false.
From reading fb-watchman, a socket error without end on an established connection (e.g. ECONNRESET) reaches the same state without even one reconnect attempt. I did not exercise that path.
Suggested fix: handle this in WatchmanWatcher: on a client error or failed #init, retry with backoff until a new subscription is live, since a re-subscribe can recover. If retrying is out of scope, keep Watchman connection failures fatal, or at least log clearly that file watching has stopped and Metro must be restarted. A test should cover connection loss followed by a failed reconnect.
Reproduced: repro/watchman-repro.js dead, using the real Watcher and fb-watchman against a fake Watchman socket. The server closes, the reconnect fails, then the server returns. Head logs watchman watcher error ... connect ENOENT, then prints RESULT connections after Watchman came back: 0 after 5s. Base exits with RESULT UNCAUGHT: connect ENOENT. · id 79065045
There was a problem hiding this comment.
Fixed in e051d28. Failed Watchman reconnects now retry with exponential backoff from 1 second up to 30 seconds, resetting after a successful subscription. Coverage includes transport, watch-project, clock and subscribe failures, a later successful subscription, continued file events and cancellation on stop. The existing fresh-clock semantics are unchanged; this does not claim replay of changes during an outage. Full validation passes: 165 suites, 3,076 tests, 495 snapshots (14 skipped), plus Flow/lint/format/build.
| ); | ||
|
|
||
| watcher.onError(error => { | ||
| this.#options.console.error( |
There was a problem hiding this comment.
should fix · Hitting the watch limit logs a separate error and stack for every remaining directory
FallbackWatcher.#watchdir reports each fs.watch failure and carries on, so with this listener ENOSPC or EMFILE produces one console.error, with its stack, per unwatched directory.
Failure case: on Linux, a project with more directories than fs.inotify.max_user_watches used to stop on the first error, with a clear message. It now starts normally after printing an error for every directory past the limit. That can be many thousands of stack traces, burying the Metro UI and the one useful line, and the directories past the limit go unwatched.
Suggested fix: report resource-limit errors once per backend, or as one summary after startWatching, saying that changes in unwatched directories won't be detected and how to raise the limit. Add a test that fails fs.watch for many directories through the real Watcher and FallbackWatcher.
Reproduced: repro/flood-repro.js <checkout> 3000 100, using the real Watcher and FallbackWatcher with fs.watch throwing ENOSPC after 100 watches (a simulated limit, not real inotify exhaustion). Head prints RESULT started normally; console.error calls: 2901 of 3000 dirs (limit 100). Base prints RESULT process would exit, unhandled rejection: ENOSPC.... · id 488248c3
There was a problem hiding this comment.
Fixed in e051d28. ENOSPC and EMFILE are reported once per code per backend instead of once per remaining directory, with explicit guidance that unwatched directories will not deliver changes. The regression uses the real FallbackWatcher over 128 temporary directories with simulated fs.watch limit failures, verifies the crawl still visits them, and verifies a separate root gets its own diagnostic. No actual OS-wide watch limit was exhausted.
| this.#emitter.removeAllListeners(); | ||
| // An in-flight filesystem operation can still report an error after the | ||
| // owner has detached. Do not turn that late error into an uncaught throw. | ||
| this.#emitter.on('error', () => {}); |
There was a problem hiding this comment.
should fix · A Watchman client error after shutdown still crashes the process
The no-op listener only protects the backend's own emitter. WatchmanWatcher.stopWatching then calls this.#client.removeAllListeners() and this.#client.end(), which leaves the fb-watchman Client with no error listener while its socket can still fail. The client re-emits socket errors on itself, so a late one is an uncaught exception. The changelog says errors from in-flight work after shutdown are covered, and this is one such case.
Failure case: Watcher.close() runs while the backend is reconnecting after losing Watchman. The pending connect then fails and the process dies with an uncaught connect ENOENT. The window is narrow, but it's the crash this PR sets out to remove, and it can hit during normal shutdown.
Suggested fix: after removeAllListeners() in WatchmanWatcher.stopWatching, add this.#client.on('error', () => {}) before end(). Do the same for the old client that #init detaches on reconnect. Add a regression test that emits a client error after stopWatching; the new AbstractWatcher test can't catch this because it never involves a client.
Reproduced: repro/watchman-repro.js stop on head, using the real fb-watchman client and calling Watcher.close() on the tick after the Lost connection warning, prints close() called during reconnect, then RESULT UNCAUGHT: connect ENOENT .../w34883.sock. · id 8665ef2f
There was a problem hiding this comment.
Fixed in e051d28. Stopping now cancels reconnect timers and retires the client; stale command/subscription callbacks are ignored. Retired clients close late connections and absorb/close late errors, including a socket discovered after teardown. Two regressions exercise the actual fb-watchman client while mocking only its process/socket boundary, alongside lifecycle tests for replaced clients and in-flight replies. Full validation passes: 165 suites and 3,076 tests. This overlaps the Watchman shutdown portion of #2150, so those should be coordinated when landing.
| this.emitError(error); | ||
| this.#scheduleReconnect(); | ||
| } else { | ||
| onError(error); |
There was a problem hiding this comment.
should fix · A backend that fails during startup still terminates Metro, now through an unhandled rejection
onFailure now routes a client error during startup to onError, which rejects startWatching(). That promise is awaited inside the async executor at Watcher.js:382 (new Promise(async (resolve, reject) => {... await watcher.startWatching(); ...})), so nothing catches the rejection. The executor's own promise rejects unhandled, the promise watch() is waiting on stays pending, and Node exits the process.
Failure case: Watchman goes away between a successful crawl and the watcher's subscription, so the socket connect fails with connect ENOENT. Metro dies with an uncaught error from a promise, and watch() never gets to reject. The base also crashes here, by the unhandled error event, so this isn't a regression. But the changelog says file-watcher errors no longer terminate Metro, and this path still does. The same executor covers every backend: NativeWatcher.startWatching throwing ENOENT from fs.watch for a missing root kills the process the same way, and so can a failure while FileMap adds a root through watchRoot.
Suggested fix: in createWatcherBackend, wrap the await watcher.startWatching() in try/catch, then clear the timeout and call reject(error). watch() and watchRoot() then reject normally and their callers can report the failure. Add a test with a real backend whose startWatching rejects.
Reproduced: repro/startup-failure.js runs the real Watcher in a plain Node process. With WATCHMAN_SOCK pointing at a dead socket, head dies at triggerUncaughtException(err, true /* fromPromise */) with connect ENOENT, and the watch() handlers never run. The native-missing-root scenario dies with ENOENT on syscall: 'watch'. Base dies in both scenarios too, through emitError for Watchman. Exit codes weren't captured; the fatal stack traces show the deaths. · id 5feb8592
There was a problem hiding this comment.
Fixed in 0faadf1. Backend startup uses a synchronous Promise executor and forwards the original rejection while clearing the deadline. The added real NativeWatcher missing-root regression reports ENOENT promptly and leaves no timer. Broader pending/sibling/late-start cleanup remains covered by #2150, so those changes should be coordinated when landing.
| this.#deferringStates = new Set(resp['asserted-states']); | ||
| } | ||
|
|
||
| ready = true; |
There was a problem hiding this comment.
should fix · After a Watchman outage, Metro recovers without saying that changes made during it were missed
Each reconnect subscribes from a fresh clock, and a successful one logs nothing. Retrying is the right fix for the dead watcher, but it makes this state longer-lived and more likely to go unnoticed. Previously a failed reconnect left a dead watcher or a crash, and a restart meant a fresh crawl. Now Metro keeps serving through an outage of any length and then appears healthy.
Failure case: Watchman stops for 30 seconds, and the user edits src/a.js and deletes src/b.js. The subscription returns with a clock taken after those changes, so neither is ever reported. Metro keeps serving the old a.js and still resolves b.js, until each file happens to change again. The only output is the Lost connection warning and connect ENOENT errors, and none of it mentions missed changes. Even a clean Watchman restart with no failed attempts now has a blind window of at least 1 second, because the first reconnect used to be immediate and is now delayed.
The PR description declares replay out of scope, which is a reasonable place to draw the line, but users should still be told.
Suggested fix: at minimum, warn once when a reconnect succeeds, saying that changes made while disconnected may have been missed and that a restart resyncs them. Better, resynchronize: resubscribe since the last clock seen, handling is_fresh_instance if Watchman itself restarted, or have Watcher recrawl the root after a reconnect. Watcher currently rejects recrawl events from Watchman, so the recrawl route would need a change there. Consider making the first reconnect after a clean end immediate.
Evidence: repro/outage.js runs the real WatchmanWatcher and fb-watchman against a fake BSER server on a Unix socket. With the server down for 4 seconds, head retried at about 1s and 3s and resubscribed at about 7s, with since set to a new clock, not the last one. The only output was the Lost connection warning and two connect ENOENT errors, with nothing logged on recovery. The fake server emits no events, so the missed edit itself follows from the fresh since; it wasn't observed against real Watchman. · id f6ccda9f
There was a problem hiding this comment.
Fixed in 0faadf1. A successful reconnect now emits one warning naming the root, explaining that changes during disconnection may have been missed, and directing a restart to resynchronize. Tests cover failed retries, successful recovery, repeated outages, and startup without a recovery warning. Subscription clock behavior is preserved.
| MAX_WAIT_TIME, | ||
| ); | ||
|
|
||
| watcher.onError(error => { |
There was a problem hiding this comment.
should fix · An error on NativeWatcher's FSWatcher still terminates Metro
This listener only sees errors that go through emitError. NativeWatcher.startWatching (NativeWatcher.js:84) creates its FSWatcher without an error listener, unlike FallbackWatcher.js:216, so an error event on that object is still uncaught.
Failure case: on macOS without Watchman, Metro is running on NativeWatcher and its recursive FSWatcher emits error. The event throws instead of reaching the new reporting, and the process exits. The PR's changelog and summary cover filesystem errors from watcher backends generally, so this path is in scope. I triggered it by emitting the event on the real FSWatcher, and didn't establish which OS condition produces it.
Suggested fix: in NativeWatcher.startWatching, add this.#fsWatcher.on('error', error => this.emitError(error)), plus a test.
Reproduced: repro.js native-fswatcher-error on head and base, on macOS with the real Watcher and NativeWatcher: after watch() resolved, emitting error (code EIO) on the backend's FSWatcher threw Error: watch failed out of emit, so the survival log line never printed. · id dd9e1d91
There was a problem hiding this comment.
Fixed in 0faadf1. NativeWatcher forwards the filesystem handle's error event through the backend error channel. Tests cover original-error identity, continued file delivery, and safe late errors after stopping.
| ? 'Increase fs.inotify.max_user_watches on Linux.' | ||
| : 'Increase the open file limit (ulimit -n).'); | ||
| } | ||
| this.#options.console.error( |
There was a problem hiding this comment.
should fix · A lost FallbackWatcher directory watch is logged without saying changes will be missed
When a directory's FSWatcher errors, FallbackWatcher.js:216-224 forgets the handle and nothing watches that directory again. On base the unhandled error ended the process. Now the error is logged and Metro keeps running, with that directory silently out of date for the rest of the session.
Failure case: on Linux or Windows, the FSWatcher for src/feature errors with a code that isn't ignorable, such as EIO on a flaky mount. Metro logs fallback watcher error in <root>: watch failed, and later edits under src/feature produce no events. The message names neither the directory nor the consequence. The new ENOSPC/EMFILE guidance does state the consequence, but other codes get none. I triggered this by closing the real handle and emitting error, as Node does when it closes a failed watcher; I didn't produce a real OS failure.
Suggested fix: in FallbackWatcher, try to re-watch the directory and emit a recrawl for it. At minimum, report that changes under that directory won't be detected until a restart, and include the path.
Reproduced: repro.js fallback-lost-watch on head (macOS, real FallbackWatcher forced on): events before the error were ["sub/before.js","sub"]. After close() and emit('error') on the handle for sub, writing sub/after.js gave events after error [], with one generic console.error. On base the same emit threw. · id bb4c86e5
There was a problem hiding this comment.
Fixed in 0faadf1. A lost directory watch reports its path, the missed-change consequence, and restart guidance. The wrapper preserves the original cause and error code, retaining ENOSPC/EMFILE deduplication and existing ignorable-error behavior. Tests exercise real fallback watcher startup with controlled failing handles.
|
Closed #2151 in favour of this. As this one fixes the same crash and also covers the follow-ups raised in review of #2151 (de-duplicating ENOSPC/EMFILE, Watchman reconnects and late client errors). Thanks @vzaidman for the pointer and the review. One gap that's still open that I hit while testing this PR: NativeWatcher creates its recursive fs.watch handle without an 'error' listener (unlike FallbackWatcher), so an error emitted on that handle is still uncaught and ends the process. Forwarding it fixes it: It can be tested with the existing fs.watch mock in NativeWatcher-test.js by returning an EventEmitter as the handle and emitting 'error' on it. @sorinc03, happy to help there if useful. |
Propagate backend startup failures, forward native watch errors, and report lost directory watches and successful Watchman recovery. Preserve the underlying error and resource-limit handling. Test Plan: All 165 suites pass with 3094 tests passed, 14 skipped, and 495 snapshots. The added real NativeWatcher missing-root regression fails against the original async executor and passes with this change.
|
The NativeWatcher gap reported here is fixed in 0faadf1. Filesystem-handle errors now reach the backend error channel, including safe handling after shutdown. The update also forwards startup failures correctly and adds recovery/lost-watch diagnostics. Full checks pass with 3,094 tests and 495 snapshots, with 14 existing skips. Coverage includes a real NativeWatcher missing-root regression. |
| }); | ||
| }, | ||
| ); | ||
| this.#fsWatcher.on('error', error => this.emitError(error)); |
There was a problem hiding this comment.
should fix · A NativeWatcher FSWatcher error stops all watching for the root without saying so
Forwarding the error stops the crash, but Node has already closed the handle by the time FSWatcher emits error. NativeWatcher uses one recursive handle per root and never re-creates it, so every later change under that root is lost. On base this ended the process. Now Metro keeps running and looks healthy.
Failure case: on macOS without Watchman, the recursive FSWatcher for the project root fails. Metro logs metro-file-map: native watcher error in <root>: Error: EIO: i/o error, watch and nothing else. Later edits produce no events, so Metro keeps serving stale code until it restarts. The message gives neither the consequence nor the remedy. FallbackWatcher now gives both when it loses a single directory, while this case loses the whole root. The PR description names users being unaware that changes have stopped arriving as a problem it addresses.
Suggested fix: at minimum, wrap the error as FallbackWatcher does with WatchLostError: keep the cause and code, and say changes under the root won't be detected until a restart. Better, re-create the fs.watch handle and emit a recrawl for the root, unless the watcher has stopped. Add a test that checks the reported message, using the existing fs.watch mock in NativeWatcher-test.js.
Reproduced: Repro test packages/metro-file-map/src/__tests__/reviewNativeDead-test.js runs a real Watcher and NativeWatcher on macOS and calls the real handle's _handle.onchange(-5, ...), Node's path for a negative libuv status. Output: events [..., 'before.js'] with no after.js, handle: null, and only the one log line quoted above. The negative status was injected; I didn't produce a real OS failure or establish which condition causes one. · id 9e975cb7
Summary
Watcher backend errors can terminate Metro or leave users unaware that file changes have stopped arriving. Handle those errors through the existing diagnostics and preserve startup failures for callers.
Fixes #2143.
Changelog: [Fix] Report file-watcher errors and recover Watchman connections without unhandled backend errors
Test plan
yarn test --no-watchman --runInBandpasses Flow, ESLint, Markdown lint, format, declaration/build generation, and all 165 Jest suites.Validation ran on macOS. The separate startup-cleanup PR #2150 covers sibling, pending, and late-start resource cleanup and should be coordinated when landing. Reconnect subscriptions retain the existing fresh-clock behavior, with restart guidance for missed changes.