fix: handle rebased config mutation races

This commit is contained in:
Peter Steinberger
2026-05-13 11:34:14 +01:00
parent 743cbc2f13
commit 07c5e2465b
6 changed files with 230 additions and 55 deletions

View File

@@ -8,6 +8,25 @@ const mocks = vi.hoisted(() => ({
replaceConfigFile: vi.fn(async ({ nextConfig }: { nextConfig: OpenClawConfig }) => { replaceConfigFile: vi.fn(async ({ nextConfig }: { nextConfig: OpenClawConfig }) => {
await mocks.writeConfigFile(nextConfig); 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( resolveGatewayAuth: vi.fn(
({ ({
authConfig, authConfig,
@@ -53,6 +72,7 @@ const mocks = vi.hoisted(() => ({
vi.mock("../config/config.js", () => ({ vi.mock("../config/config.js", () => ({
getRuntimeConfig: mocks.getRuntimeConfig, getRuntimeConfig: mocks.getRuntimeConfig,
replaceConfigFile: mocks.replaceConfigFile, replaceConfigFile: mocks.replaceConfigFile,
mutateConfigFile: mocks.mutateConfigFile,
})); }));
vi.mock("../gateway/startup-auth.js", () => ({ vi.mock("../gateway/startup-auth.js", () => ({
@@ -146,6 +166,7 @@ describe("ensureBrowserControlAuth", () => {
vi.restoreAllMocks(); vi.restoreAllMocks();
mocks.getRuntimeConfig.mockClear(); mocks.getRuntimeConfig.mockClear();
mocks.writeConfigFile.mockClear(); mocks.writeConfigFile.mockClear();
mocks.mutateConfigFile.mockClear();
mocks.resolveGatewayAuth.mockClear(); mocks.resolveGatewayAuth.mockClear();
mocks.ensureGatewayStartupAuth.mockClear(); mocks.ensureGatewayStartupAuth.mockClear();
}); });

View File

@@ -8,7 +8,27 @@ import type { BrowserRouteContext, BrowserServerState } from "./server-context.j
import { movePathToTrash } from "./trash.js"; import { movePathToTrash } from "./trash.js";
const configMocks = vi.hoisted(() => ({ const configMocks = vi.hoisted(() => ({
getRuntimeConfig: vi.fn<() => OpenClawConfig>(),
writeConfigFile: vi.fn<(cfg: OpenClawConfig) => Promise<void>>(async (_cfg) => {}), writeConfigFile: vi.fn<(cfg: OpenClawConfig) => Promise<void>>(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; const writeConfigFile = configMocks.writeConfigFile;
@@ -16,10 +36,11 @@ vi.mock("../config/config.js", async () => {
const actual = await vi.importActual<typeof import("../config/config.js")>("../config/config.js"); const actual = await vi.importActual<typeof import("../config/config.js")>("../config/config.js");
return { return {
...actual, ...actual,
getRuntimeConfig: vi.fn(),
replaceConfigFile: vi.fn(async ({ nextConfig }: { nextConfig: OpenClawConfig }) => { replaceConfigFile: vi.fn(async ({ nextConfig }: { nextConfig: OpenClawConfig }) => {
await configMocks.writeConfigFile(nextConfig); await configMocks.writeConfigFile(nextConfig);
}), }),
mutateConfigFile: configMocks.mutateConfigFile,
getRuntimeConfig: configMocks.getRuntimeConfig,
}; };
}); });

View File

@@ -423,7 +423,29 @@ vi.mock("../config/config.js", async () => {
}, },
}; };
}; };
const writeConfigFile = vi.fn(async () => {}); const writeConfigFile = vi.fn(async (_cfg?: ReturnType<typeof loadConfig>) => {});
const mutateConfigFile = vi.fn(
async (params: {
mutate: (
draft: ReturnType<typeof loadConfig>,
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 { return {
...actual, ...actual,
createConfigIO: vi.fn(() => ({ createConfigIO: vi.fn(() => ({
@@ -434,6 +456,7 @@ vi.mock("../config/config.js", async () => {
getRuntimeConfigSnapshot: vi.fn(() => null), getRuntimeConfigSnapshot: vi.fn(() => null),
loadConfig, loadConfig,
writeConfigFile, writeConfigFile,
mutateConfigFile,
}; };
}); });

View File

@@ -128,7 +128,7 @@ function installPerKeySequentializer(): void {
} }
function mockTelegramConfigWrites() { function mockTelegramConfigWrites() {
return vi.spyOn(configMutation, "replaceConfigFile").mockResolvedValue({} as never); return vi.spyOn(configMutation, "mutateConfigFile").mockResolvedValue({} as never);
} }
async function withEnvAsync(env: Record<string, string | undefined>, fn: () => Promise<void>) { async function withEnvAsync(env: Record<string, string | undefined>, fn: () => Promise<void>) {

View File

@@ -583,6 +583,23 @@ describe("agents.create", () => {
expect(mocks.writeConfigFile).not.toHaveBeenCalled(); 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 () => { it("rejects invalid params (missing name)", async () => {
const { respond, promise } = makeCall("agents.create", { const { respond, promise } = makeCall("agents.create", {
workspace: "/tmp/ws", workspace: "/tmp/ws",
@@ -770,6 +787,22 @@ describe("agents.update", () => {
expectNotFoundResponseAndNoWrite(respond); 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 () => { it("ensures workspace when workspace changes", async () => {
const { promise } = makeCall("agents.update", { const { promise } = makeCall("agents.update", {
agentId: "test-agent", agentId: "test-agent",
@@ -1132,6 +1165,22 @@ describe("agents.delete", () => {
expectNotFoundResponseAndNoWrite(respond); 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 () => { it("rejects invalid params (missing agentId)", async () => {
const { respond, promise } = makeCall("agents.delete", {}); const { respond, promise } = makeCall("agents.delete", {});
await promise; await promise;

View File

@@ -52,6 +52,13 @@ import {
import { listAgentsForGateway } from "../session-utils.js"; import { listAgentsForGateway } from "../session-utils.js";
import type { GatewayRequestHandlers, RespondFn } from "./types.js"; import type { GatewayRequestHandlers, RespondFn } from "./types.js";
type AgentDeleteMutationResult = {
workspaceDir: string;
agentDir: string;
sessionsDir: string;
removedBindings: number;
};
const BOOTSTRAP_FILE_NAMES = [ const BOOTSTRAP_FILE_NAMES = [
DEFAULT_AGENTS_FILENAME, DEFAULT_AGENTS_FILENAME,
DEFAULT_SOUL_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`)); 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<void> { async function moveToTrashBestEffort(pathname: string): Promise<void> {
if (!pathname) { if (!pathname) {
return; return;
@@ -546,23 +582,31 @@ export const agentsHandlers: GatewayRequestHandlers = {
return; return;
} }
} }
await mutateConfigFileWithRetry({ try {
afterWrite: { mode: "auto" }, await mutateConfigFileWithRetry({
mutate: (draft) => { afterWrite: { mode: "auto" },
if (findAgentEntryIndex(listAgentEntries(draft), agentId) >= 0) { mutate: (draft) => {
throw new Error(`agent "${agentId}" already exists`); if (findAgentEntryIndex(listAgentEntries(draft), agentId) >= 0) {
} throw new AgentConfigPreconditionError("already-exists", agentId);
const latestNextConfig = applyAgentConfig(draft, { }
agentId, const latestNextConfig = applyAgentConfig(draft, {
name: safeName, agentId,
workspace: workspaceDir, name: safeName,
model, workspace: workspaceDir,
identity, model,
agentDir, identity,
}); agentDir,
Object.assign(draft, latestNextConfig); });
}, 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); respond(true, { ok: true, agentId, name: safeName, workspace: workspaceDir, model }, undefined);
}, },
@@ -651,22 +695,30 @@ export const agentsHandlers: GatewayRequestHandlers = {
} }
} }
await mutateConfigFileWithRetry({ try {
afterWrite: { mode: "auto" }, await mutateConfigFileWithRetry({
mutate: (draft) => { afterWrite: { mode: "auto" },
if (!isConfiguredAgent(draft, agentId)) { mutate: (draft) => {
throw new Error(`agent "${agentId}" not found`); if (!isConfiguredAgent(draft, agentId)) {
} throw new AgentConfigPreconditionError("not-found", agentId);
const latestNextConfig = applyAgentConfig(draft, { }
agentId, const latestNextConfig = applyAgentConfig(draft, {
...(safeName ? { name: safeName } : {}), agentId,
...(workspaceDir ? { workspace: workspaceDir } : {}), ...(safeName ? { name: safeName } : {}),
...(model ? { model } : {}), ...(workspaceDir ? { workspace: workspaceDir } : {}),
...(identity ? { identity } : {}), ...(model ? { model } : {}),
}); ...(identity ? { identity } : {}),
Object.assign(draft, latestNextConfig); });
}, Object.assign(draft, latestNextConfig);
}); },
});
} catch (error) {
if (error instanceof AgentConfigPreconditionError) {
respondAgentConfigPreconditionError(respond, error);
return;
}
throw error;
}
respond(true, { ok: true, agentId }, undefined); respond(true, { ok: true, agentId }, undefined);
}, },
@@ -692,25 +744,34 @@ export const agentsHandlers: GatewayRequestHandlers = {
} }
const deleteFiles = typeof params.deleteFiles === "boolean" ? params.deleteFiles : true; const deleteFiles = typeof params.deleteFiles === "boolean" ? params.deleteFiles : true;
const committed = await mutateConfigFileWithRetry({ let committed: Awaited<ReturnType<typeof mutateConfigFileWithRetry<AgentDeleteMutationResult>>>;
afterWrite: { mode: "auto" }, try {
mutate: (draft) => { committed = await mutateConfigFileWithRetry({
if (!isConfiguredAgent(draft, agentId)) { afterWrite: { mode: "auto" },
throw new Error(`Agent "${agentId}" not found`); mutate: (draft) => {
} if (!isConfiguredAgent(draft, agentId)) {
const workspaceDir = resolveAgentWorkspaceDir(draft, agentId); throw new AgentConfigPreconditionError("not-found", agentId);
const agentDir = resolveAgentDir(draft, agentId); }
const sessionsDir = resolveSessionTranscriptsDirForAgent(agentId); const workspaceDir = resolveAgentWorkspaceDir(draft, agentId);
const result = pruneAgentConfig(draft, agentId); const agentDir = resolveAgentDir(draft, agentId);
Object.assign(draft, result.config); const sessionsDir = resolveSessionTranscriptsDirForAgent(agentId);
return { const result = pruneAgentConfig(draft, agentId);
workspaceDir, Object.assign(draft, result.config);
agentDir, return {
sessionsDir, workspaceDir,
removedBindings: result.removedBindings, agentDir,
}; sessionsDir,
}, removedBindings: result.removedBindings,
}); };
},
});
} catch (error) {
if (error instanceof AgentConfigPreconditionError) {
respondAgentConfigPreconditionError(respond, error);
return;
}
throw error;
}
const deleteResult = committed.result; const deleteResult = committed.result;
if (!deleteResult) { if (!deleteResult) {
respond(false, undefined, errorShape(ErrorCodes.UNAVAILABLE, "agent delete did not commit")); respond(false, undefined, errorShape(ErrorCodes.UNAVAILABLE, "agent delete did not commit"));