From 0c80537d835ab07a1800afd630227ef5a221ce52 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Fri, 5 Jun 2026 18:59:03 -0400 Subject: [PATCH] fix(@angular/build): prevent concurrent stylesheet bundling esbuild context leaks When multiple components in the same file define identical inline styles, concurrent calls to the stylesheet bundler invoke Cache.getOrCreate concurrently with the same key. Because getOrCreate is not concurrency-safe, multiple BundlerContext instances are created. Additionally, if multiple bundle() requests occur concurrently on a BundlerContext when the esbuild context is not yet initialized, they both invoke context(), causing one to overwrite and leak the other. Leaked esbuild contexts keep the Node event loop active forever, hanging the test runner. Resolve the Cache race by using an internal Map of in-flight promises to share the same active request, along with a version counter to detect mid-flight writes and safely retry. Resolve the BundlerContext race by memoizing and sharing the active performBundle promise. Fixes #33317 --- .../src/tools/esbuild/bundler-context.ts | 24 ++- .../angular/build/src/tools/esbuild/cache.ts | 101 +++++++++++- .../build/src/tools/esbuild/cache_spec.ts | 144 ++++++++++++++++++ 3 files changed, 261 insertions(+), 8 deletions(-) create mode 100644 packages/angular/build/src/tools/esbuild/cache_spec.ts diff --git a/packages/angular/build/src/tools/esbuild/bundler-context.ts b/packages/angular/build/src/tools/esbuild/bundler-context.ts index dd2764c8c608..7fe28e5a51d4 100644 --- a/packages/angular/build/src/tools/esbuild/bundler-context.ts +++ b/packages/angular/build/src/tools/esbuild/bundler-context.ts @@ -59,6 +59,8 @@ export class BundlerContext { #esbuildContext?: BuildContext<{ metafile: true; write: false }>; #esbuildOptions?: BuildOptions & { metafile: true; write: false }; #esbuildResult?: BundleContextResult; + #activeBundlePromise?: Promise; + #disposed = false; #optionsFactory: BundlerOptionsFactory; #shouldCacheResult: boolean; #loadCache?: MemoryLoadResultCache; @@ -177,7 +179,18 @@ export class BundlerContext { return this.#esbuildResult; } - const result = await this.#performBundle(); + if (!force && this.#activeBundlePromise) { + return this.#activeBundlePromise; + } + + const bundlePromise = this.#performBundle().finally(() => { + if (this.#activeBundlePromise === bundlePromise) { + this.#activeBundlePromise = undefined; + } + }); + this.#activeBundlePromise = bundlePromise; + + const result = await bundlePromise; if (this.#shouldCacheResult) { this.#esbuildResult = result; } @@ -207,7 +220,12 @@ export class BundlerContext { } else { // Create a build context and perform the build. // Context creation does not perform a build. - this.#esbuildContext = await context(this.#esbuildOptions); + const esbuildContext = await context(this.#esbuildOptions); + if (this.#disposed) { + await esbuildContext.dispose(); + throw new Error('BundlerContext was disposed during build.'); + } + this.#esbuildContext = esbuildContext; result = await this.#esbuildContext.rebuild(); } } catch (failure) { @@ -446,9 +464,11 @@ export class BundlerContext { * @returns A promise that resolves when disposal is complete. */ async dispose(): Promise { + this.#disposed = true; try { this.#esbuildOptions = undefined; this.#esbuildResult = undefined; + this.#activeBundlePromise = undefined; this.#loadCache = undefined; await this.#esbuildContext?.dispose(); } finally { diff --git a/packages/angular/build/src/tools/esbuild/cache.ts b/packages/angular/build/src/tools/esbuild/cache.ts index 5bd0dc84d73f..0634ec5268e8 100644 --- a/packages/angular/build/src/tools/esbuild/cache.ts +++ b/packages/angular/build/src/tools/esbuild/cache.ts @@ -44,11 +44,26 @@ export interface CacheStore { * to use a cache. */ export class Cache = CacheStore> { + // In-flight creator promises to deduplicate concurrent requests for the same key. + readonly #requests = new Map>(); + // Track how many writes occurred for a key to detect mutations during await gaps. + readonly #writeCounts = new Map(); + // Count the number of active, pending getOrCreate operations per key to avoid memory leaks. + readonly #pendingGets = new Map(); + constructor( protected readonly store: S, readonly namespace?: string, ) {} + #incrementWrite(key: string) { + // Only track write counts if there is a pending getOrCreate operation active for the key. + // This ensures that write counts are not leaked when no concurrent gets are running. + if (this.#pendingGets.has(key)) { + this.#writeCounts.set(key, (this.#writeCounts.get(key) || 0) + 1); + } + } + /** * Prefixes a key with the cache namespace if present. * @param key A key string to prefix. @@ -72,14 +87,75 @@ export class Cache = CacheStore> { */ async getOrCreate(key: string, creator: () => V | Promise): Promise { const namespacedKey = this.withNamespace(key); - let value = await this.store.get(namespacedKey); - if (value === undefined) { - value = await creator(); - await this.store.set(namespacedKey, value); + // 1. If another call is already running the creator for this key, share its promise. + let activeRequest = this.#requests.get(namespacedKey); + if (activeRequest !== undefined) { + return activeRequest; } - return value; + // Increment pending gets count to enable write-tracking for this key. + const currentPending = this.#pendingGets.get(namespacedKey) || 0; + this.#pendingGets.set(namespacedKey, currentPending + 1); + + try { + const startWriteCount = this.#writeCounts.get(namespacedKey) || 0; + + // 2. Query the backing store. Since store.get can be async, we yield to the event loop. + const value = await this.store.get(namespacedKey); + + // If a write (e.g. put) occurred during the store.get await gap, we must abort + // the current execution and restart to ensure we return the newly written value. + if ((this.#writeCounts.get(namespacedKey) || 0) !== startWriteCount) { + return this.getOrCreate(key, creator); + } + + if (value !== undefined) { + return value; + } + + // 3. Recheck active request after the await gap in case another concurrent call + // initiated a creator during the store.get wait. + activeRequest = this.#requests.get(namespacedKey); + if (activeRequest !== undefined) { + return activeRequest; + } + + // 4. Run the creator to produce the new value, and store its promise in #requests. + activeRequest = Promise.resolve(creator()).then( + async (newValue) => { + // Ensure this request is still the active one before writing back to the store + // (prevents overwriting newer data if put() was called before resolution). + if (this.#requests.get(namespacedKey) === activeRequest) { + this.#incrementWrite(namespacedKey); + await this.store.set(namespacedKey, newValue); + this.#requests.delete(namespacedKey); + } + + return newValue; + }, + (error) => { + // Clean up the active request if the creator fails. + if (this.#requests.get(namespacedKey) === activeRequest) { + this.#requests.delete(namespacedKey); + } + throw error; + }, + ); + + this.#requests.set(namespacedKey, activeRequest); + + return activeRequest; + } finally { + // Clean up write counts and pending gets once all concurrent gets for this key finish. + const current = this.#pendingGets.get(namespacedKey) || 0; + if (current <= 1) { + this.#pendingGets.delete(namespacedKey); + this.#writeCounts.delete(namespacedKey); + } else { + this.#pendingGets.set(namespacedKey, current - 1); + } + } } /** @@ -100,7 +176,19 @@ export class Cache = CacheStore> { * @param value A value to put in the cache. */ async put(key: string, value: V): Promise { - await this.store.set(this.withNamespace(key), value); + const namespacedKey = this.withNamespace(key); + this.#requests.delete(namespacedKey); + this.#incrementWrite(namespacedKey); + await this.store.set(namespacedKey, value); + } + + /** + * Clears the base class internal state (requests, write counts, and pending gets). + */ + protected clearInternal(): void { + this.#requests.clear(); + this.#writeCounts.clear(); + this.#pendingGets.clear(); } } @@ -116,6 +204,7 @@ export class MemoryCache extends Cache> { * Removes all entries from the cache instance. */ clear() { + this.clearInternal(); this.store.clear(); } diff --git a/packages/angular/build/src/tools/esbuild/cache_spec.ts b/packages/angular/build/src/tools/esbuild/cache_spec.ts new file mode 100644 index 000000000000..a30852836eff --- /dev/null +++ b/packages/angular/build/src/tools/esbuild/cache_spec.ts @@ -0,0 +1,144 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.dev/license + */ + +import { MemoryCache } from './cache'; + +describe('MemoryCache', () => { + let cache: MemoryCache; + + beforeEach(() => { + cache = new MemoryCache(); + }); + + it('should return cached value on subsequent getOrCreate calls', async () => { + let callCount = 0; + const creator = () => { + callCount++; + + return 'value'; + }; + + const val1 = await cache.getOrCreate('key', creator); + const val2 = await cache.getOrCreate('key', creator); + + expect(val1).toBe('value'); + expect(val2).toBe('value'); + expect(callCount).toBe(1); + }); + + it('should call creator only once for concurrent getOrCreate calls with the same key', async () => { + let callCount = 0; + let resolveCreator!: (value: string) => void; + const promise = new Promise((resolve) => { + resolveCreator = resolve; + }); + + const creator = () => { + callCount++; + + return promise; + }; + + const p1 = cache.getOrCreate('key', creator); + const p2 = cache.getOrCreate('key', creator); + + resolveCreator('concurrent-value'); + + const [val1, val2] = await Promise.all([p1, p2]); + + expect(val1).toBe('concurrent-value'); + expect(val2).toBe('concurrent-value'); + expect(callCount).toBe(1); + }); + + it('should call creator multiple times for concurrent getOrCreate calls with different keys', async () => { + let callCount = 0; + const creator = (val: string) => { + callCount++; + + return Promise.resolve(val); + }; + + const p1 = cache.getOrCreate('key1', () => creator('value1')); + const p2 = cache.getOrCreate('key2', () => creator('value2')); + + const [val1, val2] = await Promise.all([p1, p2]); + + expect(val1).toBe('value1'); + expect(val2).toBe('value2'); + expect(callCount).toBe(2); + }); + + it('should clean up active request if creator throws/rejects', async () => { + let callCount = 0; + let rejectCreator!: (err: Error) => void; + const promise = new Promise((_, reject) => { + rejectCreator = reject; + }); + + const creator = () => { + callCount++; + + return promise; + }; + + const p1 = cache.getOrCreate('key', creator); + const p2 = cache.getOrCreate('key', creator); + + rejectCreator(new Error('creator error')); + + await expectAsync(p1).toBeRejectedWithError('creator error'); + await expectAsync(p2).toBeRejectedWithError('creator error'); + + // Subsequent call should trigger the creator again + const p3 = cache.getOrCreate('key', () => { + callCount++; + + return Promise.resolve('new-value'); + }); + const val3 = await p3; + expect(val3).toBe('new-value'); + expect(callCount).toBe(2); + }); + + it('should override/clear active requests when put is called', async () => { + let resolveCreator!: (value: string) => void; + const promise = new Promise((resolve) => { + resolveCreator = resolve; + }); + + let creatorStarted!: (value: void) => void; + const creatorStartedPromise = new Promise((resolve) => { + creatorStarted = resolve; + }); + + const creator = () => { + creatorStarted(); + + return promise; + }; + + const p1 = cache.getOrCreate('key', creator); + + // Wait for the creator to be called so that the active request is created + await creatorStartedPromise; + + // Call put before the creator promise resolves + await cache.put('key', 'override-value'); + + resolveCreator('original-value'); + + const val1 = await p1; + // p1 was already returned, so it resolves to original-value + expect(val1).toBe('original-value'); + + // Subsequent getOrCreate should return the put/overridden value, not the resolved original-value + const val2 = await cache.getOrCreate('key', () => 'should-not-run'); + expect(val2).toBe('override-value'); + }); +});