From aa35346a8ef1f88c90f621985a0621b49ee82f31 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 12 Aug 2026 20:44:22 -0700 Subject: [PATCH] fix(cli): fail fast when onboarding profile is busy (#122968) --- .../onboard-non-interactive.gateway.test.ts | 31 ++++------ src/gateway/server-methods/setup-admission.ts | 10 ++-- .../setup.migration-snapshot.lock.test.ts | 60 +++++++++++++++++++ src/wizard/setup.migration-snapshot.ts | 53 ++++++++++++---- 4 files changed, 118 insertions(+), 36 deletions(-) create mode 100644 src/wizard/setup.migration-snapshot.lock.test.ts diff --git a/src/commands/onboard-non-interactive.gateway.test.ts b/src/commands/onboard-non-interactive.gateway.test.ts index 365631ab34a6..19d3769c0e9e 100644 --- a/src/commands/onboard-non-interactive.gateway.test.ts +++ b/src/commands/onboard-non-interactive.gateway.test.ts @@ -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((resolve) => { ensureWorkspaceAndSessionsMock.mockImplementation(async () => { workspaceSetupCalls += 1; - activeWorkspaceSetups += 1; - maxActiveWorkspaceSetups = Math.max(maxActiveWorkspaceSetups, activeWorkspaceSetups); - try { - if (workspaceSetupCalls === 1) { - resolve(); - await new Promise((release) => { - releaseFirstSetup = release; - }); - } - } finally { - activeWorkspaceSetups -= 1; + if (workspaceSetupCalls === 1) { + resolve(); + await new Promise((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((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 { diff --git a/src/gateway/server-methods/setup-admission.ts b/src/gateway/server-methods/setup-admission.ts index 2f489fdc5e00..397bae4de5b8 100644 --- a/src/gateway/server-methods/setup-admission.ts +++ b/src/gateway/server-methods/setup-admission.ts @@ -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( 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; diff --git a/src/wizard/setup.migration-snapshot.lock.test.ts b/src/wizard/setup.migration-snapshot.lock.test.ts new file mode 100644 index 000000000000..53d185c1e462 --- /dev/null +++ b/src/wizard/setup.migration-snapshot.lock.test.ts @@ -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 | 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"); + }); + }); +}); diff --git a/src/wizard/setup.migration-snapshot.ts b/src/wizard/setup.migration-snapshot.ts index 49cef2a417b2..557176741ec9 100644 --- a/src/wizard/setup.migration-snapshot.ts +++ b/src/wizard/setup.migration-snapshot.ts @@ -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 { try { await fs.access(candidate); @@ -308,7 +327,6 @@ export async function prepareSetupMigrationAttemptBoundary(params: { export async function withSetupMigrationTargetLock( stateDir: string, fn: () => Promise, - options?: { wait?: boolean }, ): Promise { const resolvedStateDir = path.resolve(stateDir); const activeStateDir = activeSetupMigrationTargetLock.getStore(); @@ -320,16 +338,25 @@ export async function withSetupMigrationTargetLock( } 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: {