fix(config): coordinate Grok and link writers

scripts/test-layout/layout.json

@@ -126,7 +126,7 @@
     "account-import.test.ts": "server",
     "account-pool-management-api.test.ts": "server",
     "link-listener-admission.test.ts": "server",
-    "link-listener-lifecycle.test.ts": "server",
+    "link-listener-lifecycle.test.ts": "server", "link-listener-mutation.test.ts": "server",
     "link-join-route.test.ts": "server",
     "core-link-boundary.test.ts": "lab",
     "acl-error-classification.test.ts": "lib",
@@ -993,7 +993,7 @@
     "gpt6-native-rows.test.ts": "codex-integration",
     "gpt61-sol-rows.test.ts": "codex-integration",
     "grok-attribution.test.ts": "providers/xai",
-    "grok-config-inject.test.ts": "providers/xai",
+    "grok-config-inject.test.ts": "providers/xai", "grok-config-write-lock.test.ts": "providers/xai",
     "grok-effort-inject.test.ts": "providers/xai",
     "grok-lifecycle.test.ts": "providers/xai",
     "grok-management-api.test.ts": "providers/xai",

src/server/index/link-listener.ts

@@ -8,9 +8,18 @@ import {
 } from "../../link/store";
 import { linkStorePath } from "../../link/paths";
 import { linkRouteAllowed } from "../../link/routes";
+import { withConfigMutationLockSync } from "../../config/mutation-lock";
 
 export const LINK_INGRESS_HOSTNAME = "opencodex-link.invalid";
 
+/** Internal signal: the store's port differs from the port this bind landed on. */
+class LinkListenerPortMismatchError extends Error {
+  constructor(storedPort: number, boundPort: number) {
+    super(`stored port ${storedPort} differs from bound port ${boundPort}`);
+    this.name = "LinkListenerPortMismatchError";
+  }
+}
+
 export interface LinkListenerStartContext<T> {
   dispatch: (req: Request, server: Server<T>) => Promise<Response>;
   maxRequestBodySize: number;
@@ -118,29 +127,33 @@ export function createLinkListenerLifecycle<T>(deps: LinkListenerDeps = {}): Lin
       closeWithoutAwait(bound);
       return;
     }
