fix(rpc): scope loopback servers by project - #145
Conversation
|
A sharper way to state the class, worth more than the three instances — it came out of comparing this against the sibling plugin, which has the same RPC bug and is fixing it in parallel. The test is not "is this module-scope state" but does the cardinality of the guard match the cardinality of the work it guards. A per-process latch over per-process work is correct; the bug is a per-process guard over per-directory work, in a host where one process legitimately serves several directories. Applying that to every piece of process-global state in the plugin, so the sweep is on the record rather than just the two fixes:
One thing that row eight does not cover, and I would rather flag it than leave it implied: The sibling plugin ran the same sweep on its tree and came back clean on all four of its globals, which is the useful control: the framing finds real instances where they exist and does not manufacture them where they do not. |
|
Two corrections to the comment above, both making it less flattering. The control was weaker than I said. I wrote that the sibling plugin ran the same sweep and came back clean on all four of its globals. Its sweep covered The Still not reachable: the only production caller passes |
|
One more category the table above does not distinguish, which I think is the more useful half of it. Some of those "correct" rows are correct by structure and some are correct only contingently — and the second group all fail together the day one assumption changes. Correct by structure: Correct only contingently: the memoized settings in It is not correct by design. If this plugin ever gains a per-project config path — a I am not scoping them here: there is no reachable defect, and it would be a much larger change than this PR should carry. The same reasoning applied to the cachekeep manager, which is why it needed the registry — it was only ever defensible because the account store cannot differ per project inside one process, and the moment that held less firmly than the RPC directory did, it was already broken. Also flagging rather than fixing: The sibling plugin ran the same class-derived sweep over its tree — 24 module-scope bindings — and found the identical contingent group (its dump, fast-mode and 1h-cache knobs) resting on the identical assumption about its own storage layer. Two independently written plugins, same tripwire under the same feature. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
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/rpc-server.ts">
<violation number="1" location="packages/opencode/src/rpc/rpc-server.ts:180">
P1: When a stale handle stops concurrently with a same-directory successor, this ownership check is still vulnerable to a read/unlink race and can remove the successor’s port file. Serialize per-directory start/stop or use an atomic ownership mechanism instead of separate `readFile` and `unlink` calls.</violation>
</file>
<file name="packages/opencode/src/tests/rpc-server.test.ts">
<violation number="1" location="packages/opencode/src/tests/rpc-server.test.ts:519">
P2: The test replaces `globalThis.fetch` with a mock that always returns `new Response('{}')` and never asserts the request URL or count. The RPC `apply` handler this PR exercises can make real backend calls for several commands (e.g. `openai-quota`/`openai-reset` reach the Codex backend); for any such path the canned `{}` is silently consumed and the test would still pass (200) while the response was fabricated, masking regressions in the very round-trip these tests aim to protect. Scope the mock to only the calls the loader actually makes (e.g. assert the URL) or use a spy that records/asserts requests, rather than a blanket success stub.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const current = await readFile(portFile, 'utf8') | ||
| .then((raw) => JSON.parse(raw) as { port?: unknown; token?: unknown }) | ||
| .catch(() => undefined) | ||
| if (current?.port === port && current.token === token) |
There was a problem hiding this comment.
P1: When a stale handle stops concurrently with a same-directory successor, this ownership check is still vulnerable to a read/unlink race and can remove the successor’s port file. Serialize per-directory start/stop or use an atomic ownership mechanism instead of separate readFile and unlink calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/rpc/rpc-server.ts, line 180:
<comment>When a stale handle stops concurrently with a same-directory successor, this ownership check is still vulnerable to a read/unlink race and can remove the successor’s port file. Serialize per-directory start/stop or use an atomic ownership mechanism instead of separate `readFile` and `unlink` calls.</comment>
<file context>
@@ -164,9 +173,12 @@ export async function startRpcServer(
+ const current = await readFile(portFile, 'utf8')
+ .then((raw) => JSON.parse(raw) as { port?: unknown; token?: unknown })
+ .catch(() => undefined)
+ if (current?.port === port && current.token === token)
+ await unlink(portFile).catch(() => {})
},
</file context>
| root, | ||
| 'auth-state.json', | ||
| ) | ||
| globalThis.fetch = (async () => |
There was a problem hiding this comment.
P2: The test replaces globalThis.fetch with a mock that always returns new Response('{}') and never asserts the request URL or count. The RPC apply handler this PR exercises can make real backend calls for several commands (e.g. openai-quota/openai-reset reach the Codex backend); for any such path the canned {} is silently consumed and the test would still pass (200) while the response was fabricated, masking regressions in the very round-trip these tests aim to protect. Scope the mock to only the calls the loader actually makes (e.g. assert the URL) or use a spy that records/asserts requests, rather than a blanket success stub.
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-server.test.ts, line 519:
<comment>The test replaces `globalThis.fetch` with a mock that always returns `new Response('{}')` and never asserts the request URL or count. The RPC `apply` handler this PR exercises can make real backend calls for several commands (e.g. `openai-quota`/`openai-reset` reach the Codex backend); for any such path the canned `{}` is silently consumed and the test would still pass (200) while the response was fabricated, masking regressions in the very round-trip these tests aim to protect. Scope the mock to only the calls the loader actually makes (e.g. assert the URL) or use a spy that records/asserts requests, rather than a blanket success stub.</comment>
<file context>
@@ -323,4 +502,210 @@ describe('rpc-server', () => {
+ root,
+ 'auth-state.json',
+ )
+ globalThis.fetch = (async () =>
+ new Response('{}')) as unknown as typeof globalThis.fetch
+
</file context>
3ad8f58 to
fe187e1
Compare
|
Squashed to one commit, Cubic's P1 was real and is fixed. Disposal compared identity against Worth noting the cachekeep manager in the same dispose block did not have this — its binding was already loader-local. One block, correct for one handle and wrong for the other. The CI failure was a separate problem, and I did not prove which. What was independently wrong is the assertion: it checked So: two CI reds, then green after the assertion fix, with the P1 fixed in the same push. That is consistent with the assertion shape being the cause, but I have not separated it from the P1 fix and one green run is not determinism. If it recurs, the order-sensitive explanation is at least out of the candidate set. Verification on the squashed head: 1144 pass, 0 fail across three consecutive local runs; the same under |
|
Hold this one - I found a defect in my own change while verifying the build, and the PR currently claims something that is not true. There are two OpenCode never calls the loader's return as a lifecycle object. const options = yield* Effect.promise(() => plugin.auth!.loader!(...))
const opts = options ?? {}
const patch: Partial<Info> = providers[providerID] ? { options: opts } : { source: "custom", options: opts }
So, concretely, on this branch:
The build is what caught it. I grepped the bundled Fixing both disposes and re-pushing. The registry, the port-ownership check in |
One opencode server process can host several project directories - the
plugin factory is scoped per directory (opencode `plugin/index.ts:134-179`).
This plugin kept a single process-global RPC server handle, so each new
instantiation stopped the previous server and started a new one, and
`stop()` unlinked `port-<pid>.json` from the directory it had been started
with. The first project's port file disappeared, its TUI discovered
nothing, and `/openai-*` commands silently stopped opening a modal for the
life of the process.
Found on a machine running one process across three projects: its port
file sat under the hash of a directory it was not serving, and there was
none under its own.
The RPC server and the cachekeep manager are now per-directory registries.
Re-instantiating the same directory stops and replaces its entry as
before; a different directory starts an additional server and touches
nothing else. Teardown removes only this instance's entries, matched on
directory and handle identity, and `stop()` unlinks a port file only when
it still names its own port and token - without that, a late dispose from
a superseded instance deletes a live successor's file and reproduces the
original outage through the cleanup path. Each defence is pinned by its
own test: reverting either one reddens that test and no other.
The cachekeep manager had the same shape with a quieter symptom. A second
project's instantiation called `.stop()` on the first's manager and
installed a fresh one with an empty target map, so project A's tracked
idle sessions were dropped and never warmed again - no error, prewarms
simply stopped happening.
Teardown runs from the plugin Hooks dispose, which is the only dispose
opencode invokes (`plugin/index.ts:265-278`, with per-directory disposers
at `project/instance-store.ts:94-105` and `:126-145`). The object returned
from `auth.loader` is provider options, not a lifecycle object
(`provider/provider.ts:1614-1622` merges it as `{ options }`), so a
dispose placed there never runs; a test now asserts the loader result
carries no dispose at all. A packaging check refuses a build whose bundle
still contains the singular global, after first asserting the bundle
exists and carries the registry global, so it cannot pass by matching
nothing.
Also fixed here, pre-existing and independent of the registry: a drain of
the notification queue without a session id matched every notification and
pruned it, so one drain swallowed and deleted other sessions' pending
dialogs. The queue is module-global and never referenced the server, so
this was already cross-session with a single server; per-project servers
only widen it to cross-project. Delivery is unchanged, including the ack
cursor - an unscoped drain still returns everything above it and nothing
at or below it - and pruning no longer happens. Rejecting the call instead
would have been stricter and worse: its failure mode is dialogs that never
appear. Connectivity is scoped the same way, so one project's TUI polling
can no longer make another project's session look connected and suppress
its fallback message.
fe187e1 to
af6ea21
Compare
|
Fixed and re-squashed to one commit, The teardown now lives in the plugin Hooks dispose. A test asserts the loader result carries no dispose at all, so the wrong function cannot be chosen again by accident. Reverting each hunk on its own:
Why the earlier tests passed over a dead function. Two reasons, one per test, and neither of them was the teardown working. Worth flagging separately: that first test predates this PR. It asserted that the loader dispose works, for a path opencode never invokes - which is likely why the teardown ended up there in the first place. A test over an unreachable path does not just fail to catch bugs; it signals the path matters and pulls later work into it. It is now inverted to assert the loader must not expose a dispose. One more from the same thread: Bundle check. This check is what caught the original defect - I grepped the built bundle for the old global expecting zero and got 1145 pass, 0 fail (1137 at |
There was a problem hiding this comment.
4 issues found across 9 files
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/index.ts">
<violation number="1" location="packages/opencode/src/index.ts:1056">
P2: When an OAuth loader has fallback accounts, disposing the plugin leaves its `FallbackAccountManager` interval running. Track each fallback manager and call `stopBackgroundRefresh()` during disposal.</violation>
<violation number="2" location="packages/opencode/src/index.ts:1962">
P1: When two loaders for one directory start concurrently, both can pass the empty-map check before either asynchronous `startRpcServer` call registers. Serialize startup or reserve a per-directory entry before awaiting, otherwise the first server and its port-file state can be orphaned.</violation>
</file>
<file name="packages/opencode/src/tests/tui-packaging.test.ts">
<violation number="1" location="packages/opencode/src/tests/tui-packaging.test.ts:187">
P2: This test reads the gitignored build artifact `dist/index.js` (root `.gitignore` lists `packages/*/dist/`), which `bun run test` does not produce. On any fresh checkout where the developer runs `bun test src/tests` (or `bun run test`) without first running `bun run build`, this test now throws and fails the whole suite — a regression from the other tests in this file, which are source-based and need no prior build. It also silently validates a stale bundle after editing `src/index.ts`: if a contributor reintroduces the singular global and runs `bun test` without rebuilding, the test passes even though the source regressed, so the guard gives misleading local results and only enforces correctly in CI. Consider running build as part of the test step, reading the relevant source modules instead of the bundle, or gating this check to CI so it does not break the plain local `bun test` flow.</violation>
</file>
<file name="packages/opencode/src/tests/rpc-server.test.ts">
<violation number="1" location="packages/opencode/src/tests/rpc-server.test.ts:526">
P3: The four new per-project tests each duplicate the same env-var capture, globalThis.fetch override, `{ __openaiAuthRpcServers?... }` registry cast, and 6-line finally restore block. Factor the setup/teardown into the shared helpers (e.g. a `withProjectEnv(root, fn)` wrapper or a beforeEach/afterEach) so a future change to env or registry handling doesn't have to be made in four places.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }) | ||
| rpcGlobal.__openaiAuthRpcServer = rpcServer | ||
| activeRpcServer = rpcServer | ||
| rpcServers.set(rpcDir.dir, rpcServer) |
There was a problem hiding this comment.
P1: When two loaders for one directory start concurrently, both can pass the empty-map check before either asynchronous startRpcServer call registers. Serialize startup or reserve a per-directory entry before awaiting, otherwise the first server and its port-file state can be orphaned.
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 1962:
<comment>When two loaders for one directory start concurrently, both can pass the empty-map check before either asynchronous `startRpcServer` call registers. Serialize startup or reserve a per-directory entry before awaiting, otherwise the first server and its port-file state can be orphaned.</comment>
<file context>
@@ -1934,8 +1959,8 @@ export async function CodexAuthPlugin(
})
- rpcGlobal.__openaiAuthRpcServer = rpcServer
- activeRpcServer = rpcServer
+ rpcServers.set(rpcDir.dir, rpcServer)
+ ownedRpcServers.set(rpcDir.dir, rpcServer)
} catch {
</file context>
| if (rpcGlobal.__openaiAuthRpcServer === activeRpcServer) { | ||
| rpcGlobal.__openaiAuthRpcServer = undefined | ||
| } | ||
| ownedCacheKeepManagers.clear() |
There was a problem hiding this comment.
P2: When an OAuth loader has fallback accounts, disposing the plugin leaves its FallbackAccountManager interval running. Track each fallback manager and call stopBackgroundRefresh() during disposal.
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 1056:
<comment>When an OAuth loader has fallback accounts, disposing the plugin leaves its `FallbackAccountManager` interval running. Track each fallback manager and call `stopBackgroundRefresh()` during disposal.</comment>
<file context>
@@ -1041,16 +1042,29 @@ export async function CodexAuthPlugin(
- if (rpcGlobal.__openaiAuthRpcServer === activeRpcServer) {
- rpcGlobal.__openaiAuthRpcServer = undefined
+ }
+ ownedCacheKeepManagers.clear()
+
+ const rpcGlobal = globalThis as {
</file context>
| // bundle directly here keeps the test dependent on the same freshness | ||
| // guarantee CI provides instead of rebuilding inside the test. | ||
| const bundle = join(PKG_DIR, 'dist', 'index.js') | ||
| if (!existsSync(bundle)) { |
There was a problem hiding this comment.
P2: This test reads the gitignored build artifact dist/index.js (root .gitignore lists packages/*/dist/), which bun run test does not produce. On any fresh checkout where the developer runs bun test src/tests (or bun run test) without first running bun run build, this test now throws and fails the whole suite — a regression from the other tests in this file, which are source-based and need no prior build. It also silently validates a stale bundle after editing src/index.ts: if a contributor reintroduces the singular global and runs bun test without rebuilding, the test passes even though the source regressed, so the guard gives misleading local results and only enforces correctly in CI. Consider running build as part of the test step, reading the relevant source modules instead of the bundle, or gating this check to CI so it does not break the plain local bun test flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/tui-packaging.test.ts, line 187:
<comment>This test reads the gitignored build artifact `dist/index.js` (root `.gitignore` lists `packages/*/dist/`), which `bun run test` does not produce. On any fresh checkout where the developer runs `bun test src/tests` (or `bun run test`) without first running `bun run build`, this test now throws and fails the whole suite — a regression from the other tests in this file, which are source-based and need no prior build. It also silently validates a stale bundle after editing `src/index.ts`: if a contributor reintroduces the singular global and runs `bun test` without rebuilding, the test passes even though the source regressed, so the guard gives misleading local results and only enforces correctly in CI. Consider running build as part of the test step, reading the relevant source modules instead of the bundle, or gating this check to CI so it does not break the plain local `bun test` flow.</comment>
<file context>
@@ -177,4 +177,34 @@ describe('tui packaging (compiled ./tui entry shim)', () => {
+ // bundle directly here keeps the test dependent on the same freshness
+ // guarantee CI provides instead of rebuilding inside the test.
+ const bundle = join(PKG_DIR, 'dist', 'index.js')
+ if (!existsSync(bundle)) {
+ throw new Error(
+ 'Built plugin bundle is missing: dist/index.js (run `bun run build` first)',
</file context>
| root, | ||
| 'auth-state.json', | ||
| ) | ||
| globalThis.fetch = (async () => |
There was a problem hiding this comment.
P3: The four new per-project tests each duplicate the same env-var capture, globalThis.fetch override, { __openaiAuthRpcServers?... } registry cast, and 6-line finally restore block. Factor the setup/teardown into the shared helpers (e.g. a withProjectEnv(root, fn) wrapper or a beforeEach/afterEach) so a future change to env or registry handling doesn't have to be made in four places.
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-server.test.ts, line 526:
<comment>The four new per-project tests each duplicate the same env-var capture, globalThis.fetch override, `{ __openaiAuthRpcServers?... }` registry cast, and 6-line finally restore block. Factor the setup/teardown into the shared helpers (e.g. a `withProjectEnv(root, fn)` wrapper or a beforeEach/afterEach) so a future change to env or registry handling doesn't have to be made in four places.</comment>
<file context>
@@ -323,4 +509,282 @@ describe('rpc-server', () => {
+ root,
+ 'auth-state.json',
+ )
+ globalThis.fetch = (async () =>
+ new Response('{}')) as unknown as typeof globalThis.fetch
+
</file context>
1 participant
One opencode server process can host several project directories — the plugin factory is scoped per directory (
plugin/index.ts:134-179in opencode). This plugin kept a single process-global RPC server handle, so each new instantiation stopped the previous server and started a new one, andstop()unlinkedport-<pid>.jsonfrom the directory it had been started with. The first project's port file disappeared, its TUI discovered nothing, and/openai-*commands silently stopped opening a modal for the life of the process. No error anywhere.Found on this machine: pid 2799321, cwd
~/projects/brandon, also servinghomelabandcortexkit. Its port file was under sha256(homelab) and there was none under sha256(brandon). Last instantiation wins; the rest go dark.@cortexkit/anthropic-authhas the same shape and is fixing it in parallel (#216). Several findings below came out of comparing the two, including two that only showed up because the other seat measured something I had assumed.The fix
globalThis.__openaiAuthRpcServers, a map keyed by the resolved RPC directory. Same directory re-instantiated stops and replaces its entry as before; a different directory starts an additional server and touches nothing else.disposestops and removes only this instance's entry, matched on directory and handle identity.The same treatment for
__openaiAuthCacheKeepManager: a second project's instantiation used to call.stop()on the first's manager and install a fresh one with an empty target map, so project A's tracked idle sessions were dropped and never warmed again — no error, just prewarms that stop happening.Two things the registry could have broken, and what pins them
Teardown resurrection.
port-<pid>.jsonis the same filename for every server the process starts in a directory. Instance 1 starts S1, instance 2 replaces it with S2 under that same name, then instance 1's dispose runs late and a blindstop()unlinks S2's live port file — the original outage, re-entered through the cleanup. Two defences: dispose only acts when the map entry is still this instance's handle, andstop()unlinks only if the file on disk still names its own port and token.Both are pinned independently, which took two attempts. A test asserting "the successor's port file survives" is satisfied by the ownership check alone and says nothing about the identity guard — when two defences protect the same observable, no assertion on that observable pins either one. The isolating assertion is behavioural: a non-owning dispose must not call
stop()at all. Reverting each hunk alone:disposing a replaced plugin instance does not stop its stale RPC handlefails; the port-file test still passesstopping a stale server leaves its successor port file and health endpoint livefails; the dispose test still passesUnbounded growth. Replacing one handle with a map trades a bounded leak for an unbounded one if nothing ever tears entries down.
dispose?: () => Promise<void>is in the published SDK we build against (@opencode-ai/plugin/dist/index.d.ts:174, resolved 1.18.25) and opencode invokes it — the hook finalizer atplugin/index.ts:265-278, with per-directory disposers atproject/instance-store.ts:94-105and:126-145. Cardinality is bounded by live directory instances.One pre-existing bug fixed on the way
drainNotifications(lastReceivedId, sessionId?)with an undefinedsessionIdmatched every notification and pruned it, so one drain without a session id swallowed and deleted other sessions' pending dialogs. This predates the registry — the queue is module-global and never referenced the server, so it was already cross-session with a single server; per-project servers only widen it to cross-project.The change is one line: don't prune on an unscoped drain. Delivery is unchanged, including the ack cursor, so an unscoped
drain(0)still returns everything above the cursor and an unscopeddrain(2)still returns nothing at or below it.I checked reachability rather than assuming it: the TUI's
pending(lastReceivedId, sessionId)call and the drain'ssessionIdparameter were introduced in the same commit (7142ad0), so no shipped TUI build can produce an unscoped drain. What reaches it is a malformed or third-party client on the loopback socket (rpc-server.ts:112coerces a non-string param toundefined) and any future caller. Rejecting the call instead would have been stricter and worse: its failure mode is dialogs that never appear, which is the outage this PR fixes.Verification
1142 pass, 0 fail (baseline 1137 at
fb1402e); typecheck, Biome, order scanner 14/14, clean tree. Red proof against pristinefb1402e:expect(portA).not.toBeNull()→Received: null.Reviewed adversarially on a different model family: approve, no blocking findings, all three hazards traced with live probes. Two review findings were test-discrimination defects rather than production bugs, both fixed in
61c02e2— the context test passed on port-distinctness rather than on the context, and the late-dispose test needed both defences reverted before it failed.Separate, not fixed here
bootQuotaSeedStarted(index.ts:185onfb1402e) looks like the same class. It is a per-process latch, but it guardsquotaManager.seedFallbacksFromAccounts()at:2904, which mutates a per-instanceQuotaManager— so in a process serving two projects the first trips the latch and later projects start with no seeded fallback quota. The sidebar write and API refresh in that block are correctly once-per-process; the seed is not. Admission also reads the shared sidebar file, so the likely symptom is degraded sticky/admission decisions until the first live push rather than a visible failure. Happy to fold it in here or file it separately — say which.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes RPC servers and cachekeep managers being process-global singletons, so a second project in the same opencode process no longer stops the first project's server, deletes its port file, or drops its warm cache targets.
Bug Fixes
globalThismaps keyed by resolved RPC directory; same-directory reloads still replace, different directories coexist.disposeonly stops a server or manager if the map entry still matches this instance's handle, so a late dispose from a replaced instance can't kill the successor.stop()unlinks the port file only if it still names this server's port and token.drainNotificationsno longer prunes when called without asessionId, so an unscoped drain can't swallow other sessions' pending dialogs.sessionId; the unscoped connectivity state is removed.Separately,
bootQuotaSeedStartedlooks like the same class of bug: a per-process latch guarding a per-instance quota seed, so later projects start with no seeded fallback quota. Not fixed here.Written for commit af6ea21. Summary will update on new commits.