-
Notifications
You must be signed in to change notification settings - Fork 52
fix(v4/webpack): Deduplicate webpack deploys (#875) #888
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,16 @@ import { | |
| import { glob } from "glob"; | ||
| import { defaultRewriteSourcesHook, prepareBundleForDebugIdUpload } from "./debug-id-upload"; | ||
|
|
||
| // Module-level guard to prevent duplicate deploy records when multiple bundler plugin | ||
| // instances run in the same process (e.g. Next.js creates separate webpack compilers | ||
| // for client, server, and edge). Keyed by release name. | ||
| const _deployedReleases = new Set<string>(); | ||
|
|
||
| /** @internal Exported for testing only. */ | ||
| export function _resetDeployedReleasesForTesting(): void { | ||
| _deployedReleases.clear(); | ||
| } | ||
|
|
||
| export type SentryBuildPluginManager = { | ||
| /** | ||
| * A logger instance that takes the options passed to the build plugin manager into account. (for silencing and log level etc.) | ||
|
|
@@ -534,8 +544,9 @@ export function createSentryBuildPluginManager( | |
| await cliInstance.releases.finalize(options.release.name); | ||
| } | ||
|
|
||
| if (options.release.deploy) { | ||
| if (options.release.deploy && !_deployedReleases.has(options.release.name)) { | ||
| await cliInstance.releases.newDeploy(options.release.name, options.release.deploy); | ||
| _deployedReleases.add(options.release.name); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition in deploy deduplication guardHigh Severity The check-then-act pattern ( |
||
| } | ||
| } catch (e) { | ||
| sentryScope.captureException('Error in "releaseManagementPlugin" writeBundle hook'); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,14 @@ | ||
| import { createSentryBuildPluginManager } from "../src/build-plugin-manager"; | ||
| import { | ||
| createSentryBuildPluginManager, | ||
| _resetDeployedReleasesForTesting, | ||
| } from "../src/build-plugin-manager"; | ||
| import fs from "fs"; | ||
| import { glob } from "glob"; | ||
| import { prepareBundleForDebugIdUpload } from "../src/debug-id-upload"; | ||
|
|
||
| const mockCliExecute = jest.fn(); | ||
| const mockCliUploadSourceMaps = jest.fn(); | ||
| const mockCliNewDeploy = jest.fn(); | ||
|
|
||
| jest.mock("@sentry/cli", () => { | ||
| return jest.fn().mockImplementation(() => ({ | ||
|
|
@@ -14,7 +18,7 @@ jest.mock("@sentry/cli", () => { | |
| new: jest.fn(), | ||
| finalize: jest.fn(), | ||
| setCommits: jest.fn(), | ||
| newDeploy: jest.fn(), | ||
| newDeploy: mockCliNewDeploy, | ||
| }, | ||
| })); | ||
| }); | ||
|
|
@@ -633,4 +637,136 @@ describe("createSentryBuildPluginManager", () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("createRelease deploy deduplication", () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| _resetDeployedReleasesForTesting(); | ||
| }); | ||
|
|
||
| it("should create a deploy record on the first call", async () => { | ||
| const manager = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { | ||
| name: "test-release", | ||
| deploy: { env: "production" }, | ||
| }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| await manager.createRelease(); | ||
|
|
||
| expect(mockCliNewDeploy).toHaveBeenCalledTimes(1); | ||
| expect(mockCliNewDeploy).toHaveBeenCalledWith("test-release", { env: "production" }); | ||
| }); | ||
|
|
||
| it("should not create duplicate deploy records when createRelease is called multiple times on the same instance", async () => { | ||
| const manager = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { | ||
| name: "test-release", | ||
| deploy: { env: "production" }, | ||
| }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| await manager.createRelease(); | ||
| await manager.createRelease(); | ||
| await manager.createRelease(); | ||
|
|
||
| expect(mockCliNewDeploy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("should not create duplicate deploy records across separate plugin instances with the same release name", async () => { | ||
| const managerA = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { | ||
| name: "test-release", | ||
| deploy: { env: "production" }, | ||
| }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| const managerB = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { | ||
| name: "test-release", | ||
| deploy: { env: "production" }, | ||
| }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| await managerA.createRelease(); | ||
| await managerB.createRelease(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests don't verify concurrent execution scenarioMedium Severity The test "should not create duplicate deploy records across separate plugin instances" uses sequential |
||
|
|
||
| expect(mockCliNewDeploy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("should allow deploys for different release names", async () => { | ||
| const managerA = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { | ||
| name: "release-1", | ||
| deploy: { env: "production" }, | ||
| }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| const managerB = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { | ||
| name: "release-2", | ||
| deploy: { env: "production" }, | ||
| }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| await managerA.createRelease(); | ||
| await managerB.createRelease(); | ||
|
|
||
| expect(mockCliNewDeploy).toHaveBeenCalledTimes(2); | ||
| expect(mockCliNewDeploy).toHaveBeenCalledWith("release-1", { env: "production" }); | ||
| expect(mockCliNewDeploy).toHaveBeenCalledWith("release-2", { env: "production" }); | ||
| }); | ||
|
|
||
| it("should not create a deploy when deploy option is not set", async () => { | ||
| const manager = createSentryBuildPluginManager( | ||
| { | ||
| authToken: "test-token", | ||
| org: "test-org", | ||
| project: "test-project", | ||
| release: { name: "test-release" }, | ||
| }, | ||
| { buildTool: "webpack", loggerPrefix: "[sentry-webpack-plugin]" } | ||
| ); | ||
|
|
||
| await manager.createRelease(); | ||
|
|
||
| expect(mockCliNewDeploy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unbounded Set growth in long-running processes
Low Severity
The
_deployedReleasesSet accumulates release names indefinitely without cleanup. Since release names typically use git SHAs or CI commit identifiers (changing with each build), the Set grows unbounded in long-running production build processes, causing a memory leak. While dev mode skips deploy creation, production CI/CD pipelines or continuous deployment systems could accumulate entries.