Skip to content

fix(sveltekit): Ensure source maps deletion is called after source maps upload #14942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jan 8, 2025

This PR fixes a source maps deletion bug in the Sentry SvelteKit SDK reported in #14131.

After debugging this extensively, the root cause turned out to be a timing issue. For SvelteKit, we have to move all invocations of the writeBundle hook in the sub-plugins of the original Sentry Vite plugin to the closeBundle hook, to ensure that the second build made when the SvelteKit adapter is invoked is also awaited correctly. We did this correctly for the debug id source maps upload plugin but did not do it for the release management and file deletion plugins.

The changed order of things caused a deadlock situations due to our deletion plugin waiting on upload and release plugins to finish. Since the deletion plugin was started earlier than the upload plugin, it was awaiting something that wasn't even started yet.

This PR now:

  • replaces the original release management plugin with a custom one where we start release creation on closeBundle
  • replaces the original file deletion plugin with a custom one where we start deletion on closeBundle
  • in both cases, further ensures that the plugin is only invoked for build and as late as possible (enforce: 'post'). All of these three things ensure now that the deadlock situation is resolved.
  • searches for the release management plugin by its old and new name introduced in ref(core): Rename release management plugin name sentry-javascript-bundler-plugins#647
  • slightly renames the custom source maps uploading plugin for better convention; as well as some variables for better readability
  • adds unit tests for the new custom sub plugins

closes #14131
fixes #12660

@Lms24 Lms24 self-assigned this Jan 8, 2025
@Lms24 Lms24 requested review from lforst and s1gr1d January 8, 2025 13:54
Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for debugging and solving this 🙌

@Lms24 Lms24 merged commit 6779dfe into develop Jan 9, 2025
39 checks passed
@Lms24 Lms24 deleted the lms/fix-sveltekit-sourcemap-deletion-ii branch January 9, 2025 12:55
Lms24 added a commit that referenced this pull request Jan 9, 2025
…ps upload (#14942)

- replace the original release management plugin with a custom one
where we start release creation on `closeBundle`
- replace the original file deletion plugin with a custom one where we
start deletion on `closeBundle`
- in both cases, further ensuresthat the plugin is only invoked for
build and as late as possible (`enforce: 'post'`). 
- searche for the release management plugin by its old and new name
introduced in
getsentry/sentry-javascript-bundler-plugins#647
- slightly rename the custom source maps uploading plugin for better
convention; as well as some variables for better readability
- add unit tests for the new custom sub plugins
Lms24 added a commit that referenced this pull request Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants