metro-cache: Cleanup FileStore caches, deprecate AutoCleanFileStore (#1570)

Summary:
Pull Request resolved: #1570

Modernise `FileStore` and `AutoCleanFileStore` tests and implementations, using built-in recursive directory scans, `memfs` over `metro-memory-fs`, and `#`-private over `_`-private.

## Deprecating `AutoCleanFileStore`
Deprecate `AutoCleanFileStore` because the current implementation isn't used by default and has a number of flaws that should prevent us recommending it:
 - It re-scans all files every interval, even though it's in a position to know when files are read and written by the cache.
 - It will delete recently used entries just because they weren't recently written.
 - It's fully synchronous (event-loop blocking)

The current mechanism could easily exist outside Metro, e.g., as a simple cron of:

```
find /cache/root -mtime +3 -exec rm {} \;
```

Typically, caches are written to OS "temp" directories whose lifetimes are managed by the OS.

A better *Metro* cache solution would be an LRU cache, but this could be fully implemented in userland and we haven't had any requests to include one in core.

Changelog:
```
 - **[Deprecated]**: metro-cache: Deprecate AutoCleanFileStore
```

Reviewed By: huntie

Differential Revision: D81051149

fbshipit-source-id: 2e0d44829a6ecaac7bf93c88b4170cadb5e019bf

Author: robhogan

docs/Caching.md

@@ -20,7 +20,7 @@ The main option for configuring the Metro cache is [`cacheStores`](./Configurati
 Metro provides a number of built-in cache store implementations for use with the [`cacheStores`](./Configuration.md#cachestores) config option:
 
 * **`FileStore({root: string})`** will store cache entries as files under the directory specified by `root`.
-* **`AutoCleanFileStore()`** is a `FileStore` that periodically cleans up old entries. It accepts the same options as `FileStore` plus the following:
+* **`AutoCleanFileStore()`** <div class="label deprecated">Deprecated</div> is a `FileStore` that periodically cleans up old entries. It accepts the same options as `FileStore` plus the following:
   * **`options.intervalMs: number`** is the time in milliseconds between cleanup attempts. Defaults to 10 minutes.
   * **`options.cleanupThresholdMs: number`** is the minimum time in milliseconds since the last modification of an entry before it can be deleted. Defaults to 3 days.
 * **`HttpStore(options)`** is a bare-bones remote cache client that reads (`GET`) and writes (`PUT`) compressed cache artifacts over HTTP or HTTPS.

packages/metro-cache/package.json

@@ -23,7 +23,7 @@
     "metro-core": "0.83.1"
   },
   "devDependencies": {
-    "metro-memory-fs": "*"
+    "memfs": "^4.38.2"
   },
   "license": "MIT",
   "engines": {

packages/metro-cache/src/stores/AutoCleanFileStore.js

@@ -4,8 +4,8 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
+ * @flow strict-local
  * @format
- * @flow
  */
 
 import type {Options} from './FileStore';
@@ -14,90 +14,72 @@ import FileStore from './FileStore';
 import fs from 'fs';
 import path from 'path';
 
-type CleanOptions = {
+type CleanOptions = $ReadOnly<{
   ...Options,
   intervalMs?: number,
   cleanupThresholdMs?: number,
-  ...
-};
-
-type FileList = {
-  path: string,
-  stats: fs.Stats,
-  ...
-};
-
-// List all files in a directory in Node.js recursively in a synchronous fashion
-const walkSync = function (
-  dir: string,
-  filelist: Array<FileList>,
-): Array<FileList> {
-  const files = fs.readdirSync(dir);
-  filelist = filelist || [];
-  files.forEach(function (file) {
-    const fullPath = path.join(dir, file);
-    const stats = fs.statSync(fullPath);
-    if (stats.isDirectory()) {
-      filelist = walkSync(fullPath + path.sep, filelist);
-    } else {
-      filelist.push({path: fullPath, stats});
-    }
-  });
-  return filelist;
-};
-
-function get<T>(property: ?T, defaultValue: T): T {
-  if (property == null) {
-    return defaultValue;
-  }
-
-  return property;
-}
+}>;
 
 /**
- * A FileStore that cleans itself up in a given interval
+ * A FileStore that, at a given interval, stats the content of the cache root
+ * and deletes any file last modified a set threshold in the past.
+ *
+ * @deprecated This is not efficiently implemented and may cause significant
+ * redundant I/O when caches are large. Prefer your own cleanup scripts, or a
+ * custom Metro cache that uses watches, hooks get/set, and/or implements LRU.
  */
 export default class AutoCleanFileStore<T> extends FileStore<T> {
-  _intervalMs: number;
-  _cleanupThresholdMs: number;
-  _root: string;
+  +#intervalMs: number;
+  +#cleanupThresholdMs: number;
+  +#root: string;
 
   constructor(opts: CleanOptions) {
     super({root: opts.root});
 
-    this._intervalMs = get(opts.intervalMs, 10 * 60 * 1000); // 10 minutes
-    this._cleanupThresholdMs = get(
-      opts.cleanupThresholdMs,
-      3 * 24 * 60 * 60 * 1000, // 3 days
-    );
+    this.#root = opts.root;
+    this.#intervalMs = opts.intervalMs ?? 10 * 60 * 1000; // 10 minutes
+    this.#cleanupThresholdMs =
+      opts.cleanupThresholdMs ?? 3 * 24 * 60 * 60 * 1000; // 3 days
 
-    this._scheduleCleanup();
+    this.#scheduleCleanup();
   }
 
-  _scheduleCleanup() {
-    // $FlowFixMe[method-unbinding] added when improving typing for this parameters
-    setTimeout(this._doCleanup.bind(this), this._intervalMs);
+  #scheduleCleanup() {
+    setTimeout(() => this.#doCleanup(), this.#intervalMs);
   }
 
-  _doCleanup() {
-    const files = walkSync(this._root, []);
+  #doCleanup() {
+    const dirents = fs.readdirSync(this.#root, {
+      recursive: true,
+      withFileTypes: true,
+    });
 
     let warned = false;
-    files.forEach(file => {
-      if (file.stats.mtimeMs < Date.now() - this._cleanupThresholdMs) {
+    const minModifiedTime = Date.now() - this.#cleanupThresholdMs;
+    dirents
+      .filter(dirent => dirent.isFile())
+      .forEach(dirent => {
+        const absolutePath = path.join(
+          // $FlowFixMe[prop-missing] - dirent.parentPath added in Node 20.12
+          dirent.parentPath,
+          dirent.name.toString(),
+        );
         try {
-          fs.unlinkSync(file.path);
+          if (fs.statSync(absolutePath).mtimeMs < minModifiedTime) {
+            fs.unlinkSync(absolutePath);
+          }
         } catch (e) {
           if (!warned) {
             console.warn(
-              'Problem cleaning up cache for ' + file.path + ': ' + e.message,
+              'Problem cleaning up cache for ' +
+                absolutePath +
+                ': ' +
+                e.message,
             );
             warned = true;
           }
         }
-      }
-    });
-
-    this._scheduleCleanup();
+      });
+    this.#scheduleCleanup();
   }
 }

packages/metro-cache/src/stores/FileStore.js

@@ -4,8 +4,8 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
- * @format
  * @flow
+ * @format
  */
 
 import fs from 'fs';
@@ -14,20 +14,20 @@ import path from 'path';
 const NULL_BYTE = 0x00;
 const NULL_BYTE_BUFFER = Buffer.from([NULL_BYTE]);
 
-export type Options = {
+export type Options = $ReadOnly<{
   root: string,
-};
+}>;
 
 export default class FileStore<T> {
-  _root: string;
+  +#root: string;
 
   constructor(options: Options) {
-    this._root = options.root;
+    this.#root = options.root;
   }
 
   async get(key: Buffer): Promise<?T> {
     try {
-      const data = await fs.promises.readFile(this._getFilePath(key));
+      const data = await fs.promises.readFile(this.#getFilePath(key));
 
       if (data[0] === NULL_BYTE) {
         return (data.slice(1): any);
@@ -44,20 +44,20 @@ export default class FileStore<T> {
   }
 
   async set(key: Buffer, value: T): Promise<void> {
-    const filePath = this._getFilePath(key);
+    const filePath = this.#getFilePath(key);
     try {
-      await this._set(filePath, value);
+      await this.#set(filePath, value);
     } catch (err) {
       if (err.code === 'ENOENT') {
         fs.mkdirSync(path.dirname(filePath), {recursive: true});
-        await this._set(filePath, value);
+        await this.#set(filePath, value);
       } else {
         throw err;
       }
     }
   }
 
-  async _set(filePath: string, value: T): Promise<void> {
+  async #set(filePath: string, value: T): Promise<void> {
     let content;
     if (value instanceof Buffer) {
       content = Buffer.concat([NULL_BYTE_BUFFER, value]);
@@ -68,20 +68,20 @@ export default class FileStore<T> {
   }
 
   clear() {
-    this._removeDirs();
+    this.#removeDirs();
   }
 
-  _getFilePath(key: Buffer): string {
+  #getFilePath(key: Buffer): string {
     return path.join(
-      this._root,
+      this.#root,
       key.slice(0, 1).toString('hex'),
       key.slice(1).toString('hex'),
     );
   }
 
-  _removeDirs() {
+  #removeDirs() {
     for (let i = 0; i < 256; i++) {
-      fs.rmSync(path.join(this._root, ('0' + i.toString(16)).slice(-2)), {
+      fs.rmSync(path.join(this.#root, ('0' + i.toString(16)).slice(-2)), {
         force: true,
         recursive: true,
       });

packages/metro-cache/src/stores/__tests__/AutoCleanFileStore-test.js

@@ -9,7 +9,7 @@
  * @oncall react_native
  */
 
-'use strict';
+import {memfs} from 'memfs';
 
 describe('AutoCleanFileStore', () => {
   let AutoCleanFileStore;
@@ -19,22 +19,23 @@ describe('AutoCleanFileStore', () => {
     jest
       .resetModules()
       .resetAllMocks()
-      .mock('fs', () => new (require('metro-memory-fs'))());
-
+      .mock('fs', () => memfs().fs);
     AutoCleanFileStore = require('../AutoCleanFileStore').default;
     fs = require('fs');
+    jest.spyOn(fs, 'statSync');
     jest.spyOn(fs, 'unlinkSync');
   });
 
   test('sets and writes into the cache', async () => {
-    // $FlowFixMe[underconstrained-implicit-instantiation]
-    const fileStore = new AutoCleanFileStore({
+    const fileStore = new AutoCleanFileStore<mixed>({
       root: '/root',
       intervalMs: 49,
-      cleanupThresholdMs: 0,
+      cleanupThresholdMs: 90,
     });
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
 
+    expect(fs.statSync).toHaveBeenCalledTimes(0);
+
     await fileStore.set(cache, {foo: 42});
     expect(await fileStore.get(cache)).toEqual({foo: 42});
 
@@ -43,17 +44,28 @@ describe('AutoCleanFileStore', () => {
 
     expect(await fileStore.get(cache)).toEqual({foo: 42});
 
+    // And there should have been no cleanup
+    expect(fs.statSync).not.toHaveBeenCalled();
+
     // Run to 50ms so that we've exceeded the 49ms cleanup interval
     jest.advanceTimersByTime(20);
 
-    // mtime doesn't work very well in in-memory-store, so we couldn't test that
-    // functionality
+    expect(fs.statSync).toHaveBeenCalledTimes(1);
+
+    // At 50ms we should have checked the file, but it's still fresh enough
+    expect(await fileStore.get(cache)).toEqual({foo: 42});
+    expect(fs.unlinkSync).not.toHaveBeenCalled();
+
+    jest.advanceTimersByTime(50);
+
+    // After another 50ms, we should have checked the file again and deleted it
+    expect(fs.statSync).toHaveBeenCalledTimes(2);
+    expect(fs.unlinkSync).toHaveBeenCalledTimes(1);
     expect(await fileStore.get(cache)).toEqual(null);
   });
 
   test('returns null when reading a non-existing file', async () => {
-    // $FlowFixMe[underconstrained-implicit-instantiation]
-    const fileStore = new AutoCleanFileStore({root: '/root'});
+    const fileStore = new AutoCleanFileStore<mixed>({root: '/root'});
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
 
     expect(await fileStore.get(cache)).toEqual(null);

packages/metro-cache/src/stores/__tests__/FileStore-test.js

@@ -4,13 +4,12 @@
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
  *
+ * @flow strict-local
  * @format
  * @oncall react_native
  */
 
-'use strict';
-
-const {dirname} = require('path');
+import {memfs} from 'memfs';
 
 describe('FileStore', () => {
   let FileStore;
@@ -20,39 +19,38 @@ describe('FileStore', () => {
     jest
       .resetModules()
       .resetAllMocks()
-      .mock('fs', () => new (require('metro-memory-fs'))());
+      .mock('fs', () => memfs().fs);
 
     FileStore = require('../FileStore').default;
     fs = require('fs');
     jest.spyOn(fs, 'unlinkSync');
   });
 
   test('sets and writes into the cache', async () => {
-    const fileStore = new FileStore({root: '/root'});
+    const fileStore = new FileStore<mixed>({root: '/root'});
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
 
     await fileStore.set(cache, {foo: 42});
     expect(await fileStore.get(cache)).toEqual({foo: 42});
   });
 
   test('returns null when reading a non-existing file', async () => {
-    const fileStore = new FileStore({root: '/root'});
+    const fileStore = new FileStore<mixed>({root: '/root'});
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
 
     expect(await fileStore.get(cache)).toEqual(null);
   });
 
   test('returns null when reading a empty file', async () => {
-    const fileStore = new FileStore({root: '/root'});
+    const fileStore = new FileStore<mixed>({root: '/root'});
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
-    const filePath = fileStore._getFilePath(cache);
-    fs.mkdirSync(dirname(filePath), {recursive: true});
-    fs.writeFileSync(filePath, '');
+    jest.spyOn(fs.promises, 'readFile').mockImplementation(async () => '');
     expect(await fileStore.get(cache)).toEqual(null);
+    expect(fs.promises.readFile).toHaveBeenCalledWith(expect.any(String));
   });
 
   test('writes into cache if folder is missing', async () => {
-    const fileStore = new FileStore({root: '/root'});
+    const fileStore = new FileStore<mixed>({root: '/root'});
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
     const data = Buffer.from([0xca, 0xc4, 0xe5]);
 
@@ -62,7 +60,7 @@ describe('FileStore', () => {
   });
 
   test('reads and writes binary data', async () => {
-    const fileStore = new FileStore({root: '/root'});
+    const fileStore = new FileStore<mixed>({root: '/root'});
     const cache = Buffer.from([0xfa, 0xce, 0xb0, 0x0c]);
     const data = Buffer.from([0xca, 0xc4, 0xe5]);