fix(sandbox): move registry file migration to doctor

2026-05-06 05:10:44 +00:00 · 2026-05-03 12:26:07 +01:00
parent 1cebe32d76
commit 1402997489
6 changed files with 268 additions and 29 deletions
--- 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" }),
+    ]);
  });
 });
--- 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<RegistryFil
 }

 export async function readRegistry(): Promise<SandboxRegistry> {
-  await migrateMonolithicIfNeeded(SANDBOX_REGISTRY_PATH, SANDBOX_CONTAINERS_DIR);
  const entries = await readShardedEntries<SandboxRegistryEntry>(SANDBOX_CONTAINERS_DIR);
  return {
    entries: entries.map((entry) => normalizeSandboxRegistryEntry(entry)),
@@ -197,7 +216,7 @@ async function readShardedEntries<T extends RegistryEntry>(dir: string): Promise
  );
 }

-async function quarantineLegacyRegistry(registryPath: string): Promise<void> {
+async function quarantineLegacyRegistry(registryPath: string): Promise<string> {
  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<void> {
      await fs.rm(registryPath, { force: true });
    }
  });
+  return quarantinePath;
 }

-async function migrateMonolithicIfNeeded(registryPath: string, shardedDir: string): Promise<void> {
+async function migrateMonolithicIfNeeded(
+  target: LegacyRegistryTarget,
+): Promise<LegacySandboxRegistryMigrationResult> {
+  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<SandboxRegistryEntry | null> {
-  await migrateMonolithicIfNeeded(SANDBOX_REGISTRY_PATH, SANDBOX_CONTAINERS_DIR);
  const entry = await readShardedEntry<SandboxRegistryEntry>(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<SandboxRegistryEntry>(
      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<SandboxBrowserRegistry> {
-  await migrateMonolithicIfNeeded(SANDBOX_BROWSER_REGISTRY_PATH, SANDBOX_BROWSERS_DIR);
  return { entries: await readShardedEntries<SandboxBrowserRegistryEntry>(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<SandboxBrowserRegistryEntry>(
      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);
  });
--- 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<void> {
+  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 : [];
--- 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",
+    );
+  });
+});
--- 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(),
 }));

--- a/src/flows/doctor-health-contributions.ts
+++ b/src/flows/doctor-health-contributions.ts
@@ -333,8 +333,9 @@ async function runLegacyCronHealth(ctx: DoctorHealthFlowContext): Promise<void>
 }

 async function runSandboxHealth(ctx: DoctorHealthFlowContext): Promise<void> {
-  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);
 }