fix: Re-work Sentry minidump loader

package.json

@@ -50,7 +50,7 @@
     "update-electron-versions": "electron-latest-versions --start 2 > ./test/e2e/versions.json",
     "update-sdk-versions": "node ./scripts/update-sdk-versions.mjs",
     "pretest": "yarn build",
-    "test": "cross-env TS_NODE_PROJECT=tsconfig.test.json xvfb-maybe electron-mocha --require ts-node/register/transpile-only --timeout 120000 ./test/unit/**/*.ts",
+    "test": "cross-env TS_NODE_PROJECT=tsconfig.test.json xvfb-maybe electron-mocha --require ts-node/register/transpile-only --timeout 12000 ./test/unit/**/*.ts",
     "pree2e": "rimraf test/e2e/dist/**/node_modules/@sentry/** test/e2e/dist/**/yarn.lock test/e2e/dist/**/package-lock.json && node scripts/clean-cache.js && yarn build && npm pack",
     "e2e": "cross-env TS_NODE_PROJECT=tsconfig.test.json xvfb-maybe mocha --require ts-node/register/transpile-only --retries 3 ./test/e2e/*.ts"
   },

src/main/integrations/sentry-minidump/index.ts

@@ -1,7 +1,7 @@
 import { captureEvent, getCurrentHub, Scope } from '@sentry/core';
 import { NodeClient } from '@sentry/node';
 import { Event, Integration } from '@sentry/types';
-import { basename, logger, SentryError } from '@sentry/utils';
+import { logger, SentryError } from '@sentry/utils';
 import { app, crashReporter } from 'electron';
 
 import { mergeEvents } from '../../../common';
@@ -12,7 +12,7 @@ import { getRendererProperties, trackRendererProperties } from '../../renderers'
 import { ElectronMainOptions } from '../../sdk';
 import { checkPreviousSession, sessionCrashed } from '../../sessions';
 import { BufferedWriteStore } from '../../store';
