mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-12 21:53:00 -06:00
fix(cli): fail fast when onboarding profile is busy (#122968)
This commit is contained in:
committed by
GitHub
parent
c61ee511ab
commit
aa35346a8e
@@ -408,26 +408,18 @@ describe("onboard (non-interactive): gateway and remote auth", () => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it("serializes concurrent onboarding runs sharing one state directory", async () => {
|
||||
it("rejects concurrent onboarding runs sharing one state directory", async () => {
|
||||
await withStateDir("state-concurrent-onboard-", async (stateDir) => {
|
||||
let activeWorkspaceSetups = 0;
|
||||
let maxActiveWorkspaceSetups = 0;
|
||||
let workspaceSetupCalls = 0;
|
||||
let releaseFirstSetup!: () => void;
|
||||
const firstSetupEntered = new Promise<void>((resolve) => {
|
||||
ensureWorkspaceAndSessionsMock.mockImplementation(async () => {
|
||||
workspaceSetupCalls += 1;
|
||||
activeWorkspaceSetups += 1;
|
||||
maxActiveWorkspaceSetups = Math.max(maxActiveWorkspaceSetups, activeWorkspaceSetups);
|
||||
try {
|
||||
if (workspaceSetupCalls === 1) {
|
||||
resolve();
|
||||
await new Promise<void>((release) => {
|
||||
releaseFirstSetup = release;
|
||||
});
|
||||
}
|
||||
} finally {
|
||||
activeWorkspaceSetups -= 1;
|
||||
if (workspaceSetupCalls === 1) {
|
||||
resolve();
|
||||
await new Promise<void>((release) => {
|
||||
releaseFirstSetup = release;
|
||||
});
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -446,9 +438,10 @@ describe("onboard (non-interactive): gateway and remote auth", () => {
|
||||
await firstSetupEntered;
|
||||
const readsBeforeSecond = readConfigFileSnapshotMock.mock.calls.length;
|
||||
const writesBeforeSecond = capturedReplaceConfigFileCalls.length;
|
||||
const second = runNonInteractiveSetup(options, runtime);
|
||||
await new Promise<void>((resolve) => {
|
||||
setTimeout(resolve, 100);
|
||||
await expect(runNonInteractiveSetup(options, runtime)).rejects.toMatchObject({
|
||||
name: "SetupTargetLockedError",
|
||||
code: "setup_target_locked",
|
||||
holderPid: process.pid,
|
||||
});
|
||||
|
||||
expect(readConfigFileSnapshotMock).toHaveBeenCalledTimes(readsBeforeSecond);
|
||||
@@ -456,8 +449,8 @@ describe("onboard (non-interactive): gateway and remote auth", () => {
|
||||
expect(ensureWorkspaceAndSessionsMock).toHaveBeenCalledOnce();
|
||||
|
||||
releaseFirstSetup();
|
||||
await Promise.all([first, second]);
|
||||
expect(maxActiveWorkspaceSetups).toBe(1);
|
||||
await first;
|
||||
await runNonInteractiveSetup(options, runtime);
|
||||
expect(configWritePluginLeaseDepths).toHaveLength(2);
|
||||
expect(configWritePluginLeaseDepths.every((depth) => depth > 0)).toBe(true);
|
||||
} finally {
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
import { resolveStateDir } from "../../config/paths.js";
|
||||
import { FILE_LOCK_TIMEOUT_ERROR_CODE } from "../../infra/file-lock.js";
|
||||
import { withSetupMigrationTargetLock } from "../../wizard/setup.migration-snapshot.js";
|
||||
import {
|
||||
SetupTargetLockedError,
|
||||
withSetupMigrationTargetLock,
|
||||
} from "../../wizard/setup.migration-snapshot.js";
|
||||
|
||||
export const SETUP_ADMISSION_BUSY_MESSAGE =
|
||||
"OpenClaw setup is already in progress; try again when it finishes.";
|
||||
@@ -19,9 +21,9 @@ export async function runExclusiveSystemAgentSetupActivation<T>(
|
||||
return await task();
|
||||
};
|
||||
try {
|
||||
return await withSetupMigrationTargetLock(resolveStateDir(), admittedTask, { wait: false });
|
||||
return await withSetupMigrationTargetLock(resolveStateDir(), admittedTask);
|
||||
} catch (error) {
|
||||
if (!admitted && (error as { code?: unknown }).code === FILE_LOCK_TIMEOUT_ERROR_CODE) {
|
||||
if (!admitted && error instanceof SetupTargetLockedError) {
|
||||
throw new SetupAdmissionBusyError(SETUP_ADMISSION_BUSY_MESSAGE);
|
||||
}
|
||||
throw error;
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { createDeferred } from "../../test/helpers/promise.js";
|
||||
import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js";
|
||||
import { withEnvAsync } from "../test-utils/env.js";
|
||||
import { withSetupMigrationTargetLock } from "./setup.migration-snapshot.js";
|
||||
|
||||
const tempDirs = useAutoCleanupTempDirTracker(afterEach);
|
||||
|
||||
describe("setup migration target lock", () => {
|
||||
it("rejects a concurrent profile operation with the active holder", async () => {
|
||||
await withEnvAsync({ OPENCLAW_PROFILE: "lock-test" }, async () => {
|
||||
const stateDir = tempDirs.make("openclaw-setup-target-lock-");
|
||||
const firstAcquired = createDeferred();
|
||||
const releaseFirst = createDeferred();
|
||||
const first = withSetupMigrationTargetLock(stateDir, async () => {
|
||||
firstAcquired.resolve();
|
||||
await releaseFirst.promise;
|
||||
});
|
||||
await firstAcquired.promise;
|
||||
|
||||
let secondRan = false;
|
||||
const second = withSetupMigrationTargetLock(stateDir, async () => {
|
||||
secondRan = true;
|
||||
});
|
||||
let waitTimer: ReturnType<typeof setTimeout> | undefined;
|
||||
const outcome = await Promise.race([
|
||||
second.then(
|
||||
() => ({ kind: "acquired" as const }),
|
||||
(error: unknown) => ({ kind: "rejected" as const, error }),
|
||||
),
|
||||
new Promise<{ kind: "waiting" }>((resolve) => {
|
||||
waitTimer = setTimeout(() => resolve({ kind: "waiting" }), 1_000);
|
||||
}),
|
||||
]);
|
||||
clearTimeout(waitTimer);
|
||||
|
||||
releaseFirst.resolve();
|
||||
await first;
|
||||
if (outcome.kind === "waiting") {
|
||||
await second;
|
||||
}
|
||||
|
||||
expect(outcome.kind).toBe("rejected");
|
||||
if (outcome.kind !== "rejected") {
|
||||
return;
|
||||
}
|
||||
expect(outcome.error).toMatchObject({
|
||||
name: "SetupTargetLockedError",
|
||||
code: "setup_target_locked",
|
||||
holderPid: process.pid,
|
||||
});
|
||||
expect((outcome.error as Error).message).toBe(
|
||||
`Another onboarding/config operation is running for profile lock-test (pid ${process.pid}). Finish or abort it, then re-run.`,
|
||||
);
|
||||
expect(secondRan).toBe(false);
|
||||
|
||||
await expect(withSetupMigrationTargetLock(stateDir, async () => "ok")).resolves.toBe("ok");
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -5,14 +5,15 @@ import { createReadStream } from "node:fs";
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import type { OpenClawConfig } from "../config/types.openclaw.js";
|
||||
import { withFileLock } from "../infra/file-lock.js";
|
||||
import { FILE_LOCK_TIMEOUT_ERROR_CODE, withFileLock } from "../infra/file-lock.js";
|
||||
import { readJsonFile } from "../infra/json-files.js";
|
||||
import { isNotFoundPathError } from "../infra/path-guards.js";
|
||||
import type { MigrationPlan } from "../plugins/types.js";
|
||||
import { resolveUserPath } from "../utils.js";
|
||||
import { canonicalizeSetupMigrationValue } from "./setup.migration-canonical.js";
|
||||
|
||||
const ONBOARDING_TARGET_LOCK_OPTIONS = {
|
||||
retries: { retries: 1_200, factor: 1, minTimeout: 500, maxTimeout: 500 },
|
||||
retries: { retries: 0, factor: 1, minTimeout: 1, maxTimeout: 1 },
|
||||
stale: 30 * 60 * 1000,
|
||||
staleRecovery: "remove-if-unchanged" as const,
|
||||
};
|
||||
@@ -30,6 +31,24 @@ const MEANINGFUL_WORKSPACE_ENTRIES = [
|
||||
const IMPORT_BLOCKING_STATE_ENTRIES = ["credentials", "sessions", "agents"] as const;
|
||||
const MIGRATION_TARGET_STATE_ENTRIES = [...IMPORT_BLOCKING_STATE_ENTRIES, "state"] as const;
|
||||
|
||||
export class SetupTargetLockedError extends Error {
|
||||
readonly code = "setup_target_locked";
|
||||
|
||||
constructor(
|
||||
public readonly holderPid: number | undefined,
|
||||
profile: string | undefined,
|
||||
cause: unknown,
|
||||
) {
|
||||
const target = profile ? `profile ${profile}` : "the current profile";
|
||||
const owner = holderPid === undefined ? "" : ` (pid ${holderPid})`;
|
||||
super(
|
||||
`Another onboarding/config operation is running for ${target}${owner}. Finish or abort it, then re-run.`,
|
||||
{ cause },
|
||||
);
|
||||
this.name = "SetupTargetLockedError";
|
||||
}
|
||||
}
|
||||
|
||||
async function exists(candidate: string): Promise<boolean> {
|
||||
try {
|
||||
await fs.access(candidate);
|
||||
@@ -308,7 +327,6 @@ export async function prepareSetupMigrationAttemptBoundary(params: {
|
||||
export async function withSetupMigrationTargetLock<T>(
|
||||
stateDir: string,
|
||||
fn: () => Promise<T>,
|
||||
options?: { wait?: boolean },
|
||||
): Promise<T> {
|
||||
const resolvedStateDir = path.resolve(stateDir);
|
||||
const activeStateDir = activeSetupMigrationTargetLock.getStore();
|
||||
@@ -320,16 +338,25 @@ export async function withSetupMigrationTargetLock<T>(
|
||||
}
|
||||
const migrationDir = path.join(resolvedStateDir, "migration");
|
||||
await fs.mkdir(migrationDir, { recursive: true, mode: 0o700 });
|
||||
return await withFileLock(
|
||||
path.join(migrationDir, "onboarding.lock-target"),
|
||||
options?.wait === false
|
||||
? {
|
||||
...ONBOARDING_TARGET_LOCK_OPTIONS,
|
||||
retries: { ...ONBOARDING_TARGET_LOCK_OPTIONS.retries, retries: 0 },
|
||||
}
|
||||
: ONBOARDING_TARGET_LOCK_OPTIONS,
|
||||
async () => await activeSetupMigrationTargetLock.run(resolvedStateDir, fn),
|
||||
);
|
||||
const lockTarget = path.join(migrationDir, "onboarding.lock-target");
|
||||
let acquired = false;
|
||||
try {
|
||||
return await withFileLock(lockTarget, ONBOARDING_TARGET_LOCK_OPTIONS, async () => {
|
||||
acquired = true;
|
||||
return await activeSetupMigrationTargetLock.run(resolvedStateDir, fn);
|
||||
});
|
||||
} catch (error) {
|
||||
if (acquired || (error as { code?: unknown }).code !== FILE_LOCK_TIMEOUT_ERROR_CODE) {
|
||||
throw error;
|
||||
}
|
||||
const payload = await readJsonFile<{ pid?: unknown }>(`${lockTarget}.lock`, {
|
||||
maxBytes: 1_024,
|
||||
});
|
||||
const pid = payload?.pid;
|
||||
const holderPid =
|
||||
typeof pid === "number" && Number.isSafeInteger(pid) && pid > 0 ? pid : undefined;
|
||||
throw new SetupTargetLockedError(holderPid, process.env.OPENCLAW_PROFILE?.trim(), error);
|
||||
}
|
||||
}
|
||||
|
||||
export function assertFreshSetupMigrationTarget(freshness: {
|
||||
|
||||
Reference in New Issue
Block a user