Uh oh!
There was an error while loading. Please reload this page.
fix(core): prevent allBounded listener leak via PubSub subscription - #38939
Open
Shalin-Shah-2002 wants to merge 1 commit into
Open
fix(core): prevent allBounded listener leak via PubSub subscription#38939Shalin-Shah-2002 wants to merge 1 commit into
Shalin-Shah-2002 wants to merge 1 commit into
Conversation
Changes allBounded to subscribe via PubSub.subscribe(pubsub.all) instead of events.listen(), avoiding accumulation in the global listeners Set when scope cleanup is delayed or skipped on location expiry/reboot cycles. Also fixes related event cleanup issues: - Add missing Queue.shutdown to opencode SSE handler finalizer - Collect and finalize share-next event listener unsubscribes Fixesanomalyco#36677
Contributor
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
Shalin-Shah-2002
commented
Jul 26, 2026
Author
This PR is ready for review. All tests pass (1080/1080), typecheck is clean across all 36 packages, and the fix directly addresses the root cause of the allocation loop described in #36677. |
renekris added a commit
to renekris/opencode-lowmem
that referenced
this pull request
Aug 22, 2026
…ray semantics Deviations from the ported upstream hunks, per review: - anomalyco#43881: the empty-stream guard now fails only when the attempt produced no text/reasoning delta and no tool-call event. Providers may stream real content but omit the usage block and finish reason; retrying those would duplicate already-persisted output (up to 6 attempts). Empty-content deltas still count as empty, so the original clean-EOF retry is intact. - anomalyco#38939: listeners are an Array again. The Set container silently deduplicated identical callback registrations and removed every entry on the first unsubscribe; duplicates must deliver independently. - adds the missing multi-byte UTF-8 split regression for anomalyco#43607's streaming TextDecoder
renekris added a commit
to renekris/opencode-lowmem
that referenced
this pull request
Aug 22, 2026
…port of upstream anomalyco#38939) allBounded now owns a bounded dropping queue instead of leasing the unbounded global listener stream. Deviation from the upstream patch: listeners stay an Array (a Set would silently deduplicate identical callback registrations and remove every entry on first unsubscribe); covered by regression tests.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue for this PR
Closes#36677
Type of change
What does this PR do?
The root cause of #36677 is that
EventV2.allBoundedrelied onevents.listen(), which registers the callback in a globallistenersSet inside the EventV2 service. The cleanup of that registration depended onEffect.addFinalizerrunning inside the scope provided byStream.unwrap→Channel.unwrap. When that scope didn't close reliably (location expiry/reboot cycles, or SSE client disconnect patterns that skip channel finalization), the listener stayed in the Set permanently.What this means for a leaked listener: every call to
EventV2.notifyiterates the entirelistenersSet and calls each listener function. A leakedallBoundedlistener callsQueue.offeron every published event, allocating regardless of whether the queue is already failed. Over hours of uptime with repeated location reboots, these allocations push the JSC heap into continuous GC cycles — the 80% CPU / constant madvise behavior reported in the issue.The fix has three parts:
1. Move
allBoundedout ofevents.listen()and into the Interface directly.The old module-level function:
The new Interface method inside the layer:
The key difference:
PubSub.subscribe(pubsub.all)registers the subscriber directly on the PubSub, not in a separate global Set. The subscription is managed by Scope — when the scope closes (stream ends or is interrupted), the subscriber is removed atomically as part of PubSub's internal subscriber tracking. There is no separate cleanup step that can be skipped.PubSub.subscribeis also synchronous — it returns the subscriber immediately after registering it. This means there is no race between subscribing and publishing events, so the existing test (which creates bounded streams then immediately publishes) continues to pass.2. Added missing
Queue.shutdownin the opencode SSE handler finalizer.At
packages/opencode/src/server/routes/instance/httpapi/handlers/event.ts:32, the finalizer was callingunsubscribebut neverQueue.shutdown(queue). This meant the unbounded queue survived the SSE disconnect, holding references to buffered events.3. Collected unsubscription effects in share-next
watchhelper.At
packages/opencode/src/share/share-next.ts, thewatchfunction calledevents.listen()but never stored or ran the returnedUnsubscribeeffect. The listeners accumulated for the process lifetime. The fix stores each unsubscribe and registers them as a scope finalizer.How did you verify your code works?
ends only an overflowing bounded subscriber without blocking other listenerswhich tests the exact bounded-stream pattern used in production.packages/coreandpackages/server.bun turbo typecheckpasses across all 36 packages.Screenshots / recordings
Not applicable — logic change only.
Checklist