Fix inconsistent filesystem state after a Haste collision

[robhogan]

Summary

Prior to D45033364 / f443e5d, Haste collisions arising after startup (in watch mode) would log a warning to console, but would otherwise be fully processed. This was true even if throwOnModuleCollisions was true, because _watch() mutates this._options.throwOnModuleCollision:

metro/packages/metro-file-map/src/index.js

Lines 872 to 874 in 462cc9d

	// In watch mode, we'll only warn about module collisions and we'll retain
	// all files, even changes to node_modules.
	this._options.throwOnModuleCollision = false;

In D45033364, this behaviour unintentionally regressed, because MutableHasteMap, newly responsible for warning/throwing, is passed throwOnModuleCollision (by its original value) in its constructor, and so the mutation in _watch had no effect:

metro/packages/metro-file-map/src/index.js

Lines 415 to 420 in 462cc9d

	const hasteMap = new MutableHasteMap({
	console: this._console,
	platforms: new Set(this._options.platforms),
	rootDir: this._options.rootDir,
	throwOnModuleCollision: this._options.throwOnModuleCollision,
	});

Weirdly, I apparently did realise this at the time, because I gave MutableHasteMap a setThrowsOnModuleCollision method that has never been used, maybe the call got lost in a merge or something 🤷‍♂️:

metro/packages/metro-file-map/src/lib/MutableHasteMap.js

Lines 287 to 289 in 462cc9d

	setThrowOnModuleCollision(shouldThrow: boolean) {
	this.#throwOnModuleCollision = shouldThrow;
	}

The throw during watch does not actually kill Metro, it just gets logged:

metro/packages/metro-file-map/src/index.js

Lines 1071 to 1075 in 462cc9d

	.catch((error: Error) => {
	this._console.error(
	`metro-file-map: watch error:\n ${error.stack}\n`,
	);
	});

But critically, a throw like this during _processFile prevents items being added to the fileSystem (TreeFS) and prevents change events firing:

metro/packages/metro-file-map/src/index.js

