fix(components): close deploy-validation Scopes to stop deployLifecycle listener leak - #1465
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Reviewed; no blockers found. |
kriszyp
marked this pull request as ready for review
June 25, 2026 04:28
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
cb1kenobi
reviewed
Jul 8, 2026
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| try { | ||
| for (let i = 0; i < loads; i++) { | ||
| componentLoader.loadedPaths.clear(); // a fresh deploy re-validates past the path-load guard | ||
| await componentLoader.loadComponent(componentDir, { isWorker: true, set: sinon.stub() }, 'test-origin'); |
Contributor
There was a problem hiding this comment.
Suggestion (non-blocking): sinon.stub() here (and on line 381) can be replaced with a plain () => {} — the stub's return value is never asserted. Per AGENTS.md, new tests in unitTests/components/ should avoid adding new sinon uses.
Suggested change
| await componentLoader.loadComponent(componentDir, { isWorker: true, set: sinon.stub() }, 'test-origin'); | |
| await componentLoader.loadComponent(componentDir, { isWorker: true, set: () => {} }, 'test-origin'); |
| for (let i = 0; i < loads; i++) { | ||
| componentLoader.loadedPaths.clear(); | ||
| const collectScopes = new Set(); | ||
| await componentLoader.loadComponent(componentDir, { isWorker: true, set: sinon.stub() }, 'test-origin', { |
Contributor
There was a problem hiding this comment.
Suggestion (non-blocking): same as line 349 — sinon.stub() can be () => {} since the call is never asserted.
Suggested change
| await componentLoader.loadComponent(componentDir, { isWorker: true, set: sinon.stub() }, 'test-origin', { | |
| await componentLoader.loadComponent(componentDir, { isWorker: true, set: () => {} }, 'test-origin', { |
cb1kenobi
approved these changes
Jul 10, 2026
…le listener leak The deploy_component pre-flight validation load creates Scopes (isWorker) that register deploy:start/deploy:end listeners on the shared deployLifecycle emitter but were never closed, so the listeners accumulated on the worker across deploys and eventually tripped MaxListenersExceededWarning (#1462). loadComponent gains a collectScopes option: when provided, created Scopes are added to the set instead of being registered for worker-shutdown auto-close, and the deploy validation closes them once validation completes. The load+close is tracked via trackScopeClose so a concurrent worker shutdown still waits for native-runtime disposal before realExit. Fixes #1462 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… of swallowing them
kriszyp
force-pushed
the
kris/deploy-lifecycle-scope-leak
branch
from
July 10, 2026 21:23
69acdd3 to
6ea910f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1462
Summary
deploy_component's pre-flight validation load (components/operations.js,deployComponent) loads the freshly-prepared component on a worker — withpseudoResources.isWorker = true— purely to surface load-time errors early. BecauseisWorkeris set,loadComponentcreates realScopes, each of which registersdeploy:start/deploy:endlisteners on the shareddeployLifecycleemitter (components/Scope.ts:138-139). These throwaway validation Scopes were never closed —Scope.close()only ran via the per-Scope worker-SHUTDOWNhandler — so their listeners accumulated on the worker across deploys, eventually trippingMaxListenersExceededWarning(11 deploy:start listeners). This is the leak in #1462.The invariant the rest of the system relies on is stated in
components/scopeShutdown.ts: "scopes are created once at load and closed once at shutdown, so nothing accumulates." The validation load violated it.Changes
components/componentLoader.ts— added acollectScopes?: Set<Scope>option toloadComponent. When provided, each Scope created during the load is added to the set instead of being registered for worker-shutdown auto-close; the caller then owns closing them. Threaded through the sub-component recursion so packaged sub-components are collected too.components/operations.js(deployComponent) — the validation load now collects its Scopes and closes them in afinallyonce validation completes, so theirdeployLifecyclelisteners are removed immediately. The load+close is wrapped intrackScopeClose(...)so a concurrent worker shutdown still waits for these Scopes to dispose (a plugin may start a native runtime inhandleApplication) beforerealExit. Also dropped the stale trailing positional args toloadComponent(the 4th+ args were already ignored by the current(dir, resources, origin, options)signature).unitTests/components/componentLoader.test.js— a characterization test (each unclosed validation load leaks exactly onedeploy:startlistener) and a regression test (withcollectScopesclosed after each load, the listener count stays at baseline across 12 loads — past the defaultmaxListenersof 10).Where to look
SHUTDOWNauto-close for collected Scopes would otherwise drop them from the worker'swhenScopesClosed()wait. ThetrackScopeClose(validation)wrapping restores that guarantee — worth a look that this is the right mechanism. (Raised by the Codex cross-model review and addressed here.)startOnMainThread-once core fix (branchkris/start-on-main-thread-once). No overlap: that fix gates main-thread init; this fixes worker-side validation-load Scope cleanup.Local validation:
unitTests/components/componentLoader.test.jsandunitTests/components/Scope.test.jspass; the deploy integration suite is left to CI.🤖 Generated by an LLM (Claude Opus 4.8).