From 0b7716342e74ef6e24f89860db9975a6480c452b Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 24 Jun 2024 17:04:32 +0200 Subject: [PATCH 1/2] fix: Wait for tasks depending on sourcemaps before deleting --- .../src/debug-id-upload.ts | 7 ++- packages/bundler-plugin-core/src/index.ts | 52 +++++++++++++++---- .../src/plugins/release-management.ts | 8 +-- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/packages/bundler-plugin-core/src/debug-id-upload.ts b/packages/bundler-plugin-core/src/debug-id-upload.ts index 9f38a371..a49c91f9 100644 --- a/packages/bundler-plugin-core/src/debug-id-upload.ts +++ b/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; sentryCliOptions: { url: string; authToken: string; @@ -34,6 +33,7 @@ interface DebugIdUploadPluginOptions { silent: boolean; headers?: Record; }; + completeTaskDependingOnSourcemaps: () => void; } export function createDebugIdUploadFunction({ @@ -47,7 +47,7 @@ export function createDebugIdUploadFunction({ sentryClient, sentryCliOptions, rewriteSourcesHook, - deleteFilesUpForDeletion, + completeTaskDependingOnSourcemaps, }: 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(); + completeTaskDependingOnSourcemaps(); await safeFlushTelemetry(sentryClient); } }; diff --git a/packages/bundler-plugin-core/src/index.ts b/packages/bundler-plugin-core/src/index.ts index 2780524c..36515075 100644 --- a/packages/bundler-plugin-core/src/index.ts +++ b/packages/bundler-plugin-core/src/index.ts @@ -185,7 +185,37 @@ export function sentryUnpluginFactory({ }) ); + const tasks = new Set(); + const subscribers: (() => void)[] = []; + + function notifySubscribers() { + subscribers.forEach((subscriber) => { + subscriber(); + }); + } + + function createTaskDependingOnSourcemaps() { + const taskIdentifier = Symbol(); + tasks.add(taskIdentifier); + return function completeTaskDependingOnSourcemaps() { + tasks.delete(taskIdentifier); + notifySubscribers(); + }; + } + async function deleteFilesUpForDeletion() { + await new Promise((resolve) => { + subscribers.push(() => { + if (tasks.size === 0) { + resolve(); + } + }); + + if (tasks.size === 0) { + resolve(); + } + }); + const filesToDeleteAfterUpload = options.sourcemaps?.filesToDeleteAfterUpload ?? options.sourcemaps?.deleteFilesAfterUpload; @@ -326,22 +356,13 @@ export function sentryUnpluginFactory({ vcsRemote: options.release.vcsRemote, headers: options.headers, }, - deleteFilesUpForDeletion, + completeTaskDependingOnSourcemaps: createTaskDependingOnSourcemaps(), }) ); } 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 +381,7 @@ export function sentryUnpluginFactory({ createDebugIdUploadFunction({ assets: options.sourcemaps?.assets, ignore: options.sourcemaps?.ignore, - deleteFilesUpForDeletion, + completeTaskDependingOnSourcemaps: createTaskDependingOnSourcemaps(), dist: options.release.dist, releaseName: options.release.name, logger: logger, @@ -396,6 +417,15 @@ export function sentryUnpluginFactory({ } } + plugins.push( + fileDeletionPlugin({ + deleteFilesUpForDeletion, + handleRecoverableError, + sentryHub, + sentryClient, + }) + ); + return plugins; }); } diff --git a/packages/bundler-plugin-core/src/plugins/release-management.ts b/packages/bundler-plugin-core/src/plugins/release-management.ts index acf73870..d1c90d4b 100644 --- a/packages/bundler-plugin-core/src/plugins/release-management.ts +++ b/packages/bundler-plugin-core/src/plugins/release-management.ts @@ -27,7 +27,7 @@ interface ReleaseManagementPluginOptions { silent: boolean; headers?: Record; }; - deleteFilesUpForDeletion: () => Promise; + completeTaskDependingOnSourcemaps: () => void; } export function releaseManagementPlugin({ @@ -42,7 +42,7 @@ export function releaseManagementPlugin({ sentryHub, sentryClient, sentryCliOptions, - deleteFilesUpForDeletion, + completeTaskDependingOnSourcemaps, }: 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 { + completeTaskDependingOnSourcemaps(); } }, }; From e35e0d072cb0ed8bd47d66eb2aeed8873ce8fb70 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 25 Jun 2024 10:20:38 +0200 Subject: [PATCH 2/2] Clean up --- .../src/debug-id-upload.ts | 6 +- packages/bundler-plugin-core/src/index.ts | 80 ++++++++----------- .../src/plugins/release-management.ts | 6 +- .../src/plugins/sourcemap-deletion.ts | 40 +++++++++- 4 files changed, 78 insertions(+), 54 deletions(-) diff --git a/packages/bundler-plugin-core/src/debug-id-upload.ts b/packages/bundler-plugin-core/src/debug-id-upload.ts index a49c91f9..7ff8f226 100644 --- a/packages/bundler-plugin-core/src/debug-id-upload.ts +++ b/packages/bundler-plugin-core/src/debug-id-upload.ts @@ -33,7 +33,7 @@ interface DebugIdUploadPluginOptions { silent: boolean; headers?: Record; }; - completeTaskDependingOnSourcemaps: () => void; + freeDependencyOnSourcemapFiles: () => void; } export function createDebugIdUploadFunction({ @@ -47,7 +47,7 @@ export function createDebugIdUploadFunction({ sentryClient, sentryCliOptions, rewriteSourcesHook, - completeTaskDependingOnSourcemaps, + freeDependencyOnSourcemapFiles, }: DebugIdUploadPluginOptions) { return async (buildArtifactPaths: string[]) => { const artifactBundleUploadTransaction = sentryHub.startTransaction({ @@ -194,7 +194,7 @@ export function createDebugIdUploadFunction({ cleanupSpan.finish(); } artifactBundleUploadTransaction.finish(); - completeTaskDependingOnSourcemaps(); + freeDependencyOnSourcemapFiles(); await safeFlushTelemetry(sentryClient); } }; diff --git a/packages/bundler-plugin-core/src/index.ts b/packages/bundler-plugin-core/src/index.ts index 36515075..8f91ced2 100644 --- a/packages/bundler-plugin-core/src/index.ts +++ b/packages/bundler-plugin-core/src/index.ts @@ -185,62 +185,46 @@ export function sentryUnpluginFactory({ }) ); - const tasks = new Set(); - const subscribers: (() => void)[] = []; - - function notifySubscribers() { - subscribers.forEach((subscriber) => { + // 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(); + const sourcemapFileDependencySubscribers: (() => void)[] = []; + + function notifySourcemapFileDependencySubscribers() { + sourcemapFileDependencySubscribers.forEach((subscriber) => { subscriber(); }); } - function createTaskDependingOnSourcemaps() { - const taskIdentifier = Symbol(); - tasks.add(taskIdentifier); - return function completeTaskDependingOnSourcemaps() { - tasks.delete(taskIdentifier); - notifySubscribers(); + function createDependencyOnSourcemapFiles() { + const dependencyIdentifier = Symbol(); + dependenciesOnSourcemapFiles.add(dependencyIdentifier); + + return function freeDependencyOnSourcemapFiles() { + dependenciesOnSourcemapFiles.delete(dependencyIdentifier); + notifySourcemapFileDependencySubscribers(); }; } - async function deleteFilesUpForDeletion() { - await new Promise((resolve) => { - subscribers.push(() => { - if (tasks.size === 0) { + /** + * Returns a Promise that resolves when all the currently active dependencies are freed again. + */ + function waitUntilSourcemapFileDependenciesAreFreed() { + return new Promise((resolve) => { + sourcemapFileDependencySubscribers.push(() => { + if (dependenciesOnSourcemapFiles.size === 0) { resolve(); } }); - if (tasks.size === 0) { + if (dependenciesOnSourcemapFiles.size === 0) { resolve(); } }); - - const filesToDeleteAfterUpload = - options.sourcemaps?.filesToDeleteAfterUpload ?? options.sourcemaps?.deleteFilesAfterUpload; - - if (filesToDeleteAfterUpload) { - const filePathsToDelete = await glob(filesToDeleteAfterUpload, { - absolute: true, - nodir: true, - }); - - 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 - ); - }) - ) - ); - } } if (options.bundleSizeOptimizations) { @@ -356,7 +340,7 @@ export function sentryUnpluginFactory({ vcsRemote: options.release.vcsRemote, headers: options.headers, }, - completeTaskDependingOnSourcemaps: createTaskDependingOnSourcemaps(), + freeDependencyOnSourcemapFiles: createDependencyOnSourcemapFiles(), }) ); } @@ -381,7 +365,7 @@ export function sentryUnpluginFactory({ createDebugIdUploadFunction({ assets: options.sourcemaps?.assets, ignore: options.sourcemaps?.ignore, - completeTaskDependingOnSourcemaps: createTaskDependingOnSourcemaps(), + freeDependencyOnSourcemapFiles: createDependencyOnSourcemapFiles(), dist: options.release.dist, releaseName: options.release.name, logger: logger, @@ -419,7 +403,13 @@ export function sentryUnpluginFactory({ plugins.push( fileDeletionPlugin({ - deleteFilesUpForDeletion, + // 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, diff --git a/packages/bundler-plugin-core/src/plugins/release-management.ts b/packages/bundler-plugin-core/src/plugins/release-management.ts index d1c90d4b..d984cc4e 100644 --- a/packages/bundler-plugin-core/src/plugins/release-management.ts +++ b/packages/bundler-plugin-core/src/plugins/release-management.ts @@ -27,7 +27,7 @@ interface ReleaseManagementPluginOptions { silent: boolean; headers?: Record; }; - completeTaskDependingOnSourcemaps: () => void; + freeDependencyOnSourcemapFiles: () => void; } export function releaseManagementPlugin({ @@ -42,7 +42,7 @@ export function releaseManagementPlugin({ sentryHub, sentryClient, sentryCliOptions, - completeTaskDependingOnSourcemaps, + freeDependencyOnSourcemapFiles, }: ReleaseManagementPluginOptions): UnpluginOptions { return { name: "sentry-debug-id-upload-plugin", @@ -90,7 +90,7 @@ export function releaseManagementPlugin({ await safeFlushTelemetry(sentryClient); handleRecoverableError(e); } finally { - completeTaskDependingOnSourcemaps(); + freeDependencyOnSourcemapFiles(); } }, }; diff --git a/packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts b/packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts index 032deade..53043df0 100644 --- a/packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts +++ b/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; + dependenciesAreFreedPromise: Promise; 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);