From 07c5e2465b92c60508a15ad055e22e8722775aa8 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 13 May 2026 11:34:14 +0100 Subject: [PATCH] fix: handle rebased config mutation races --- .../browser/control-auth.auto-token.test.ts | 21 +++ .../src/browser/profiles-service.test.ts | 23 ++- .../server.control-server.test-harness.ts | 25 ++- .../src/bot.create-telegram-bot.test.ts | 2 +- .../server-methods/agents-mutate.test.ts | 49 ++++++ src/gateway/server-methods/agents.ts | 165 ++++++++++++------ 6 files changed, 230 insertions(+), 55 deletions(-) diff --git a/extensions/browser/src/browser/control-auth.auto-token.test.ts b/extensions/browser/src/browser/control-auth.auto-token.test.ts index fe978ee1a44..75bcd390528 100644 --- a/extensions/browser/src/browser/control-auth.auto-token.test.ts +++ b/extensions/browser/src/browser/control-auth.auto-token.test.ts @@ -8,6 +8,25 @@ const mocks = vi.hoisted(() => ({ replaceConfigFile: vi.fn(async ({ nextConfig }: { nextConfig: OpenClawConfig }) => { await mocks.writeConfigFile(nextConfig); }), + mutateConfigFile: vi.fn( + async (params: { + mutate: (draft: OpenClawConfig, context: { snapshot: { path: string } }) => unknown; + }) => { + const draft = structuredClone(mocks.getRuntimeConfig()); + const result = await params.mutate(draft, { snapshot: { path: "/tmp/openclaw.json" } }); + await mocks.writeConfigFile(draft); + return { + path: "/tmp/openclaw.json", + previousHash: "test-hash", + snapshot: { path: "/tmp/openclaw.json" }, + nextConfig: draft, + result, + attempts: 1, + afterWrite: { mode: "auto" }, + followUp: { action: "none" }, + }; + }, + ), resolveGatewayAuth: vi.fn( ({ authConfig, @@ -53,6 +72,7 @@ const mocks = vi.hoisted(() => ({ vi.mock("../config/config.js", () => ({ getRuntimeConfig: mocks.getRuntimeConfig, replaceConfigFile: mocks.replaceConfigFile, + mutateConfigFile: mocks.mutateConfigFile, })); vi.mock("../gateway/startup-auth.js", () => ({ @@ -146,6 +166,7 @@ describe("ensureBrowserControlAuth", () => { vi.restoreAllMocks(); mocks.getRuntimeConfig.mockClear(); mocks.writeConfigFile.mockClear(); + mocks.mutateConfigFile.mockClear(); mocks.resolveGatewayAuth.mockClear(); mocks.ensureGatewayStartupAuth.mockClear(); }); diff --git a/extensions/browser/src/browser/profiles-service.test.ts b/extensions/browser/src/browser/profiles-service.test.ts index cb8c87ec037..e998920f70e 100644 --- a/extensions/browser/src/browser/profiles-service.test.ts +++ b/extensions/browser/src/browser/profiles-service.test.ts @@ -8,7 +8,27 @@ import type { BrowserRouteContext, BrowserServerState } from "./server-context.j import { movePathToTrash } from "./trash.js"; const configMocks = vi.hoisted(() => ({ + getRuntimeConfig: vi.fn<() => OpenClawConfig>(), writeConfigFile: vi.fn<(cfg: OpenClawConfig) => Promise>(async (_cfg) => {}), + mutateConfigFile: vi.fn( + async (params: { + mutate: (draft: OpenClawConfig, context: { snapshot: { path: string } }) => unknown; + }) => { + const draft = structuredClone(configMocks.getRuntimeConfig()); + const result = await params.mutate(draft, { snapshot: { path: "/tmp/openclaw.json" } }); + await configMocks.writeConfigFile(draft); + return { + path: "/tmp/openclaw.json", + previousHash: "test-hash", + snapshot: { path: "/tmp/openclaw.json" }, + nextConfig: draft, + result, + attempts: 1, + afterWrite: { mode: "auto" }, + followUp: { action: "none" }, + }; + }, + ), })); const writeConfigFile = configMocks.writeConfigFile; @@ -16,10 +36,11 @@ vi.mock("../config/config.js", async () => { const actual = await vi.importActual("../config/config.js"); return { ...actual, - getRuntimeConfig: vi.fn(), replaceConfigFile: vi.fn(async ({ nextConfig }: { nextConfig: OpenClawConfig }) => { await configMocks.writeConfigFile(nextConfig); }), + mutateConfigFile: configMocks.mutateConfigFile, + getRuntimeConfig: configMocks.getRuntimeConfig, }; }); diff --git a/extensions/browser/src/browser/server.control-server.test-harness.ts b/extensions/browser/src/browser/server.control-server.test-harness.ts index f1067bb8ebc..ca342c103e9 100644 --- a/extensions/browser/src/browser/server.control-server.test-harness.ts +++ b/extensions/browser/src/browser/server.control-server.test-harness.ts @@ -423,7 +423,29 @@ vi.mock("../config/config.js", async () => { }, }; }; - const writeConfigFile = vi.fn(async () => {}); + const writeConfigFile = vi.fn(async (_cfg?: ReturnType) => {}); + const mutateConfigFile = vi.fn( + async (params: { + mutate: ( + draft: ReturnType, + context: { snapshot: { path: string } }, + ) => unknown; + }) => { + const draft = structuredClone(loadConfig()); + const result = await params.mutate(draft, { snapshot: { path: "/tmp/openclaw.json" } }); + await writeConfigFile(draft); + return { + path: "/tmp/openclaw.json", + previousHash: "test-hash", + snapshot: { path: "/tmp/openclaw.json" }, + nextConfig: draft, + result, + attempts: 1, + afterWrite: { mode: "auto" }, + followUp: { action: "none" }, + }; + }, + ); return { ...actual, createConfigIO: vi.fn(() => ({ @@ -434,6 +456,7 @@ vi.mock("../config/config.js", async () => { getRuntimeConfigSnapshot: vi.fn(() => null), loadConfig, writeConfigFile, + mutateConfigFile, }; }); diff --git a/extensions/telegram/src/bot.create-telegram-bot.test.ts b/extensions/telegram/src/bot.create-telegram-bot.test.ts index 70cd9ded769..6179ac62767 100644 --- a/extensions/telegram/src/bot.create-telegram-bot.test.ts +++ b/extensions/telegram/src/bot.create-telegram-bot.test.ts @@ -128,7 +128,7 @@ function installPerKeySequentializer(): void { } function mockTelegramConfigWrites() { - return vi.spyOn(configMutation, "replaceConfigFile").mockResolvedValue({} as never); + return vi.spyOn(configMutation, "mutateConfigFile").mockResolvedValue({} as never); } async function withEnvAsync(env: Record, fn: () => Promise) { diff --git a/src/gateway/server-methods/agents-mutate.test.ts b/src/gateway/server-methods/agents-mutate.test.ts index 05936356f49..e1e5f0b5228 100644 --- a/src/gateway/server-methods/agents-mutate.test.ts +++ b/src/gateway/server-methods/agents-mutate.test.ts @@ -583,6 +583,23 @@ describe("agents.create", () => { expect(mocks.writeConfigFile).not.toHaveBeenCalled(); }); + it("returns an invalid request when a concurrent create wins the config race", async () => { + let findCallCount = 0; + mocks.findAgentEntryIndex.mockImplementation(() => { + findCallCount += 1; + return findCallCount >= 2 ? 0 : -1; + }); + + const { respond, promise } = makeCall("agents.create", { + name: "Race Agent", + workspace: "/tmp/ws", + }); + await promise; + + expectRespondErrorContaining(respond, "already exists"); + expect(mocks.writeConfigFile).not.toHaveBeenCalled(); + }); + it("rejects invalid params (missing name)", async () => { const { respond, promise } = makeCall("agents.create", { workspace: "/tmp/ws", @@ -770,6 +787,22 @@ describe("agents.update", () => { expectNotFoundResponseAndNoWrite(respond); }); + it("returns not found when a concurrent delete wins the update race", async () => { + let findCallCount = 0; + mocks.findAgentEntryIndex.mockImplementation(() => { + findCallCount += 1; + return findCallCount >= 2 ? -1 : 0; + }); + + const { respond, promise } = makeCall("agents.update", { + agentId: "test-agent", + model: "gpt-5.5", + }); + await promise; + + expectNotFoundResponseAndNoWrite(respond); + }); + it("ensures workspace when workspace changes", async () => { const { promise } = makeCall("agents.update", { agentId: "test-agent", @@ -1132,6 +1165,22 @@ describe("agents.delete", () => { expectNotFoundResponseAndNoWrite(respond); }); + it("returns not found when a concurrent delete wins the delete race", async () => { + let findCallCount = 0; + mocks.findAgentEntryIndex.mockImplementation(() => { + findCallCount += 1; + return findCallCount >= 2 ? -1 : 0; + }); + + const { respond, promise } = makeCall("agents.delete", { + agentId: "test-agent", + }); + await promise; + + expectNotFoundResponseAndNoWrite(respond); + expect(mocks.movePathToTrash).not.toHaveBeenCalled(); + }); + it("rejects invalid params (missing agentId)", async () => { const { respond, promise } = makeCall("agents.delete", {}); await promise; diff --git a/src/gateway/server-methods/agents.ts b/src/gateway/server-methods/agents.ts index f9b6810cb23..c54852a753b 100644 --- a/src/gateway/server-methods/agents.ts +++ b/src/gateway/server-methods/agents.ts @@ -52,6 +52,13 @@ import { import { listAgentsForGateway } from "../session-utils.js"; import type { GatewayRequestHandlers, RespondFn } from "./types.js"; +type AgentDeleteMutationResult = { + workspaceDir: string; + agentDir: string; + sessionsDir: string; + removedBindings: number; +}; + const BOOTSTRAP_FILE_NAMES = [ DEFAULT_AGENTS_FILENAME, DEFAULT_SOUL_FILENAME, @@ -291,6 +298,35 @@ function respondAgentNotFound(respond: RespondFn, agentId: string): void { respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, `agent "${agentId}" not found`)); } +class AgentConfigPreconditionError extends Error { + constructor( + readonly kind: "already-exists" | "not-found", + readonly agentId: string, + ) { + super( + kind === "already-exists" + ? `agent "${agentId}" already exists` + : `agent "${agentId}" not found`, + ); + this.name = "AgentConfigPreconditionError"; + } +} + +function respondAgentConfigPreconditionError( + respond: RespondFn, + error: AgentConfigPreconditionError, +): void { + if (error.kind === "not-found") { + respondAgentNotFound(respond, error.agentId); + return; + } + respond( + false, + undefined, + errorShape(ErrorCodes.INVALID_REQUEST, `agent "${error.agentId}" already exists`), + ); +} + async function moveToTrashBestEffort(pathname: string): Promise { if (!pathname) { return; @@ -546,23 +582,31 @@ export const agentsHandlers: GatewayRequestHandlers = { return; } } - await mutateConfigFileWithRetry({ - afterWrite: { mode: "auto" }, - mutate: (draft) => { - if (findAgentEntryIndex(listAgentEntries(draft), agentId) >= 0) { - throw new Error(`agent "${agentId}" already exists`); - } - const latestNextConfig = applyAgentConfig(draft, { - agentId, - name: safeName, - workspace: workspaceDir, - model, - identity, - agentDir, - }); - Object.assign(draft, latestNextConfig); - }, - }); + try { + await mutateConfigFileWithRetry({ + afterWrite: { mode: "auto" }, + mutate: (draft) => { + if (findAgentEntryIndex(listAgentEntries(draft), agentId) >= 0) { + throw new AgentConfigPreconditionError("already-exists", agentId); + } + const latestNextConfig = applyAgentConfig(draft, { + agentId, + name: safeName, + workspace: workspaceDir, + model, + identity, + agentDir, + }); + Object.assign(draft, latestNextConfig); + }, + }); + } catch (error) { + if (error instanceof AgentConfigPreconditionError) { + respondAgentConfigPreconditionError(respond, error); + return; + } + throw error; + } respond(true, { ok: true, agentId, name: safeName, workspace: workspaceDir, model }, undefined); }, @@ -651,22 +695,30 @@ export const agentsHandlers: GatewayRequestHandlers = { } } - await mutateConfigFileWithRetry({ - afterWrite: { mode: "auto" }, - mutate: (draft) => { - if (!isConfiguredAgent(draft, agentId)) { - throw new Error(`agent "${agentId}" not found`); - } - const latestNextConfig = applyAgentConfig(draft, { - agentId, - ...(safeName ? { name: safeName } : {}), - ...(workspaceDir ? { workspace: workspaceDir } : {}), - ...(model ? { model } : {}), - ...(identity ? { identity } : {}), - }); - Object.assign(draft, latestNextConfig); - }, - }); + try { + await mutateConfigFileWithRetry({ + afterWrite: { mode: "auto" }, + mutate: (draft) => { + if (!isConfiguredAgent(draft, agentId)) { + throw new AgentConfigPreconditionError("not-found", agentId); + } + const latestNextConfig = applyAgentConfig(draft, { + agentId, + ...(safeName ? { name: safeName } : {}), + ...(workspaceDir ? { workspace: workspaceDir } : {}), + ...(model ? { model } : {}), + ...(identity ? { identity } : {}), + }); + Object.assign(draft, latestNextConfig); + }, + }); + } catch (error) { + if (error instanceof AgentConfigPreconditionError) { + respondAgentConfigPreconditionError(respond, error); + return; + } + throw error; + } respond(true, { ok: true, agentId }, undefined); }, @@ -692,25 +744,34 @@ export const agentsHandlers: GatewayRequestHandlers = { } const deleteFiles = typeof params.deleteFiles === "boolean" ? params.deleteFiles : true; - const committed = await mutateConfigFileWithRetry({ - afterWrite: { mode: "auto" }, - mutate: (draft) => { - if (!isConfiguredAgent(draft, agentId)) { - throw new Error(`Agent "${agentId}" not found`); - } - const workspaceDir = resolveAgentWorkspaceDir(draft, agentId); - const agentDir = resolveAgentDir(draft, agentId); - const sessionsDir = resolveSessionTranscriptsDirForAgent(agentId); - const result = pruneAgentConfig(draft, agentId); - Object.assign(draft, result.config); - return { - workspaceDir, - agentDir, - sessionsDir, - removedBindings: result.removedBindings, - }; - }, - }); + let committed: Awaited>>; + try { + committed = await mutateConfigFileWithRetry({ + afterWrite: { mode: "auto" }, + mutate: (draft) => { + if (!isConfiguredAgent(draft, agentId)) { + throw new AgentConfigPreconditionError("not-found", agentId); + } + const workspaceDir = resolveAgentWorkspaceDir(draft, agentId); + const agentDir = resolveAgentDir(draft, agentId); + const sessionsDir = resolveSessionTranscriptsDirForAgent(agentId); + const result = pruneAgentConfig(draft, agentId); + Object.assign(draft, result.config); + return { + workspaceDir, + agentDir, + sessionsDir, + removedBindings: result.removedBindings, + }; + }, + }); + } catch (error) { + if (error instanceof AgentConfigPreconditionError) { + respondAgentConfigPreconditionError(respond, error); + return; + } + throw error; + } const deleteResult = committed.result; if (!deleteResult) { respond(false, undefined, errorShape(ErrorCodes.UNAVAILABLE, "agent delete did not commit"));