-    if (store.listenerPort === null) {
-      const port = bound.port;
-      if (!port || port === 0) {
-        closeWithoutAwait(bound);
-        reportFailure("bind", new Error("Bun did not report a concrete listener port"), "bind");
-        return;
-      }
-      try {
+    const port = bound.port;
+    if (!port || port === 0) {
+      closeWithoutAwait(bound);
+      reportFailure("bind", new Error("Bun did not report a concrete listener port"), "bind");
+      return;
+    }
+    try {
+      // Revalidate the admission and persist a new port in one mutation section.
+      // Even a known-port bind can race the deletion of its last recorded link.
+      const needed = withConfigMutationLockSync(() => {
         const current = readStore(storePath);
+        if (current.links.length === 0) return false;
         if (current.listenerPort !== null && current.listenerPort !== port) {
-          // Another writer fixed a different port while this bind ran. Tunnels target the stored
-          // port, so serving on this one would strand them: give it up and let the next
-          // ensureStarted() bind the stored port.
-          closeWithoutAwait(bound);
-          reportFailure("listenerPort persistence", new Error(`stored port ${current.listenerPort} differs from bound port ${port}`), "persist");
-          return;
+          throw new LinkListenerPortMismatchError(current.listenerPort, port);
         }
-        writeStore(storePath, { ...current, listenerPort: port });
-      } catch (error) {
+        if (current.listenerPort === null) writeStore(storePath, { ...current, listenerPort: port });
+        return true;
+      });
+      if (!needed) {
         closeWithoutAwait(bound);
-        reportFailure("listenerPort persistence", error, "persist");
+        setStatus("off", null, null);
         return;
       }
+    } catch (error) {
+      closeWithoutAwait(bound);
+      reportFailure("listenerPort persistence", error, "persist");
+      return;
     }
     listener = bound as Server<T>;
     relaySessions = sessions;

src/server/management/link-routes.ts

@@ -14,6 +14,7 @@ import { projectLinkStatus, type LinkStatusDto } from "../../link/status-project
 import { isLinkPort } from "../../link/ports";
 import { joinHome, type ClientLinkJoinDeps } from "../../client/link-join";
 import { clientLinkTunnelStatus } from "../../client/link-tunnel";
+import { withConfigMutationLockSync } from "../../config/mutation-lock";
 import type { ManagementContext } from "./context";
 import { readManagementJsonBodyOr } from "./body";
 import { issueApiKeyInProcess, revokeApiKeyInProcess, type IssuedApiKey } from "./oauth-account-routes";
@@ -228,22 +229,29 @@ async function compensateNewLink(ctx: ManagementContext, state: LinkRouteState,
   if (!revoked) {
     try { await state.supervisor.stopLink(record.id); } catch { /* retain the record and failure marker */ }
     try {
-      const current = readStoreFor(ctx);
-      if (!current.links.some(link => link.id === record.id)) {
-        writeStoreFor(ctx, { ...current, links: [...current.links, record] });
-      }
+      withConfigMutationLockSync(() => {
+        const current = readStoreFor(ctx);
+        if (!current.links.some(link => link.id === record.id)) {
+          writeStoreFor(ctx, { ...current, links: [...current.links, record] });
+        }
+      });
     } catch {
       // The in-memory marker still makes the residual visible when persistence is unavailable.
     }
     return compensationFailure(ctx, state, record, "revoke");
   }
   try { await state.supervisor.stopLink(record.id); } catch { return compensationFailure(ctx, state, record, "stop"); }
   try {
-    const current = readStoreFor(ctx);
-    const next = { ...current, links: current.links.filter(link => link.id !== record.id) };
-    if (next.links.length !== current.links.length) {
-      writeStoreFor(ctx, next);
-    }
+    // Read→filter→write stays one section of the shared mutation lock: a sibling
+    // route's create must not land between the read and the rename and vanish.
+    const next = withConfigMutationLockSync(() => {
+      const current = readStoreFor(ctx);
+      const filtered = { ...current, links: current.links.filter(link => link.id !== record.id) };
+      if (filtered.links.length !== current.links.length) {
+        writeStoreFor(ctx, filtered);
+      }
+      return filtered;
+    });
     clearCompensationFailed(record.id, compensationPath());
     state.compensationFailures?.delete(record.id);
     if (next.links.length === 0) await state.listener.close();
@@ -362,14 +370,20 @@ async function apply(ctx: ManagementContext, state: LinkRouteState): Promise<Res
   const record = { id, alias: body.alias, direction: "hub-initiated" as const, hostKeyFingerprint: confirmed.fingerprint, tunnelPort: remotePort, apiKeyId: issued.id, createdAt: new Date((ctx.deps.now ?? Date.now)()).toISOString() };
   let store: LinkStore;
   try {
-    const current = readStoreFor(ctx);
-    if (current.links.some(link => link.alias === body.alias && link.direction === "hub-initiated")) {
+    // Duplicate-check and append under one mutation-lock section: two writers
+    // racing this check could otherwise both pass it and append, losing one link.
+    const writeOutcome = withConfigMutationLockSync((): "dup" | "written" => {
+      const current = readStoreFor(ctx);
+      if (current.links.some(link => link.alias === body.alias && link.direction === "hub-initiated")) return "dup";
+      store = { ...current, links: [...current.links, record] };
+      writeStoreFor(ctx, store);
+      return "written";
+    });
+    if (writeOutcome === "dup") {
       const compensation = await compensateNewLink(ctx, state, record);
       if (compensation) return compensation;
       return fail("link_exists", "A link for this alias already exists.", 409);
     }
-    store = { ...current, links: [...current.links, record] };
-    writeStoreFor(ctx, store);
     await state.listener.ensureStarted();
     if (state.listener.status().state !== "listening") {
       const compensation = await compensateNewLink(ctx, state, record);
@@ -430,13 +444,18 @@ async function issue(ctx: ManagementContext): Promise<Response> {
   const record = { id, alias: body.alias, direction: "client-initiated" as const, hostKeyFingerprint: null, tunnelPort: body.tunnelPort, apiKeyId: issued.id, createdAt: new Date((ctx.deps.now ?? Date.now)()).toISOString() };
   let failureCode = "link_issue_failed";
   try {
-    const current = readStoreFor(ctx);
-    if (current.links.some(link => link.id === id)) throw new Error("link id collision");
-    if (current.links.some(link => link.alias === body.alias && link.direction === "client-initiated")) {
+    // Same atomic duplicate-check+append as the hub-initiated branch.
+    const writeOutcome = withConfigMutationLockSync((): "dup" | "written" => {
+      const current = readStoreFor(ctx);
+      if (current.links.some(link => link.id === id)) throw new Error("link id collision");
+      if (current.links.some(link => link.alias === body.alias && link.direction === "client-initiated")) return "dup";
+      writeStoreFor(ctx, { ...current, links: [...current.links, record] });
+      return "written";
+    });
+    if (writeOutcome === "dup") {
       const compensation = await compensateNewLink(ctx, state, record);
       return compensation ?? fail("link_exists", "A link for this alias already exists.", 409);
     }
-    writeStoreFor(ctx, { ...current, links: [...current.links, record] });
     await state.listener.ensureStarted();
     if (state.listener.status().state !== "listening") {
       failureCode = "listener_unavailable";
@@ -538,9 +557,12 @@ async function remove(ctx: ManagementContext, state: LinkRouteState, id: string)
   if (!revokeKeyIdempotent(ctx, record.apiKeyId)) return fail("key_revoke_failed", "The link key could not be revoked.", 502);
   let next: LinkStore;
   try {
-    const latest = readStoreFor(ctx);
-    next = { ...latest, links: latest.links.filter(link => link.id !== id) };
-    if (next.links.length !== latest.links.length) writeStoreFor(ctx, next);
+    next = withConfigMutationLockSync(() => {
+      const latest = readStoreFor(ctx);
+      const filtered = { ...latest, links: latest.links.filter(link => link.id !== id) };
+      if (filtered.links.length !== latest.links.length) writeStoreFor(ctx, filtered);
+      return filtered;
+    });
     clearCompensationFailed(id, compensationPath());
   } catch { return fail("link_remove_failed", "The link record could not be removed.", 503); }
   state.compensationFailures?.delete(id);

structure/remote-link.md

@@ -12,7 +12,7 @@ Every remote `ocx` call goes through `remoteOcxArgv`, which runs `sh -c` with a
 
 `src/link/routes.ts` holds the one route and method table for linked traffic; the hub-link listener and the client relay both decide admission from it.
 
-`src/link/store.ts` persists link records in `<configDir>/link/links.json` with private permissions. Records hold aliases, ports, confirmed host-key fingerprints and data-key ids, never keys. The file is a trust boundary: unknown fields, malformed values and duplicate ids are errors, and `hasLinks` reports false for a damaged file. A null host-key fingerprint is accepted only for a client-initiated link, because the hub never opens SSH to that client.
+`src/link/store.ts` persists link records in `<configDir>/link/links.json` with private permissions. Records hold aliases, ports, confirmed host-key fingerprints and data-key ids, never keys. The file is a trust boundary: unknown fields, malformed values and duplicate ids are errors, and `hasLinks` reports false for a damaged file. A null host-key fingerprint is accepted only for a client-initiated link, because the hub never opens SSH to that client. Link create/delete, compensation, and listener-port persistence re-read and mutate the store under `withConfigMutationLockSync`. A bind revalidates both the stored port and the presence of a recorded link before activation; deleting the last link during bind closes that listener without publishing a port.
 
 ## Client-initiated links
 

structure/runtime.md

@@ -179,7 +179,7 @@ factory), `relay-eager.ts` (#314 gated eager bounded passthrough relay), `memory
 (warn-only RSS sampler), `management/system-routes.ts` (`/api/system/*`), and `auth-cors.ts` own
 server infrastructure (`src/lib/bun-stream-caps.ts` owns the Bun stream-capability gate); and
 static GUI, WebSocket bridge, port/liveness, decompression, and adapter-resolution helpers live in
-their own files.
+their own files. `src/grok/inject.ts` holds the shared canonical config-write lock across injection and cleanup. It validates the locked destination before backup and every publication, preserves refresh-only admission and no-follow checks, and distinguishes retryable contention from an unsafe namespace. A refresh-only destination change before rename preserves files and returns the existing refresh skip result. Native Grok and manual editors do not participate in this advisory lock. `src/server/management/link-routes.ts` and `src/server/index/link-listener.ts` put their link-store read/decide/write sections under the existing config mutation transaction; SSH and other asynchronous work stays outside it. Link-store coordination follows [Remote Link](remote-link.md).
 
 ## Lifecycle
 

tests/fixtures/test-layout-expected.json

@@ -128,7 +128,8 @@
   "abort-idle-deadline.test.ts": "lib", "tool-envelope-echo-whole-line.test.ts": "adapters",
   "abort-race.test.ts": "adapters", "account-import.test.ts": "server",
   "account-pool-management-api.test.ts": "server", "link-listener-admission.test.ts": "server",
-  "link-listener-lifecycle.test.ts": "server", "link-join-route.test.ts": "server",
+  "link-listener-lifecycle.test.ts": "server",
+    "link-listener-mutation.test.ts": "server", "link-join-route.test.ts": "server",
   "core-link-boundary.test.ts": "lab", "acl-error-classification.test.ts": "lib",
   "active-registry-admission.test.ts": "codex-integration", "adapter-buffered-tool-conformance.test.ts": "adapters",
   "adapter-error-inline.test.ts": "adapters", "adapter-event-oauth-failover.test.ts": "oauth",
@@ -626,6 +627,7 @@
   "google-wire-compiler.test.ts": "adapters/google", "google-wire-shape.test.ts": "adapters/google",
   "gpt6-native-rows.test.ts": "codex-integration", "gpt61-sol-rows.test.ts": "codex-integration",
   "grok-attribution.test.ts": "providers/xai", "grok-config-inject.test.ts": "providers/xai",
+    "grok-config-write-lock.test.ts": "providers/xai",
   "grok-effort-inject.test.ts": "providers/xai", "grok-lifecycle.test.ts": "providers/xai",
   "grok-management-api.test.ts": "providers/xai", "grok-models-effort-list.test.ts": "providers/xai",
   "grok-orphan-adoption.test.ts": "providers/xai", "grok-reset-coupon-cli.test.ts": "providers/xai",

tests/providers/xai/grok-config-write-lock.test.ts

@@ -0,0 +1,65 @@
+import { afterEach, beforeEach, expect, spyOn, test } from "bun:test";
+import * as fs from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { withConfigWriteLock, configWriteLockPath } from "../../../src/codex/config-write-lock";
+import { buildGrokManagedBlock, injectGrokConfig, stripGrokConfig } from "../../../src/grok/inject";
+import { removeTreeWithRetry } from "../../helpers/remove-tree";
+
+let home: string;
+const models = [{ id: "fixture-model" }];
+beforeEach(() => { home = fs.mkdtempSync(join(tmpdir(), "ocx-grok-write-lock-")); });
+afterEach(() => { removeTreeWithRetry(home); });
+
+test("injection and cleanup refuse a held lock without touching config or backup", () => {
+  const path = join(home, "config.toml");
+  const bytes = buildGrokManagedBlock(10100, models) + "\n";
+  fs.writeFileSync(path, bytes);
+  const result = withConfigWriteLock(path, () => {
+    expect(injectGrokConfig(10101, models, { grokHome: home })).toMatchObject({ ok: false, changed: false, skippedReason: "locked", retryable: true });
+    expect(stripGrokConfig({ grokHome: home })).toMatchObject({ ok: false, changed: false, skippedReason: "locked", retryable: true });
+    expect(fs.readFileSync(path, "utf8")).toBe(bytes);
+    expect(fs.existsSync(join(home, "config.toml.bak-opencodex"))).toBe(false);
+  });
+  expect(result.ok).toBe(true);
+});
+
+test("unsafe lock namespaces are distinct from retryable contention", () => {
+  const path = join(home, "config.toml");
+  fs.writeFileSync(path, "theme = \"light\"\n");
+  fs.mkdirSync(configWriteLockPath(path));
+  expect(injectGrokConfig(10100, models, { grokHome: home })).toMatchObject({ ok: false, changed: false, skippedReason: "unsafe", retryable: false });
+  expect(stripGrokConfig({ grokHome: home })).toMatchObject({ ok: false, changed: false, skippedReason: "unsafe", retryable: false });
+  expect(fs.readFileSync(path, "utf8")).toBe("theme = \"light\"\n");
+});
+
+for (const action of ["inject", "strip"] as const) {
+  test(`${action} refuses an alias retarget after reading the locked destination`, () => {
+    const path = join(home, "config.toml");
+    const first = join(home, "first.toml");
+    const second = join(home, "second.toml");
+    const firstBytes = `theme = "first"\n${buildGrokManagedBlock(10100, models)}\n`;
+    const secondBytes = 'theme = "second"\n';
+    fs.writeFileSync(first, firstBytes);
+    fs.writeFileSync(second, secondBytes);
+    fs.symlinkSync(first, path);
+    const read = fs.readFileSync;
+    let retargeted = false;
+    const spy = spyOn(fs, "readFileSync").mockImplementation(((file: unknown, ...args: unknown[]) => {
+      const value = (read as Function)(file, ...args);
+      if (String(file) === path && !retargeted) {
+        retargeted = true;
+        fs.unlinkSync(path);
+        fs.symlinkSync(second, path);
+      }
+      return value;
+    }) as typeof fs.readFileSync);
+    try {
+      const result = action === "inject" ? injectGrokConfig(10101, models, { grokHome: home }) : stripGrokConfig({ grokHome: home });
+      expect(retargeted).toBe(true);
+      expect(result).toMatchObject({ ok: false, changed: false });
+      expect(fs.readFileSync(first, "utf8")).toBe(firstBytes);
+      expect(fs.readFileSync(second, "utf8")).toBe(secondBytes);
+    } finally { spy.mockRestore(); }
+  });
+}