diff --git a/src/agents/sandbox/registry.test.ts b/src/agents/sandbox/registry.test.ts index 783c4e05574..87a29798765 100644 --- a/src/agents/sandbox/registry.test.ts +++ b/src/agents/sandbox/registry.test.ts @@ -70,6 +70,7 @@ vi.mock("../../infra/json-files.js", async () => { }); import { + migrateLegacySandboxRegistryFiles, readBrowserRegistry, readRegistry, readRegistryEntry, @@ -172,7 +173,15 @@ async function seedContainerRegistry(entries: SandboxRegistryEntry[]) { } describe("registry race safety", () => { - it("normalizes legacy registry entries on read", async () => { + it("does not migrate legacy registry files from runtime reads", async () => { + await seedContainerRegistry([containerEntry({ containerName: "legacy-container" })]); + + await expect(readRegistry()).resolves.toEqual({ entries: [] }); + await expect(readRegistryEntry("legacy-container")).resolves.toBeNull(); + await expect(fs.access(SANDBOX_REGISTRY_PATH)).resolves.toBeUndefined(); + }); + + it("normalizes legacy registry entries after explicit migration", async () => { await seedContainerRegistry([ { containerName: "legacy-container", @@ -183,6 +192,7 @@ describe("registry race safety", () => { }, ]); + await migrateLegacySandboxRegistryFiles(); const registry = await readRegistry(); expect(registry.entries).toEqual([ expect.objectContaining({ @@ -194,6 +204,33 @@ describe("registry race safety", () => { ]); }); + it("does not overwrite newer sharded entries during legacy migration", async () => { + await updateRegistry( + containerEntry({ + containerName: "container-a", + sessionKey: "new-session", + lastUsedAtMs: 10, + }), + ); + await seedContainerRegistry([ + containerEntry({ + containerName: "container-a", + sessionKey: "legacy-session", + lastUsedAtMs: 1, + }), + ]); + + await migrateLegacySandboxRegistryFiles(); + + const entry = await readRegistryEntry("container-a"); + expect(entry).toEqual( + expect.objectContaining({ + sessionKey: "new-session", + lastUsedAtMs: 10, + }), + ); + }); + it("reads a single sharded entry without scanning the full registry", async () => { await updateRegistry(containerEntry({ containerName: "container-x", sessionKey: "sess:x" })); await updateRegistry(containerEntry({ containerName: "container-y", sessionKey: "sess:y" })); @@ -241,7 +278,7 @@ describe("registry race safety", () => { }); it("stores unsafe container names as encoded shard filenames", async () => { - await seedContainerRegistry([containerEntry({ containerName: "../escape" })]); + await updateRegistry(containerEntry({ containerName: "../escape" })); const registry = await readRegistry(); @@ -299,24 +336,23 @@ describe("registry race safety", () => { it("quarantines malformed legacy registry files during migration", async () => { await seedMalformedContainerRegistry("{bad json"); await seedMalformedBrowserRegistry("{bad json"); - await expect(updateRegistry(containerEntry())).resolves.toBeUndefined(); - await expect(updateBrowserRegistry(browserEntry())).resolves.toBeUndefined(); + const results = await migrateLegacySandboxRegistryFiles(); await expect(fs.access(SANDBOX_REGISTRY_PATH)).rejects.toThrow(); await expect(fs.access(SANDBOX_BROWSER_REGISTRY_PATH)).rejects.toThrow(); - await expect(readRegistry()).resolves.toEqual({ - entries: [expect.objectContaining({ containerName: "container-a" })], - }); - await expect(readBrowserRegistry()).resolves.toEqual({ - entries: [expect.objectContaining({ containerName: "browser-a" })], - }); + expect(results.map((result) => result.status)).toEqual([ + "quarantined-invalid", + "quarantined-invalid", + ]); }); it("quarantines legacy registry files with invalid entries during migration", async () => { const invalidEntries = `{"entries":[{"sessionKey":"agent:main"}]}`; await seedMalformedContainerRegistry(invalidEntries); await seedMalformedBrowserRegistry(invalidEntries); - await expect(updateRegistry(containerEntry())).resolves.toBeUndefined(); - await expect(updateBrowserRegistry(browserEntry())).resolves.toBeUndefined(); + await expect(migrateLegacySandboxRegistryFiles()).resolves.toEqual([ + expect.objectContaining({ kind: "containers", status: "quarantined-invalid" }), + expect.objectContaining({ kind: "browsers", status: "quarantined-invalid" }), + ]); }); }); diff --git a/src/agents/sandbox/registry.ts b/src/agents/sandbox/registry.ts index 29f5b8b07bd..99876823547 100644 --- a/src/agents/sandbox/registry.ts +++ b/src/agents/sandbox/registry.ts @@ -53,6 +53,26 @@ type RegistryFile = { entries: RegistryEntryPayload[]; }; +type LegacyRegistryKind = "containers" | "browsers"; + +type LegacyRegistryTarget = { + kind: LegacyRegistryKind; + registryPath: string; + shardedDir: string; +}; + +export type LegacySandboxRegistryInspection = LegacyRegistryTarget & { + exists: boolean; + valid: boolean; + entries: number; +}; + +export type LegacySandboxRegistryMigrationResult = LegacyRegistryTarget & { + status: "missing" | "migrated" | "removed-empty" | "quarantined-invalid"; + entries: number; + quarantinePath?: string; +}; + const RegistryEntrySchema = z .object({ containerName: z.string(), @@ -103,7 +123,6 @@ async function readLegacyRegistryFile(registryPath: string): Promise { - await migrateMonolithicIfNeeded(SANDBOX_REGISTRY_PATH, SANDBOX_CONTAINERS_DIR); const entries = await readShardedEntries(SANDBOX_CONTAINERS_DIR); return { entries: entries.map((entry) => normalizeSandboxRegistryEntry(entry)), @@ -197,7 +216,7 @@ async function readShardedEntries(dir: string): Promise ); } -async function quarantineLegacyRegistry(registryPath: string): Promise { +async function quarantineLegacyRegistry(registryPath: string): Promise { const quarantinePath = `${registryPath}.invalid-${Date.now()}`; await fs.rename(registryPath, quarantinePath).catch(async (error) => { const code = (error as { code?: string } | null)?.code; @@ -205,49 +224,107 @@ async function quarantineLegacyRegistry(registryPath: string): Promise { await fs.rm(registryPath, { force: true }); } }); + return quarantinePath; } -async function migrateMonolithicIfNeeded(registryPath: string, shardedDir: string): Promise { +async function migrateMonolithicIfNeeded( + target: LegacyRegistryTarget, +): Promise { + const { registryPath, shardedDir } = target; try { await fs.access(registryPath); } catch (error) { const code = (error as { code?: string } | null)?.code; if (code === "ENOENT") { - return; + return { ...target, status: "missing", entries: 0 }; } throw error; } - await withRegistryLock(registryPath, async () => { + return await withRegistryLock(registryPath, async () => { const registry = await readLegacyRegistryFile(registryPath); if (!registry) { - await quarantineLegacyRegistry(registryPath); - return; + const quarantinePath = await quarantineLegacyRegistry(registryPath); + return { ...target, status: "quarantined-invalid", entries: 0, quarantinePath }; } if (registry.entries.length === 0) { await fs.rm(registryPath, { force: true }); - return; + return { ...target, status: "removed-empty", entries: 0 }; } await fs.mkdir(shardedDir, { recursive: true }); for (const entry of registry.entries) { await withEntryLock(shardedDir, entry.containerName, async () => { - await writeShardedEntry(shardedDir, entry); + const existing = await readShardedEntry(shardedDir, entry.containerName); + if (!existing) { + await writeShardedEntry(shardedDir, entry); + } }); } await fs.rm(registryPath, { force: true }); + return { ...target, status: "migrated", entries: registry.entries.length }; }); } +function legacyRegistryTargets(): LegacyRegistryTarget[] { + return [ + { + kind: "containers", + registryPath: SANDBOX_REGISTRY_PATH, + shardedDir: SANDBOX_CONTAINERS_DIR, + }, + { + kind: "browsers", + registryPath: SANDBOX_BROWSER_REGISTRY_PATH, + shardedDir: SANDBOX_BROWSERS_DIR, + }, + ]; +} + +export async function inspectLegacySandboxRegistryFiles(): Promise< + LegacySandboxRegistryInspection[] +> { + const inspections: LegacySandboxRegistryInspection[] = []; + for (const target of legacyRegistryTargets()) { + try { + await fs.access(target.registryPath); + } catch (error) { + const code = (error as { code?: string } | null)?.code; + if (code === "ENOENT") { + inspections.push({ ...target, exists: false, valid: true, entries: 0 }); + continue; + } + throw error; + } + + const registry = await readLegacyRegistryFile(target.registryPath); + inspections.push({ + ...target, + exists: true, + valid: Boolean(registry), + entries: registry?.entries.length ?? 0, + }); + } + return inspections; +} + +export async function migrateLegacySandboxRegistryFiles(): Promise< + LegacySandboxRegistryMigrationResult[] +> { + const results: LegacySandboxRegistryMigrationResult[] = []; + for (const target of legacyRegistryTargets()) { + results.push(await migrateMonolithicIfNeeded(target)); + } + return results; +} + export async function readRegistryEntry( containerName: string, ): Promise { - await migrateMonolithicIfNeeded(SANDBOX_REGISTRY_PATH, SANDBOX_CONTAINERS_DIR); const entry = await readShardedEntry(SANDBOX_CONTAINERS_DIR, containerName); return entry ? normalizeSandboxRegistryEntry(entry) : null; } export async function updateRegistry(entry: SandboxRegistryEntry) { - await migrateMonolithicIfNeeded(SANDBOX_REGISTRY_PATH, SANDBOX_CONTAINERS_DIR); await withEntryLock(SANDBOX_CONTAINERS_DIR, entry.containerName, async () => { const existing = await readShardedEntry( SANDBOX_CONTAINERS_DIR, @@ -266,19 +343,16 @@ export async function updateRegistry(entry: SandboxRegistryEntry) { } export async function removeRegistryEntry(containerName: string) { - await migrateMonolithicIfNeeded(SANDBOX_REGISTRY_PATH, SANDBOX_CONTAINERS_DIR); await withEntryLock(SANDBOX_CONTAINERS_DIR, containerName, async () => { await removeShardedEntry(SANDBOX_CONTAINERS_DIR, containerName); }); } export async function readBrowserRegistry(): Promise { - await migrateMonolithicIfNeeded(SANDBOX_BROWSER_REGISTRY_PATH, SANDBOX_BROWSERS_DIR); return { entries: await readShardedEntries(SANDBOX_BROWSERS_DIR) }; } export async function updateBrowserRegistry(entry: SandboxBrowserRegistryEntry) { - await migrateMonolithicIfNeeded(SANDBOX_BROWSER_REGISTRY_PATH, SANDBOX_BROWSERS_DIR); await withEntryLock(SANDBOX_BROWSERS_DIR, entry.containerName, async () => { const existing = await readShardedEntry( SANDBOX_BROWSERS_DIR, @@ -294,7 +368,6 @@ export async function updateBrowserRegistry(entry: SandboxBrowserRegistryEntry) } export async function removeBrowserRegistryEntry(containerName: string) { - await migrateMonolithicIfNeeded(SANDBOX_BROWSER_REGISTRY_PATH, SANDBOX_BROWSERS_DIR); await withEntryLock(SANDBOX_BROWSERS_DIR, containerName, async () => { await removeShardedEntry(SANDBOX_BROWSERS_DIR, containerName); }); diff --git a/src/commands/doctor-sandbox.ts b/src/commands/doctor-sandbox.ts index fda5fd5d849..cd17df21f6f 100644 --- a/src/commands/doctor-sandbox.ts +++ b/src/commands/doctor-sandbox.ts @@ -7,10 +7,18 @@ import { isDockerDaemonUnavailable, resolveSandboxScope, } from "../agents/sandbox.js"; +import { + inspectLegacySandboxRegistryFiles, + migrateLegacySandboxRegistryFiles, + type LegacySandboxRegistryInspection, + type LegacySandboxRegistryMigrationResult, +} from "../agents/sandbox/registry.js"; +import { formatCliCommand } from "../cli/command-format.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { runCommandWithTimeout, runExec } from "../process/exec.js"; import type { RuntimeEnv } from "../runtime.js"; import { note } from "../terminal/note.js"; +import { shortenHomePath } from "../utils.js"; import type { DoctorPrompter } from "./doctor-prompter.js"; type SandboxScriptInfo = { @@ -266,6 +274,53 @@ export async function maybeRepairSandboxImages( return next; } +function formatLegacyRegistryInspectionLine(file: LegacySandboxRegistryInspection): string { + const status = file.valid ? `${file.entries} entr${file.entries === 1 ? "y" : "ies"}` : "invalid"; + return `- ${file.kind}: ${shortenHomePath(file.registryPath)} (${status})`; +} + +function formatLegacyRegistryMigrationLine(result: LegacySandboxRegistryMigrationResult): string { + const file = shortenHomePath(result.registryPath); + if (result.status === "migrated") { + return `- Migrated ${result.kind} registry from ${file} into ${result.entries} shard${result.entries === 1 ? "" : "s"}.`; + } + if (result.status === "removed-empty") { + return `- Removed empty legacy ${result.kind} registry ${file}.`; + } + if (result.status === "quarantined-invalid") { + const quarantine = result.quarantinePath ? ` to ${shortenHomePath(result.quarantinePath)}` : ""; + return `- Quarantined invalid legacy ${result.kind} registry ${file}${quarantine}.`; + } + return ""; +} + +export async function maybeRepairSandboxRegistryFiles(prompter: DoctorPrompter): Promise { + const legacyFiles = (await inspectLegacySandboxRegistryFiles()).filter((file) => file.exists); + if (legacyFiles.length === 0) { + return; + } + + if (!prompter.shouldRepair) { + note( + [ + "Legacy sandbox registry files detected.", + ...legacyFiles.map(formatLegacyRegistryInspectionLine), + `Run ${formatCliCommand("openclaw doctor --fix")} to migrate them to sharded registry files.`, + ].join("\n"), + "Sandbox", + ); + return; + } + + const results = (await migrateLegacySandboxRegistryFiles()) + .filter((result) => result.status !== "missing") + .map(formatLegacyRegistryMigrationLine) + .filter((line) => line.length > 0); + if (results.length > 0) { + note(results.join("\n"), "Doctor changes"); + } +} + export function noteSandboxScopeWarnings(cfg: OpenClawConfig) { const globalSandbox = cfg.agents?.defaults?.sandbox; const agents = Array.isArray(cfg.agents?.list) ? cfg.agents.list : []; diff --git a/src/commands/doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts b/src/commands/doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts index 681188fe269..9b514e007e5 100644 --- a/src/commands/doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts +++ b/src/commands/doctor-sandbox.warns-sandbox-enabled-without-docker.test.ts @@ -6,6 +6,8 @@ import type { DoctorRepairMode } from "./doctor-repair-mode.js"; const runExec = vi.fn(); const note = vi.fn(); +const inspectLegacySandboxRegistryFiles = vi.fn(); +const migrateLegacySandboxRegistryFiles = vi.fn(); vi.mock("../process/exec.js", () => ({ runExec, @@ -19,11 +21,17 @@ vi.mock("../agents/sandbox.js", () => ({ resolveSandboxScope: vi.fn(() => "shared"), })); +vi.mock("../agents/sandbox/registry.js", () => ({ + inspectLegacySandboxRegistryFiles, + migrateLegacySandboxRegistryFiles, +})); + vi.mock("../terminal/note.js", () => ({ note, })); -const { maybeRepairSandboxImages } = await import("./doctor-sandbox.js"); +const { maybeRepairSandboxImages, maybeRepairSandboxRegistryFiles } = + await import("./doctor-sandbox.js"); describe("maybeRepairSandboxImages", () => { const mockRuntime: RuntimeEnv = { @@ -45,6 +53,8 @@ describe("maybeRepairSandboxImages", () => { beforeEach(() => { vi.clearAllMocks(); + inspectLegacySandboxRegistryFiles.mockResolvedValue([]); + migrateLegacySandboxRegistryFiles.mockResolvedValue([]); }); function createSandboxConfig(mode: "off" | "all" | "non-main"): OpenClawConfig { @@ -114,3 +124,66 @@ describe("maybeRepairSandboxImages", () => { expect(dockerUnavailableWarning).toBeUndefined(); }); }); + +describe("maybeRepairSandboxRegistryFiles", () => { + const mockPrompter = { + shouldRepair: false, + } as DoctorPrompter; + + beforeEach(() => { + vi.clearAllMocks(); + inspectLegacySandboxRegistryFiles.mockResolvedValue([]); + migrateLegacySandboxRegistryFiles.mockResolvedValue([]); + }); + + it("warns about legacy registry files without migrating outside doctor --fix", async () => { + inspectLegacySandboxRegistryFiles.mockResolvedValue([ + { + kind: "containers", + registryPath: "/tmp/openclaw/sandbox/containers.json", + shardedDir: "/tmp/openclaw/sandbox/containers", + exists: true, + valid: true, + entries: 2, + }, + ]); + + await maybeRepairSandboxRegistryFiles(mockPrompter); + + expect(migrateLegacySandboxRegistryFiles).not.toHaveBeenCalled(); + expect(note).toHaveBeenCalledWith(expect.stringContaining("openclaw doctor --fix"), "Sandbox"); + }); + + it("migrates legacy registry files during doctor --fix", async () => { + inspectLegacySandboxRegistryFiles.mockResolvedValue([ + { + kind: "containers", + registryPath: "/tmp/openclaw/sandbox/containers.json", + shardedDir: "/tmp/openclaw/sandbox/containers", + exists: true, + valid: true, + entries: 2, + }, + ]); + migrateLegacySandboxRegistryFiles.mockResolvedValue([ + { + kind: "containers", + registryPath: "/tmp/openclaw/sandbox/containers.json", + shardedDir: "/tmp/openclaw/sandbox/containers", + status: "migrated", + entries: 2, + }, + ]); + + await maybeRepairSandboxRegistryFiles({ + ...mockPrompter, + shouldRepair: true, + } as DoctorPrompter); + + expect(migrateLegacySandboxRegistryFiles).toHaveBeenCalledTimes(1); + expect(note).toHaveBeenCalledWith( + expect.stringContaining("Migrated containers"), + "Doctor changes", + ); + }); +}); diff --git a/src/commands/doctor.fast-path-mocks.ts b/src/commands/doctor.fast-path-mocks.ts index 2b363a6dbe3..c7970586188 100644 --- a/src/commands/doctor.fast-path-mocks.ts +++ b/src/commands/doctor.fast-path-mocks.ts @@ -41,6 +41,7 @@ vi.mock("./doctor-platform-notes.js", () => ({ vi.mock("./doctor-sandbox.js", () => ({ maybeRepairSandboxImages: vi.fn(async (cfg: unknown) => cfg), + maybeRepairSandboxRegistryFiles: vi.fn().mockResolvedValue(undefined), noteSandboxScopeWarnings: vi.fn(), })); diff --git a/src/flows/doctor-health-contributions.ts b/src/flows/doctor-health-contributions.ts index 9206c18778a..9ef4ecf9424 100644 --- a/src/flows/doctor-health-contributions.ts +++ b/src/flows/doctor-health-contributions.ts @@ -333,8 +333,9 @@ async function runLegacyCronHealth(ctx: DoctorHealthFlowContext): Promise } async function runSandboxHealth(ctx: DoctorHealthFlowContext): Promise { - const { maybeRepairSandboxImages, noteSandboxScopeWarnings } = + const { maybeRepairSandboxImages, maybeRepairSandboxRegistryFiles, noteSandboxScopeWarnings } = await import("../commands/doctor-sandbox.js"); + await maybeRepairSandboxRegistryFiles(ctx.prompter); ctx.cfg = await maybeRepairSandboxImages(ctx.cfg, ctx.runtime, ctx.prompter); noteSandboxScopeWarnings(ctx.cfg); }