Skip to content

Conversation

@hendem
Copy link

@hendem hendem commented Jan 6, 2026

Summary

This PR wires up cleanup to be called when ACP connections end, ensuring memory is properly freed.

Problem

When an ACP connection ends (e.g., IDE closes, client disconnects):

  • Event subscription loops (for await in setupEventSubscriptions) run forever
  • Session state in ACPSessionManager.sessions Map accumulates indefinitely
  • sessionAbortControllers never get cleaned up

This leads to the memory leak crashes reported in #4315 and #5363.

Solution

  1. ACP.Agent.dispose() - New method that:

    • Aborts all active event subscriptions via AbortController
    • Clears all session state from the session manager
    • Logs cleanup for debugging
  2. AbortController pattern in setupEventSubscriptions() - Each session's event subscription loop now:

    • Has an associated AbortController
    • Checks controller.signal.aborted in the loop
    • Cleans up controller reference in finally block
    • Handles errors gracefully during abort
  3. Cleanup wired to connection lifecycle - In acp.ts command:

    • Agent instance is captured when created
    • agent.dispose() is called after stdin ends
    • Ensures cleanup happens even on unexpected disconnection

Changes

File Change
src/acp/agent.ts Add dispose(), cleanupSession(), AbortController pattern
src/acp/session.ts Add remove(), size(), sessionIds(), clear() methods
src/cli/cmd/acp.ts Capture agent instance and call dispose() on connection end
test/acp/session.test.ts Tests for session manager cleanup methods

Testing

  • 5 tests added for ACPSessionManager
  • All tests pass
  • Typecheck passes
  • Build passes

Related Issues

Related PRs

- Add dispose() method to ACP.Agent that aborts all event subscriptions and clears session state
- Add AbortController pattern to setupEventSubscriptions() for clean shutdown
- Add cleanupSession() to abort individual session subscriptions
- Wire up Agent.dispose() call when ACP command stdin closes
- Add remove(), size(), sessionIds(), clear() methods to ACPSessionManager

This ensures that when an ACP connection ends, all event subscription loops
are properly terminated and session state is cleaned up, preventing memory
accumulation during long-running ACP sessions.

Related to anomalyco#4315, anomalyco#5363
Copilot AI review requested due to automatic review settings January 6, 2026 05:15
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2026

The following comment was made by an LLM, it may be inaccurate:

Summary

No duplicate PRs found. However, PR #7043 is part of a related series:

Related PRs (Not Duplicates):

  1. fix(core): add dispose functions to prevent subscription memory leaks #7032 - fix(core): add dispose functions to prevent subscription memory leaks

    • Adds cleanup methods to Share, ShareNext, Format, Plugin bus subscriptions
    • Referenced in the PR description as foundational work
  2. fix: add cleanup mechanisms to prevent memory leaks #7039 - fix: add cleanup mechanisms to prevent memory leaks

    • Adds session/state cleanup methods to ACPSessionManager, Rpc timeout, FileTime, OAuth TTL
    • Also referenced as foundational work

Relationship:

PR #7043 complements #7032 and #7039 by actually wiring up the cleanup to be called when ACP connections end, rather than just adding the cleanup methods. The three PRs form a complete solution to the memory leak issues (#4315, #5363).

Conclusion: PR #7043 is the integration point that makes the previous cleanup work actually functional—not a duplicate.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses memory leaks in ACP agent connections by implementing a cleanup mechanism that runs when connections end. The fix adds disposal methods, introduces AbortController-based event subscription management, and wires cleanup to the connection lifecycle.

  • Adds dispose() method to the Agent class to abort event subscriptions and clear session state
  • Implements AbortController pattern for managing event subscription lifecycles
  • Extends ACPSessionManager with cleanup methods (remove, size, sessionIds, clear)

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
packages/opencode/src/acp/agent.ts Adds dispose(), cleanupSession(), AbortController tracking, and abort signal checks in event subscription loop
packages/opencode/src/acp/session.ts Adds session cleanup methods for memory management
packages/opencode/src/cli/cmd/acp.ts Captures agent instance and calls dispose() when stdin ends
packages/opencode/test/acp/session.test.ts Adds comprehensive tests for new ACPSessionManager cleanup methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hendem
Copy link
Author

hendem commented Jan 6, 2026

Addressing Review Comments

Re: break statements in finally blocks (lines 168, 372)
Fixed - moved the break statements outside of the finally blocks. The break is still needed to prevent switch fallthrough, but placing it in finally was causing early loop termination. Also added proper catch blocks for error handling.

Re: AbortController signal not passed to subscribe()
Good point. The SDK's event.subscribe() doesn't currently support an AbortSignal parameter. However, I've added a check for both controller.signal.aborted and this.disposed at the top of the event loop, which will break out on the next iteration after abort/dispose. While this doesn't immediately close the underlying stream, it prevents further processing and allows graceful exit.

Re: disposed checks in async operations
Fixed - added a this.disposed check at the top of the event processing loop alongside the abort signal check. This ensures that after dispose() is called, no further events will be processed.

@hendem
Copy link
Author

hendem commented Jan 6, 2026

Additional fix for disposed checks:

Added more comprehensive this.disposed checks:

  1. At the start of each event case (permission.asked, message.part.updated) to prevent starting new operations
  2. After key await calls to prevent follow-up operations if dispose() was called during the await

This ensures that in-flight async operations will exit gracefully rather than continuing to use potentially invalid resources.

@hendem
Copy link
Author

hendem commented Jan 6, 2026

Fixed: AbortController signal now passed to subscribe()

After investigating, the SDK's event.subscribe() does support a signal option. The SSE client uses it to:

  1. Check signal.aborted at the start of each retry loop
  2. Register an abort handler that calls reader.cancel() to properly close the stream

Updated the code to pass { signal: controller.signal } to subscribe(), which ensures the stream is properly closed when dispose() or cleanupSession() is called.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Mark Henderson and others added 2 commits January 6, 2026 21:29
Address Copilot review comments - ensure all test SDK mocks have
consistent interface with both create and get methods.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 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.

@hendem
Copy link
Author

hendem commented Jan 9, 2026

Closing as superseded by #7032 which now includes the ACP agent cleanup functionality (dispose method, sessionAbortControllers, cleanupSession, and connection.closed handler). The session manager utility methods (remove, size, sessionIds, clear) can be added in a follow-up PR if needed.

@hendem hendem closed this Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant