From 983db14fc1a0a3e48afb680a28826e7094ed8bed 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 | 69 ++++++++- .../build/src/tools/esbuild/cache_spec.ts | 131 ++++++++++++++++++ 3 files changed, 216 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..bd6e4cee72fc 100644 --- a/packages/angular/build/src/tools/esbuild/cache.ts +++ b/packages/angular/build/src/tools/esbuild/cache.ts @@ -44,11 +44,18 @@ export interface CacheStore { * to use a cache. */ export class Cache = CacheStore> { + readonly #requests = new Map>(); + readonly #writeCounts = new Map(); + constructor( protected readonly store: S, readonly namespace?: string, ) {} + #incrementWrite(key: string) { + 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 +79,52 @@ 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); + let activeRequest = this.#requests.get(namespacedKey); + if (activeRequest !== undefined) { + return activeRequest; } - return value; + const startWriteCount = this.#writeCounts.get(namespacedKey) || 0; + + const value = await this.store.get(namespacedKey); + + // If a write occurred during the await gap, we must re-evaluate + if ((this.#writeCounts.get(namespacedKey) || 0) !== startWriteCount) { + return this.getOrCreate(key, creator); + } + + if (value !== undefined) { + return value; + } + + // Recheck active request after the await gap in case another one was initiated + activeRequest = this.#requests.get(namespacedKey); + if (activeRequest !== undefined) { + return activeRequest; + } + + activeRequest = Promise.resolve(creator()).then( + async (newValue) => { + if (this.#requests.get(namespacedKey) === activeRequest) { + this.#incrementWrite(namespacedKey); + await this.store.set(namespacedKey, newValue); + this.#requests.delete(namespacedKey); + } + + return newValue; + }, + (error) => { + if (this.#requests.get(namespacedKey) === activeRequest) { + this.#requests.delete(namespacedKey); + } + throw error; + }, + ); + + this.#requests.set(namespacedKey, activeRequest); + + return activeRequest; } /** @@ -100,7 +145,18 @@ 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 and write counts). + */ + protected clearInternal(): void { + this.#requests.clear(); + this.#writeCounts.clear(); } } @@ -116,6 +172,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..bcd70cda9153 --- /dev/null +++ b/packages/angular/build/src/tools/esbuild/cache_spec.ts @@ -0,0 +1,131 @@ +/** + * @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; + }); + + const creator = () => promise; + + const p1 = cache.getOrCreate('key', creator); + + // Call put before the creator promise resolves + await cache.put('key', 'override-value'); + + resolveCreator('original-value'); + + const val1 = await p1; + expect(val1).toBe('override-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'); + }); +});