fix(opencode): stop per-instance background services on dispose - #217
Merged
ualtinok merged 4 commits intoSep 14, 2026
Merged
Conversation
…rocess The plugin kept one RPC server handle per process on `globalThis.__anthropicAuthRpcServer`. Every plugin instantiation stopped the existing handle first, and `stop()` unlinks `port-<pid>.json` from the directory that handle was started with. So once a process instantiated the plugin for a second project directory, the first project's port file was deleted and its server stopped: that project's TUI found nothing to poll and `/claude-*` opened no modal for the life of the process, with no error anywhere. One process legitimately serves several directories -- request routing accepts `?directory=`/`x-opencode-directory` and the plugin factory is scoped per directory (opencode `plugin/index.ts:134-179`, `workspace-routing.ts:86-88`). Key the servers by resolved RPC directory instead. Re-instantiating the same directory stops and replaces as before; a different directory starts an additional server and leaves the others alone. Bound the registry with `Hooks.dispose` (declared in `@opencode-ai/plugin` 1.18.21 and invoked by opencode's instance finalizer, `plugin/index.ts:265-278`; disposers also run on per-project reload, `project/instance-store.ts:126-145`). Teardown acts only when the registry entry is still its own handle: the port filename is identical for every server a process starts in one directory, so a dispose arriving after a same-directory replace would otherwise unlink the live successor's port file and put that project back in the dark. Cleanup steps are isolated from the RPC shutdown so a failing one cannot skip it. `stop()` unlinks the port file only when it still names its own port and pid, so a stale handle cannot remove a successor's file even if a future path forgets the identity check. The two defences protect the same observable and would mask each other, so each has its own test and each was mutated alone: removing the identity check reddens only the dispose-behaviour test, removing the port/pid match reddens only the stale-server test. A third test drives real HTTP against both directories' servers and asserts each answers through its own instance's closure -- a regression collapsing them onto one shared closure passes every other test here.
One notification queue serves every RPC server in the process, so anything in it that is not keyed by session is keyed by nothing once a process holds more than one project. Require a session id on queued notices. A notice without one was delivered to every draining TUI and survived one session's ack, which across servers means crossing project boundaries. Require a session id on the connectivity probe and delete the process-wide `lastDrainAtAny` it fell back on. That timestamp was written by every project's drain, so an unscoped call could report a TUI connected for project A because project B's TUI polled -- and the caller skips the desktop fallback when it believes a TUI is present, so the notice would go nowhere. Both call sites already passed an id; this removes the possibility rather than relying on it. Stop an unscoped drain from deleting other sessions' notices. It pruned every acknowledged notice regardless of owner, so one client both swallowed and destroyed other sessions' pending dialogs. This predates the per-directory registry -- with a single server it was cross-session inside one project -- and is fixed as deliver-but-never-prune: the TUI has sent a session id since polling was introduced, so the clients that can omit it are malformed or third-party ones, and rejecting them would fail by silently not delivering, which is the symptom this change exists to fix. With no producer able to queue a session-less notice, the matching branch in the drain filter is unreachable and removed. The wire field stays optional so an older TUI still parses what it is sent.
The per-directory registry __anthropicAuthRpcServers supersedes the process-wide __anthropicAuthRpcServer handle. Production never read the singular identifier for a decision; the conditional clear was a clear-if-mine that no reader depended on, and the dispose never cleared it on teardown, leaving a dangling reference to a stopped server for the lifetime of the process. The test helper gains a per-directory cleanup proof that, for every rpc dir this file started a server in, the registry holds no entry for it and its port-<pid>.json is gone. A post-build gate (packages/opencode/scripts/check-bundle-globals.ts) verifies the bundle still contains the registry identifier and has zero matches for the singular form, wired into the build script and runnable via bun run check:bundle.
The dispose hook previously stopped only the RPC server (plus the quota-header feed registry and Claustrum credential cache). Two per-instance services were never stopped: - fallbackManager (FallbackAccountManager.stopBackgroundRefresh) — kept polling quota and attempting token refreshes after dispose. - cacheKeepManager (CacheKeepManager.stop) — never called. The third service is shared via prime-manager-registry: dispose now calls releasePrimeManager(accountStoragePath, ctx.directory ?? 'default'), which removes this instance's slot and stops the shared PrimeManager only when the last slot is gone — preserving it for sibling projects on the same storage path. This is pre-existing; the per-instance background timers were already running before the RPC teardown was added. The RPC guard (early return when this instance no longer owns the registry entry) does not apply to per-instance cleanup, so the new steps run before that guard.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 10 files
Confidence score: 2/5
packages/opencode/src/prime-manager-registry.tscan remove a successor slot and stop its livePrimeManagerwhen an older instance disposes after re-adoption, potentially breaking active manager behavior; track an adoption generation or lease before releasing the slot.packages/opencode/src/index.tsleavesmainBackgroundRefreshTimerrunning after plugin disposal, so reloads can accumulate background refresh work and retain resources; clear it withruntimeTimers.clearIntervaland set it tonullduring disposal.packages/opencode/src/rpc/notifications.tsdrops legacy TUI modal notifications when polling withoutsessionId, causingcommand.execute.beforeto ignore messages; preserve an unspecific connection timestamp for this path.packages/opencode/src/tests/dispose-cleanup.test.tsdoes not actually exercise tracked timer cleanup because its timer overrides are no-ops, andpackages/opencode/src/tests/rpc-multi-project.test.tsleaves created plugins undisposed; wire the real tracked timer and dispose each test plugin to make leak coverage meaningful.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/prime-manager-registry.ts">
<violation number="1" location="packages/opencode/src/prime-manager-registry.ts:63">
P1: When a slot is re-adopted under the same storage fingerprint before an older instance disposes, this removes the successor's slot and stops its live `PrimeManager`. Track an adoption generation or lease and release only the matching adoption.</violation>
</file>
<file name="packages/opencode/src/rpc/notifications.ts">
<violation number="1" location="packages/opencode/src/rpc/notifications.ts:53">
P2: When a legacy TUI polls without `sessionId`, this function leaves no connection timestamp, so `command.execute.before` falls back to `sendIgnoredMessage` instead of enqueueing the modal notification. Preserve an unscoped liveness fallback for legacy clients or explicitly remove that compatibility path.</violation>
</file>
<file name="packages/opencode/src/index.ts">
<violation number="1" location="packages/opencode/src/index.ts:2859">
P1: After the auth loader starts `mainBackgroundRefreshTimer`, disposing this plugin never clears it. Clear `mainBackgroundRefreshTimer` with `runtimeTimers.clearInterval` and set it to `null` during disposal so reloads do not leave stale auth refresh workers running.</violation>
</file>
<file name="packages/opencode/src/tests/dispose-cleanup.test.ts">
<violation number="1" location="packages/opencode/src/tests/dispose-cleanup.test.ts:80">
P2: The leak-detection assertion in afterEach is vacuous: this file wires the plugin with disabledPluginTimerOverrides (no-op setInterval/clearInterval mocks) and never installs the trackedSetInterval that would populate activeIntervals, so `expect(activeIntervals.size).toBe(0)` can never fail. Because the PR's whole purpose is preventing real background timers from keeping the process alive, the test gives false assurance — a real timer leak on dispose would still pass. Wire the plugin with trackedSetInterval/trackedClearInterval (as the createTimerTracking helper intends) so an actual leaked interval is caught, or drop the vacuous assertion.</violation>
</file>
<file name="packages/opencode/src/tests/rpc-multi-project.test.ts">
<violation number="1" location="packages/opencode/src/tests/rpc-multi-project.test.ts:203">
P3: Several tests create plugins via getPlugin() but never call dispose(), so the per-instance background services this PR adds teardown for (fallback background refresh, cacheKeepManager, prime manager) are left running and are only masked by the setInterval mock in disabledPluginRuntimeOverrides. afterEach only stops RPC servers. Either call dispose() at the end of these tests (matching the 'dispose stops...' tests) or assert that the teardown happens, so the tests actually cover the lifecycle this PR implements and don't silently depend on the timer mock to avoid leaks.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Host as OpenCode Host
participant Plugin as Plugin Factory
participant Services as Background Services
participant Registry as Prime Manager Registry
participant RPC as RPC Server Map
participant DB as Account Storage
Note over Host,DB: Plugin Lifecycle & Resource Cleanup
Host->>Plugin: load plugin (per project)
Plugin->>Services: create FallbackAccountManager
Plugin->>Services: create CacheKeepManager
Plugin->>Registry: adoptPrimeManager(storagePath, slot)
Registry->>DB: get shared manager for storagePath
Registry-->>Plugin: return shared PrimeManager
Plugin->>RPC: startRpcServer for project directory
RPC-->>Plugin: RpcServerHandle
Note over Host,DB: Runtime Operation
Services->>Services: startBackgroundRefresh() (quota polling)
Services->>Services: cache keepalive (on demand)
Registry->>DB: prime polling (shared, multi-slot)
Note over Host,DB: Dispose (project unload)
Host->>Plugin: dispose()
Plugin->>Services: stopBackgroundRefresh()
Plugin->>Services: stop()
Plugin->>Registry: releasePrimeManager(storagePath, slot)
alt Last slot for storage path
Registry->>DB: stop() + evict manager
else Sibling slots still active
Registry->>DB: keep manager running
end
Plugin->>RPC: stop server for this directory
alt RPC handle is still current
RPC->>RPC: close server, remove port file
else Stale handle (replaced by successor)
RPC->>RPC: leave successor untouched
end
Note over Host,DB: Late release edge case
Host->>Plugin: dispose() (delayed/out-of-order)
Plugin->>Registry: releasePrimeManager(pathX, slotD)
alt Slot D now adopted under pathY
Registry->>DB: preserve successor mapping
else Slot D still original
Registry->>DB: clean slot mapping
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const fingerprint = primeStorageFingerprint(storagePath) | ||
| const entry = primeManagers.get(fingerprint) | ||
| if (entry) { | ||
| entry.slots.delete(slot) |
There was a problem hiding this comment.
P1: When a slot is re-adopted under the same storage fingerprint before an older instance disposes, this removes the successor's slot and stops its live PrimeManager. Track an adoption generation or lease and release only the matching adoption.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/prime-manager-registry.ts, line 63:
<comment>When a slot is re-adopted under the same storage fingerprint before an older instance disposes, this removes the successor's slot and stops its live `PrimeManager`. Track an adoption generation or lease and release only the matching adoption.</comment>
<file context>
@@ -52,3 +52,24 @@ export function adoptPrimeManager(
+ const fingerprint = primeStorageFingerprint(storagePath)
+ const entry = primeManagers.get(fingerprint)
+ if (entry) {
+ entry.slots.delete(slot)
+ if (entry.slots.size === 0) {
+ entry.manager.stop()
</file context>
| // rest of the process. Each step is isolated: one failure cannot skip | ||
| // the others. | ||
| try { | ||
| fallbackManager.stopBackgroundRefresh() |
There was a problem hiding this comment.
P1: After the auth loader starts mainBackgroundRefreshTimer, disposing this plugin never clears it. Clear mainBackgroundRefreshTimer with runtimeTimers.clearInterval and set it to null during disposal so reloads do not leave stale auth refresh workers running.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/index.ts, line 2859:
<comment>After the auth loader starts `mainBackgroundRefreshTimer`, disposing this plugin never clears it. Clear `mainBackgroundRefreshTimer` with `runtimeTimers.clearInterval` and set it to `null` during disposal so reloads do not leave stale auth refresh workers running.</comment>
<file context>
@@ -2806,27 +2809,88 @@ const anthropicAuthPlugin = async (
+ // rest of the process. Each step is isolated: one failure cannot skip
+ // the others.
+ try {
+ fallbackManager.stopBackgroundRefresh()
+ } catch (error) {
+ logger.warn('fallback-background', 'failed to stop', {
</file context>
Suggested change
| fallbackManager.stopBackgroundRefresh() | |
| fallbackManager.stopBackgroundRefresh() | |
| if (mainBackgroundRefreshTimer) { | |
| runtimeTimers.clearInterval(mainBackgroundRefreshTimer) | |
| mainBackgroundRefreshTimer = null | |
| } |
| } | ||
|
|
||
| export function isTuiConnected(sessionId?: string): boolean { | ||
| export function isTuiConnected(sessionId: string): boolean { |
There was a problem hiding this comment.
P2: When a legacy TUI polls without sessionId, this function leaves no connection timestamp, so command.execute.before falls back to sendIgnoredMessage instead of enqueueing the modal notification. Preserve an unscoped liveness fallback for legacy clients or explicitly remove that compatibility path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/rpc/notifications.ts, line 53:
<comment>When a legacy TUI polls without `sessionId`, this function leaves no connection timestamp, so `command.execute.before` falls back to `sendIgnoredMessage` instead of enqueueing the modal notification. Preserve an unscoped liveness fallback for legacy clients or explicitly remove that compatibility path.</comment>
<file context>
@@ -21,34 +30,35 @@ export function drainNotifications(
}
-export function isTuiConnected(sessionId?: string): boolean {
+export function isTuiConnected(sessionId: string): boolean {
const now = Date.now()
- if (sessionId !== undefined) {
</file context>
| await rm(tempDir, { recursive: true, force: true }).catch(() => {}) | ||
| } | ||
| } finally { | ||
| expect(activeIntervals.size).toBe(0) |
There was a problem hiding this comment.
P2: The leak-detection assertion in afterEach is vacuous: this file wires the plugin with disabledPluginTimerOverrides (no-op setInterval/clearInterval mocks) and never installs the trackedSetInterval that would populate activeIntervals, so expect(activeIntervals.size).toBe(0) can never fail. Because the PR's whole purpose is preventing real background timers from keeping the process alive, the test gives false assurance — a real timer leak on dispose would still pass. Wire the plugin with trackedSetInterval/trackedClearInterval (as the createTimerTracking helper intends) so an actual leaked interval is caught, or drop the vacuous assertion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/dispose-cleanup.test.ts, line 80:
<comment>The leak-detection assertion in afterEach is vacuous: this file wires the plugin with disabledPluginTimerOverrides (no-op setInterval/clearInterval mocks) and never installs the trackedSetInterval that would populate activeIntervals, so `expect(activeIntervals.size).toBe(0)` can never fail. Because the PR's whole purpose is preventing real background timers from keeping the process alive, the test gives false assurance — a real timer leak on dispose would still pass. Wire the plugin with trackedSetInterval/trackedClearInterval (as the createTimerTracking helper intends) so an actual leaked interval is caught, or drop the vacuous assertion.</comment>
<file context>
@@ -0,0 +1,358 @@
+ await rm(tempDir, { recursive: true, force: true }).catch(() => {})
+ }
+ } finally {
+ expect(activeIntervals.size).toBe(0)
+ }
+})
</file context>
| const directoryA = join(testRoot, 'project-a') | ||
| const directoryB = join(testRoot, 'project-b') | ||
|
|
||
| await getPlugin(directoryA) |
There was a problem hiding this comment.
P3: Several tests create plugins via getPlugin() but never call dispose(), so the per-instance background services this PR adds teardown for (fallback background refresh, cacheKeepManager, prime manager) are left running and are only masked by the setInterval mock in disabledPluginRuntimeOverrides. afterEach only stops RPC servers. Either call dispose() at the end of these tests (matching the 'dispose stops...' tests) or assert that the teardown happens, so the tests actually cover the lifecycle this PR implements and don't silently depend on the timer mock to avoid leaks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/rpc-multi-project.test.ts, line 203:
<comment>Several tests create plugins via getPlugin() but never call dispose(), so the per-instance background services this PR adds teardown for (fallback background refresh, cacheKeepManager, prime manager) are left running and are only masked by the setInterval mock in disabledPluginRuntimeOverrides. afterEach only stops RPC servers. Either call dispose() at the end of these tests (matching the 'dispose stops...' tests) or assert that the teardown happens, so the tests actually cover the lifecycle this PR implements and don't silently depend on the timer mock to avoid leaks.</comment>
<file context>
@@ -0,0 +1,340 @@
+ const directoryA = join(testRoot, 'project-a')
+ const directoryB = join(testRoot, 'project-b')
+
+ await getPlugin(directoryA)
+ await getPlugin(directoryB)
+
</file context>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Symptom
Nothing user-visible. A long-lived opencode process that has loaded the plugin for several projects, or reloaded one, accumulates background workers that nobody stopped: each keeps polling quota, attempting token refreshes, and contending for the account-store file lock for the life of the process.
Mechanism
The plugin factory starts three background services. Until #216 there was no
disposehook at all, so none of them could be stopped — this is pre-existing, not a regression from that PR. #216 adds the hook and stops the RPC server; these three were still running:FallbackAccountManagerindex.ts:1856)startBackgroundRefresh()(:2200)stopBackgroundRefresh()had no production callerCacheKeepManager:2404)stop()had no production callerPrimeManageradoptPrimeManager(:2856)start()/claude-prime offThe timers are
unref'd, so they do not hold the process open — they just keep doing work on behalf of a project that is gone.Ownership is the whole risk
The first two are per-instance: the disposing instance owns them and stops them.
PrimeManageris not — it is adopted from a process-wide registry keyed by account-storage path, shared by every project using that path. CallingprimeManager.stop()fromdisposewould stop priming for live sibling projects.So the registry gains
releasePrimeManager(storagePath, slot): it drops the slot, and stops and evicts the manager only when that was the last slot. Releasing an unknown slot, or releasing twice, is a no-op.A late release must not clobber a successor.
slotFingerprintsmaps slot → the fingerprint that slot currently belongs to, andadoptPrimeManagerreads it to detach a slot when its storage path changes. A release arriving after the slot was re-adopted elsewhere would otherwise delete the successor's mapping, so the next adopt skips the detach and leaves a stale slot behind — which keeps that manager's slot set non-empty and prevents it from ever being torn down. The deletion is therefore guarded on the mapping still pointing at the fingerprint being released. This is the same hazard the RPC teardown in #216 guards against, one map over.disposekeeps the isolation discipline already in that function: each step is independent, so a failure in one cannot skip the RPC shutdown, and the additions run regardless of whether this instance still owns the registry entry.Verification
Each behaviour is pinned by a mutation of the implemented logic, not by the absence of a symbol — a missing-export failure proves only that the function was new.
(fail) dispose clears the fallback background refresh interval — Received: 0;(fail) dispose calls cacheKeepManager.stop — Received: 0.slots.size === 0guard so release stops unconditionally reddensreleasing one of two slots keeps the shared manager alive for the siblingandreleasing an unknown slot is a no-op, while the last-slot and per-instance tests stay green.slotFingerprints.deletereddens exactly one test —a late release does not clobber a successor slot mapping(managerY.isStopped()receivedfalse, expectedtrue) — and nothing else. Adopt slot D under path X, re-adopt D under path Y, release(pathX, D)late, then adopt under path Z and assert Y detached D.registry.size === 0, which would depend on every other test in the process and on ordering.Not included
The
'default'slot fallback whenctx.directoryis undefined means two directory-less instances share one slot. That is pre-existing inadoptPrimeManagerand unchanged here; it is only reachable when the host provides no directory, and both such instances would share a storage path anyway.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes
disposeso per-instance background services stop when a project is unloaded; previously only the RPC server was torn down, leaving quota polling and token refreshes running for the life of the process. The sharedPrimeManageris now released per slot so sibling projects on the same storage path stay untouched.Key details
releasePrimeManagerdrops a slot and stops the shared manager only when the last slot is released; unknown or double releases are no-ops.Written for commit 353d158. Summary will update on new commits.