fix(core): add dispose functions to prevent subscription memory leaks #7914

hendem · 2026-01-12T05:31:21Z

Summary

Re-implementation of #7032 rebased cleanly on current dev branch (original had merge conflicts).

Add dispose() functions to Share, ShareNext, Plugin, and Format namespaces
Add cleanupSession() and dispose() to ACP Agent for event subscription cleanup
Add Bus._getSubscriptionCount() test helpers
Add memory tests to verify subscription cleanup works correctly

Problem

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:

ShareNext: Tracks all 4 subscriptions, clears pending timeout queue on dispose
Share: Tracks subscription with disposed flag check
Plugin/Format: Store and cleanup subscription references
ACP Agent: Uses AbortController per session to cancel event streams

All 12 memory tests pass.

Supersedes #7032
Fixes #3013

github-actions · 2026-01-12T05:31:47Z

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
#7032

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

Copilot

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() and dispose() 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.

packages/opencode/src/share/share.ts

packages/opencode/src/share/share-next.ts

packages/opencode/src/acp/agent.ts

packages/opencode/src/plugin/index.ts

packages/opencode/test/memory/subscription-cleanup.test.ts

packages/opencode/src/bus/index.ts

packages/opencode/src/share/share-next.ts

packages/opencode/src/share/share.ts

- 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

Copilot

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.

packages/opencode/test/memory/profile.ts

packages/opencode/src/acp/agent.ts

packages/opencode/src/share/share.ts

packages/opencode/src/acp/agent.ts

packages/opencode/src/share/share-next.ts

packages/opencode/test/memory/acp-cleanup.test.ts

packages/opencode/test/memory/subscription-cleanup.test.ts

packages/opencode/src/share/share-next.ts

packages/opencode/src/share/share.ts

- 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

Copilot

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.

packages/opencode/src/share/share-next.ts

packages/opencode/test/memory/profile.ts

…ription

Copilot

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.

…ference

…rence

Copilot

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.

packages/opencode/src/share/share-next.ts

Copilot

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

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

packages/opencode/src/share/share-next.ts:188

There's a race condition between lines 166-188. A timeout is created and then added to the queue. If dispose() is called between setTimeout and queue.set(), the timeout won't be in the queue yet and won't be cleared. The timeout will then fire later and access the (now cleared) queue. While the generation check at line 168 will prevent the operation, the timeout still fires unnecessarily. Consider adding the timeout to the queue before or atomically with its creation.

    const timeout = setTimeout(async () => {
      // Check generation before processing queued data
      if (gen !== generation) return
      const queued = queue.get(sessionID)
      if (!queued) return
      queue.delete(sessionID)
      const share = await get(sessionID).catch(() => undefined)
      if (!share) return
      // Check generation after async operation
      if (gen !== generation) return

      await fetch(`${await url()}/api/share/${share.id}/sync`, {
        method: "POST",
        headers: {
          "Content-Type": "application/json",
        },
        body: JSON.stringify({
          secret: share.secret,
          data: Array.from(queued.data.values()),
        }),
      })
    }, 1000)
    queue.set(sessionID, { timeout, data: dataMap })

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

Copilot · 2026-01-12T17:28:27Z

packages/opencode/src/share/share-next.ts

+    // Use short timeout for tests - this is a no-op callback that won't cause issues if it fires
+    const timeout = setTimeout(() => {}, 100)
+    queue.set(sessionID, { timeout, data: dataMap })


The test helper function _addToQueueForTesting creates a timeout with an empty callback that fires after 100ms. While the comment says it's a "no-op callback that won't cause issues if it fires," the timeout will still consume resources and fire during test execution. Consider using a much longer timeout (e.g., 10000ms) or clearTimeout immediately after adding to the queue if the goal is just to test disposal without the timeout actually firing.

Suggested change

// Use short timeout for tests - this is a no-op callback that won't cause issues if it fires

const timeout = setTimeout(() => {}, 100)

queue.set(sessionID, { timeout, data: dataMap })

