fix: Wait for tasks depending on sourcemaps before deleting (#557)

Author: lforst

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

@@ -24,7 +24,6 @@ interface DebugIdUploadPluginOptions {
   handleRecoverableError: (error: unknown) => void;
   sentryHub: Hub;
   sentryClient: NodeClient;
-  deleteFilesUpForDeletion: () => Promise<void>;
   sentryCliOptions: {
     url: string;
     authToken: string;
@@ -34,6 +33,7 @@ interface DebugIdUploadPluginOptions {
     silent: boolean;
     headers?: Record<string, string>;
   };
+  freeDependencyOnSourcemapFiles: () => void;
 }
 
 export function createDebugIdUploadFunction({
@@ -47,7 +47,7 @@ export function createDebugIdUploadFunction({
   sentryClient,
   sentryCliOptions,
   rewriteSourcesHook,
-  deleteFilesUpForDeletion,
+  freeDependencyOnSourcemapFiles,
 }: DebugIdUploadPluginOptions) {
   return async (buildArtifactPaths: string[]) => {
     const artifactBundleUploadTransaction = sentryHub.startTransaction({
@@ -179,8 +179,6 @@ export function createDebugIdUploadFunction({
         uploadSpan.finish();
         logger.info("Successfully uploaded source maps to Sentry");
       }
-
-      await deleteFilesUpForDeletion();
     } catch (e) {
       sentryHub.withScope((scope) => {
         scope.setSpan(artifactBundleUploadTransaction);
@@ -196,6 +194,7 @@ export function createDebugIdUploadFunction({
         cleanupSpan.finish();
       }
       artifactBundleUploadTransaction.finish();
+      freeDependencyOnSourcemapFiles();
       await safeFlushTelemetry(sentryClient);
     }
   };

Diff for: packages/bundler-plugin-core/src/index.ts

@@ -185,32 +185,46 @@ export function sentryUnpluginFactory({
       })
     );
 
-    async function deleteFilesUpForDeletion() {
-      const filesToDeleteAfterUpload =
-        options.sourcemaps?.filesToDeleteAfterUpload ?? options.sourcemaps?.deleteFilesAfterUpload;
-
-      if (filesToDeleteAfterUpload) {
-        const filePathsToDelete = await glob(filesToDeleteAfterUpload, {
-          absolute: true,
-          nodir: true,
-        });
+    // We have multiple plugins depending on generated source map files. (debug ID upload, legacy upload)
+    // Additionally, we also want to have the functionality to delete files after uploading sourcemaps.
+    // All of these plugins and the delete functionality need to run in the same hook (`writeBundle`).
+    // Since the plugins among themselves are not aware of when they run and finish, we need a system to
+    // track their dependencies on the generated files, so that we can initiate the file deletion only after
+    // nothing depends on the files anymore.
+    const dependenciesOnSourcemapFiles = new Set<symbol>();
+    const sourcemapFileDependencySubscribers: (() => void)[] = [];
+
+    function notifySourcemapFileDependencySubscribers() {
+      sourcemapFileDependencySubscribers.forEach((subscriber) => {
+        subscriber();
+      });
+    }
+
+    function createDependencyOnSourcemapFiles() {
+      const dependencyIdentifier = Symbol();
+      dependenciesOnSourcemapFiles.add(dependencyIdentifier);
 
-        filePathsToDelete.forEach((filePathToDelete) => {
-          logger.debug(`Deleting asset after upload: ${filePathToDelete}`);
+      return function freeDependencyOnSourcemapFiles() {
+        dependenciesOnSourcemapFiles.delete(dependencyIdentifier);
+        notifySourcemapFileDependencySubscribers();
+      };
+    }
+
+    /**
+     * Returns a Promise that resolves when all the currently active dependencies are freed again.
+     */
+    function waitUntilSourcemapFileDependenciesAreFreed() {
+      return new Promise<void>((resolve) => {
+        sourcemapFileDependencySubscribers.push(() => {
+          if (dependenciesOnSourcemapFiles.size === 0) {
+            resolve();
+          }
         });
 
-        await Promise.all(
-          filePathsToDelete.map((filePathToDelete) =>
-            fs.promises.rm(filePathToDelete, { force: true }).catch((e) => {
-              // This is allowed to fail - we just don't do anything
-              logger.debug(
-                `An error occurred while attempting to delete asset: ${filePathToDelete}`,
-                e
-              );
-            })
-          )
-        );
-      }
+        if (dependenciesOnSourcemapFiles.size === 0) {
+          resolve();
+        }
+      });
     }
 
     if (options.bundleSizeOptimizations) {
@@ -326,22 +340,13 @@ export function sentryUnpluginFactory({
             vcsRemote: options.release.vcsRemote,
             headers: options.headers,
           },
-          deleteFilesUpForDeletion,
+          freeDependencyOnSourcemapFiles: createDependencyOnSourcemapFiles(),
         })
       );
     }
 
     plugins.push(debugIdInjectionPlugin(logger));
 
-    plugins.push(
-      fileDeletionPlugin({
-        deleteFilesUpForDeletion,
-        handleRecoverableError,
-        sentryHub,
-        sentryClient,
-      })
-    );
-
     if (!options.authToken) {
       logger.warn(
         "No auth token provided. Will not upload source maps. Please set the `authToken` option. You can find information on how to generate a Sentry auth token here: https://docs.sentry.io/api/auth/"
@@ -360,7 +365,7 @@ export function sentryUnpluginFactory({
           createDebugIdUploadFunction({
             assets: options.sourcemaps?.assets,
             ignore: options.sourcemaps?.ignore,
-            deleteFilesUpForDeletion,
+            freeDependencyOnSourcemapFiles: createDependencyOnSourcemapFiles(),
             dist: options.release.dist,
             releaseName: options.release.name,
             logger: logger,
@@ -396,6 +401,21 @@ export function sentryUnpluginFactory({
       }
     }
 
+    plugins.push(
+      fileDeletionPlugin({
+        // It is very important that this is only called after all other dependencies have been created with `createDependencyOnSourcemapFiles`.
+        // Ideally, we always register this plugin last.
+        dependenciesAreFreedPromise: waitUntilSourcemapFileDependenciesAreFreed(),
+        filesToDeleteAfterUpload:
+          options.sourcemaps?.filesToDeleteAfterUpload ??
+          options.sourcemaps?.deleteFilesAfterUpload,
+        logger,
+        handleRecoverableError,
+        sentryHub,
+        sentryClient,
+      })
+    );
+
     return plugins;
   });
 }

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

@@ -27,7 +27,7 @@ interface ReleaseManagementPluginOptions {
     silent: boolean;
     headers?: Record<string, string>;
   };
-  deleteFilesUpForDeletion: () => Promise<void>;
+  freeDependencyOnSourcemapFiles: () => void;
 }
 
 export function releaseManagementPlugin({
@@ -42,7 +42,7 @@ export function releaseManagementPlugin({
   sentryHub,
   sentryClient,
   sentryCliOptions,
-  deleteFilesUpForDeletion,
+  freeDependencyOnSourcemapFiles,
 }: ReleaseManagementPluginOptions): UnpluginOptions {
   return {
     name: "sentry-debug-id-upload-plugin",
@@ -85,12 +85,12 @@ export function releaseManagementPlugin({
         if (deployOptions) {
           await cliInstance.releases.newDeploy(releaseName, deployOptions);
         }
-
-        await deleteFilesUpForDeletion();
       } catch (e) {
         sentryHub.captureException('Error in "releaseManagementPlugin" writeBundle hook');
         await safeFlushTelemetry(sentryClient);
         handleRecoverableError(e);
+      } finally {
+        freeDependencyOnSourcemapFiles();
       }
     },
   };

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

@@ -1,25 +1,59 @@
 import { Hub, NodeClient } from "@sentry/node";
+import { glob } from "glob";
 import { UnpluginOptions } from "unplugin";
+import { Logger } from "../sentry/logger";
 import { safeFlushTelemetry } from "../sentry/telemetry";
+import fs from "fs";
 
 interface FileDeletionPlugin {
   handleRecoverableError: (error: unknown) => void;
-  deleteFilesUpForDeletion: () => Promise<void>;
+  dependenciesAreFreedPromise: Promise<void>;
   sentryHub: Hub;
   sentryClient: NodeClient;
+  filesToDeleteAfterUpload: string | string[] | undefined;
+  logger: Logger;
 }
 
 export function fileDeletionPlugin({
   handleRecoverableError,
   sentryHub,
   sentryClient,
-  deleteFilesUpForDeletion,
+  filesToDeleteAfterUpload,
+  dependenciesAreFreedPromise,
+  logger,
 }: FileDeletionPlugin): UnpluginOptions {
   return {
     name: "sentry-file-deletion-plugin",
     async writeBundle() {
       try {
-        await deleteFilesUpForDeletion();
+        if (filesToDeleteAfterUpload !== undefined) {
+          const filePathsToDelete = await glob(filesToDeleteAfterUpload, {
+            absolute: true,
+            nodir: true,
+          });
+
+          logger.debug(
+            "Waiting for dependencies on generated files to be freed before deleting..."
+          );
+
+          await dependenciesAreFreedPromise;
+
+          filePathsToDelete.forEach((filePathToDelete) => {
+            logger.debug(`Deleting asset after upload: ${filePathToDelete}`);
+          });
+
+          await Promise.all(
+            filePathsToDelete.map((filePathToDelete) =>
+              fs.promises.rm(filePathToDelete, { force: true }).catch((e) => {
+                // This is allowed to fail - we just don't do anything
+                logger.debug(
+                  `An error occurred while attempting to delete asset: ${filePathToDelete}`,
+                  e
+                );
+              })
+            )
+          );
+        }
       } catch (e) {
         sentryHub.captureException('Error in "sentry-file-deletion-plugin" buildEnd hook');
         await safeFlushTelemetry(sentryClient);