fix(workspace): the newer add wins whichever completes first, and a global disable is honoured before the project write

Two more of the same shape, on the last review round.

`MCP.createAndStore` guarded its failure path against a client another
caller registered meanwhile, but not its success path: an older creation
completing after a newer add stored its client, closing the newer one and
handing the runtime back to what the older call was asked to start. The
newer call now wins whichever completes first — a late result is closed,
not stored, and the late call answers with what is serving.

`persist` checked the node it was about to replace, which is the PROJECT
file's; intent can also live in the global config the project inherits
from, and a project pin written over a global disable shadows it for good,
since project wins the merge. The merged view is asked once more,
immediately before the write. Same window as the write's own read; named,
not closed.

The lifecycle mock gains a one-shot connect delay so an older add can
complete after a newer one; the real-file staging that asserts the W3
residual keys to the write's own read, now the second after the guard.
Reverting either fix fails a named test.

Author: ralphstodomingo

packages/opencode/src/altimate/workspace/engine-config.ts

@@ -38,6 +38,23 @@ export async function persist(name: string, cfg: LocalMcpConfig, configPath?: st
   // one write to one file is not atomic, and a disable landing between the read
   // and the `write` syscall is still lost. That residual is named on the PR
   // rather than papered over; closing it needs write-then-verify.
+  // The node on disk is the PROJECT file's; intent can also live in the global
+  // config the project inherits from. A global disable landing after the
+  // caller's merged read would not be on the text below — and a project pin
+  // written over it shadows that disable for good, since project wins the
+  // merge. So the merged view is asked once more, immediately before the
+  // write. Same window as the write's own read; named, not closed.
+  let merged: ExistingEntry | null
+  try {
+    merged = await existingEntry(name)
+  } catch (err) {
+    log.warn("could not confirm intent before writing the engine entry; not writing", { name, err: String(err) })
+    return "disabled"
+  }
+  if (merged?.enabled === false) {
+    log.info("refusing to write a project entry over a disable in the merged config", { name })
+    return "disabled"
+  }
   if ((await addMcpToConfig(name, cfg, configPath, { refuseIfDisabled: true })) === null) {
     log.info("refusing to write over an entry that is disabled on disk", { name })
     return "disabled"

packages/opencode/src/mcp/index.ts

@@ -957,9 +957,18 @@ export const layer = Layer.effect(
         // altimate_change end
         return result.status
       }
-      // altimate_change start — recorded after the bail-early check above, so a
-      // failed replacement never overwrites the status of a client another
-      // caller registered while it was coming up.
+      // altimate_change start — the newer call wins, whichever completes first.
+      // If another caller registered a client under this key while this one was
+      // coming up, this result is the OLDER intent arriving late: storing it
+      // would close their newer client and hand the runtime back to whatever
+      // this call was asked to start. Close what we made instead, leave theirs
+      // — client, status and launch record — and answer with what is serving.
+      if (s.clients[name] !== replacing) {
+        yield* Effect.tryPromise(() => result.mcpClient!.close()).pipe(Effect.ignore)
+        return s.status[name] ?? result.status
+      }
+      // Recorded after the checks above, so neither a failed nor a superseded
+      // replacement overwrites the status of a client another caller registered.
       s.status[name] = result.status
       // altimate_change end
 

packages/opencode/test/altimate/workspace/config-on-disk.test.ts

@@ -181,6 +181,30 @@ describe("the write checks the text it is about to modify", () => {
       expect(after?.command).toEqual(["datamate", "start-stdio"])
       expect(h.added).toHaveLength(0)
     })
+
+    test("a GLOBAL disable landing after the guard is refused before the project write", async () => {
+      // The project file holds an enabled node, so the write's own on-disk check
+      // sees nothing wrong. Intent lives in the global config the project
+      // inherits from, and a project pin written over a global disable shadows
+      // it for good (project wins the merge). So persist asks the MERGED view
+      // once more, immediately before writing.
+      const h = install([{ datamate: { status: "connected" } }, { datamate: { status: "connected" } }], () => null, {
+        realPersist: true,
+      })
+      syncInternals.projectConfigPath = async () => file
+      syncInternals.projectEntry = async () => (await diskEntry()) ?? null
+      syncInternals.existingEntry = async () => {
+        const e = (await diskEntry()) ?? null
+        h.reads.push(e?.enabled)
+        // reads: inspection (1), the guard (2), persist's merged re-read (3) —
+        // the global disable is visible from the third read on, never on disk.
+        return h.reads.length >= 3 && e ? { ...e, enabled: false } : e
+      }
+      const first = await ensure("s1")
+      expect(first.kind, "wrote a project pin over a global disable").toBe("entry-disabled")
+      expect((await diskEntry())?.command, "the project file was written").toEqual(["datamate", "start-stdio"])
+      expect(h.added).toHaveLength(0)
+    })
   })
 
   describe("a disable landing before the revive is honoured", () => {

packages/opencode/test/altimate/workspace/undo-and-teardown.test.ts

@@ -508,8 +508,9 @@ describe("the restore refuses on the text it edits", () => {
     })
     const diskEntry = async () => (await readMcpEntryFromDisk("datamate", file)) as ExistingEntry | undefined
 
-    /** After the guard's intent read, config-file readText #1 is now addMcpToConfig's
-     * ONLY read (persist has no separate check read any more). */
+    /** After the guard's intent read, config-file readText #1 is persist's merged
+     * intent re-read (the global-disable check) and #2 is addMcpToConfig's own
+     * read — the one the write modifies. The window under test is the write's. */
     function stage(where: "intent-read-end" | "before-write-read" | "after-write-read") {
       const h = install([{ datamate: { status: "connected" } }, { datamate: { status: "connected" } }], () => null, { realPersist: true })
       syncInternals.projectConfigPath = async () => file
@@ -532,7 +533,7 @@ describe("the restore refuses on the text it edits", () => {
       Filesystem.readText = async (p: string) => {
         if (!armed || p !== file || landed) return originalReadText(p)
         n += 1
-        if (n !== 1) return originalReadText(p)
+        if (n !== 2) return originalReadText(p)
         landed = true
         if (where === "before-write-read") {
           writeFileSync(file, DISABLED_FILE)

packages/opencode/test/mcp/lifecycle.test.ts

@@ -89,6 +89,11 @@ function getOrCreateClientState(name?: string): MockClientState {
   return state
 }
 
+// altimate_change start — a one-shot connect delay, so a test can make an older
+// add complete AFTER a newer one for the same key.
+let connectDelayOnceMs = 0
+// altimate_change end
+
 // Mock transport that succeeds or fails based on connectShouldFail / connectShouldHang
 class MockStdioTransport {
   stderr: null = null
@@ -99,6 +104,13 @@ class MockStdioTransport {
   async start() {
     if (connectShouldHang) return new Promise<void>(() => {}) // never resolves
     if (connectShouldFail) throw new Error(connectError)
+    // altimate_change start
+    if (connectDelayOnceMs) {
+      const delay = connectDelayOnceMs
+      connectDelayOnceMs = 0
+      await new Promise<void>((resolve) => setTimeout(resolve, delay))
+    }
+    // altimate_change end
   }
   async close() {
     transportCloseCount++
@@ -1326,6 +1338,38 @@ it.instance(
     ),
   { config: { mcp: {} } },
 )
+
+it.instance(
+  "an older add that completes after a newer one does not replace the newer client",
+  () =>
+    MCP.Service.use((mcp: MCPNS.Interface) =>
+      Effect.gen(function* () {
+        // The other half of the same race: both creations SUCCEED, the older
+        // one last. Storing it would close the newer client and hand the runtime
+        // back to what the older call was asked to start. The newer call wins
+        // whichever completes first; the late result is closed, not stored.
+        lastCreatedClientName = "racing2"
+        getOrCreateClientState("racing2")
+        yield* mcp.add("racing2", { type: "local", command: ["echo", "one"] })
+
+        connectDelayOnceMs = 150
+        const slow = yield* Effect.forkChild(mcp.add("racing2", { type: "local", command: ["echo", "two"] }))
+        yield* Effect.sleep("20 millis") // the slow add is inside its delayed connect
+        yield* mcp.add("racing2", { type: "local", command: ["echo", "three"] })
+        const newer = (yield* mcp.clients())["racing2"]
+
+        const late = yield* Fiber.join(slow) // completes late, and must not win
+        expect(localCommand(yield* mcp.spawned("racing2")), "an older add that completed late replaced the newer client").toEqual([
+          "echo",
+          "three",
+        ])
+        expect((yield* mcp.clients())["racing2"], "the newer client was closed by the late result").toBe(newer)
+        // The late call answers with what is serving, not with what it started.
+        expect(((late.status as any)["racing2"] ?? late.status).status).toBe("connected")
+      }),
+    ),
+  { config: { mcp: {} } },
+)
 // altimate_change end
 
 // altimate_change start — "removed means the runtime forgets it"