Add a file descriptor budget (#804)

* WIP on a file budget * Add a filesystem budget This adds a wrapper around the 'fs/promises' module that will limit parallelism of filesystem operations. Larger projects (e.g. monorepos) have had crashes that look like file descriptor exhaustion. * Fix lint, minor cleanup. * Add test, pick better names, add Sempahore class * lint and format * Revert package-lock changes * Revert typescript version. * Changelog * Add license Co-authored-by: Al Marks <aomarks@gmail.com> * Add license Co-authored-by: Al Marks <aomarks@gmail.com> * Update README.md Co-authored-by: Al Marks <aomarks@gmail.com> * Format README --------- Co-authored-by: Al Marks <aomarks@gmail.com>
google · Jul 10, 2023 · b76c863 · b76c863
1 parent 28cb48b
commit b76c863
Show file tree

Hide file tree

Showing 18 changed files with 362 additions and 26 deletions.
diff --git a/.eslintrc.cjs b/.eslintrc.cjs
@@ -14,5 +14,8 @@ module.exports = {
   rules: {
     'no-constant-condition': 'off',
     'no-only-tests/no-only-tests': 'error',
+    // maybe we should turn this on, but it started suddenly triggering in
+    // an unrelated change.
+    '@typescript-eslint/no-non-null-assertion': 'off',
   },
 };
diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -1,5 +1,7 @@
 {
   "javascript.preferences.importModuleSpecifierEnding": "js",
   "typescript.preferences.importModuleSpecifierEnding": "js",
-  "editor.defaultFormatter": "esbenp.prettier-vscode"
+  "editor.defaultFormatter": "esbenp.prettier-vscode",
+  "typescript.tsdk": "node_modules\\typescript\\lib",
+  "typescript.enablePromptUseWorkspaceTsdk": true
 }
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,8 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 - Added tracking of metrics for successful script executions. Metrics are emitted
   at the end of each run where at least one successful execution occurred.
 
+- Wireit now limits its number of file descriptors. This is to prevent crashes, and the default value of 200 should be high enough not to regress performance. Set the WIREIT_MAX_OPEN_FILES env variable to override the default.
+
 ## [0.9.5] - 2023-02-06
 
 ### Changed

diff --git a/README.md b/README.md
@@ -847,12 +847,13 @@ The following syntaxes can be used in the `wireit.<script>.dependencies` array:
 
 The following environment variables affect the behavior of Wireit:
 
-| Variable          | Description                                                                                                                                                                                                                                                                                                                                                                                                                          |
-| ----------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
-| `WIREIT_FAILURES` | [How to handle script failures](#failures-and-errors).<br><br>Options:<br><ul><li>[`no-new`](#failures-and-errors) (default): Allow running scripts to finish, but don't start new ones.</li><li>[`continue`](#continue): Allow running scripts to continue, and start new ones unless any of their dependencies failed.</li><li>[`kill`](#kill): Immediately kill running scripts, and don't start new ones.</li></ul>              |
-| `WIREIT_PARALLEL` | [Maximum number of scripts to run at one time](#parallelism).<br><br>Defaults to 2×logical CPU cores.<br><br>Must be a positive integer or `infinity`.                                                                                                                                                                                                                                                                               |
-| `WIREIT_CACHE`    | [Caching mode](#caching).<br><br>Defaults to `local` unless `CI` is `true`, in which case defaults to `none`.<br><br>Automatically set to `github` by the [`google/wireit@setup-github-actions-caching/v1`](#github-actions-caching) action.<br><br>Options:<ul><li>[`local`](#local-caching): Cache to local disk.</li><li>[`github`](#github-actions-caching): Cache to GitHub Actions.</li><li>`none`: Disable caching.</li></ul> |
-| `CI`              | Affects the default value of `WIREIT_CACHE`.<br><br>Automatically set to `true` by [GitHub Actions](https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables) and most other CI (continuous integration) services.<br><br>Must be exactly `true`. If unset or any other value, interpreted as `false`.                                                                            |
+| Variable                | Description                                                                                                                                                                                                                                                                                                                                                                                                                          |
+| ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+| `WIREIT_FAILURES`       | [How to handle script failures](#failures-and-errors).<br><br>Options:<br><ul><li>[`no-new`](#failures-and-errors) (default): Allow running scripts to finish, but don't start new ones.</li><li>[`continue`](#continue): Allow running scripts to continue, and start new ones unless any of their dependencies failed.</li><li>[`kill`](#kill): Immediately kill running scripts, and don't start new ones.</li></ul>              |
+| `WIREIT_PARALLEL`       | [Maximum number of scripts to run at one time](#parallelism).<br><br>Defaults to 2×logical CPU cores.<br><br>Must be a positive integer or `infinity`.                                                                                                                                                                                                                                                                               |
+| `WIREIT_CACHE`          | [Caching mode](#caching).<br><br>Defaults to `local` unless `CI` is `true`, in which case defaults to `none`.<br><br>Automatically set to `github` by the [`google/wireit@setup-github-actions-caching/v1`](#github-actions-caching) action.<br><br>Options:<ul><li>[`local`](#local-caching): Cache to local disk.</li><li>[`github`](#github-actions-caching): Cache to GitHub Actions.</li><li>`none`: Disable caching.</li></ul> |
+| `CI`                    | Affects the default value of `WIREIT_CACHE`.<br><br>Automatically set to `true` by [GitHub Actions](https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables) and most other CI (continuous integration) services.<br><br>Must be exactly `true`. If unset or any other value, interpreted as `false`.                                                                            |
+| `WIREIT_MAX_OPEN_FILES` | Limits the number of file descriptors Wireit will have open concurrently. Prevents resource exhaustion when checking large numbers of cached files. Set to a lower number if you hit file descriptor limits.                                                                                                                                                                                                                         |
 
 ### Glob patterns
 

diff --git a/package.json b/package.json
@@ -44,6 +44,7 @@
     "test:errors-usage": "wireit",
     "test:failures": "wireit",
     "test:freshness": "wireit",
+    "test:fs": "wireit",
     "test:ide": "wireit",
     "test:gc": "wireit",
     "test:glob": "wireit",
@@ -89,6 +90,7 @@
         "test:errors-usage",
         "test:failures",
         "test:freshness",
+        "test:fs",
         "test:glob",
         "test:ide",
         "test:gc",
@@ -368,6 +370,17 @@
       ],
       "files": [],
       "output": []
+    },
+    "test:fs": {
+      "command": "uvu lib/test \"^fs\\.test\\.js$\"",
+      "env": {
+        "NODE_OPTIONS": "--enable-source-maps"
+      },
+      "dependencies": [
+        "build"
+      ],
+      "files": [],
+      "output": []
     }
   },
   "devDependencies": {

diff --git a/src/analyzer.ts b/src/analyzer.ts
@@ -5,7 +5,7 @@
  */
 
 import * as pathlib from 'path';
-import * as fs from 'fs/promises';
+import * as fs from './util/fs.js';
 import {
   CachingPackageJsonReader,
   FileSystem,

diff --git a/src/caching/github-actions-cache.ts b/src/caching/github-actions-cache.ts
@@ -5,13 +5,13 @@
  */
 
 import * as pathlib from 'path';
-import * as fs from 'fs/promises';
+import * as unbudgetedFs from 'fs/promises';
+import * as fs from '../util/fs.js';
 import * as https from 'https';
 import {createHash} from 'crypto';
 import {scriptReferenceToString} from '../config.js';
 import {getScriptDataDir} from '../util/script-data-dir.js';
 import {execFile} from 'child_process';
-import {createReadStream, createWriteStream} from 'fs';
 
 import type * as http from 'http';
 import type {Cache, CacheHit} from './cache.js';
@@ -20,6 +20,7 @@ import type {Fingerprint} from '../fingerprint.js';
 import type {Logger} from '../logging/logger.js';
 import type {AbsoluteEntry} from '../util/glob.js';
 import type {Result} from '../error.js';
+import {fileBudget} from '../util/fs.js';
 
 /**
  * Caches script output to the GitHub Actions caching service.
@@ -233,7 +234,10 @@ export class GitHubActionsCache implements Cache {
     // Reference:
     // https://github.com/actions/toolkit/blob/500d0b42fee2552ae9eeb5933091fe2fbf14e72d/packages/cache/src/options.ts#L59
     const maxChunkSize = 32 * 1024 * 1024;
-    const tarballHandle = await fs.open(tarballPath, 'r');
+    // TODO: update to TypeScript 5.2 and use the new `using` syntax for the
+    // budget object.
+    const reservation = await fileBudget.reserve();
+    const tarballHandle = await unbudgetedFs.open(tarballPath, 'r');
     let offset = 0;
     try {
       // TODO(aomarks) Chunks could be uploaded in parallel.
@@ -243,7 +247,7 @@ export class GitHubActionsCache implements Cache {
         const end = offset + chunkSize - 1;
         offset += maxChunkSize;
 
-        const tarballChunkStream = createReadStream(tarballPath, {
+        const tarballChunkStream = await fs.createReadStream(tarballPath, {
           fd: tarballHandle.fd,
           start,
           end,
@@ -282,6 +286,7 @@ export class GitHubActionsCache implements Cache {
       return true;
     } finally {
       await tarballHandle.close();
+      reservation[Symbol.dispose]();
     }
   }
 
@@ -578,8 +583,8 @@ class GitHubActionsCacheHit implements CacheHit {
         `GitHub Cache download HTTP ${String(response.statusCode)} error`
       );
     }
+    const writeTarballStream = await fs.createWriteStream(tarballPath);
     await new Promise<void>((resolve, reject) => {
-      const writeTarballStream = createWriteStream(tarballPath);
       writeTarballStream.on('error', (error) => reject(error));
       response.on('error', (error) => reject(error));
       response.pipe(writeTarballStream);

diff --git a/src/caching/local-cache.ts b/src/caching/local-cache.ts
@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as fs from 'fs/promises';
+import * as fs from '../util/fs.js';
 import * as pathlib from 'path';
 import {createHash} from 'crypto';
 import {getScriptDataDir} from '../util/script-data-dir.js';

diff --git a/src/cli-options.ts b/src/cli-options.ts
@@ -5,7 +5,7 @@
  */
 
 import * as os from 'os';
-import * as fs from 'fs/promises';
+import * as fs from './util/fs.js';
 import * as pathlib from 'path';
 import {Result} from './error.js';
 import {MetricsLogger} from './logging/metrics-logger.js';

diff --git a/src/execution/standard.ts b/src/execution/standard.ts
@@ -5,7 +5,7 @@
  */
 
 import * as pathlib from 'path';
-import * as fs from 'fs/promises';
+import * as fs from '../util/fs.js';
 import {WorkerPool} from '../util/worker-pool.js';
 import {getScriptDataDir} from '../util/script-data-dir.js';
 import {unreachable} from '../util/unreachable.js';
@@ -183,7 +183,7 @@ export class StandardScriptExecution extends BaseExecutionWithCommand<StandardSc
     // details proper-lockfile handles.
     const lockFile = pathlib.join(this._dataDir, 'lock');
     await fs.mkdir(pathlib.dirname(lockFile), {recursive: true});
-    await fs.writeFile(lockFile, '');
+    await fs.writeFile(lockFile, '', 'utf8');
     let loggedLocked = false;
     while (true) {
       try {

diff --git a/src/fingerprint.ts b/src/fingerprint.ts
@@ -5,7 +5,7 @@
  */
 
 import {createHash} from 'crypto';
-import {createReadStream} from 'fs';
+import {createReadStream} from './util/fs.js';
 import {glob} from './util/glob.js';
 import {scriptReferenceToString} from './config.js';
 
@@ -176,7 +176,7 @@ export class Fingerprint {
         files.map(async (file): Promise<[string, FileSha256HexDigest]> => {
           const absolutePath = file.path;
           const hash = createHash('sha256');
-          for await (const chunk of createReadStream(absolutePath)) {
+          for await (const chunk of await createReadStream(absolutePath)) {
             hash.update(chunk as Buffer);
           }
           return [file.path, hash.digest('hex') as FileSha256HexDigest];

diff --git a/src/ide.ts b/src/ide.ts
@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as fs from 'fs/promises';
+import * as fs from './util/fs.js';
 import {Analyzer} from './analyzer.js';
 import * as url from 'url';
 import * as pathlib from 'path';

diff --git a/src/test/fs.test.ts b/src/test/fs.test.ts
@@ -0,0 +1,42 @@
+/**
+ * @license
+ * Copyright 2023 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import {Semaphore} from '../util/fs.js';
+import {test} from 'uvu';
+import * as assert from 'uvu/assert';
+
+async function wait(ms: number): Promise<void> {
+  return new Promise((resolve) => setTimeout(resolve, ms));
+}
+
+test('Semaphore restricts resource access', async () => {
+  const semaphore = new Semaphore(1);
+  const reservation1 = await semaphore.reserve();
+  const reservation2Promise = semaphore.reserve();
+  let hasResolved = false;
+  void reservation2Promise.then(() => {
+    hasResolved = true;
+  });
+  // Wait a bit to make sure the promise has had a chance to resolve.
+  await wait(100);
+  // The semaphore doesn't let the second reservation happen yet, it would
+  // be over budget.
+  assert.is(hasResolved, false);
+  reservation1[Symbol.dispose]();
+  // Now it can happen.
+  await reservation2Promise;
+  assert.is(hasResolved, true);
+});
+
+test('Semaphore reservation happens immediately when not under contention', async () => {
+  const semaphore = new Semaphore(3);
+  await semaphore.reserve();
+  await semaphore.reserve();
+  await semaphore.reserve();
+  // If the test finishes, then we were able to reserve three slots.
+});
+
+test.run();
diff --git a/src/util/copy.ts b/src/util/copy.ts
@@ -4,10 +4,9 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as fs from 'fs/promises';
+import * as fs from './fs.js';
 import * as pathlib from 'path';
 import {optimizeMkdirs} from './optimize-mkdirs.js';
-import {constants} from 'fs';
 import {IS_WINDOWS} from '../util/windows.js';
 
 import type {AbsoluteEntry} from './glob.js';
@@ -80,7 +79,7 @@ export const copyEntries = async (
  */
 const copyFileGracefully = async (src: string, dest: string): Promise<void> => {
   try {
-    await fs.copyFile(src, dest, constants.COPYFILE_EXCL);
+    await fs.copyFile(src, dest, fs.constants.COPYFILE_EXCL);
   } catch (error) {
     const {code} = error as {code: string};
     if (code === /* does not exist */ 'ENOENT') {

diff --git a/src/util/delete.ts b/src/util/delete.ts
@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as fs from 'fs/promises';
+import * as fs from './fs.js';
 import * as pathlib from 'path';
 
 import type {AbsoluteEntry} from './glob.js';