-import { deleteMinidump, getMinidumpLoader, MinidumpLoader } from './minidump-loader';
+import { getMinidumpLoader, MinidumpLoader } from './minidump-loader';
 
 interface PreviousRun {
   scope: Scope;
@@ -231,79 +231,51 @@ export class SentryMinidump implements Integration {
     //     about it. Just use the breadcrumbs and context information we have
     //     right now and hope that the delay was not too long.
 
-    let event: Event | null = eventIn;
-
     if (this._minidumpLoader === undefined) {
       throw new SentryError('Invariant violation: Native crashes not enabled');
     }
 
-    try {
-      const minidumps = await this._minidumpLoader();
-
-      if (minidumps.length > 0) {
-        const hub = getCurrentHub();
-        const client = hub.getClient();
-
-        if (!client) {
-          return true;
-        }
-
-        const enabled = client.getOptions().enabled;
-
-        // If the SDK is not enabled, we delete the minidump files so they
-        // don't accumulate and/or get sent later
-        if (enabled === false) {
-          minidumps.forEach(deleteMinidump);
-          return false;
-        }
-
-        // If this is a native main process crash, we need to apply the scope and context from the previous run
-        if (event?.tags?.['event.process'] === 'browser') {
-          const previousRun = await this._scopeLastRun;
-
-          const storedScope = Scope.clone(previousRun?.scope);
-          event = await storedScope.applyToEvent(event);
-
-          if (event && previousRun) {
-            event.release = previousRun.event?.release || event.release;
-            event.environment = previousRun.event?.environment || event.environment;
-            event.contexts = previousRun.event?.contexts || event.contexts;
-          }
-        }
-
-        const hubScope = hub.getScope();
-        event = hubScope && event ? await hubScope.applyToEvent(event) : event;
-
-        if (!event) {
-          return false;
-        }
-
-        for (const minidump of minidumps) {
-          const data = await minidump.load();
-
-          if (data) {
-            captureEvent(event, {
-              attachments: [
-                {
-                  attachmentType: 'event.minidump',
-                  filename: basename(minidump.path),
-                  data,
-                },
-              ],
-            });
-          }
-
-          void deleteMinidump(minidump);
-        }
-
-        // Unset to recover memory
-        this._scopeLastRun = undefined;
-        return true;
+    const hub = getCurrentHub();
+    const client = hub.getClient();
+
+    if (!client) {
+      return true;
+    }
+
+    let event: Event | null = eventIn;
+
+    // If this is a native main process crash, we need to apply the scope and context from the previous run
+    if (event.tags?.['event.process'] === 'browser') {
+      const previousRun = await this._scopeLastRun;
+
+      const storedScope = Scope.clone(previousRun?.scope);
+      event = await storedScope.applyToEvent(event);
+
+      if (event && previousRun) {
+        event.release = previousRun.event?.release || event.release;
+        event.environment = previousRun.event?.environment || event.environment;
+        event.contexts = previousRun.event?.contexts || event.contexts;
       }
-    } catch (_oO) {
-      logger.error('Error while sending native crash.');
     }
 
-    return false;
+    const hubScope = hub.getScope();
+    event = hubScope && event ? await hubScope.applyToEvent(event) : event;
+
+    if (!event) {
+      return false;
+    }
+
+    // If the SDK is not enabled, tell the loader to delete all minidumps
+    const deleteAll = client.getOptions().enabled === false;
+
+    let minidumpSent = false;
+    await this._minidumpLoader(deleteAll, (attachment) => {
+      captureEvent(event as Event, { attachments: [attachment] });
+      minidumpSent = true;
+    });
+
+    // Unset to recover memory
+    this._scopeLastRun = undefined;
+    return minidumpSent;
   }
 }

src/main/integrations/sentry-minidump/minidump-loader.ts

@@ -1,73 +1,112 @@
-import { logger } from '@sentry/utils';
+import { Attachment } from '@sentry/types';
+import { basename, logger } from '@sentry/utils';
 import { join } from 'path';
 
+import { Mutex } from '../../../common/mutex';
 import { getCrashesDirectory, usesCrashpad } from '../../electron-normalize';
 import { readDirAsync, readFileAsync, statAsync, unlinkAsync } from '../../fs';
 
 /** Maximum number of days to keep a minidump before deleting it. */
-const MAX_AGE = 30;
-
-export interface MinidumpFile {
-  path: string;
-  load(): Promise<Uint8Array | undefined>;
+const MAX_AGE_DAYS = 30;
+const MS_PER_DAY = 24 * 3_600 * 1_000;
+/** Minimum number of milliseconds a minidump should not be modified for before we assume writing is complete */
+const NOT_MODIFIED_MS = 1_000;
+const MAX_RETRY_MS = 5_000;
+const RETRY_DELAY_MS = 500;
+const MAX_RETRIES = MAX_RETRY_MS / RETRY_DELAY_MS;
+
+const MINIDUMP_HEADER = 'MDMP';
+
+function delay(ms: number): Promise<void> {
+  return new Promise((resolve) => setTimeout(resolve, ms));
 }
 
-export type MinidumpLoader = () => Promise<MinidumpFile[]>;
+/**
+ * A function that loads minidumps
+ * @param deleteAll Whether to just delete all minidumps
+ * @param callback A callback to call with the attachment ready to send
+ */
+export type MinidumpLoader = (deleteAll: boolean, callback: (attachment: Attachment) => void) => Promise<void>;
 
-async function filterAsync<T>(
-  array: T[],
-  predicate: (item: T) => Promise<boolean> | boolean,
-  thisArg?: any,
-): Promise<T[]> {
-  const verdicts = await Promise.all(array.map(predicate, thisArg));
-  return array.filter((_, index) => verdicts[index]);
-}
+/**
+ * Creates a minidump loader
+ * @param getMinidumpPaths A function that returns paths to minidumps
+ * @param preProcessFile A function that pre-processes the minidump file
+ * @returns A function to fetch minidumps
+ */
+export function createMinidumpLoader(
+  getMinidumpPaths: () => Promise<string[]>,
+  preProcessFile: (file: Buffer) => Buffer = (file) => file,
+): MinidumpLoader {
+  // The mutex protects against a whole host of reentrancy issues and race conditions.
+  const mutex = new Mutex();
+
+  return async (deleteAll, callback) => {
+    // any calls to this function will be queued and run exclusively
+    await mutex.runExclusive(async () => {
+      for (const path of await getMinidumpPaths()) {
+        try {
+          if (deleteAll) {
+            continue;
+          }
 
-/** Deletes a minidump */
-export async function deleteMinidump(dump: MinidumpFile): Promise<void> {
-  try {
-    await unlinkAsync(dump.path);
-  } catch (e) {
-    logger.warn('Could not delete', dump.path);
-  }
-}
+          logger.log('Found minidump', path);
 
-function createMinidumpLoader(fetchMinidumpsImpl: MinidumpLoader): MinidumpLoader {
-  const knownPaths: string[] = [];
+          let stats = await statAsync(path);
 
-  return async () => {
-    const minidumps = await fetchMinidumpsImpl();
-    logger.log(`Found ${minidumps.length} minidumps`);
+          const thirtyDaysAgo = new Date().getTime() - MAX_AGE_DAYS * MS_PER_DAY;
 
-    const oldestMs = new Date().getTime() - MAX_AGE * 24 * 3_600 * 1_000;
-    return filterAsync(minidumps, async (dump) => {
-      // Skip files that we have seen before
-      if (knownPaths.indexOf(dump.path) >= 0) {
-        return false;
-      }
+          if (stats.mtimeMs < thirtyDaysAgo) {
+            logger.log(`Ignoring minidump as it is over ${MAX_AGE_DAYS} days old`);
+            continue;
+          }
 
-      // Lock this minidump until we have uploaded it or an error occurs and we
-      // remove it from the file system.
-      knownPaths.push(dump.path);
+          let retries = 0;
 
-      const stats = await statAsync(dump.path);
+          while (retries <= MAX_RETRIES) {
+            const twoSecondsAgo = new Date().getTime() - NOT_MODIFIED_MS;
 
-      // We do not want to upload minidumps that have been generated before a
-      // certain threshold. Those old files can be deleted immediately.
-      const tooOld = stats.birthtimeMs < oldestMs;
-      const tooSmall = stats.size < 1024;
+            if (stats.mtimeMs < twoSecondsAgo) {
+              const file = await readFileAsync(path);
+              const data = preProcessFile(file);
 
-      if (tooSmall) {
-        logger.log('Minidump too small to be valid', dump.path);
-      }
+              if (data.length < 10_000 || data.subarray(0, 4).toString() !== MINIDUMP_HEADER) {
+                logger.warn('Dropping minidump as it appears invalid.');
+                break;
+              }
 
-      if (tooOld || tooSmall) {
-        await deleteMinidump(dump);
-        knownPaths.splice(knownPaths.indexOf(dump.path), 1);
-        return false;
-      }
+              logger.log('Sending minidump');
 
-      return true;
+              callback({
+                attachmentType: 'event.minidump',
+                filename: basename(path),
+                data,
+              });
+
+              break;
+            }
+
+            logger.log(`Waiting. Minidump has been modified in the last ${NOT_MODIFIED_MS} milliseconds.`);
+            retries += 1;
+            await delay(RETRY_DELAY_MS);
+            // update the stats
+            stats = await statAsync(path);
+          }
+
+          if (retries >= MAX_RETRIES) {
+            logger.warn('Timed out waiting for minidump to stop being modified');
+          }
+        } catch (e) {
+          logger.error('Failed to load minidump', e);
+        } finally {
+          // We always attempt to delete the minidump
+          try {
+            await unlinkAsync(path);
+          } catch (e) {
+            logger.warn('Could not delete minidump', path);
+          }
+        }
+      }
     });
   };
 }
@@ -119,32 +158,24 @@ function crashpadMinidumpLoader(): MinidumpLoader {
 
   return createMinidumpLoader(async () => {
     await deleteCrashpadMetadataFile(crashesDirectory).catch((error) => logger.error(error));
-
     const files = await readDirsAsync(dumpDirectories);
-    return files
-      .filter((file) => file.endsWith('.dmp'))
-      .map((path) => {
-        return {
-          path,
-          load: () => readFileAsync(path),
-        };
-      });
+    return files.filter((file) => file.endsWith('.dmp'));
   });
 }
 
 /** Crudely parses the minidump from the Breakpad multipart file */
-function minidumpFromBreakpadMultipart(file: Buffer): Buffer | undefined {
+function minidumpFromBreakpadMultipart(file: Buffer): Buffer {
   const binaryStart = file.lastIndexOf('Content-Type: application/octet-stream');
   if (binaryStart > 0) {
-    const dumpStart = file.indexOf('MDMP', binaryStart);
+    const dumpStart = file.indexOf(MINIDUMP_HEADER, binaryStart);
     const dumpEnd = file.lastIndexOf('----------------------------');
 
     if (dumpStart > 0 && dumpEnd > 0 && dumpEnd > dumpStart) {
       return file.subarray(dumpStart, dumpEnd);
     }
   }
 
-  return undefined;
+  return file;
 }
 
 function removeBreakpadMetadata(crashesDirectory: string, paths: string[]): void {
@@ -170,24 +201,9 @@ function breakpadMinidumpLoader(): MinidumpLoader {
     // Breakpad stores all minidump files along with a metadata file directly in
     // the crashes directory.
     const files = await readDirAsync(crashesDirectory);
-
     removeBreakpadMetadata(crashesDirectory, files);
-
-    return files
-      .filter((file) => file.endsWith('.dmp'))
-      .map((file) => {
-        const path = join(crashesDirectory, file);
-
-        return {
-          path,
-          load: async () => {
-            const file = await readFileAsync(path);
-            return minidumpFromBreakpadMultipart(file) || file;
-          },
-        };
-      })
-      .filter((m) => !!m);
-  });
+    return files.filter((file) => file.endsWith('.dmp')).map((file) => join(crashesDirectory, file));
+  }, minidumpFromBreakpadMultipart);
 }
 
 /**

test/e2e/context.ts

@@ -136,7 +136,7 @@ export class TestContext {
   public async waitForTrue(
     method: () => boolean | Promise<boolean>,
     message: () => string = () => 'Timeout',
-    timeout: number = 8_000,
+    timeout: number = 12_000,
   ): Promise<void> {
     if (!this.mainProcess) {
       throw new Error('Invariant violation: Call .start() first');