fix(core): add dispose functions to prevent subscription memory leaks #7032
Conversation
|
The following comment was made by an LLM, it may be inaccurate: SummaryNo duplicate PRs found. All searches returned only PR #7032 (the current PR), with one additional unrelated result about Intent abstraction (PR #6549). The searches covered:
This appears to be a unique PR addressing subscription memory leaks through new dispose functionality. |
There was a problem hiding this comment.
Pull request overview
This PR adds dispose() functions to multiple namespaces to prevent Bus subscription memory leaks that were causing Bun to run out of memory during extended use. The implementation adds cleanup mechanisms for Share, ShareNext, Plugin, and Format modules, along with AbortController-based session cleanup for the ACP Agent.
Key Changes:
- Added
dispose()functions that unsubscribe from Bus events across Share, ShareNext, Plugin, and Format namespaces - Implemented AbortController-based cleanup for ACP Agent session event subscriptions
- Added Bus test helpers
_getSubscriptionCount()and_getTotalSubscriptionCount()for testing
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
packages/opencode/src/bus/index.ts |
Added internal test helper functions to query subscription counts |
packages/opencode/src/share/share.ts |
Added subscriptions array and dispose() function to clean up Bus subscriptions and pending queue |
packages/opencode/src/share/share-next.ts |
Added subscriptions array and dispose() function to clean up Bus subscriptions and queued timeouts |
packages/opencode/src/plugin/index.ts |
Added subscriptions array and dispose() function to clean up wildcard event subscriptions |
packages/opencode/src/format/index.ts |
Added subscriptions array and dispose() function to clean up File.Event.Edited subscriptions |
packages/opencode/src/acp/agent.ts |
Added sessionAbortControllers map, cleanupSession() and dispose() methods to manage session-scoped event subscriptions using AbortControllers |
packages/opencode/test/memory/subscription-cleanup.test.ts |
Added comprehensive unit tests verifying dispose functions work correctly for all modules |
packages/opencode/test/memory/profile.ts |
Added memory profiling script to validate no memory leaks occur during init/dispose cycles |
packages/opencode/test/memory/acp-cleanup.test.ts |
Added tests for ACP Agent session cleanup and AbortController management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
All Copilot review feedback has been addressed:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
packages/opencode/src/acp/agent.ts:616
- This PR removes the
rawInputfield from the tool_call_update during session replay when status is "completed". While this field is present in live event handling, removing it from replay creates inconsistency. The ACP client may not be able to display the tool's input parameters when viewing historical completed tool calls. This change appears unrelated to the memory leak fix and should be in a separate PR or justified in the description.
update: {
sessionUpdate: "tool_call_update",
toolCallId: part.callID,
status: "completed",
kind,
content,
title: part.state.title,
rawOutput: {
output: part.state.output,
metadata: part.state.metadata,
},
},
packages/opencode/src/acp/agent.ts:643
- This PR removes the
kind,title, andrawInputfields from the tool_call_update during session replay when status is "failed". While these fields are present in live event handling, removing them from replay creates inconsistency. The ACP client may not be able to properly display failed tool call information when viewing session history. This change appears unrelated to the memory leak fix and should be in a separate PR or justified in the description.
await this.connection
.sessionUpdate({
sessionId,
update: {
sessionUpdate: "tool_call_update",
toolCallId: part.callID,
status: "failed",
content: [
{
type: "content",
content: {
type: "text",
text: part.state.error,
},
},
],
rawOutput: {
error: part.state.error,
},
},
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add sessionAbortControllers and cleanupSession() to ACP Agent for event subscription cleanup - Add subscriptions array and dispose() to Share, ShareNext, Plugin, and Format namespaces - Add Bus._getSubscriptionCount() and Bus._getTotalSubscriptionCount() test helpers - Add memory tests to verify subscription cleanup works correctly Fixes memory leak where Bus subscriptions accumulated during extended use, eventually causing Bun to run out of memory.
…re-init - Add dispose() call at start of init() in Share, ShareNext, Format, Plugin to prevent subscription accumulation if init() is called multiple times - Add .catch() handler to ACP event subscription promise to log errors - Fix race condition in ACP finally block by checking controller identity - Add test for multiple init() calls not accumulating subscriptions
- Add _getQueueSize() test helper to ShareNext - Add assertion to verify queue is cleared after dispose()
Added _addToQueueForTesting helper and a new test that verifies dispose() properly clears queue items with pending timeouts.
…eanup - Reset queue Promise chain in Share.dispose() to fully clean up state - Fix async generator in test to use finite loop with abort signal check to prevent background runaway after test completion
…or directory Since directory is optional in the SDK, we can always call abort even if session is not found in the manager. This ensures the SDK is never left in an inconsistent state.
…dispose to connection close - Refactor share.ts, share-next.ts, format/index.ts, plugin/index.ts to use Instance.state() with dispose callbacks for automatic subscription cleanup - Add connection.closed handler in ACP Agent constructor to call dispose() when connection ends - Update test mocks to include closed property
- Have Instance.state dispose callbacks delegate to exported dispose() functions to avoid duplication - Reorder init() to call dispose() before getting state reference for clearer semantics - Add afterAll cleanup in subscription-cleanup.test.ts to clean up temp directory - Wrap profile.ts test functions in Instance.provide for proper Instance context - Add closed property to ACP mock in profile.ts
- Perform Instance.state dispose cleanup inline to prevent state() reinitialization during Instance disposal - Add AbortController to cancel in-flight fetch requests during dispose - Pass abort signal to SDK event.subscribe to properly cancel SSE stream - Update comments to accurately reflect full dispose behavior - Add proper async generator cleanup in test
…and adding abort checks
fwang
force-pushed
the
fix/memory-leak-subscription-cleanup
branch
from
f5297c8 to
0bcf7bb
Compare
- Add dispose() to Share, ShareNext, Plugin, and Format namespaces - Add cleanupSession() and dispose() to ACP Agent with AbortControllers - Add Bus._getSubscriptionCount() test helpers - Add memory tests to verify cleanup works correctly Supersedes anomalyco#7032 Fixes anomalyco#3013
|
This PR has been superseded by #7914, which is a clean re-implementation on current dev (this PR had merge conflicts that were complex to resolve). The new PR includes the same functionality:
Leaving this PR open per request, but #7914 should be reviewed instead. |
- Add dispose() to Share, ShareNext, Plugin, and Format namespaces - Add cleanupSession() and dispose() to ACP Agent with AbortControllers - Add Bus._getSubscriptionCount() test helpers - Add memory tests to verify cleanup works correctly Supersedes anomalyco#7032 Fixes anomalyco#3013
2 participants
Fixes #3013
Summary
dispose()functions to Share, ShareNext, Plugin, and Format namespaces to clean up Bus subscriptionscleanupSession()anddispose()to ACP Agent for event subscription cleanup using AbortControllersBus._getSubscriptionCount()andBus._getTotalSubscriptionCount()test helpersProblem
Bus subscriptions were accumulating during extended use because the return values from
Bus.subscribe()were being ignored. Over time, this caused memory growth that could contribute to Bun running out of memory.Solution
Each module that subscribes to Bus events now:
Bus.subscribe()dispose()function that calls all stored unsubscribe functionsFor ACP Agent specifically, session-scoped subscriptions use AbortControllers so they can be cleaned up when a session ends.
Testing
Added 10 unit tests in
test/memory/that verify:dispose()properly unsubscribesAll tests pass:
bun test test/memory/