From 3aec297be03f77cc474059a2fbe92dea7da53109 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 8 Aug 2026 22:03:58 -0700 Subject: [PATCH] refactor(sandbox): simplify legacy registry migration (#120895) --- .../doctor-sandbox-legacy-registry.test.ts | 85 ++----- .../doctor-sandbox-legacy-registry.ts | 212 ++++++++---------- src/commands/doctor-sandbox.ts | 16 +- ...rns-sandbox-enabled-without-docker.test.ts | 18 +- 4 files changed, 124 insertions(+), 207 deletions(-) diff --git a/src/commands/doctor-sandbox-legacy-registry.test.ts b/src/commands/doctor-sandbox-legacy-registry.test.ts index d5faed532e5b..e94a926b4a48 100644 --- a/src/commands/doctor-sandbox-legacy-registry.test.ts +++ b/src/commands/doctor-sandbox-legacy-registry.test.ts @@ -36,6 +36,7 @@ vi.mock("../agents/sandbox/constants.js", () => ({ SANDBOX_BROWSERS_DIR, })); +import { writeJsonFile } from "../../test/helpers/temp-repo.js"; import { hashTextSha256 } from "../agents/sandbox/hash.js"; import { readBrowserRegistry, @@ -98,37 +99,13 @@ function containerEntry(overrides: Partial = {}): SandboxR }; } -async function seedContainerRegistry(entries: SandboxRegistryEntry[]) { - await fs.writeFile(SANDBOX_REGISTRY_PATH, `${JSON.stringify({ entries }, null, 2)}\n`, "utf-8"); +function seedRegistry(registryPath: string, entries: readonly unknown[]) { + writeJsonFile(registryPath, { entries }); } -async function seedBrowserRegistry(entries: SandboxBrowserRegistryEntry[]) { - await fs.writeFile( - SANDBOX_BROWSER_REGISTRY_PATH, - `${JSON.stringify({ entries }, null, 2)}\n`, - "utf-8", - ); -} - -async function seedShardedContainerRegistry(entries: SandboxRegistryEntry[]) { - await fs.mkdir(SANDBOX_CONTAINERS_DIR, { recursive: true }); +function seedShardedRegistry(dir: string, entries: readonly { containerName: string }[]) { for (const entry of entries) { - await fs.writeFile( - path.join(SANDBOX_CONTAINERS_DIR, `${hashTextSha256(entry.containerName)}.json`), - `${JSON.stringify(entry, null, 2)}\n`, - "utf-8", - ); - } -} - -async function seedShardedBrowserRegistry(entries: SandboxBrowserRegistryEntry[]) { - await fs.mkdir(SANDBOX_BROWSERS_DIR, { recursive: true }); - for (const entry of entries) { - await fs.writeFile( - path.join(SANDBOX_BROWSERS_DIR, `${hashTextSha256(entry.containerName)}.json`), - `${JSON.stringify(entry, null, 2)}\n`, - "utf-8", - ); + writeJsonFile(path.join(dir, `${hashTextSha256(entry.containerName)}.json`), entry); } } @@ -162,29 +139,8 @@ function requireMigrationResult( } describe("legacy sandbox registry migration", () => { - it("normalizes legacy registry entries after explicit migration", async () => { - await seedContainerRegistry([ - { - containerName: "legacy-container", - sessionKey: "agent:main", - createdAtMs: 1, - lastUsedAtMs: 1, - image: "openclaw-sandbox:test", - }, - ]); - - await migrateLegacySandboxRegistryFiles(); - const registry = await readRegistry(); - expect(registry.entries).toHaveLength(1); - const [entry] = registry.entries; - expect(entry?.containerName).toBe("legacy-container"); - expect(entry?.backendId).toBe("docker"); - expect(entry?.runtimeLabel).toBe("legacy-container"); - expect(entry?.configLabelKind).toBe("Image"); - }); - it("migrates legacy monolithic container and browser registry files after explicit repair", async () => { - await seedContainerRegistry([ + seedRegistry(SANDBOX_REGISTRY_PATH, [ containerEntry({ containerName: "legacy-container", sessionKey: "agent:legacy", @@ -192,7 +148,7 @@ describe("legacy sandbox registry migration", () => { configHash: "legacy-container-hash", }), ]); - await seedBrowserRegistry([ + seedRegistry(SANDBOX_BROWSER_REGISTRY_PATH, [ browserEntry({ containerName: "legacy-browser", sessionKey: "agent:legacy", @@ -205,12 +161,10 @@ describe("legacy sandbox registry migration", () => { await seedStaleLock(`${SANDBOX_BROWSER_REGISTRY_PATH}.lock`); const migrationResults = await migrateLegacySandboxRegistryFiles(); - const containerMigration = requireMigrationResult(migrationResults, "containers"); - const browserMigration = requireMigrationResult(migrationResults, "browsers"); - expect(containerMigration.status).toBe("migrated"); - expect(containerMigration.entries).toBe(1); - expect(browserMigration.status).toBe("migrated"); - expect(browserMigration.entries).toBe(1); + expect(migrationResults).toEqual([ + { kind: "containers", status: "migrated", entries: 1 }, + { kind: "browsers", status: "migrated", entries: 1 }, + ]); await expectPathMissing(SANDBOX_REGISTRY_PATH); await expectPathMissing(SANDBOX_BROWSER_REGISTRY_PATH); @@ -222,6 +176,7 @@ describe("legacy sandbox registry migration", () => { expect(container?.containerName).toBe("legacy-container"); expect(container?.backendId).toBe("docker"); expect(container?.runtimeLabel).toBe("legacy-container"); + expect(container?.configLabelKind).toBe("Image"); expect(container?.sessionKey).toBe("agent:legacy"); expect(container?.configHash).toBe("legacy-container-hash"); const browserRegistry = await readBrowserRegistry(); @@ -235,7 +190,7 @@ describe("legacy sandbox registry migration", () => { }); it("migrates legacy sharded container and browser registry files after explicit repair", async () => { - await seedShardedContainerRegistry([ + seedShardedRegistry(SANDBOX_CONTAINERS_DIR, [ containerEntry({ containerName: "legacy-container", sessionKey: "agent:legacy", @@ -243,7 +198,7 @@ describe("legacy sandbox registry migration", () => { configHash: "legacy-container-hash", }), ]); - await seedShardedBrowserRegistry([ + seedShardedRegistry(SANDBOX_BROWSERS_DIR, [ browserEntry({ containerName: "legacy-browser", sessionKey: "agent:legacy", @@ -270,7 +225,7 @@ describe("legacy sandbox registry migration", () => { lastUsedAtMs: 10, }), ); - await seedContainerRegistry([ + seedRegistry(SANDBOX_REGISTRY_PATH, [ containerEntry({ containerName: "container-a", sessionKey: "legacy-session", @@ -286,14 +241,14 @@ describe("legacy sandbox registry migration", () => { }); it("prefers newer sharded entries over stale monolithic entries during legacy migration", async () => { - await seedContainerRegistry([ + seedRegistry(SANDBOX_REGISTRY_PATH, [ containerEntry({ containerName: "container-a", sessionKey: "legacy-session", lastUsedAtMs: 1, }), ]); - await seedShardedContainerRegistry([ + seedShardedRegistry(SANDBOX_CONTAINERS_DIR, [ containerEntry({ containerName: "container-a", sessionKey: "sharded-session", @@ -333,12 +288,10 @@ describe("legacy sandbox registry migration", () => { }); it("quarantines malformed sharded registry directories during migration", async () => { - await fs.mkdir(SANDBOX_CONTAINERS_DIR, { recursive: true }); - await fs.mkdir(SANDBOX_BROWSERS_DIR, { recursive: true }); - await seedShardedContainerRegistry([ + seedShardedRegistry(SANDBOX_CONTAINERS_DIR, [ containerEntry({ containerName: "valid-container", sessionKey: "agent:valid" }), ]); - await seedShardedBrowserRegistry([ + seedShardedRegistry(SANDBOX_BROWSERS_DIR, [ browserEntry({ containerName: "valid-browser", sessionKey: "agent:valid" }), ]); await fs.writeFile(path.join(SANDBOX_CONTAINERS_DIR, "bad.json"), "{bad json", "utf-8"); diff --git a/src/commands/doctor-sandbox-legacy-registry.ts b/src/commands/doctor-sandbox-legacy-registry.ts index 5de1ca9dc116..d43f04b5241f 100644 --- a/src/commands/doctor-sandbox-legacy-registry.ts +++ b/src/commands/doctor-sandbox-legacy-registry.ts @@ -12,47 +12,9 @@ import { insertSandboxBrowserRegistryEntryIfMissing, insertSandboxRegistryEntryIfMissing, } from "../agents/sandbox/registry.js"; -import { acquireFileLock } from "../infra/file-lock.js"; +import { withFileLock } from "../infra/file-lock.js"; import { safeParseJsonWithSchema } from "../utils/zod-parse.js"; -type RegistryEntry = { - containerName: string; -}; - -type RegistryEntryPayload = RegistryEntry & Record; - -type RegistryFile = { - entries: RegistryEntryPayload[]; -}; - -type ShardedRegistryRead = { - entries: T[]; - validFiles: string[]; - invalidFiles: string[]; -}; - -type LegacyRegistryKind = "containers" | "browsers"; - -type LegacyRegistryTarget = { - kind: LegacyRegistryKind; - registryPath: string; - shardedDir: string; -}; - -export type LegacySandboxRegistryInspection = LegacyRegistryTarget & { - exists: boolean; - valid: boolean; - entries: number; - source: "monolithic" | "sharded"; -}; - -export type LegacySandboxRegistryMigrationResult = LegacyRegistryTarget & { - status: "missing" | "migrated" | "removed-empty" | "quarantined-invalid"; - entries: number; - source?: "monolithic" | "sharded"; - quarantinePath?: string; -}; - const RegistryEntrySchema = z .object({ containerName: z.string(), @@ -63,23 +25,46 @@ const RegistryFileSchema = z.object({ entries: z.array(RegistryEntrySchema), }); -async function withRegistryLock(registryPath: string, fn: () => Promise): Promise { - const lock = await acquireFileLock(registryPath, { - stale: 30_000, - retries: { retries: 59, factor: 1, minTimeout: 1_000, maxTimeout: 1_000 }, - }); - try { - return await fn(); - } finally { - await lock.release(); - } -} +type RegistryEntryPayload = z.infer; +type RegistryFile = z.infer; + +type ShardedRegistryRead = { + entries: RegistryEntryPayload[]; + invalidFiles: string[]; +}; + +type LegacyRegistryKind = "containers" | "browsers"; + +type LegacyRegistryTarget = { + kind: LegacyRegistryKind; + registryPath: string; + shardedDir: string; +}; + +export type LegacySandboxRegistryInspection = { + kind: LegacyRegistryKind; + path: string; + exists: boolean; + valid: boolean; + entries: number; + source: "monolithic" | "sharded"; +}; + +export type LegacySandboxRegistryMigrationResult = { kind: LegacyRegistryKind } & ( + | { status: "missing" } + | { status: "migrated"; entries: number } + | { status: "removed-empty" } + | { + status: "quarantined-invalid"; + path: string; + quarantinePath: string; + } +); async function readLegacyRegistryFile(registryPath: string): Promise { try { const raw = await fs.readFile(registryPath, "utf-8"); - const parsed = safeParseJsonWithSchema(RegistryFileSchema, raw) as RegistryFile | null; - return parsed; + return safeParseJsonWithSchema(RegistryFileSchema, raw); } catch (error) { const code = (error as { code?: string } | null)?.code; if (code === "ENOENT") { @@ -92,22 +77,19 @@ async function readLegacyRegistryFile(registryPath: string): Promise( - dir: string, -): Promise> { +async function readShardedEntriesDetailed(dir: string): Promise { let files: string[]; try { files = await fs.readdir(dir); } catch (error) { const code = (error as { code?: string } | null)?.code; if (code === "ENOENT") { - return { entries: [], validFiles: [], invalidFiles: [] }; + return { entries: [], invalidFiles: [] }; } throw error; } const invalidFiles: string[] = []; - const validFiles: string[] = []; const entries = await Promise.all( files .filter((name) => name.endsWith(".json")) @@ -116,11 +98,9 @@ async function readShardedEntriesDetailed( const filePath = path.join(dir, name); try { const raw = await fs.readFile(filePath, "utf-8"); - const entry = safeParseJsonWithSchema(RegistryEntrySchema, raw) as T | null; + const entry = safeParseJsonWithSchema(RegistryEntrySchema, raw); if (!entry) { invalidFiles.push(filePath); - } else { - validFiles.push(filePath); } return entry; } catch { @@ -129,17 +109,10 @@ async function readShardedEntriesDetailed( } }), ); - const validEntries: T[] = []; - for (const entry of entries) { - if (entry) { - validEntries.push(entry); - } - } return { - entries: validEntries.toSorted((left, right) => - left.containerName.localeCompare(right.containerName), - ), - validFiles: validFiles.toSorted(), + entries: entries + .filter((entry) => entry !== null) + .toSorted((left, right) => left.containerName.localeCompare(right.containerName)), invalidFiles: invalidFiles.toSorted(), }; } @@ -174,10 +147,6 @@ async function quarantineInvalidShards( return quarantineDir; } -async function removeFiles(files: readonly string[]): Promise { - await Promise.all(files.map((file) => fs.rm(file, { force: true }))); -} - function writeLegacyEntryIfMissing(kind: LegacyRegistryKind, entry: RegistryEntryPayload): void { if (kind === "containers") { insertSandboxRegistryEntryIfMissing({ @@ -210,38 +179,43 @@ async function migrateMonolithicIfNeeded( } catch (error) { const code = (error as { code?: string } | null)?.code; if (code === "ENOENT") { - return { ...target, source: "monolithic", status: "missing", entries: 0 }; + return { kind: target.kind, status: "missing" }; } throw error; } - return await withRegistryLock(registryPath, async () => { - const registry = await readLegacyRegistryFile(registryPath); - if (!registry) { - const quarantinePath = await quarantineLegacyRegistry(registryPath); - return { - ...target, - source: "monolithic", - status: "quarantined-invalid", - entries: 0, - quarantinePath, - }; - } - if (registry.entries.length === 0) { + return await withFileLock( + registryPath, + { + stale: 30_000, + retries: { retries: 59, factor: 1, minTimeout: 1_000, maxTimeout: 1_000 }, + }, + async () => { + const registry = await readLegacyRegistryFile(registryPath); + if (!registry) { + const quarantinePath = await quarantineLegacyRegistry(registryPath); + return { + kind: target.kind, + status: "quarantined-invalid", + path: registryPath, + quarantinePath, + }; + } + if (registry.entries.length === 0) { + await fs.rm(registryPath, { force: true }); + return { kind: target.kind, status: "removed-empty" }; + } + for (const entry of registry.entries) { + writeLegacyEntryIfMissing(target.kind, entry); + } await fs.rm(registryPath, { force: true }); - return { ...target, source: "monolithic", status: "removed-empty", entries: 0 }; - } - for (const entry of registry.entries) { - writeLegacyEntryIfMissing(target.kind, entry); - } - await fs.rm(registryPath, { force: true }); - return { - ...target, - source: "monolithic", - status: "migrated", - entries: registry.entries.length, - }; - }); + return { + kind: target.kind, + status: "migrated", + entries: registry.entries.length, + }; + }, + ); } async function migrateShardedIfNeeded( @@ -258,34 +232,31 @@ async function migrateShardedIfNeeded( } } if (!dirExists) { - return { ...target, source: "sharded", status: "missing", entries: 0 }; + return { kind: target.kind, status: "missing" }; } - const { entries, validFiles, invalidFiles } = - await readShardedEntriesDetailed(target.shardedDir); + const { entries, invalidFiles } = await readShardedEntriesDetailed(target.shardedDir); if (invalidFiles.length > 0) { for (const entry of entries) { writeLegacyEntryIfMissing(target.kind, entry); } - await removeFiles(validFiles); const quarantinePath = await quarantineInvalidShards(target.shardedDir, invalidFiles); await fs.rm(target.shardedDir, { recursive: true, force: true }); return { - ...target, - source: "sharded", + kind: target.kind, status: "quarantined-invalid", - entries: entries.length, + path: target.shardedDir, quarantinePath, }; } if (entries.length === 0) { await fs.rm(target.shardedDir, { recursive: true, force: true }); - return { ...target, source: "sharded", status: "removed-empty", entries: 0 }; + return { kind: target.kind, status: "removed-empty" }; } for (const entry of entries) { writeLegacyEntryIfMissing(target.kind, entry); } await fs.rm(target.shardedDir, { recursive: true, force: true }); - return { ...target, source: "sharded", status: "migrated", entries: entries.length }; + return { kind: target.kind, status: "migrated", entries: entries.length }; } function combineMigrationResults( @@ -299,14 +270,16 @@ function combineMigrationResults( if (sharded.status === "quarantined-invalid") { return sharded; } - const entries = monolithic.entries + sharded.entries; + const entries = + (monolithic.status === "migrated" ? monolithic.entries : 0) + + (sharded.status === "migrated" ? sharded.entries : 0); if (entries > 0) { - return { ...target, status: "migrated", entries }; + return { kind: target.kind, status: "migrated", entries }; } if (monolithic.status === "removed-empty" || sharded.status === "removed-empty") { - return { ...target, status: "removed-empty", entries: 0 }; + return { kind: target.kind, status: "removed-empty" }; } - return { ...target, status: "missing", entries: 0 }; + return { kind: target.kind, status: "missing" }; } function legacyRegistryTargets(): LegacyRegistryTarget[] { @@ -336,7 +309,8 @@ export async function inspectLegacySandboxRegistryFiles(): Promise< const code = (error as { code?: string } | null)?.code; if (code === "ENOENT") { inspections.push({ - ...target, + kind: target.kind, + path: target.registryPath, source: "monolithic", exists: false, valid: true, @@ -350,7 +324,8 @@ export async function inspectLegacySandboxRegistryFiles(): Promise< if (!inspections.some((entry) => entry.kind === target.kind && entry.source === "monolithic")) { const registry = await readLegacyRegistryFile(target.registryPath); inspections.push({ - ...target, + kind: target.kind, + path: target.registryPath, source: "monolithic", exists: true, valid: Boolean(registry), @@ -358,7 +333,7 @@ export async function inspectLegacySandboxRegistryFiles(): Promise< }); } - const sharded = await readShardedEntriesDetailed(target.shardedDir); + const sharded = await readShardedEntriesDetailed(target.shardedDir); let shardedExists = false; try { shardedExists = (await fs.stat(target.shardedDir)).isDirectory(); @@ -369,7 +344,8 @@ export async function inspectLegacySandboxRegistryFiles(): Promise< } } inspections.push({ - ...target, + kind: target.kind, + path: target.shardedDir, source: "sharded", exists: shardedExists, valid: sharded.invalidFiles.length === 0, diff --git a/src/commands/doctor-sandbox.ts b/src/commands/doctor-sandbox.ts index a87a1bf6a161..18b944e865a7 100644 --- a/src/commands/doctor-sandbox.ts +++ b/src/commands/doctor-sandbox.ts @@ -409,12 +409,7 @@ export async function maybeRepairSandboxImages( function formatLegacyRegistryInspectionLine(file: LegacySandboxRegistryInspection): string { const status = file.valid ? `${file.entries} entr${file.entries === 1 ? "y" : "ies"}` : "invalid"; - const sourcePath = legacySandboxRegistryInspectionSourcePath(file); - return `- ${file.kind} ${file.source}: ${shortenHomePath(sourcePath)} (${status})`; -} - -function legacySandboxRegistryInspectionSourcePath(file: LegacySandboxRegistryInspection): string { - return file.source === "sharded" ? file.shardedDir : file.registryPath; + return `- ${file.kind} ${file.source}: ${shortenHomePath(file.path)} (${status})`; } function formatLegacyRegistryMigrationLine(result: LegacySandboxRegistryMigrationResult): string { @@ -425,9 +420,8 @@ function formatLegacyRegistryMigrationLine(result: LegacySandboxRegistryMigratio return `- Removed empty legacy ${result.kind} registry files.`; } if (result.status === "quarantined-invalid") { - const sourcePath = result.source === "sharded" ? result.shardedDir : result.registryPath; - const file = shortenHomePath(sourcePath); - const quarantine = result.quarantinePath ? ` to ${shortenHomePath(result.quarantinePath)}` : ""; + const file = shortenHomePath(result.path); + const quarantine = ` to ${shortenHomePath(result.quarantinePath)}`; return `- Quarantined invalid legacy ${result.kind} registry ${file}${quarantine}.`; } return ""; @@ -447,7 +441,7 @@ export function legacySandboxRegistryInspectionToHealthFinding( severity: "warning", message: `Legacy sandbox registry file detected. ${formatLegacyRegistryInspectionLine(file)}`, - path: legacySandboxRegistryInspectionSourcePath(file), + path: file.path, fixHint: `Run ${formatCliCommand("openclaw doctor --fix")} to migrate valid entries to SQLite.`, }; } @@ -463,7 +457,7 @@ export function legacySandboxRegistryInspectionToRepairEffect( return { kind: "state", action, - target: legacySandboxRegistryInspectionSourcePath(file), + target: file.path, dryRunSafe: false, }; } 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 f0dc4616b83c..8dc208c3315a 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 @@ -302,8 +302,7 @@ describe("maybeRepairSandboxRegistryFiles", () => { inspectLegacySandboxRegistryFiles.mockResolvedValue([ { kind: "containers", - registryPath: "/tmp/openclaw/sandbox/containers.json", - shardedDir: "/tmp/openclaw/sandbox/containers", + path: "/tmp/openclaw/sandbox/containers.json", source: "monolithic", exists: true, valid: true, @@ -328,8 +327,7 @@ describe("maybeRepairSandboxRegistryFiles", () => { inspectLegacySandboxRegistryFiles.mockResolvedValue([ { kind: "containers", - registryPath: "/tmp/openclaw/sandbox/containers.json", - shardedDir: "/tmp/openclaw/sandbox/containers", + path: "/tmp/openclaw/sandbox/containers.json", source: "monolithic", exists: true, valid: true, @@ -339,8 +337,6 @@ describe("maybeRepairSandboxRegistryFiles", () => { migrateLegacySandboxRegistryFiles.mockResolvedValue([ { kind: "containers", - registryPath: "/tmp/openclaw/sandbox/containers.json", - shardedDir: "/tmp/openclaw/sandbox/containers", status: "migrated", entries: 2, }, @@ -361,8 +357,7 @@ describe("maybeRepairSandboxRegistryFiles", () => { it("maps legacy registry files to structured findings and dry-run effects", () => { const monolithicFile = { kind: "containers", - registryPath: "/tmp/openclaw/sandbox/containers.json", - shardedDir: "/tmp/openclaw/sandbox/containers", + path: "/tmp/openclaw/sandbox/containers.json", source: "monolithic", exists: true, valid: true, @@ -370,6 +365,7 @@ describe("maybeRepairSandboxRegistryFiles", () => { } as const; const shardedFile = { ...monolithicFile, + path: "/tmp/openclaw/sandbox/containers", source: "sharded", } as const; @@ -406,8 +402,7 @@ describe("maybeRepairSandboxRegistryFiles", () => { expect( legacySandboxRegistryInspectionToRepairEffect({ kind: "browsers", - registryPath: "/tmp/openclaw/sandbox/browsers.json", - shardedDir: "/tmp/openclaw/sandbox/browsers", + path: "/tmp/openclaw/sandbox/browsers.json", source: "monolithic", exists: true, valid: false, @@ -425,8 +420,7 @@ describe("maybeRepairSandboxRegistryFiles", () => { expect( legacySandboxRegistryInspectionToRepairEffect({ kind: "containers", - registryPath: "/tmp/openclaw/sandbox/containers.json", - shardedDir: "/tmp/openclaw/sandbox/containers", + path: "/tmp/openclaw/sandbox/containers.json", source: "monolithic", exists: true, valid: true,