fix(core): add dispose functions to prevent subscription memory leaks #7914
Conversation
|
The following comment was made by an LLM, it may be inaccurate: Potential Related PR Found:PR #7032 - fix(core): add dispose functions to prevent subscription memory leaks Why it's related: This is the original PR that the current PR #7914 is based on. According to the description, PR #7914 is a re-implementation of #7032 that was rebased cleanly on the current dev branch (the original had merge conflicts). PR #7914 explicitly states "Supersedes #7032" in its description, meaning it replaces and obsoletes the original PR. |
- 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
a00b2cb to
8142552
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds dispose() functions to prevent Bus subscription memory leaks that accumulate during extended use. The implementation tracks unsubscribe functions returned by Bus.subscribe() and provides cleanup methods for Share, ShareNext, Plugin, Format namespaces, and ACP Agent.
Changes:
- Added
dispose()methods to Share, ShareNext, Plugin, and Format namespaces to clean up Bus subscriptions - Added
cleanupSession()anddispose()to ACP Agent using AbortController pattern - Added Bus test helper methods
_getSubscriptionCount()and_getTotalSubscriptionCount() - Added comprehensive memory tests and profiling scripts
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/bus/index.ts | Added test helper methods to get subscription counts |
| packages/opencode/src/share/share.ts | Added dispose() to clean up 3 Bus subscriptions, with disposed flag checks |
| packages/opencode/src/share/share-next.ts | Added dispose() to clean up 4 subscriptions and clear pending timeout queue |
| packages/opencode/src/format/index.ts | Added dispose() to clean up File.Event.Edited subscription |
| packages/opencode/src/plugin/index.ts | Added dispose() to clean up wildcard Bus subscription |
| packages/opencode/src/acp/agent.ts | Added AbortController-based cleanup for session event subscriptions |
| packages/opencode/test/memory/subscription-cleanup.test.ts | Comprehensive tests verifying subscription cleanup works correctly |
| packages/opencode/test/memory/profile.ts | Memory profiling script to verify no leaks over 1000 cycles |
| packages/opencode/test/memory/acp-cleanup.test.ts | Tests for ACP Agent session cleanup logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add multiple disposed checks in Share.sync() after async boundaries - Use splice(0) instead of length = 0 for clearer array clearing - Reduce test helper timeout from 10s to 100ms - Use Array.from() for iteration safety in Bus test helpers
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add try-catch around unsubscribe calls to ensure cleanup completes even if one fails - Use splice(0) pattern consistently before iteration for safe array clearing - Rename cleanupSession to cleanupSessionEventSubscription for clarity - Add try-finally to ACP test for generator cleanup on test failure - Fix Format.dispose() to use splice(0) for consistency with other modules
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f2181ae to
48a68e6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new 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 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Re-implementation of #7032 rebased cleanly on current dev branch (original had merge conflicts).
dispose()functions to Share, ShareNext, Plugin, and Format namespacescleanupSession()anddispose()to ACP Agent for event subscription cleanupBus._getSubscriptionCount()test helpersProblem
Bus subscriptions accumulate during extended use because return values from
Bus.subscribe()were ignored. This contributes to memory exhaustion during long sessions.Solution
Each module now stores unsubscribe functions and provides
dispose()to clean them up:AbortControllerper session to cancel event streamsAll 12 memory tests pass.
Supersedes #7032
Fixes #3013