fix(core): Don't crash on recoverable CLI command error (#682)

Author: Lms24

packages/bundler-plugin-core/src/debug-id-upload.ts

@@ -12,6 +12,7 @@ import { stripQueryAndHashFromPath } from "./utils";
 import { setMeasurement, spanToTraceHeader, startSpan } from "@sentry/core";
 import { getDynamicSamplingContextFromSpan, Scope } from "@sentry/core";
 import { Client } from "@sentry/types";
+import { HandleRecoverableErrorFn } from "./types";
 
 interface RewriteSourcesHook {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -25,7 +26,7 @@ interface DebugIdUploadPluginOptions {
   releaseName?: string;
   dist?: string;
   rewriteSourcesHook?: RewriteSourcesHook;
-  handleRecoverableError: (error: unknown) => void;
+  handleRecoverableError: HandleRecoverableErrorFn;
   sentryScope: Scope;
   sentryClient: Client;
   sentryCliOptions: {
@@ -167,7 +168,7 @@ export function createDebugIdUploadFunction({
                   });
 
                   await cliInstance.releases.uploadSourceMaps(
-                    releaseName ?? "undefined", // unfortunetly this needs a value for now but it will not matter since debug IDs overpower releases anyhow
+                    releaseName ?? "undefined", // unfortunately this needs a value for now but it will not matter since debug IDs overpower releases anyhow
                     {
                       include: [
                         {
@@ -187,7 +188,7 @@ export function createDebugIdUploadFunction({
           }
         } catch (e) {
           sentryScope.captureException('Error in "debugIdUploadPlugin" writeBundle hook');
-          handleRecoverableError(e);
+          handleRecoverableError(e, false);
         } finally {
           if (folderToCleanUp) {
             void startSpan({ name: "cleanup", scope: sentryScope }, async () => {

packages/bundler-plugin-core/src/index.ts

@@ -155,7 +155,16 @@ export function sentryUnpluginFactory({
       "SENTRY_PIPELINE"
     ] = `${unpluginMetaContext.framework}-plugin/${__PACKAGE_VERSION__}`;
 
-    function handleRecoverableError(unknownError: unknown) {
+    /**
+     * Handles errors caught and emitted in various areas of the plugin.
+     *
+     * Also sets the sentry session status according to the error handling.
+     *
+     * If users specify their custom `errorHandler` we'll leave the decision to throw
+     * or continue up to them. By default, @param throwByDefault controls if the plugin
+     * should throw an error (which causes a build fail in most bundlers) or continue.
+     */
+    function handleRecoverableError(unknownError: unknown, throwByDefault: boolean) {
       sentrySession.status = "abnormal";
       try {
         if (options.errorHandler) {
@@ -170,17 +179,24 @@ export function sentryUnpluginFactory({
             throw e;
           }
         } else {
+          // setting the session to "crashed" b/c from a plugin perspective this run failed.
+          // However, we're intentionally not rethrowing the error to avoid breaking the user build.
           sentrySession.status = "crashed";
-          throw unknownError;
+          if (throwByDefault) {
+            throw unknownError;
+          }
+          logger.error("An error occurred. Couldn't finish all operations:", unknownError);
         }
       } finally {
         endSession();
       }
     }
 
     if (!validateOptions(options, logger)) {
+      // Throwing by default to avoid a misconfigured plugin going unnoticed.
       handleRecoverableError(
-        new Error("Options were not set correctly. See output above for more details.")
+        new Error("Options were not set correctly. See output above for more details."),
+        true
       );
     }
 

packages/bundler-plugin-core/src/plugins/release-management.ts

@@ -3,7 +3,7 @@ import { Scope } from "@sentry/core";
 import { UnpluginOptions } from "unplugin";
 import { Logger } from "../sentry/logger";
 import { safeFlushTelemetry } from "../sentry/telemetry";
-import { IncludeEntry } from "../types";
+import { HandleRecoverableErrorFn, IncludeEntry } from "../types";
 import { arrayify } from "../utils";
 import { Client } from "@sentry/types";
 
@@ -16,7 +16,7 @@ interface ReleaseManagementPluginOptions {
   setCommitsOption?: SentryCliCommitsOptions;
   deployOptions?: SentryCliNewDeployOptions;
   dist?: string;
-  handleRecoverableError: (error: unknown) => void;
+  handleRecoverableError: HandleRecoverableErrorFn;
   sentryScope: Scope;
   sentryClient: Client;
   sentryCliOptions: {
@@ -100,7 +100,7 @@ export function releaseManagementPlugin({
       } catch (e) {
         sentryScope.captureException('Error in "releaseManagementPlugin" writeBundle hook');
         await safeFlushTelemetry(sentryClient);
-        handleRecoverableError(e);
+        handleRecoverableError(e, false);
       } finally {
         freeGlobalDependencyOnSourcemapFiles();
         freeWriteBundleInvocationDependencyOnSourcemapFiles();

packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts

@@ -5,9 +5,10 @@ import { safeFlushTelemetry } from "../sentry/telemetry";
 import fs from "fs";
 import { Scope } from "@sentry/core";
 import { Client } from "@sentry/types";
+import { HandleRecoverableErrorFn } from "../types";
 
 interface FileDeletionPlugin {
-  handleRecoverableError: (error: unknown) => void;
+  handleRecoverableError: HandleRecoverableErrorFn;
   waitUntilSourcemapFileDependenciesAreFreed: () => Promise<void>;
   sentryScope: Scope;
   sentryClient: Client;
@@ -59,7 +60,9 @@ export function fileDeletionPlugin({
       } catch (e) {
         sentryScope.captureException('Error in "sentry-file-deletion-plugin" buildEnd hook');
         await safeFlushTelemetry(sentryClient);
-        handleRecoverableError(e);
+        // We throw by default if we get here b/c not being able to delete
+        // source maps could leak them to production
+        handleRecoverableError(e, true);
       }
     },
   };

packages/bundler-plugin-core/src/types.ts

@@ -552,3 +552,5 @@ type DeployOptions = {
    */
   url?: string;
 };
+
+export type HandleRecoverableErrorFn = (error: unknown, throwByDefault: boolean) => void;

packages/integration-tests/fixtures/errorhandling/build-esbuild.ts

@@ -0,0 +1,18 @@
+import { sentryEsbuildPlugin } from "@sentry/esbuild-plugin";
+import { build } from "esbuild";
+import pluginOptions from "./plugin-options";
+import path from "path";
+
+build({
+  entryPoints: [path.join(__dirname, "input", "bundle.js")],
+  outdir: path.join(__dirname, "out", "esbuild"),
+  plugins: [sentryEsbuildPlugin(pluginOptions)],
+  minify: true,
+  bundle: true,
+  format: "cjs",
+  sourcemap: true,
+}).catch((e) => {
+  // eslint-disable-next-line no-console
+  console.error(e);
+  process.exit(1);
+});

packages/integration-tests/fixtures/errorhandling/build-rollup.ts

@@ -0,0 +1,24 @@
+import { sentryRollupPlugin } from "@sentry/rollup-plugin";
+import * as path from "path";
+import * as rollup from "rollup";
+import pluginOptions from "./plugin-options";
+
+rollup
+  .rollup({
+    input: {
+      index: path.join(__dirname, "input", "bundle.js"),
+    },
+    plugins: [sentryRollupPlugin(pluginOptions)],
+  })
+  .then((bundle) =>
+    bundle.write({
+      dir: path.join(__dirname, "out", "rollup"),
+      format: "cjs",
+      exports: "named",
+    })
+  )
+  .catch((e) => {
+    // eslint-disable-next-line no-console
+    console.error(e);
+    process.exit(1);
+  });

packages/integration-tests/fixtures/errorhandling/build-vite.ts

@@ -0,0 +1,27 @@
+import { sentryVitePlugin } from "@sentry/vite-plugin";
+import * as path from "path";
+import * as vite from "vite";
+import pluginOptions from "./plugin-options";
+
+vite
+  .build({
+    clearScreen: false,
+    build: {
+      outDir: path.join(__dirname, "out", "vite"),
+      rollupOptions: {
+        input: {
+          index: path.join(__dirname, "input", "bundle.js"),
+        },
+        output: {
+          format: "cjs",
+          entryFileNames: "[name].js",
+        },
+      },
+    },
+    plugins: [sentryVitePlugin(pluginOptions)],
+  })
+  .catch((e) => {
+    // eslint-disable-next-line no-console
+    console.error(e);
+    process.exit(1);
+  });

packages/integration-tests/fixtures/errorhandling/build-webpack4.ts

@@ -0,0 +1,26 @@
+import { sentryWebpackPlugin } from "@sentry/webpack-plugin";
+import * as path from "path";
+import { default as webpack4 } from "webpack4";
+import pluginOptions from "./plugin-options";
+
+webpack4(
+  {
+    devtool: "source-map",
+    cache: false,
+    entry: {
+      index: path.join(__dirname, "input", "bundle.js"),
+    },
+    output: {
+      path: path.join(__dirname, "out", "webpack4"),
+      libraryTarget: "commonjs",
+    },
+    mode: "production",
+    target: "node", // needed for webpack 4 so we can access node api
+    plugins: [sentryWebpackPlugin(pluginOptions)],
+  },
+  (err) => {
+    if (err) {
+      process.exit(1);
+    }
+  }
+);

packages/integration-tests/fixtures/errorhandling/build-webpack5.ts

@@ -0,0 +1,27 @@
+import { sentryWebpackPlugin } from "@sentry/webpack-plugin";
+import * as path from "path";
+import { webpack as webpack5 } from "webpack5";
+import pluginOptions from "./plugin-options";
+
+webpack5(
+  {
+    devtool: "source-map",
+    cache: false,
+    entry: {
+      index: path.join(__dirname, "input", "bundle.js"),
+    },
+    output: {
+      path: path.join(__dirname, "out", "webpack5"),
+      library: {
+        type: "commonjs",
+      },
+    },
+    mode: "production",
+    plugins: [sentryWebpackPlugin(pluginOptions)],
+  },
+  (err) => {
+    if (err) {
+      process.exit(1);
+    }
+  }
+);

packages/integration-tests/fixtures/errorhandling/error-no-handler.test.ts

@@ -0,0 +1,60 @@
+/* eslint-disable jest/no-standalone-expect */
+/* eslint-disable jest/expect-expect */
+import path from "path";
+import { spawn } from "child_process";
+
+jest.setTimeout(10_000);
+
+describe("Error throwing by default (no errorHandler)", () => {
+  const FAKE_SENTRY_PORT = "9876";
+
+  const sentryServer = spawn("node", [path.join(__dirname, "fakeSentry.js")], {
+    cwd: __dirname,
+    stdio: "ignore", // <-- set to "inherit" to get server logs. Deactivated to avoid test logs.
+    env: { ...process.env, FAKE_SENTRY_PORT },
+    shell: true,
+  });
+
+  beforeAll(async () => {
+    await new Promise<void>((resolve) =>
+      sentryServer.on("spawn", () => {
+        resolve();
+      })
+    );
+  });
+
+  afterAll(() => {
+    sentryServer.kill();
+  });
+
+  const bundlersToTest = ["vite", "rollup", "webpack5", "esbuild"];
+  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+  if (parseInt(process.version.split(".")[0]!.slice(1)) < 18) {
+    bundlersToTest.push("webpack4");
+  }
+
+  test.each(bundlersToTest)(
+    "doesn't throw when Sentry server responds with error code for %s",
+    async (bundler) => {
+      const buildProcess = spawn("yarn", ["ts-node", path.join(__dirname, `build-${bundler}.ts`)], {
+        env: { ...process.env, FAKE_SENTRY_PORT },
+        stdio: "ignore", // <-- set to "inherit" to get build output. Deactivated to avoid spamming test logs.
+        shell: true,
+      });
+
+      const exitCode = await new Promise<number>((resolve, reject) => {
+        buildProcess.on("exit", (code) => {
+          resolve(code ?? 99);
+        });
+
+        buildProcess.on("error", (err) => {
+          reject(err);
+        });
+      });
+
+      expect(exitCode).toBe(0);
+
+      buildProcess.kill();
+    }
+  );
+});

packages/integration-tests/fixtures/errorhandling/fakeSentry.js

@@ -0,0 +1,16 @@
+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const { createServer } = require("http");
+
+const port = process.env["FAKE_SENTRY_PORT"] || 3000;
+
+const server = createServer((req, res) => {
+  // eslint-disable-next-line no-console
+  console.log("[SANTRY] incoming request", req.url);
+  res.statusCode = 503;
+  res.end("Error: Santry unreachable");
+});
+
+server.listen(port, () => {
+  // eslint-disable-next-line no-console
+  console.log(`[SANTRY] running on http://localhost:${port}/`);
+});

packages/integration-tests/fixtures/errorhandling/input/bundle.js

@@ -0,0 +1,2 @@
+// eslint-disable-next-line no-console
+console.log("whatever");

packages/integration-tests/fixtures/errorhandling/plugin-options.ts

@@ -0,0 +1,10 @@
+export default {
+  url: `http://localhost:${process.env["FAKE_SENTRY_PORT"] || 3000}`,
+  authToken: "fake-auth",
+  org: "fake-org",
+  project: "fake-project",
+  release: {
+    name: "1.0.0",
+  },
+  debug: true,
+};