Log cache I/O errors to reporter without killing the server

Summary: TSIA Changelog: **[Fix]** Prevent cache write errors from killing the server, log them to `reporter` instead. Reviewed By: robhogan Differential Revision: D52043562 fbshipit-source-id: aed5216cf32b77cc9690ef3566329e10b03420f4
facebook · Apr 4, 2024 · 15815d0 · 15815d0
1 parent 7250388
commit 15815d0
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 46 deletions.
diff --git a/packages/metro/src/DeltaBundler/Transformer.js b/packages/metro/src/DeltaBundler/Transformer.js
@@ -131,7 +131,16 @@ class Transformer {
     }
 
     let fullKey = Buffer.concat([partialKey, Buffer.from(sha1, 'hex')]);
-    const result = await cache.get(fullKey);
+    let result;
+    try {
+      result = await cache.get(fullKey);
+    } catch (error) {
+      this._config.reporter.update({
+        type: 'cache_read_error',
+        error,
+      });
+      throw error;
+    }
 
     // A valid result from the cache is used directly; otherwise we call into
     // the transformer to computed the corresponding result.
@@ -154,7 +163,12 @@ class Transformer {
     }
 
     // Fire-and-forget cache set promise.
-    void cache.set(fullKey, data.result);
+    cache.set(fullKey, data.result).catch(error => {
+      this._config.reporter.update({
+        type: 'cache_write_error',
+        error,
+      });
+    });
 
     return {
       ...data.result,

diff --git a/packages/metro/src/DeltaBundler/__tests__/Transformer-test.js b/packages/metro/src/DeltaBundler/__tests__/Transformer-test.js
@@ -101,11 +101,58 @@ describe('Transformer', function () {
     );
   });
 
-  it('short-circuits the transformer cache key when the cache is disabled', async () => {
+  it('logs cache read errors to reporter', async () => {
+    const readError = new Error('Cache write error');
+    const get = jest.fn().mockImplementation(() => {
+      throw readError;
+    });
+    const set = jest.fn();
+    const mockReporter = {
+      update: jest.fn(),
+    };
+
     const transformerInstance = new Transformer(
       {
         ...commonOptions,
-        cacheStores: [],
+        reporter: mockReporter,
+        cacheStores: [{get, set}],
+        watchFolders,
+      },
+      getSha1,
+    );
+
+    require('../WorkerFarm').prototype.transform.mockReturnValue({
+      sha1: 'abcdefabcdefabcdefabcdefabcdefabcdefabcd',
+      result: {},
+    });
+
+    await expect(
+      transformerInstance.transformFile('./foo.js', {}),
+    ).rejects.toBe(readError);
+
+    expect(get).toHaveBeenCalledTimes(1);
+
+    expect(mockReporter.update).toBeCalledWith({
+      type: 'cache_read_error',
+      error: readError,
+    });
+  });
+
+  it('logs cache write errors to reporter', async () => {
+    const get = jest.fn();
+    const writeError = new Error('Cache write error');
+    const set = jest.fn().mockImplementation(() => {
+      throw writeError;
+    });
+    const mockReporter = {
+      update: jest.fn(),
+    };
+
+    const transformerInstance = new Transformer(
+      {
+        ...commonOptions,
+        reporter: mockReporter,
+        cacheStores: [{get, set}],
         watchFolders,
       },
       getSha1,
@@ -118,7 +165,12 @@ describe('Transformer', function () {
 
     await transformerInstance.transformFile('./foo.js', {});
 
-    expect(require('../getTransformCacheKey')).not.toBeCalled();
+    expect(set).toHaveBeenCalledTimes(1);
+
+    expect(mockReporter.update).toBeCalledWith({
+      type: 'cache_write_error',
+      error: writeError,
+    });
   });
 
   it('short-circuits the transformer cache key when the cache is disabled', async () => {

diff --git a/packages/metro/src/lib/TerminalReporter.js b/packages/metro/src/lib/TerminalReporter.js
@@ -11,11 +11,7 @@
 
 'use strict';
 
-import type {
-  BundleDetails,
-  GlobalCacheDisabledReason,
-  ReportableEvent,
-} from './reporting';
+import type {BundleDetails, ReportableEvent} from './reporting';
 import type {Terminal} from 'metro-core';
 import type {HealthCheckResult, WatcherStatus} from 'metro-file-map';
 
@@ -53,9 +49,6 @@ type SnippetError = ErrnoError &
     snippet?: string,
   };
 
-const GLOBAL_CACHE_DISABLED_MESSAGE_FORMAT =
-  'The global cache is now disabled because %s';
-
 const DARK_BLOCK_CHAR = '\u2593';
 const LIGHT_BLOCK_CHAR = '\u2591';
 const MAX_PROGRESS_BAR_CHAR_WIDTH = 16;
@@ -141,26 +134,6 @@ class TerminalReporter {
     );
   }
 
-  _logCacheDisabled(reason: GlobalCacheDisabledReason): void {
-    const format = GLOBAL_CACHE_DISABLED_MESSAGE_FORMAT;
-    switch (reason) {
-      case 'too_many_errors':
-        reporting.logWarning(
-          this.terminal,
-          format,
-          'it has been failing too many times.',
-        );
-        break;
-      case 'too_many_misses':
-        reporting.logWarning(
-          this.terminal,
-          format,
-          'it has been missing too many consecutive keys.',
-        );
-        break;
-    }
-  }
-
   _logBundleBuildDone(buildID: string): void {
     const progress = this._activeBundles.get(buildID);
     if (progress != null) {
@@ -253,9 +226,6 @@ class TerminalReporter {
       case 'bundling_error':
         this._logBundlingError(event.error);
         break;
-      case 'global_cache_disabled':
-        this._logCacheDisabled(event.reason);
-        break;
       case 'resolver_warning':
         this._logWarning(event.message);
         break;

diff --git a/packages/metro/src/lib/reporting.js b/packages/metro/src/lib/reporting.js
@@ -20,8 +20,6 @@ const chalk = require('chalk');
 const stripAnsi = require('strip-ansi');
 const util = require('util');
 
-export type GlobalCacheDisabledReason = 'too_many_errors' | 'too_many_misses';
-
 export type BundleDetails = {
   bundleType: string,
   customResolverOptions: CustomResolverOptions,
@@ -90,13 +88,13 @@ export type ReportableEvent =
       ...
     }
   | {
-      type: 'global_cache_error',
+      type: 'cache_read_error',
       error: Error,
       ...
     }
   | {
-      type: 'global_cache_disabled',
-      reason: GlobalCacheDisabledReason,
+      type: 'cache_write_error',
+      error: Error,
       ...
     }
   | {type: 'transform_cache_reset', ...}

diff --git a/packages/metro/types/lib/reporting.d.ts b/packages/metro/types/lib/reporting.d.ts
@@ -10,8 +10,6 @@
 
 import type {HealthCheckResult, WatcherStatus} from 'metro-file-map';
 
-export type GlobalCacheDisabledReason = 'too_many_errors' | 'too_many_misses';
-
 export interface BundleDetails {
   bundleType: string;
   dev: boolean;
@@ -65,12 +63,12 @@ export type ReportableEvent =
       totalFileCount: number;
     }
   | {
-      type: 'global_cache_error';
+      type: 'cache_read_error';
       error: Error;
     }
   | {
-      type: 'global_cache_disabled';
-      reason: GlobalCacheDisabledReason;
+      type: 'cache_write_error';
+      error: Error;
     }
   | {type: 'transform_cache_reset'}
   | {