fix(opencode): run one RPC server per project directory in a shared process - #216
fix(opencode): run one RPC server per project directory in a shared process#216iceteaSA wants to merge 3 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Architecture diagram
sequenceDiagram
participant Plugin as AnthropicAuthPlugin (per project instance)
participant Registry as globalThis.__anthropicAuthRpcServers (Map<rpcDir, handle>)
participant RpcServer as RPC Server (per directory)
participant PortFile as Port File (sha256(dir)/port-<pid>.json)
participant TUI as TUI (project process)
participant NotificationQueue as Shared Notification Queue
participant Apply as applyCommand closure
Note over Plugin,Registry: Plugin instantiation for project directory
Plugin->>Plugin: Resolve rpcDir = sha256(ctx.directory)
Plugin->>Registry: Look up rpcDir in map
alt rpcDir already in registry (re-instantiation)
Registry-->>Plugin: Existing handle found
Plugin->>RpcServer: stop() existing server for this directory
RpcServer->>PortFile: Unlink port file
Plugin->>Registry: delete(rpcDir)
else new directory (this is the fix scenario)
Registry-->>Plugin: No entry - proceed to start
end
Plugin->>RpcServer: startRpcServer({ dir: rpcDir, drain, apply })
RpcServer->>PortFile: NEW: Write port-<pid>.json to sha256(dir)
Plugin->>Registry: NEW: set(rpcDir, rpcServer) - keeps other entries intact
Plugin->>Plugin: Keep reference as __anthropicAuthRpcServer (back-compat)
Note over TUI,PortFile: Project B TUI discovers its own server (previously failed)
TUI->>PortFile: discoverPortFile(sha256(projectB/dir))
PortFile-->>TUI: port-<pid>.json (live, its own)
TUI->>RpcServer: Poll for modal request
RpcServer->>NotificationQueue: Drain notification (session id)
NotificationQueue-->>RpcServer: Notice with session id
RpcServer->>Apply: Apply command for this project's instance
Apply-->>TUI: Open modal in correct project
Note over NotificationQueue: Shared across all RPC servers in process
Note over NotificationQueue: Session id guarantees correct TUI drain
Note over NotificationQueue: A session-less notice would go to whichever TUI drains first (documented invariant)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 3/5
- In
packages/opencode/src/rpc/notifications.ts, older TUI clients that poll withoutsessionIdare treated as disconnected, causingcommand.execute.beforeto send an ignored text message instead of opening the RPC modal; preserve a deliberate legacy-session fallback and add coverage for clients withoutsessionId.
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/rpc/notifications.ts">
<violation number="1" location="packages/opencode/src/rpc/notifications.ts:57">
P2: When an older TUI polls without `sessionId`, `isTuiConnected` now always reports that session as disconnected, so `command.execute.before` sends an ignored text message instead of the RPC modal. Preserve a deliberate compatibility path for unscoped clients, or stop advertising the optional wire field as backward-compatible.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| return at > 0 && now - at < TUI_CONNECTED_WINDOW_MS | ||
| } | ||
| return lastDrainAtAny > 0 && now - lastDrainAtAny < TUI_CONNECTED_WINDOW_MS | ||
| const at = lastDrainAtBySession.get(sessionId) ?? 0 |
There was a problem hiding this comment.
P2: When an older TUI polls without sessionId, isTuiConnected now always reports that session as disconnected, so command.execute.before sends an ignored text message instead of the RPC modal. Preserve a deliberate compatibility path for unscoped clients, or stop advertising the optional wire field as backward-compatible.
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 57:
<comment>When an older TUI polls without `sessionId`, `isTuiConnected` now always reports that session as disconnected, so `command.execute.before` sends an ignored text message instead of the RPC modal. Preserve a deliberate compatibility path for unscoped clients, or stop advertising the optional wire field as backward-compatible.</comment>
<file context>
@@ -54,18 +52,14 @@ export function drainNotifications(
- return at > 0 && now - at < TUI_CONNECTED_WINDOW_MS
- }
- return lastDrainAtAny > 0 && now - lastDrainAtAny < TUI_CONNECTED_WINDOW_MS
+ const at = lastDrainAtBySession.get(sessionId) ?? 0
+ return at > 0 && now - at < TUI_CONNECTED_WINDOW_MS
}
</file context>
There was a problem hiding this comment.
Checked this one against the call sites rather than the diff, and the behaviour is unchanged — the branch that was removed was already unreachable in production.
isTuiConnected has exactly two callers, and neither can pass undefined:
index.ts:3334—queueDesktopNotice(sessionId: string, …)callsisTuiConnected(sessionId); the parameter is a requiredstring.index.ts:4559—isTuiConnected(input.sessionID), immediately followed bypushNotification(payload, input.sessionID).pushNotificationnow takessessionId: string, so ifinput.sessionIDwerestring | undefinedat that point the build would fail; typecheck passing is the proof that it is narrowed tostringthere.
So the old lastDrainAtAny fallback could only ever have been reached by a caller that does not exist. Before the change an older TUI polling without a session id got false from the per-session map (an unscoped drain never populates lastDrainAtBySession); after the change it gets false from the same map. Same answer, one less way to get a wrong one — and the wrong one was cross-project: lastDrainAtAny was written by every project's drain, so an unscoped call could report project A's TUI connected because project B's polled, and queueDesktopNotice skips the desktop fallback when it believes a TUI is present. That is a silent no-notification, which is the symptom this PR exists to fix.
On the second half — the optional wire field — those are different fields and the PR body should have distinguished them, so: RpcNotification.sessionId (server → TUI, in the notification payload) stays optional, which is what keeps an older TUI parsing what it is sent. The drain request parameter and pushNotification's argument are the ones now required. The compatibility claim is about the former only; no client is required to change to keep receiving notices.
No code change for this thread. The unscoped-drain path itself is still deliberately supported and non-destructive — it delivers everything above the ack cursor and prunes nothing — which is the compatibility path for any client that omits the id.
…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.
b4bf3f8 to
9d94623
Compare
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.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Confidence score: 5/5
- In
packages/opencode/src/tests/rpc-multi-project.test.ts, theafterEachregistry assertion cannot detect leaked RPC servers becausestopRpcServers()clearsglobalThis.__anthropicAuthRpcServersfirst; reorder the assertion or preserve the registry before cleanup so the test meaningfully checks for leaks.
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/tests/rpc-multi-project.test.ts">
<violation number="1" location="packages/opencode/src/tests/rpc-multi-project.test.ts:133">
P3: The registry assertion in afterEach is vacuous: stopRpcServers() runs first and sets globalThis.__anthropicAuthRpcServers = undefined, so `__anthropicAuthRpcServers?.get(rpcDir)` can never be anything but undefined no matter how a test leaked its server. The new authoritative teardown check this loop actually provides is the `discoverPortFile(rpcDir)` assertion; the registry check gives false confidence in registry teardown and should be dropped or moved before stopRpcServers so it can detect a leak.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| (globalThis as RpcGlobal).__anthropicAuthRpcServers?.get(rpcDir), | ||
| ).toBeUndefined() | ||
| expect(await discoverPortFile(rpcDir)).toBeNull() | ||
| } | ||
| if (previousRpcDir === undefined) { |
There was a problem hiding this comment.
P3: The registry assertion in afterEach is vacuous: stopRpcServers() runs first and sets globalThis.__anthropicAuthRpcServers = undefined, so __anthropicAuthRpcServers?.get(rpcDir) can never be anything but undefined no matter how a test leaked its server. The new authoritative teardown check this loop actually provides is the discoverPortFile(rpcDir) assertion; the registry check gives false confidence in registry teardown and should be dropped or moved before stopRpcServers so it can detect a leak.
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 133:
<comment>The registry assertion in afterEach is vacuous: stopRpcServers() runs first and sets globalThis.__anthropicAuthRpcServers = undefined, so `__anthropicAuthRpcServers?.get(rpcDir)` can never be anything but undefined no matter how a test leaked its server. The new authoritative teardown check this loop actually provides is the `discoverPortFile(rpcDir)` assertion; the registry check gives false confidence in registry teardown and should be dropped or moved before stopRpcServers so it can detect a leak.</comment>
<file context>
@@ -132,6 +128,12 @@ beforeEach(async () => {
await stopRpcServers()
+ for (const rpcDir of startedRpcDirs) {
+ expect(
+ (globalThis as RpcGlobal).__anthropicAuthRpcServers?.get(rpcDir),
+ ).toBeUndefined()
+ expect(await discoverPortFile(rpcDir)).toBeNull()
</file context>
| (globalThis as RpcGlobal).__anthropicAuthRpcServers?.get(rpcDir), | |
| ).toBeUndefined() | |
| expect(await discoverPortFile(rpcDir)).toBeNull() | |
| } | |
| if (previousRpcDir === undefined) { | |
| for (const rpcDir of startedRpcDirs) { | |
| expect(await discoverPortFile(rpcDir)).toBeNull() | |
| } |
1 participant
Symptom
In some sessions the
/claude-*commands stop opening their modal. No error, no log line — the command is accepted and nothing appears. A restart fixes that session until it happens again.Mechanism
The plugin kept one RPC server handle per process on
globalThis.__anthropicAuthRpcServer. Each instantiation stopped the existing handle before starting its own, andstop()unlinksport-<pid>.jsonfrom the directory that handle was started with (rpc/rpc-server.ts). The directory issha256(ctx.directory)— the project the instance belongs to (rpc/rpc-dir.ts).So as soon as one opencode 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 resolves its own
getRpcDir(...), finds no port file, and has nothing to poll — the modal never opens again for the life of the process.Observed on a machine running eight sessions: seven had
port-<pid>.jsonundersha256(cwd)and worked; the eighth, with cwd~/projects/brandon, had no port file under its own hash and itsport-<pid>.jsonsat undersha256(~/projects/homelab), written 21 hours after that session started.One process legitimately holds several project directories: request routing accepts
?directory=/x-opencode-directory(workspace-routing.ts:86-88), the SDK sends that header (sdk/js/src/client.ts:46-50), and the plugin factory is scoped per directory (plugin/index.ts:134-179) — opencode339536bc, 1.18.25.Changes
One RPC server per project directory.
globalThis.__anthropicAuthRpcServers: Map<rpcDir, RpcServerHandle>. Re-instantiating the same directory stops and replaces as before; a different directory starts an additional server and leaves the others alone. Each server keeps its creating instance'sapplyCommandclosure, so a modal request arriving on project B's server is applied by project B's plugin.Teardown, so the map cannot grow without bound.
Hooks.dispose— declared in the package this repo builds against (@opencode-ai/plugin1.18.21,dist/index.d.ts:173-178) and invoked by opencode's instance finalizer (plugin/index.ts:265-278; disposers also run on ordinary dispose and per-project reload,project/instance-store.ts:94-105,:126-145). Teardown acts only when the registry entry is still its own handle. That condition is correctness, not hygiene:port-<pid>.jsonis the same filename 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.stop()unlinks only its own file, matching the recorded port and pid — a stale handle cannot remove a successor's port file even if a future path forgets the guard.Session-scoped notification state. The notification module is shared by every server in the process, so anything in it that is not keyed by session is keyed by nothing:
pushNotificationnow requires a session id. A notice without one broadcasts to every draining TUI and survives one session's ack, which across several servers means crossing project boundaries. (The wire field stays optional so an older TUI still parses what it is sent.)isTuiConnectednow requires a session id, and the process-widelastDrainAtAnytimestamp it fell back on is deleted. 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 (index.ts:3334) 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.drainNotifications(id, undefined)pruned every acknowledged notice regardless of owner, so one client both swallowed and destroyed other sessions' pending dialogs. This predates the registry — with a single server it was cross-session inside one project; the registry widens it to cross-project. Fixed as deliver-but-never-prune. On reachability, stated precisely: the TUI has sent the session id since polling was introduced (e2e0f4c, andgit log -S".pending("returns only that commit), so no released TUI build produces an unscoped drain; what can is a malformed or third-party client on the loopback RPC (rpc-server.ts:77-79coerces a non-stringsessionIdtoundefined) plus any future caller. The fix costs one line and cannot break a working client; rejecting unscoped drains costs the same and fails by silently not delivering, which is the symptom this PR exists to fix.The old single-handle global is deleted, not merely superseded.
__anthropicAuthRpcServerwas still declared, assigned on every start, and conditionally cleared alongside the new registry. Production never read it — the===was a clear-if-mine, not a lookup — so nothing routed off it, but every instance overwrote it,disposenever cleared it, and after teardown it held a dangling reference to a stopped server for the life of the process. Its only remaining readers were in a test helper, so a test's behaviour depended on state production writes and never reads. Removing it is this PR's own claim carried through: a registry that replaces the single global should not ship a vestigial twin of it.A post-build gate for that class (
scripts/check-bundle-globals.ts, last step ofbun run build, alsobun run check:bundle). A source grep cannot see a call site in a file nobody opened; the bundle contains everything that ships. Three ordered assertions, each with its own message: the artifact exists and is non-trivial → the positive control__anthropicAuthRpcServersmatches at least once → only then the old identifier's count is 0. The order matters, and the positive control is what makes the zero assertion mean anything: without it the gate passes on a missing or wrongly-built bundle by matching nothing.Verification
rpc-multi-project.test.ts: two directories in one process both discover live port files on different ports (red before the registry —entryAnull); dispose stops its own directory's server while a second stays discoverable; the late-dispose ordering case — start A, replace with B for the same directory, dispose A, B's port file must survive and B must still answer (the naive teardown returnedundefined); and a test pinning the dispose identity guard on its own behaviour.stopcalled once) while the port-file test stays green; removing the port/pid match alone reddens onlyrpc-server.test.ts's stale-server test. A test that fails only when both are reverted proves the pair, not the layer.rpc-notifications.test.ts: an unscoped drain delivers everything above the ack cursor, returns nothing at or below it, and prunes nothing; a drain by one session does not make another report connected (red on the old code).@ts-expect-errorpins fail to compile if either session id becomes optional again — each proven non-vacuous by restoring the optional parameter and watching typecheck fail on the unused directive.dispose; what is pinned is that the hooks object exposes it, type-checked against the SDK'sHooks. The invocation is cited above.port-<pid>.jsonis absent. Previously nothing verified teardown at all — neutering the cleanup helper left the file green. It now reddens 3 of 7 (expect(received).toBeUndefined()against a live handle). Not an aggregatesize === 0: that depends on every other test in the process and on which dispose ran first.a dispose whose entry was replaced does not stop the successor serverswaps the registry entry for a no-op mock, correctly asserts the dispose is a no-op, and then exited without stopping the real server — leaving its port file on disk. Fixed in place.Not included
Stale RPC directories are never collected. The base accumulates one hash directory per project ever seen, some holding port files from months back. A sweep at server start (unlink dead-pid port files, remove the directories they leave empty, skip any directory a live registry entry owns, and leave alone one whose server is mid-start) is a separate change.
Config mirrored into module-global state. A sweep of every module-scope binding in both packages found the rest correctly scoped — write chains serialise access to machine-global files, the notification queue is keyed by session — with one class of exception worth recording as a precondition rather than a defect:
dumpEnabled,fastModeEnabled,cache1hEnabled/cache1hMode, the logger level and the dump counters are process-global mirrors of sidecar config written by each plugin instance at boot. Two projects in one process share one set of these. That is correct only because the sidecar path is process-wide ($HOME/OPENCODE_CONFIG_DIR/OPENCODE_ANTHROPIC_AUTH_FILEare all process-scope env), so every instance reads the same file. If the plugin ever gains a per-project config path, all of them become cross-project bugs simultaneously and silently.The sibling
openai-authplugin carries the same single-handle pattern (src/index.ts:1907-1940at v0.7.1) and the same live symptom on the same session; its maintainer confirmed the diagnosis from their own source and filesystem and has a matching fix incortexkit/openai-auth#145, including two further instances of the class that this codebase does not have.Base:
main360b68e(v1.22.0).