From 1970f31480e4cbe1dfb7a630d3e7f8672054b0f2 Mon Sep 17 00:00:00 2001 From: Hugo Silva Date: Thu, 13 Aug 2026 17:30:30 +0200 Subject: [PATCH] fix(error-tracking): wait for debug ID injection before sourcemap upload --- packages/core/src/types.ts | 4 + packages/factory/src/helpers/context.test.ts | 46 ++++- packages/factory/src/helpers/context.ts | 34 ++++ .../plugins/error-tracking/src/index.test.ts | 60 ++++++- packages/plugins/error-tracking/src/index.ts | 58 +++--- packages/plugins/injection/src/esbuild.ts | 170 +++++++++--------- packages/plugins/injection/src/index.ts | 6 +- packages/tests/src/_jest/helpers/mocks.ts | 4 + 8 files changed, 277 insertions(+), 105 deletions(-) diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 6d8b4b4a2..164b18855 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -188,6 +188,8 @@ export type TriggerHook = ( ) => R; export type GlobalContext = { addMetric: (metric: Metric) => void; + artifactsPending: boolean; + artifactsReady: Promise; asyncHook: TriggerHook>; auth: AuthOptionsWithDefaults; build: BuildReport; @@ -198,6 +200,8 @@ export type GlobalContext = { git?: RepositoryData; hook: TriggerHook; inject: (item: ToInjectItem) => void; + markArtifactsPending: () => void; + markArtifactsReady: (error?: unknown) => void; pluginNames: string[]; plugins: (PluginOptions | CustomPluginOptions)[]; queue: (promise: Promise) => void; diff --git a/packages/factory/src/helpers/context.test.ts b/packages/factory/src/helpers/context.test.ts index b0f1566e3..00d3489be 100644 --- a/packages/factory/src/helpers/context.test.ts +++ b/packages/factory/src/helpers/context.test.ts @@ -4,10 +4,54 @@ import type { Options, GlobalContext } from '@dd/core/types'; import { BUNDLER_VERSIONS } from '@dd/tests/_jest/helpers/constants'; -import { defaultPluginOptions } from '@dd/tests/_jest/helpers/mocks'; +import { defaultPluginOptions, getMockData, getMockStores } from '@dd/tests/_jest/helpers/mocks'; import { BUNDLERS, runBundlers } from '@dd/tests/_jest/helpers/runBundlers'; +import { getContext } from './context'; + describe('Factory Helpers', () => { + describe('artifacts-ready barrier', () => { + const createContext = () => + getContext({ + start: Date.now(), + options: defaultPluginOptions, + data: getMockData(), + stores: getMockStores(), + }); + + test('Should start ready and wait while artifacts are pending.', async () => { + const context = createContext(); + await expect(context.artifactsReady).resolves.toBeUndefined(); + + context.markArtifactsPending(); + let ready = false; + context.artifactsReady.then(() => { + ready = true; + }); + await Promise.resolve(); + expect(ready).toBe(false); + + context.markArtifactsReady(); + await expect(context.artifactsReady).resolves.toBeUndefined(); + expect(ready).toBe(true); + }); + + test('Should reject a failed rewrite and support a later build.', async () => { + const context = createContext(); + const injectionError = new Error('injection failed'); + + context.markArtifactsPending(); + const failedBuild = context.artifactsReady; + context.markArtifactsReady(injectionError); + await expect(failedBuild).rejects.toBe(injectionError); + + context.markArtifactsPending(); + const nextBuild = context.artifactsReady; + context.markArtifactsReady(); + await expect(nextBuild).resolves.toBeUndefined(); + }); + }); + // Intercept contexts to verify it at the moment they're used. const initialContexts: Record = {}; const buildRoots: Record = {}; diff --git a/packages/factory/src/helpers/context.ts b/packages/factory/src/helpers/context.ts index b0fdcdbde..5e58550bd 100644 --- a/packages/factory/src/helpers/context.ts +++ b/packages/factory/src/helpers/context.ts @@ -38,6 +38,8 @@ export const getContext = ({ addMetric: () => { throw new Error('AddMetric function called before it was initialized.'); }, + artifactsPending: false, + artifactsReady: Promise.resolve(), auth: options.auth, pluginNames: [], bundler: { @@ -61,6 +63,8 @@ export const getContext = ({ inject: () => { throw new Error('Inject function called before it was initialized.'); }, + markArtifactsPending: () => {}, + markArtifactsReady: () => {}, plugins: [], // This will be updated in the async-queue plugin on initialization. queue: () => { @@ -71,5 +75,35 @@ export const getContext = ({ version: data.version, }; + let resolveArtifacts: (() => void) | undefined; + let rejectArtifacts: ((error: unknown) => void) | undefined; + + context.markArtifactsPending = () => { + if (resolveArtifacts || rejectArtifacts) { + return; + } + + context.artifactsReady = new Promise((resolve, reject) => { + resolveArtifacts = resolve; + rejectArtifacts = reject; + }); + context.artifactsPending = true; + context.artifactsReady.catch(() => undefined); + }; + + context.markArtifactsReady = (error?: unknown) => { + const resolve = resolveArtifacts; + const reject = rejectArtifacts; + resolveArtifacts = undefined; + rejectArtifacts = undefined; + context.artifactsPending = false; + + if (error === undefined) { + resolve?.(); + } else { + reject?.(error); + } + }; + return context; }; diff --git a/packages/plugins/error-tracking/src/index.test.ts b/packages/plugins/error-tracking/src/index.test.ts index 5f11f2726..a26cfdb9a 100644 --- a/packages/plugins/error-tracking/src/index.test.ts +++ b/packages/plugins/error-tracking/src/index.test.ts @@ -2,9 +2,15 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). // Copyright 2019-Present Datadog, Inc. +import { extractDebugId } from '@dd/error-tracking-plugin/sourcemaps/debugId'; import { uploadSourcemaps } from '@dd/error-tracking-plugin/sourcemaps/index'; import { getPlugins } from '@dd/error-tracking-plugin'; -import { getGetPluginsArg, getSourcemapsConfiguration } from '@dd/tests/_jest/helpers/mocks'; +import { + getGetPluginsArg, + getMockBuildReport, + getSourcemapsConfiguration, + hardProjectEntries, +} from '@dd/tests/_jest/helpers/mocks'; import { BUNDLERS, runBundlers } from '@dd/tests/_jest/helpers/runBundlers'; jest.mock('@dd/error-tracking-plugin/sourcemaps/index', () => { @@ -67,4 +73,56 @@ describe('Error Tracking Plugin', () => { expect(uploadSourcemapsMock).not.toHaveBeenCalled(); }); + + test('Should wait for artifacts and deduplicate concurrent lifecycle hooks.', async () => { + let markArtifactsReady!: () => void; + const artifactsReady = new Promise((resolve) => { + markArtifactsReady = resolve; + }); + const arg = getGetPluginsArg( + { + enableGit: false, + errorTracking: { sourcemaps: getSourcemapsConfiguration() }, + }, + { artifactsPending: true, artifactsReady }, + ); + const plugin = getPlugins(arg)[0]; + + const buildReportHook = plugin.buildReport!(getMockBuildReport()); + const trueEndHook = plugin.asyncTrueEnd!(); + await Promise.resolve(); + expect(uploadSourcemapsMock).not.toHaveBeenCalled(); + + markArtifactsReady(); + await Promise.all([buildReportHook, trueEndHook]); + expect(uploadSourcemapsMock).toHaveBeenCalledTimes(1); + }); + + test('Should expose all esbuild debug IDs before sourcemap upload.', async () => { + const debugIdsAtUpload: (string | undefined)[] = []; + uploadSourcemapsMock.mockImplementationOnce(async (_options, context) => { + const javascriptOutputs = (context.outputs || []).filter(({ filepath }) => + filepath.endsWith('.js'), + ); + debugIdsAtUpload.push( + ...(await Promise.all( + javascriptOutputs.map(({ filepath }) => extractDebugId(filepath)), + )), + ); + }); + + const { errors } = await runBundlers( + { + enableGit: false, + errorTracking: { sourcemaps: getSourcemapsConfiguration() }, + rum: { sourceCodeContext: { debugId: true } }, + }, + { entry: hardProjectEntries, splitting: true }, + ['esbuild'], + ); + + expect(errors).toHaveLength(0); + expect(debugIdsAtUpload.length).toBeGreaterThan(2); + expect(debugIdsAtUpload).not.toContain(undefined); + }); }); diff --git a/packages/plugins/error-tracking/src/index.ts b/packages/plugins/error-tracking/src/index.ts index 1f51ce09f..cd013c439 100644 --- a/packages/plugins/error-tracking/src/index.ts +++ b/packages/plugins/error-tracking/src/index.ts @@ -28,30 +28,46 @@ export const getPlugins: GetPlugins = ({ options, context }) => { let gitInfo: RepositoryData | undefined; let buildReport: BuildReport | undefined; let sourcemapsHandled: boolean = false; + let sourcemapsHandling: Promise | undefined; const handleSourcemaps = async () => { if (!validatedOptions.sourcemaps || sourcemapsHandled) { return; } - sourcemapsHandled = true; - const totalTime = log.time('sourcemaps process'); - await uploadSourcemaps( - // Need the "as" because Typescript doesn't understand that we've already checked for sourcemaps. - validatedOptions as ErrorTrackingOptionsWithSourcemaps, - { - apiKey: context.auth.apiKey, - bundlerName: context.bundler.name, - git: gitInfo, - addMetric: context.addMetric, - outDir: context.bundler.outDir, - outputs: buildReport?.outputs || [], - sendMetrics: sendSourcemapUploadMetrics, - site: context.auth.site, - version: context.version, - }, - log, - ); - totalTime.end(); + if (!sourcemapsHandling) { + sourcemapsHandling = (async () => { + await context.artifactsReady; + sourcemapsHandled = true; + const totalTime = log.time('sourcemaps process'); + await uploadSourcemaps( + // Need the "as" because Typescript doesn't understand that we've already checked for sourcemaps. + validatedOptions as ErrorTrackingOptionsWithSourcemaps, + { + apiKey: context.auth.apiKey, + bundlerName: context.bundler.name, + git: gitInfo, + addMetric: context.addMetric, + outDir: context.bundler.outDir, + outputs: buildReport?.outputs || [], + sendMetrics: sendSourcemapUploadMetrics, + site: context.auth.site, + version: context.version, + }, + log, + ); + totalTime.end(); + })(); + } + + await sourcemapsHandling; + }; + + const handleOrQueueSourcemaps = async () => { + if (context.artifactsPending) { + context.queue(handleSourcemaps()); + return; + } + await handleSourcemaps(); }; const plugins: ReturnType = [ @@ -62,14 +78,14 @@ export const getPlugins: GetPlugins = ({ options, context }) => { gitInfo = repoData; if (buildReport) { - await handleSourcemaps(); + await handleOrQueueSourcemaps(); } }, async buildReport(report) { buildReport = report; if (gitInfo || !shouldGetGitInfo(options)) { - await handleSourcemaps(); + await handleOrQueueSourcemaps(); } }, async asyncTrueEnd() { diff --git a/packages/plugins/injection/src/esbuild.ts b/packages/plugins/injection/src/esbuild.ts index 09ebbedcb..3f3a17435 100644 --- a/packages/plugins/injection/src/esbuild.ts +++ b/packages/plugins/injection/src/esbuild.ts @@ -94,7 +94,9 @@ export const getEsbuildPlugin = ( // InjectPosition.START and InjectPosition.END onEnd(async (result) => { if (!result.metafile) { - log.warn('Missing metafile from build result.'); + const error = new Error('Missing metafile from build result.'); + log.warn(error.message); + context.markArtifactsReady(error); return; } @@ -102,89 +104,95 @@ export const getEsbuildPlugin = ( return; } - const proms: Promise[] = []; - - // Process all output files - for (const [p, o] of Object.entries(result.metafile.outputs)) { - // Determine if this is an entry point - const isEntry = Boolean( - o.entryPoint && entries.some((e) => e.resolved.endsWith(o.entryPoint!)), - ); - - if (!isEntry && !hasChunkInjection(contentsToInject)) { - continue; - } - - const absolutePath = getAbsolutePath(context.buildRoot, p); - const { base, ext } = path.parse(absolutePath); - - // Check if file type is supported - if (!isFileSupported(ext)) { - warnUnsupportedFile(log, ext, base); - continue; - } - - // Inject content - proms.push( - (async () => { - try { - const mapPath = `${absolutePath}.map`; - const sourceOrHash = await fsp.readFile(absolutePath, 'utf-8'); - const sourcemap = await fsp - .readFile(mapPath, 'utf-8') - .catch(() => false as const); - const fileName = path.basename(absolutePath); - // Resolve static and per-chunk content in one pass. - const banner = getContentToInject( - contentsToInject, - InjectPosition.BEFORE, - { sourceOrHash, fileName, isEntry }, - ); - const footer = getContentToInject( - contentsToInject, - InjectPosition.AFTER, - { sourceOrHash, fileName, isEntry }, - ); - - if (!banner && !footer) { - return; + try { + const proms: Promise[] = []; + + // Process all output files + for (const [p, o] of Object.entries(result.metafile.outputs)) { + // Determine if this is an entry point + const isEntry = Boolean( + o.entryPoint && entries.some((e) => e.resolved.endsWith(o.entryPoint!)), + ); + + if (!isEntry && !hasChunkInjection(contentsToInject)) { + continue; + } + + const absolutePath = getAbsolutePath(context.buildRoot, p); + const { base, ext } = path.parse(absolutePath); + + // Check if file type is supported + if (!isFileSupported(ext)) { + warnUnsupportedFile(log, ext, base); + continue; + } + + // Inject content + proms.push( + (async () => { + try { + const mapPath = `${absolutePath}.map`; + const sourceOrHash = await fsp.readFile(absolutePath, 'utf-8'); + const sourcemap = await fsp + .readFile(mapPath, 'utf-8') + .catch(() => false as const); + const fileName = path.basename(absolutePath); + // Resolve static and per-chunk content in one pass. + const banner = getContentToInject( + contentsToInject, + InjectPosition.BEFORE, + { sourceOrHash, fileName, isEntry }, + ); + const footer = getContentToInject( + contentsToInject, + InjectPosition.AFTER, + { sourceOrHash, fileName, isEntry }, + ); + + if (!banner && !footer) { + return; + } + + // Strip existing sourceMappingURL and inline the map so esbuild chains it. + const cleaned = sourceOrHash.replace( + /\n?\/\/# sourceMappingURL=.*$/m, + '', + ); + const input = sourcemap + ? `${cleaned}\n//# sourceMappingURL=data:application/json;base64,${Buffer.from(sourcemap!).toString('base64')}` + : cleaned; + + const data = await esbuild.transform(input, { + loader: 'default', + banner, + footer, + sourcemap: sourcemap ? 'external' : undefined, + sourcefile: fileName, + }); + + await Promise.all([ + fsp.writeFile(absolutePath, data.code), + sourcemap && data.map ? fsp.writeFile(mapPath, data.map) : null, + ]); + } catch (e) { + if (isNodeSystemError(e) && e.code === 'ENOENT') { + // When we are using sub-builds, the entry file of sub-builds may not exist + // Hence we should skip the file injection in this case. + log.warn(`Could not inject content in ${absolutePath}: ${e}`); + } else { + throw e; + } } + })(), + ); + } - // Strip existing sourceMappingURL and inline the map so esbuild chains it. - const cleaned = sourceOrHash.replace( - /\n?\/\/# sourceMappingURL=.*$/m, - '', - ); - const input = sourcemap - ? `${cleaned}\n//# sourceMappingURL=data:application/json;base64,${Buffer.from(sourcemap!).toString('base64')}` - : cleaned; - - const data = await esbuild.transform(input, { - loader: 'default', - banner, - footer, - sourcemap: sourcemap ? 'external' : undefined, - sourcefile: fileName, - }); - - await Promise.all([ - fsp.writeFile(absolutePath, data.code), - sourcemap && data.map ? fsp.writeFile(mapPath, data.map) : null, - ]); - } catch (e) { - if (isNodeSystemError(e) && e.code === 'ENOENT') { - // When we are using sub-builds, the entry file of sub-builds may not exist - // Hence we should skip the file injection in this case. - log.warn(`Could not inject content in ${absolutePath}: ${e}`); - } else { - throw e; - } - } - })(), - ); + await Promise.all(proms); + context.markArtifactsReady(); + } catch (error) { + context.markArtifactsReady(error); + throw error; } - - await Promise.all(proms); }); }, }); diff --git a/packages/plugins/injection/src/index.ts b/packages/plugins/injection/src/index.ts index 055d693b8..a7f1b4367 100644 --- a/packages/plugins/injection/src/index.ts +++ b/packages/plugins/injection/src/index.ts @@ -14,7 +14,7 @@ import { import { PLUGIN_NAME } from './constants'; import { getEsbuildPlugin } from './esbuild'; -import { prepareInjections, getContentToInject } from './helpers'; +import { prepareInjections, getContentToInject, hasBeforeAfterInjection } from './helpers'; import { getRollupPlugin } from './rollup'; import type { ContentsToInject } from './types'; import { getXpackPlugin } from './xpack'; @@ -96,6 +96,10 @@ export const getInjectionPlugins: GetInternalPlugins = (arg: GetPluginsArg) => { plugin.buildStart = async () => { // Prepare the injections. await prepareInjections(log, injections, contentsToInject, context.buildRoot); + + if (context.bundler.name === 'esbuild' && hasBeforeAfterInjection(contentsToInject)) { + context.markArtifactsPending(); + } }; } diff --git a/packages/tests/src/_jest/helpers/mocks.ts b/packages/tests/src/_jest/helpers/mocks.ts index decb1924f..907dfda97 100644 --- a/packages/tests/src/_jest/helpers/mocks.ts +++ b/packages/tests/src/_jest/helpers/mocks.ts @@ -218,6 +218,8 @@ export const getGetPluginsArg = ( export const getContextMock = (overrides: Partial = {}): GlobalContext => { return { + artifactsPending: false, + artifactsReady: Promise.resolve(), auth: defaultAuth, bundler: { ...getMockBundler(overrides.bundler), @@ -231,6 +233,8 @@ export const getContextMock = (overrides: Partial = {}): GlobalCo addMetric: jest.fn(), hook: jest.fn(), inject: jest.fn(), + markArtifactsPending: jest.fn(), + markArtifactsReady: jest.fn(), pluginNames: [], sendLog: jest.fn(), plugins: [],