Skip to content

fix(@angular/build): prevent concurrent stylesheet bundling esbuild context leaks#33318

Open
clydin wants to merge 1 commit into
angular:mainfrom
clydin:fix-esbuild-leak-33317
Open

fix(@angular/build): prevent concurrent stylesheet bundling esbuild context leaks#33318
clydin wants to merge 1 commit into
angular:mainfrom
clydin:fix-esbuild-leak-33317

Conversation

@clydin
Copy link
Copy Markdown
Member

@clydin clydin commented Jun 5, 2026

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

…ontext 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 angular#33317
@clydin clydin added the target: patch This PR is targeted for the next patch release label Jun 5, 2026
@clydin clydin marked this pull request as ready for review June 8, 2026 12:05
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces request deduplication and write tracking to the Cache class to handle concurrent getOrCreate calls safely, alongside improvements to BundlerContext to manage active bundle promises and disposal. The review feedback highlights a potential race condition in Cache.getOrCreate where an active request might be incorrectly deleted after an await gap, a failing assertion in the new unit tests regarding promise resolution after a cache override, and a potential memory leak due to #writeCounts entries never being cleaned up.

Comment on lines +109 to +113
if (this.#requests.get(namespacedKey) === activeRequest) {
this.#incrementWrite(namespacedKey);
await this.store.set(namespacedKey, newValue);
this.#requests.delete(namespacedKey);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

There is a potential race condition here. After await this.store.set(namespacedKey, newValue) resolves, the active request in this.#requests might have changed (for example, if put() was called and then a new getOrCreate() was initiated during the await gap). Calling this.#requests.delete(namespacedKey) unconditionally inside the if block will delete the new active request instead of the current one.

To prevent this, we should re-verify that this.#requests.get(namespacedKey) === activeRequest is still true after the await before deleting it.

Suggested change
if (this.#requests.get(namespacedKey) === activeRequest) {
this.#incrementWrite(namespacedKey);
await this.store.set(namespacedKey, newValue);
this.#requests.delete(namespacedKey);
}
if (this.#requests.get(namespacedKey) === activeRequest) {
this.#incrementWrite(namespacedKey);
await this.store.set(namespacedKey, newValue);
if (this.#requests.get(namespacedKey) === activeRequest) {
this.#requests.delete(namespacedKey);
}
}

resolveCreator('original-value');

const val1 = await p1;
expect(val1).toBe('override-value');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

In this test, p1 is the promise returned by getOrCreate('key', creator). Since creator returns promise (which is resolved with 'original-value'), the promise p1 will eventually resolve to 'original-value'. Calling cache.put('key', 'override-value') will update the cache store and delete the active request from the internal map, but it cannot retroactively change the resolution value of the already-created promise p1. Therefore, await p1 will resolve to 'original-value', and expect(val1).toBe('override-value') will fail.

The assertion should be updated to expect 'original-value' for val1, while val2 (the subsequent getOrCreate call) is the one that correctly returns 'override-value'.

Suggested change
expect(val1).toBe('override-value');
expect(val1).toBe('original-value');

*/
export class Cache<V, S extends CacheStore<V> = CacheStore<V>> {
readonly #requests = new Map<string, Promise<V>>();
readonly #writeCounts = new Map<string, number>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The #writeCounts map is used to detect mid-flight writes during the store.get await gap. However, entries are added to #writeCounts but never deleted (except when the entire cache is cleared). In a long-running process like ng serve with many unique keys over time, this will cause a memory leak as #writeCounts grows indefinitely.

Since write counts are only needed to detect writes that occur during an active store.get call, we can track the number of pending get requests for each key and safely delete the write count entry when there are no more pending gets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: @angular/build target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@angular/build:unit-test hangs forever when two components in one file have identical inline styles

1 participant