sentry-file-deletion-plugin writeBundle hook Error in v2.21.0

sebellows commented 4 months ago

Environment

Vite, Vue, Quasar

What version are you running? Vite v2.9.13, Vue v3.2.29, Quasar v2.16.0

Steps to Reproduce

Install @sentry/vite-plugin v2.21.0
Run npm run build

Expected Result

The app will continue building without tripping over @sentry/vite-plugin.

Actual Result

Pinning @sentry/vite-plugin to the version listed in our previous package-lock.json file, v2.16.1, made it so the app would successfully build.

lforst commented 4 months ago

@sebellows it might be that v2 of vite doesn't like the way we define the plugin. We're only testing with vite >=3, and unofficially that's probably the minimum supported version. (We should probably add peer deps)

@ShugKnight24 and @parvez-proveway, since you gave the issue a thumbs up, what Vite versions are you using?

tyouzu1 commented 4 months ago

@sebellows it might be that v2 of vite doesn't like the way we define the plugin. We're only testing with vite >=3, and unofficially that's probably the minimum supported version. (We should probably add peer deps)

@ShugKnight24 and @parvez-proveway, since you gave the issue a thumbs up, what Vite versions are you using?

Looks like my fault, https://github.com/vitejs/vite/pull/9634 writeBundle object is only supported in v3, Do I need to add compatibility code here?

lforst commented 4 months ago

@tyouzu1 if you can come up with a way to add compatibility for v2 that isn't too hacky that would be amazing. Otherwise we might have to revert your PR because it breaks existing users which is more important than supporting new use-cases like the legacy vite compat 🤔

ShugKnight24 commented 4 months ago

In most instances I'm running Vite 5.3 or 5.3.3. If it's a lower version compatibility thing, it's likely when I use Vite with the Quasar framework. They have their own vite instance called @quasar/app-vite and the two other projects I'm running these versions

Looks like they pushed a new update yesterday, will test to see if the problem persists

sebellows commented 4 months ago

@sebellows it might be that v2 of vite doesn't like the way we define the plugin. We're only testing with vite >=3, and unofficially that's probably the minimum supported version. (We should probably add peer deps)

@ShugKnight24 and @parvez-proveway, since you gave the issue a thumbs up, what Vite versions are you using?

Adding the peer dependency would help communicate the requirement, because I would have expected this type of breaking requirement change to not happen in a minor release. However, we are definitely behind in our version of Vite because of Quasar's app-vite plugin. In the meantime, I've removed the ^ from the plugin version in our package.json and we'll need to wait for Quasar to get their app-vite plugin out of beta with Vite v5.3, because right now their last release still relies on Vite v2.9.13.

tyouzu1 commented 4 months ago

@tyouzu1 if you can come up with a way to add compatibility for v2 that isn't too hacky that would be amazing. Otherwise we might have to revert your PR because it breaks existing users which is more important than supporting new use-cases like the legacy vite compat 🤔

I took some time to check the actual problem, I realized the real reason for the wrong order of deleting tasks.

https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/c68361a636992e9f656f82f46fff031f1da18ba9/packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts#L35

The second trigger sentry-file-deletion-plugin using a promise that has already been resolved

lforst commented 4 months ago

@tyouzu1 thanks for the PR. Can you elaborate your thought process? I can't really follow why you think there is an issue. How is the order wrong?

tyouzu1 commented 4 months ago

@tyouzu1 thanks for the PR. Can you elaborate your thought process? I can't really follow why you think there is an issue. How is the order wrong?

As I said before, some vite plugins use the outputOptions of writeBundle