Lines 1032 to 1042 in 462cc9d

	try {
	await this._processFile(
	hasteMap,
	mockMap,
	absoluteFilePath,
	fileMetadata,
	{forceInBand: true}, // No need to clean up workers
	);
	fileSystem.addOrModify(relativeFilePath, fileMetadata);
	enqueueEvent(metadata);
	} catch (e) {

And worse, because the file is not in TreeFS, deleting it will not remove it from the Haste map (due to the early return in _removeIfExists), so the collision cannot be resolved without restarting Metro:

metro/packages/metro-file-map/src/index.js

Lines 826 to 841 in 462cc9d

	_removeIfExists(
	fileSystem: MutableFileSystem,
	hasteMap: MutableHasteMap,
	mockMap: RawMockMap,
	relativeFilePath: Path,
	) {
	const fileMetadata = fileSystem.remove(relativeFilePath);
	if (fileMetadata == null) {
	return;
	}
	const moduleName = fileMetadata[H.ID] || null; // Empty string indicates no module
	if (moduleName == null) {
	return;
	}
	hasteMap.removeModule(moduleName, relativeFilePath);

This is frequently seen when rebasing/moving around revisions while Metro is running, and could cause hard-to-debug errors (missing or ambiguous Haste resolution errors) that mysteriously resolve themselves by restarting Metro.

This restores the old behaviour in the simplest way by calling the setter at the same time as mutating _options, as well as adding a regression test. We'll refactor this a bit later.

Changelog:

 - **[Fix]**: Haste conflicts created after startup could cause inconsistent filesystem state

Test plan

Before (with new test)

FAIL packages/metro-file-map/src/__tests__/index-test.js
  FileMap
    ✓ exports constants (63 ms)
    ✓ ignores files given a pattern (14 ms)
    ✓ ignores vcs directories without ignore pattern (9 ms)
    ✓ ignores vcs directories with ignore pattern regex (10 ms)
    ✓ throw on ignore pattern except for regex (9 ms)
    ✓ builds a haste map on a fresh cache (17 ms)
    ✓ handles a Haste module moving between builds (26 ms)
    ✓ does not crawl native files even if requested to do so (20 ms)
    ✓ retains all files if `retainAllFiles` is specified (16 ms)
    ✓ warns on duplicate mock files (15 ms)
    ✓ warns on duplicate module ids (70 ms)
    ✓ throws on duplicate module ids if "throwOnModuleCollision" is set to true (20 ms)
    ✓ splits up modules by platform (12 ms)
    ✓ does not access the file system on a warm cache with no changes (15 ms)
    ✓ only does minimal file system access when files change (17 ms)
    ✓ correctly handles file deletions (14 ms)
    ✓ correctly handles platform-specific file additions (29 ms)
    ✓ correctly handles platform-specific file deletions (10 ms)
    ✓ correctly handles platform-specific file renames (9 ms)
    ✓ ignores files that do not exist (14 ms)
    ✓ distributes work across workers (9 ms)
    ✓ tries to crawl using node as a fallback (13 ms)
    ✓ tries to crawl using node as a fallback when promise fails once (9 ms)
    ✓ stops crawling when both crawlers fail (7 ms)
    builds a file map on a fresh cache with SHA-1s
      ✓ uses watchman: false, symlinks enabled: false (10 ms)
      ✓ uses watchman: false, symlinks enabled: true (13 ms)
      ✓ uses watchman: true, symlinks enabled: false (14 ms)
      ✓ uses watchman: true, symlinks enabled: true (12 ms)
    duplicate modules
      ✓ recovers when a duplicate file is deleted (9 ms)
      ✓ recovers when a duplicate platform-specific file is deleted (12 ms)
      ✓ recovers with the correct type when a duplicate file is deleted (17 ms)
      ✓ recovers when a duplicate module is renamed (12 ms)
    file system changes processing
      ✓ build returns a "live" fileSystem and hasteMap (41 ms)
      ✓ handles several change events at once (42 ms)
      ✓ does not emit duplicate change events (39 ms)
      ✓ suppresses backend symlink events if enableSymlinks: false (40 ms)
      ✓ emits symlink events if enableSymlinks: true (40 ms)
      ✓ emits a change even if a file in node_modules has changed (39 ms)
      ✓ does not emit changes for regular files with unwatched extensions (41 ms)
      ✓ does not emit delete events for unknown files (39 ms)
      ✓ does emit changes for symlinks with unlisted extensions (39 ms)
      ✓ symlink deletion is handled without affecting the symlink target (40 ms)
      ✓ correctly tracks changes to both platform-specific versions of a single module name (39 ms)
      ✓ correctly handles moving a Haste module (39 ms)
      recovery from duplicate module IDs
        ✕ does not throw on a duplicate created at runtime even if throwOnModuleCollision: true (9 ms)
        ✓ recovers when the oldest version of the duplicates is fixed (68 ms)
        ✓ recovers when the most recent duplicate is fixed (69 ms)
        ✓ ignore directory events (even with file-ish names) (38 ms)

  ● FileMap › file system changes processing › recovery from duplicate module IDs › does not throw on a duplicate created at runtime even if throwOnModuleCollision: true

    should not print error

      2087 |           await new Promise((resolve, reject) => {
      2088 |             console.error.mockImplementationOnce(() => {
    > 2089 |               reject(new Error('should not print error'));
           |                      ^
      2090 |             });
      2091 |             hm.once('change', resolve);
      2092 |           });

      at console.<anonymous> (packages/metro-file-map/src/__tests__/index-test.js:2089:22)
      at MutableHasteMap.setModule (packages/metro-file-map/src/lib/MutableHasteMap.js:220:30)
      at setModule (packages/metro-file-map/src/index.js:551:18)
      at packages/metro-file-map/src/index.js:1034:15

Test Suites: 1 failed, 1 total
Tests:       1 failed, 47 passed, 48 total
Snapshots:   5 passed, 5 total
Time:        2.232 s, estimated 3 s
Ran all test suites matching /metro-file-map\/src\/__tests__\/index-test/i.
error Command failed with exit code 1.

After

Tests pass

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@robhogan merged this pull request in 44bab52.