// Use timeout handle for testing dispose cleanup, but clear it immediately so it never fires

const timeout = setTimeout(() => {}, 100)

queue.set(sessionID, { timeout, data: dataMap })

clearTimeout(timeout)

Copilot · 2026-01-12T17:28:28Z

packages/opencode/src/share/share-next.ts

  export async function init() {
-    Bus.subscribe(Session.Event.Updated, async (evt) => {
-      await sync(evt.properties.info.id, [
+    // Clean up any existing subscriptions before adding new ones
+    dispose()
+    disposed = false
+    // Increment generation so in-flight operations from previous cycle are invalidated
+    const gen = ++generation


The async init() function calls dispose() synchronously at the start, but then continues with async operations. If multiple init() calls happen concurrently, they could all pass the dispose() gate and create duplicate subscriptions. The same issue exists in Share.ts but is more critical here since init() is async and takes longer to complete.

Copilot · 2026-01-12T17:28:28Z

packages/opencode/test/memory/acp-cleanup.test.ts

+              stream: (async function* () {
+                // Use finite loop with abort check to prevent background runaway
+                for (let i = 0; i < 100 && !genController.signal.aborted; i++) {
+                  await new Promise((r) => setTimeout(r, 100))
+                  if (genController.signal.aborted) break
+                  yield { type: "test" }
+                }
+                activeGenerators.delete(genController)


The test creates async generators that run for up to 100 iterations with 100ms delays (potentially 10 seconds). Even with the abort check, this creates background async work that may not complete before the test ends. The 50ms wait at line 163 may not be sufficient for all generators to exit cleanly. Consider reducing the iteration count or delay time to ensure generators complete within a reasonable timeframe, or increase the cleanup wait time.

Copilot · 2026-01-12T17:28:28Z

packages/opencode/src/share/share.ts

+
+  export function dispose() {
+    disposed = true
+    const toUnsubscribe = unsubscribers.splice(0)


The unsubscribers array is shared across multiple init() calls, but the splice(0) operation in dispose() may not be thread-safe if init() is called concurrently during dispose(). While JavaScript is single-threaded, async operations could interleave. Consider saving the reference before splicing to avoid potential race conditions where new subscribers are added during disposal.

Copilot · 2026-01-12T17:28:28Z

packages/opencode/src/share/share.ts

+    // Clean up any existing subscriptions before adding new ones
+    dispose()
+    disposed = false
+    // Increment generation so in-flight operations from previous cycle are invalidated
+    const gen = ++generation


The init() function calls dispose() at the start but then immediately sets disposed=false and increments generation before creating new subscriptions. However, there's no protection against concurrent init() calls. If init() is called twice simultaneously, both calls will increment generation and create duplicate subscriptions that won't all be tracked for cleanup. Consider adding a guard to prevent concurrent initialization.

Copilot · 2026-01-12T17:28:29Z

packages/opencode/src/share/share-next.ts

+    const toUnsubscribe = unsubscribers.splice(0)
+    for (const unsub of toUnsubscribe) {
+      try {
+        unsub()
+      } catch (error) {
+        log.error("failed to unsubscribe", { error })
+      }
+    }
+    // Hardened: snapshot and clear atomically to avoid race during iteration
+    const pending = Array.from(queue.values())
+    queue.clear()
+    for (const entry of pending) {
+      clearTimeout(entry.timeout)
+    }


The same concurrency issue exists here as in Share.ts. The unsubscribers.splice(0) operation could race with init() adding new subscribers. Additionally, the queue clearing logic snapshots the queue after clearing it, which means timeouts being added during this operation could be missed.

Suggested change

const toUnsubscribe = unsubscribers.splice(0)

for (const unsub of toUnsubscribe) {

try {

unsub()

} catch (error) {

log.error("failed to unsubscribe", { error })

}

}

// Hardened: snapshot and clear atomically to avoid race during iteration

const pending = Array.from(queue.values())

queue.clear()

for (const entry of pending) {

clearTimeout(entry.timeout)

}

// Drain the live unsubscribers array so handlers added during disposal

// are also cleaned up before we return.

while (unsubscribers.length > 0) {

const unsub = unsubscribers.pop()

if (!unsub) {

continue

}

try {

unsub()

} catch (error) {

log.error("failed to unsubscribe", { error })

}

}

// Clear all pending timeouts from the live queue and then empty it.

for (const entry of queue.values()) {

clearTimeout(entry.timeout)

}

queue.clear()

Copilot · 2026-01-12T17:28:29Z

packages/opencode/src/share/share-next.ts

          data: evt.properties.info,
        },
      ])
+      if (gen !== generation) return


There's an inconsistency in generation checks. Line 48 checks 'if (gen !== generation) return' after the first sync call, but line 32 checks both 'disposed' and 'gen !== generation' at the start of the subscription handler. For consistency and safety, all early returns should check both conditions or have a clear reason for checking only one.

Suggested change

if (gen !== generation) return

if (disposed || gen !== generation) return

Copilot AI review requested due to automatic review settings January 12, 2026 05:31

hendem mentioned this pull request Jan 12, 2026

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

Open

Copilot started reviewing on behalf of hendem January 12, 2026 05:31 View session

hendem force-pushed the fix/memory-leak-subscription-cleanup-v2 branch from a00b2cb to 8142552 Compare January 12, 2026 05:35

Copilot AI reviewed Jan 12, 2026

View reviewed changes

fix: address Copilot review feedback

3fb83f7

- 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

hendem requested a review from Copilot January 12, 2026 05:53

Copilot started reviewing on behalf of hendem January 12, 2026 05:54 View session

Merge branch 'dev' into fix/memory-leak-subscription-cleanup-v2

9bcdac9

Copilot AI reviewed Jan 12, 2026

View reviewed changes

hendem requested a review from Copilot January 12, 2026 06:24

Copilot started reviewing on behalf of hendem January 12, 2026 06:24 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

packages/opencode/src/share/share-next.ts Show resolved Hide resolved

packages/opencode/test/memory/profile.ts Outdated Show resolved Hide resolved

Mark Henderson added 2 commits January 11, 2026 23:31

fix: update profile.ts to use renamed method cleanupSessionEventSubsc…

663546a

…ription

fix: harden share-next dispose to avoid race on queue iteration

48a68e6

hendem force-pushed the fix/memory-leak-subscription-cleanup-v2 branch from f2181ae to 48a68e6 Compare January 12, 2026 06:34

hendem requested a review from Copilot January 12, 2026 06:34

Copilot started reviewing on behalf of hendem January 12, 2026 06:35 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Mark Henderson added 2 commits January 11, 2026 23:37

fix: use generation counter in share-next to prevent cross-init inter…

d1b5d6e

…ference

fix: use generation counter in share.ts to prevent cross-init interfe…

7b271ab

…rence

hendem requested a review from Copilot January 12, 2026 06:44

Copilot started reviewing on behalf of hendem January 12, 2026 06:44 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

packages/opencode/src/share/share-next.ts Show resolved Hide resolved

Copilot AI reviewed Jan 12, 2026

View reviewed changes

Merge branch 'dev' into fix/memory-leak-subscription-cleanup-v2

76f0ea6

hendem requested a review from Copilot January 12, 2026 17:23

Copilot started reviewing on behalf of hendem January 12, 2026 17:24 View session

Copilot AI reviewed Jan 12, 2026

View reviewed changes

	if (gen !== generation) return
	if (disposed \|\| gen !== generation) return

fix(core): add dispose functions to prevent subscription memory leaks #7914

Are you sure you want to change the base?

fix(core): add dispose functions to prevent subscription memory leaks #7914

Uh oh!

Conversation

hendem commented Jan 12, 2026

Summary

Problem

Solution

Uh oh!

github-actions bot commented Jan 12, 2026

Potential Related PR Found:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 12, 2026