If it is called, this is the last hook of the output generation phase and may again be followed by [outputOptions](https://rollupjs.org/plugin-development/#outputoptions) if another output is generated

For example, During the build process

after the legacy plugin builds the productxxx-legacy.js xxx-legacy.js.map, it is triggered for the first time
After the normal product is built xxx.js xxx.js.map index.html, trigger it again(I believe if there are more build artifacts, it will trigger more times)

These builds will trigger the writebundle hook, I believe this is easy to see. Of course, if there is only one build, these problems cannot be discovered.No further details

// v 2.20.1
sentryVitePlugin({
      debug: true,
      org: "xxx",
      project: "xxx",
      authToken:
        "xxxx",
      release: {
        name: "test11",
        // cleanArtifacts: true,
        uploadLegacySourcemaps: {
          paths: [`dist`],
          urlPrefix: "~/abc/",
        },
      },
      sourcemaps: {
        filesToDeleteAfterUpload: [`dist/**/*.js.map`],
      },
      url: "https://sentry-new.xxx.com/",
    }),

vite v5.3.3 building for production...
✓ 35 modules transformed.
computing gzip size (1)...[sentry-vite-plugin] Debug: No `sourcemaps.assets` option provided, falling back to uploading detected build artifacts.
[sentry-vite-plugin] Debug: Waiting for dependencies on generated files to be freed before deleting...
dist/assets/polyfills-legacy-ClJsSY4b.js   71.52 kB │ gzip: 28.90 kB
dist/assets/index-legacy-DkA8posW.js      144.97 kB │ gzip: 46.95 kB │ map: 353.86 kB
[sentry-vite-plugin] Debug: Could not determine debug ID from bundle. This can happen if you did not clean your output folder before installing the Sentry plugin. File will not be source mapped: /xxxxx/demo/vite-project/dist/assets/polyfills-legacy-ClJsSY4b.js
> Found 2 files
> Analyzing 2 sources
> Analyzing completed in 0.002s
> Adding source map references
> Bundling completed in 0.027s
> Bundled 2 files for upload
> Bundle ID: 06da3aa7-feed-5892-ab3c-63b7bb0f4293
> Optimizing completed in 0.001s
> Uploading completed in 0.396s
> Uploaded files to Sentry
error: release not found

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.
[sentry-vite-plugin] Info: Successfully uploaded source maps to Sentry
> Found 3 files
> Analyzing 3 sources
> Analyzing completed in 0.001s
> Rewriting sources
> Rewriting completed in 0.016s
> Adding source map references
> Bundling completed in 0.046s
> Bundled 3 files for upload
> Bundle ID: 1584363a-2c07-5aa9-b617-7223219cd297
> Optimizing completed in 0.001s
> Uploading completed in 0.417s
> Uploaded files to Sentry
> Processing completed in 0.129s
> File processing complete
> Organization: tlabs
> Project: tiktok-sale-chanel
> Release: test11
> Dist: None
> Upload type: release bundle

Source Map Upload Report
  Scripts
    ~/abc/assets/index-legacy-DkA8posW.js (sourcemap at index-legacy-DkA8posW.js.map)
    ~/abc/assets/polyfills-legacy-ClJsSY4b.js (sourcemap at index-legacy-DkA8posW.js.map)
  Source Maps
    ~/abc/assets/index-legacy-DkA8posW.js.map
[sentry-vite-plugin] Debug: Deleting asset after upload: /xxx/demo/vite-project/dist/assets/index-legacy-DkA8posW.js.map
computing gzip size (3)...[sentry-vite-plugin] Debug: Waiting for dependencies on generated files to be freed before deleting...
[sentry-vite-plugin] Debug: Deleting asset after upload: /xxx/demo/vite-project/dist/assets/index-9funETI3.js.map
[sentry-vite-plugin] Debug: No `sourcemaps.assets` option provided, falling back to uploading detected build artifacts.
dist/index.html                   1.73 kB │ gzip:  0.82 kB
dist/assets/react-CHdo91hT.svg    4.13 kB │ gzip:  2.05 kB
dist/assets/index-rDvlKsbK.css    1.40 kB │ gzip:  0.73 kB
dist/assets/index-9funETI3.js   144.02 kB │ gzip: 46.39 kB │ map: 349.69 kB
[sentry-vite-plugin] Debug: Could not determine debug ID from bundle. This can happen if you did not clean your output folder before installing the Sentry plugin. File will not be source mapped: /xxx/demo/vite-project/dist/assets/polyfills-legacy-ClJsSY4b.js
[sentry-vite-plugin] Debug: Could not determine source map path for bundle: /xxx/demo/vite-project/dist/assets/index-9funETI3.js - Did you turn on source map generation in your bundler?
[sentry-vite-plugin] Debug: Could not determine source map path for bundle: /xxx/demo/vite-project/dist/assets/index-legacy-DkA8posW.js - Did you turn on source map generation in your bundler?
> Found 2 files
> Analyzing 2 sources
> Analyzing completed in 0s
> Adding source map references
> Bundling completed in 0.011s
> Bundled 2 files for upload
> Bundle ID: 541561ec-2fa9-512b-8c4d-3655102a550d
> Optimizing completed in 0s
> Uploading files...
> Found 3 files
> Analyzing 3 sources
> Analyzing completed in 0s
> Rewriting sources
> Uploading completed in 0.318s
> Uploaded files to Sentry
error: release not found

Add --log-level=[info|debug] or export SENTRY_LOG_LEVEL=[info|debug] to see more output.
Please attach the full debug log to all bug reports.
[sentry-vite-plugin] Info: Successfully uploaded source maps to Sentry
> Bundling completed in 0.019s
> Bundled 3 files for upload
> Bundle ID: 6166c0b0-d3ad-53c1-ab99-8a5586a0e451
> Optimizing completed in 0s
> Uploading completed in 0.396s
> Uploaded files to Sentry
> Processing completed in 0.195s
> File upload complete (processing pending on server)
> Organization: tlabs
> Project: tiktok-sale-chanel
> Release: test11
> Dist: None
> Upload type: release bundle

Source Map Upload Report
  Scripts
    ~/abc/assets/index-9funETI3.js (sourcemap at index-9funETI3.js.map)
    ~/abc/assets/index-legacy-DkA8posW.js (sourcemap at index-legacy-DkA8posW.js.map)
    ~/abc/assets/polyfills-legacy-ClJsSY4b.js (no sourcemap ref)
      - warning: could not determine a source map reference (Could not auto-detect referenced sourcemap for ~/abc/assets/polyfills-legacy-ClJsSY4b.js)

According to the log, it is obvious that the code of the sentry plugin is triggered twice.

Let's look at the order of the debug logs

// ------------------------- [ Waiting   dependencies first time ] -------------
[sentry-vite-plugin] Debug: Waiting for dependencies on generated files to be freed before deleting...
dist/assets/polyfills-legacy-ClJsSY4b.js   71.52 kB │ gzip: 28.90 kB
dist/assets/index-legacy-DkA8posW.js      144.97 kB │ gzip: 46.95 kB │ map: 353.86 kB
.....
// ------------------------- [ Upload before Delete ] ----------
[sentry-vite-plugin] Info: Successfully uploaded source maps to Sentry
.....
// ------------------------- [ Deleting  index-legacy-DkA8posW.js.map ] ----------
[sentry-vite-plugin] Debug: Deleting asset after upload: /xxx/demo/vite-project/dist/assets/index-legacy-DkA8posW.js.map
// ------------------------- [ Waiting   dependencies second time ] -------------
computing gzip size (3)...[sentry-vite-plugin] Debug: Waiting for dependencies on generated files to be freed before deleting...
// ------------------------- [ Deleting index-9funETI3.js.map ] ----------
[sentry-vite-plugin] Debug: Deleting asset after upload: /xxxx/demo/vite-project/dist/assets/index-9funETI3.js.map
[sentry-vite-plugin] Debug: No `sourcemaps.assets` option provided, falling back to uploading detected build artifacts.
dist/index.html                   1.73 kB │ gzip:  0.82 kB
dist/assets/react-CHdo91hT.svg    4.13 kB │ gzip:  2.05 kB
dist/assets/index-rDvlKsbK.css    1.40 kB │ gzip:  0.73 kB
dist/assets/index-9funETI3.js   144.02 kB │ gzip: 46.39 kB │ map: 349.69 kB
.....
// ------------------------- [ Upload after Delete ] ----------
[sentry-vite-plugin] Info: Successfully uploaded source maps to Sentry

Should be good to observe, The order of the second execution has changed

Second Deleted before the Second Upload

Let's look at the code,

The delete task will wait for the subscription to be triggered after the writebundle is run.

https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/c68361a636992e9f656f82f46fff031f1da18ba9/packages/bundler-plugin-core/src/plugins/sourcemap-deletion.ts#L27-L52

Wait for the release of the following code

https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/c68361a636992e9f656f82f46fff031f1da18ba9/packages/bundler-plugin-core/src/debug-id-upload.ts#L197

https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/c68361a636992e9f656f82f46fff031f1da18ba9/packages/bundler-plugin-core/src/plugins/release-management.ts#L93

When the plugin is initialized, the promise created by waitUntilSourcemapFileDependenciesAreFreed is passed to the deletion task. When the first subscription is triggered, the deletion task is triggered, and the promise will be used ( Due to the plugin operation mechanism, the promsie here will only be triggered once)

https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/c68361a636992e9f656f82f46fff031f1da18ba9/packages/bundler-plugin-core/src/index.ts#L410

Then after the first deletion task is triggered, promise is used. When the second deletion task is triggered, it will not continue to wait.

So the second deletion task was brought forward, and the upload task has not yet started

Do these answer your questions?

So all I can think of is to recreate a subscription and listener every time writebundle is triggered, Same as I did

lforst commented 4 months ago

Ah, I think I understand now. Thank you! Our write hooks are called multiple times in certain setups, and that will cause the tasks to be removed before all of the invocations have finished. Your PR makes a lot of sense in that case.

tyouzu1 commented 4 months ago

Ah, I think I understand now. Thank you! Our write hooks are called multiple times in certain setups, and that will cause the tasks to be removed before all of the invocations have finished. Your PR makes a lot of sense in that case.

Although this is the case, I can only guarantee vite, I will add some test cases later. Enjoy your weekend!

lforst commented 4 months ago

For anybody watching this issue, we released version 2.21.1 which should fix this problem. Gonna close this issue to keep our issues stream clean. Let me know if there still are problems.

getsentry / sentry-javascript-bundler-plugins