fix: limit parallel file reads inside walk to 100

spec/util-walk.spec.ts

@@ -0,0 +1,156 @@
+import { mkdtemp, rm, mkdir, writeFile } from 'node:fs/promises';
+import os from 'node:os';
+import path from 'node:path';
+
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { existsSync } from 'node:fs';
+
+/**
+ * `walk` opens a FD on every single file, recursively, in the directory it walks.
+ * this makes it really easy to hit EMFILE for very large folders.
+ * This test asserts that we never hold more files than a fixed limit.
+ */
+const binaryCheck = vi.hoisted(() => ({
+  openFiles: 0,
+  peakOpenFiles: 0,
+  calls: 0,
+}));
+
+vi.mock('isbinaryfile', () => ({
+  isBinaryFile: async (filePath: string) => {
+    binaryCheck.openFiles += 1;
+    binaryCheck.calls += 1;
+    binaryCheck.peakOpenFiles = Math.max(binaryCheck.peakOpenFiles, binaryCheck.openFiles);
+
+    // Hold the FD for at least one tick of the event loop
+    await new Promise((resolve) => setTimeout(resolve, 1));
+    binaryCheck.openFiles -= 1;
+    return path.extname(filePath) === '.bin';
+  },
+}));
+
+const {
+  PromiseParallelismLimiter: ConcurrencyLimiter,
+  walk,
+  MAX_OPEN_FILE_DESCRIPTORS,
+} = await import('../src/util.js');
+
+let testWorkingDir: string;
+
+beforeEach(async () => {
+  binaryCheck.openFiles = 0;
+  binaryCheck.peakOpenFiles = 0;
+  binaryCheck.calls = 0;
+  testWorkingDir = await mkdtemp(path.join(os.tmpdir(), 'util-walk-test-workdir-'));
+});
+
+// cleanup tmp working dirs after each test is done
+afterEach(async () => {
+  await rm(testWorkingDir, { recursive: true, force: true });
+});
+
+async function writeFiles(dirPath: string, names: string[]) {
+  await mkdir(dirPath, { recursive: true });
+  for (const name of names) {
+    await writeFile(path.join(dirPath, name), 'x');
+  }
+}
+
+describe('ConcurrencyLimiter', () => {
+  it('limits promise parallelism to its passed limit', async () => {
+    const limiter = new ConcurrencyLimiter(3);
+    let runningPromises = 0;
+    let peakRunningPromises = 0;
+
+    await Promise.all(
+      Array.from({ length: 50 }, () =>
+        limiter.run(async () => {
+          runningPromises += 1;
+          peakRunningPromises = Math.max(peakRunningPromises, runningPromises);
+          await new Promise((resolve) => setTimeout(resolve, 1));
+          runningPromises -= 1;
+        }),
+      ),
+    );
+
+    expect(peakRunningPromises).toBe(3);
+    expect(runningPromises).toBe(0);
+  });
+
+  it('relases a parallelism slot if as running promises rejects', async () => {
+    const limiter = new ConcurrencyLimiter(1);
+
+    const rejectMessage = 'promise rejected';
+    await expect(limiter.run(() => Promise.reject(new Error(rejectMessage)))).rejects.toThrow(
+      rejectMessage,
+    );
+
+    const result = 'continues to run after one promise in the queue rejected';
+
+    await expect(limiter.run(async () => result)).resolves.toBe(result);
+  });
+});
+
+describe('walk', () => {
+  it('has no more than 100 files open at the same time', async () => {
+    const fileCount = 750;
+    await writeFiles(
+      path.join(testWorkingDir, 'Resources'),
+      Array.from({ length: fileCount }, (_unused, index) => `file-${index}.bin`),
+    );
+
+    const result = await walk(testWorkingDir);
+
+    expect(binaryCheck.calls).toBe(fileCount);
+    expect(binaryCheck.peakOpenFiles).toBeLessThanOrEqual(fileCount);
+    expect(result).toHaveLength(fileCount);
+  });
+
+  it('parallelizes reads up to the limit', async () => {
+    await writeFiles(
+      path.join(testWorkingDir, 'Resources'),
+      Array.from({ length: 750 }, (_unused, index) => `file-${index}.bin`),
+    );
+
+    await walk(testWorkingDir);
+
+    expect(binaryCheck.peakOpenFiles).toBe(750);
+  });
+
+  it("doesn't deadlock on deep tree", async () => {
+    // since walking each directory awaits Promise.all(walk([its children])
+    // make sure going deep doesn't deadlock
+    const treeCDepth = MAX_OPEN_FILE_DESCRIPTORS + 50;
+    let deepPath = testWorkingDir;
+    for (let level = 0; level < treeCDepth; level += 1) {
+      deepPath = path.join(deepPath, `level-${level}`);
+    }
+    await writeFiles(deepPath, ['deep.bin']);
+
+    const result = await walk(testWorkingDir);
+
+    expect(result).toEqual([path.join(deepPath, 'deep.bin')]);
+  });
+
+  it("doesn't mess with result values or order", async () => {
+    const contents = path.join(testWorkingDir, 'Contents');
+    await writeFiles(path.join(contents, 'MacOS'), ['Foo.bin', 'readme.txt']);
+    await writeFiles(path.join(contents, 'Frameworks', 'Foo.framework'), ['Foo.bin']);
+    await writeFiles(path.join(contents, 'Frameworks', 'Foo Helper.app'), ['Helper.bin']);
+    await writeFiles(path.join(contents, 'Resources'), ['icon.icns.cstemp']);
+
+    const result = await walk(testWorkingDir);
+
+    expect(new Set(result)).toEqual(
+      new Set([
+        path.join(contents, 'MacOS', 'Foo.bin'),
+        path.join(contents, 'Frameworks', 'Foo.framework', 'Foo.bin'),
+        path.join(contents, 'Frameworks', 'Foo.framework'),
+        path.join(contents, 'Frameworks', 'Foo Helper.app', 'Helper.bin'),
+        path.join(contents, 'Frameworks', 'Foo Helper.app'),
+      ]),
+    );
+
+    expect(existsSync(path.join(contents, 'Resources', 'icon.icns.cstemp'))).toBe(false);
+  });
+});

src/util.ts

@@ -47,6 +47,43 @@ export async function execFileAsync(
 type DeepListItem<T> = null | T | DeepListItem<T>[];
 type DeepList<T> = DeepListItem<T>[];
 
+/**
+ * A queue of promises, never running more than `limit` at the same time.
+ * @internal
+ */
+export class PromiseParallelismLimiter {
+  private numRunningPromsies = 0;
+  private readonly waitingPromises: (() => void)[] = [];
+
+  constructor(private maxRunningPromises: number) {
+    if (maxRunningPromises < 1) {
+      throw new Error('parallelism limit < 1 would never run anything.');
+    }
+  }
+
+  async run<T>(promise: () => Promise<T>): Promise<T> {
+      if(this.numRunningPromsies > this.maxRunningPromises) {
+      await new Promise<void>((resolve) => {
+        this.waitingPromises.push(resolve);
+      });
+    }
+
+    try {
+      // we have to await, lest the finally fire instantly
+      return await promise();
+    } finally {
+      this.numRunningPromsies--
+
+      // grab the next promise in line.
+      // if we have space, run it right now.
+      const next = this.waitingPromises.shift();
+      if (next) {
+        next();
+      }
+    }
+  }
+}
+
 export function compactFlattenedList<T>(list: DeepList<T>): T[] {
   const result: T[] = [];
 
@@ -127,6 +164,14 @@ export async function validateOptsPlatform(opts: BaseSignOptions): Promise<Elect
   return await detectElectronPlatform(opts);
 }
 
+// The max number of file handles we're allowed to hold open at once
+// `walk` opens every file in we want to sign to check if it's a binary.
+// Every single open call holds a file descriptor. and we quickly hit ulimit,
+// thus throwing an EMFILE, if you have a very lakge set of files to sign.
+// 100 is a very safe number of files to hold open concurrently.
+// It's higher than basically every ulimit.
+export const MAX_OPEN_FILE_DESCRIPTORS = 100;
+
 /**
  * This function returns a promise resolving all child paths within the directory specified.
  *
@@ -137,21 +182,26 @@ export async function validateOptsPlatform(opts: BaseSignOptions): Promise<Elect
 export async function walk(dirPath: string): Promise<string[]> {
   debugLog('Walking... ' + dirPath);
 
+  // A directory waits for its children, so  we make the limiter limit by
+  // file system operations, instead of redccursive _walkAsync calls,
+  // beacuse those could deadlock on deep directory trees.
+  const limiter = new PromiseParallelismLimiter(MAX_OPEN_FILE_DESCRIPTORS);
+
   async function _walkAsync(dirPath: string): Promise<DeepList<string>> {
-    const children = await fs.promises.readdir(dirPath);
+    const children = await limiter.run(() => fs.promises.readdir(dirPath));
     return await Promise.all(
       children.map(async (child) => {
         const filePath = path.resolve(dirPath, child);
 
-        const stat = await fs.promises.lstat(filePath);
+        const stat = await limiter.run(() => fs.promises.lstat(filePath));
         if (stat.isFile()) {
           switch (path.extname(filePath)) {
             case '.cstemp': // Temporary file generated from past codesign
               debugLog('Removing... ' + filePath);
-              await fs.promises.rm(filePath, { recursive: true, force: true });
+              await limiter.run(() => fs.promises.rm(filePath, { recursive: true, force: true }));
               return null;
             default:
-              return await getFilePathIfBinary(filePath);
+              return await limiter.run(() => getFilePathIfBinary(filePath));
           }
         } else if (stat.isDirectory() && !stat.isSymbolicLink()) {
           const walkResult = await _walkAsync(filePath);