fix: serialize sandbox registry writes

2026-02-19 18:39:20 -05:00 · 2026-02-18 04:44:23 +01:00
parent 8278903f0a
commit cc29be8c9b
3 changed files with 346 additions and 58 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,14 @@ Docs: https://docs.openclaw.ai

 ### Changes

+### Fixes
+
+- Sandbox/Registry: serialize container and browser registry writes with shared file locks and atomic replacement to prevent lost updates and delete rollback races from desyncing `sandbox list`, `prune`, and `recreate --all`. Thanks @kexinoh.
+
+## 2026.2.17
+
+### Changes
+
 - Agents/Anthropic: add opt-in 1M context beta header support for Opus/Sonnet via model `params.context1m: true` (maps to `anthropic-beta: context-1m-2025-08-07`).
 - Agents/Models: support Anthropic Sonnet 4.6 (`anthropic/claude-sonnet-4-6`) across aliases/defaults with forward-compat fallback when upstream catalogs still only expose Sonnet 4.5.
 - Commands/Subagents: add `/subagents spawn` for deterministic subagent activation from chat commands. (#18218) Thanks @JoshuaLelon.
--- a/src/agents/sandbox/registry.test.ts
+++ b/src/agents/sandbox/registry.test.ts
@@ -0,0 +1,219 @@
+import { mkdtempSync } from "node:fs";
+import fs from "node:fs/promises";
+import { tmpdir } from "node:os";
+import path from "node:path";
+import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+const TEST_STATE_DIR = mkdtempSync(path.join(tmpdir(), "openclaw-sandbox-registry-"));
+const SANDBOX_REGISTRY_PATH = path.join(TEST_STATE_DIR, "containers.json");
+const SANDBOX_BROWSER_REGISTRY_PATH = path.join(TEST_STATE_DIR, "browsers.json");
+
+vi.mock("./constants.js", () => ({
+  SANDBOX_STATE_DIR: TEST_STATE_DIR,
+  SANDBOX_REGISTRY_PATH,
+  SANDBOX_BROWSER_REGISTRY_PATH,
+}));
+
+import type { SandboxBrowserRegistryEntry, SandboxRegistryEntry } from "./registry.js";
+import {
+  readBrowserRegistry,
+  readRegistry,
+  removeBrowserRegistryEntry,
+  removeRegistryEntry,
+  updateBrowserRegistry,
+  updateRegistry,
+} from "./registry.js";
+
+type WriteDelayConfig = {
+  containerName: string;
+  browserName: string;
+  containerDelayMs: number;
+  browserDelayMs: number;
+};
+
+let writeDelayConfig: WriteDelayConfig = {
+  containerName: "container-a",
+  browserName: "browser-a",
+  containerDelayMs: 0,
+  browserDelayMs: 0,
+};
+
+const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
+const realFsWriteFile = fs.writeFile;
+
+function writeText(content: Parameters<typeof fs.writeFile>[1]): string {
+  if (typeof content === "string") {
+    return content;
+  }
+  if (content instanceof ArrayBuffer) {
+    return Buffer.from(content).toString("utf-8");
+  }
+  if (ArrayBuffer.isView(content)) {
+    return Buffer.from(content.buffer, content.byteOffset, content.byteLength).toString("utf-8");
+  }
+  return "";
+}
+
+beforeEach(() => {
+  writeDelayConfig = {
+    containerName: "container-a",
+    browserName: "browser-a",
+    containerDelayMs: 0,
+    browserDelayMs: 0,
+  };
+  vi.spyOn(fs, "writeFile").mockImplementation(async (...args) => {
+    const [target, content] = args;
+    if (typeof target !== "string") {
+      return realFsWriteFile(...args);
+    }
+
+    const payload = writeText(content);
+    if (
+      target.includes("containers.json") &&
+      payload.includes(`"containerName":"${writeDelayConfig.containerName}"`) &&
+      writeDelayConfig.containerDelayMs > 0
+    ) {
+      await delay(writeDelayConfig.containerDelayMs);
+    }
+
+    if (
+      target.includes("browsers.json") &&
+      payload.includes(`"containerName":"${writeDelayConfig.browserName}"`) &&
+      writeDelayConfig.browserDelayMs > 0
+    ) {
+      await delay(writeDelayConfig.browserDelayMs);
+    }
+    return realFsWriteFile(...args);
+  });
+});
+
+afterEach(async () => {
+  vi.restoreAllMocks();
+  await fs.rm(TEST_STATE_DIR, { recursive: true, force: true });
+  await fs.mkdir(TEST_STATE_DIR, { recursive: true });
+});
+
+afterAll(async () => {
+  await fs.rm(TEST_STATE_DIR, { recursive: true, force: true });
+});
+
+function browserEntry(
+  overrides: Partial<SandboxBrowserRegistryEntry> = {},
+): SandboxBrowserRegistryEntry {
+  return {
+    containerName: "browser-a",
+    sessionKey: "agent:main",
+    createdAtMs: 1,
+    lastUsedAtMs: 1,
+    image: "openclaw-browser:test",
+    cdpPort: 9222,
+    ...overrides,
+  };
+}
+
+function containerEntry(overrides: Partial<SandboxRegistryEntry> = {}): SandboxRegistryEntry {
+  return {
+    containerName: "container-a",
+    sessionKey: "agent:main",
+    createdAtMs: 1,
+    lastUsedAtMs: 1,
+    image: "openclaw-sandbox:test",
+    ...overrides,
+  };
+}
+
+async function seedContainerRegistry(entries: SandboxRegistryEntry[]) {
+  await fs.writeFile(SANDBOX_REGISTRY_PATH, `${JSON.stringify({ entries }, null, 2)}\n`, "utf-8");
+}
+
+async function seedBrowserRegistry(entries: SandboxBrowserRegistryEntry[]) {
+  await fs.writeFile(
+    SANDBOX_BROWSER_REGISTRY_PATH,
+    `${JSON.stringify({ entries }, null, 2)}\n`,
+    "utf-8",
+  );
+}
+
+describe("registry race safety", () => {
+  it("keeps both container updates under concurrent writes", async () => {
+    writeDelayConfig = {
+      containerName: "container-a",
+      browserName: "browser-a",
+      containerDelayMs: 80,
+      browserDelayMs: 0,
+    };
+
+    await Promise.all([
+      updateRegistry(containerEntry({ containerName: "container-a" })),
+      updateRegistry(containerEntry({ containerName: "container-b" })),
+    ]);
+
+    const registry = await readRegistry();
+    expect(registry.entries).toHaveLength(2);
+    expect(
+      registry.entries
+        .map((entry) => entry.containerName)
+        .slice()
+        .toSorted(),
+    ).toEqual(["container-a", "container-b"]);
+  });
+
+  it("prevents concurrent container remove/update from resurrecting deleted entries", async () => {
+    await seedContainerRegistry([containerEntry({ containerName: "container-x" })]);
+    writeDelayConfig = {
+      containerName: "container-x",
+      browserName: "browser-a",
+      containerDelayMs: 80,
+      browserDelayMs: 0,
+    };
+
+    await Promise.all([
+      removeRegistryEntry("container-x"),
+      updateRegistry(containerEntry({ containerName: "container-x", configHash: "updated" })),
+    ]);
+
+    const registry = await readRegistry();
+    expect(registry.entries).toHaveLength(0);
+  });
+
+  it("keeps both browser updates under concurrent writes", async () => {
+    writeDelayConfig = {
+      containerName: "container-a",
+      browserName: "browser-a",
+      containerDelayMs: 0,
+      browserDelayMs: 80,
+    };
+
+    await Promise.all([
+      updateBrowserRegistry(browserEntry({ containerName: "browser-a" })),
+      updateBrowserRegistry(browserEntry({ containerName: "browser-b", cdpPort: 9223 })),
+    ]);
+
+    const registry = await readBrowserRegistry();
+    expect(registry.entries).toHaveLength(2);
+    expect(
+      registry.entries
+        .map((entry) => entry.containerName)
+        .slice()
+        .toSorted(),
+    ).toEqual(["browser-a", "browser-b"]);
+  });
+
+  it("prevents concurrent browser remove/update from resurrecting deleted entries", async () => {
+    await seedBrowserRegistry([browserEntry({ containerName: "browser-x" })]);
+    writeDelayConfig = {
+      containerName: "container-a",
+      browserName: "browser-x",
+      containerDelayMs: 0,
+      browserDelayMs: 80,
+    };
+
+    await Promise.all([
+      removeBrowserRegistryEntry("browser-x"),
+      updateBrowserRegistry(browserEntry({ containerName: "browser-x", configHash: "updated" })),
+    ]);
+
+    const registry = await readBrowserRegistry();
+    expect(registry.entries).toHaveLength(0);
+  });
+});
--- a/src/agents/sandbox/registry.ts
+++ b/src/agents/sandbox/registry.ts
@@ -1,4 +1,7 @@
+import crypto from "node:crypto";
 import fs from "node:fs/promises";
+import path from "node:path";
+import { acquireSessionWriteLock } from "../session-write-lock.js";
 import {
  SANDBOX_BROWSER_REGISTRY_PATH,
  SANDBOX_REGISTRY_PATH,
@@ -33,86 +36,144 @@ type SandboxBrowserRegistry = {
  entries: SandboxBrowserRegistryEntry[];
 };

-export async function readRegistry(): Promise<SandboxRegistry> {
+type RegistryReadMode = "strict" | "fallback";
+
+async function withRegistryLock<T>(registryPath: string, fn: () => Promise<T>): Promise<T> {
+  const lock = await acquireSessionWriteLock({ sessionFile: registryPath });
  try {
-    const raw = await fs.readFile(SANDBOX_REGISTRY_PATH, "utf-8");
-    const parsed = JSON.parse(raw) as SandboxRegistry;
-    if (parsed && Array.isArray(parsed.entries)) {
-      return parsed;
-    }
-  } catch {
-    // ignore
+    return await fn();
+  } finally {
+    await lock.release();
  }
-  return { entries: [] };
+}
+
+async function readRegistryFromFile<T>(
+  registryPath: string,
+  mode: RegistryReadMode,
+): Promise<{ entries: T[] }> {
+  try {
+    const raw = await fs.readFile(registryPath, "utf-8");
+    const parsed = JSON.parse(raw) as { entries?: unknown };
+    if (parsed && Array.isArray(parsed.entries)) {
+      return { entries: parsed.entries as T[] };
+    }
+    if (mode === "fallback") {
+      return { entries: [] };
+    }
+    throw new Error(`Invalid sandbox registry format: ${registryPath}`);
+  } catch (error) {
+    const code = (error as { code?: string } | null)?.code;
+    if (code === "ENOENT") {
+      return { entries: [] };
+    }
+    if (mode === "fallback") {
+      return { entries: [] };
+    }
+    if (error instanceof Error) {
+      throw error;
+    }
+    throw new Error(`Failed to read sandbox registry file: ${registryPath}`, { cause: error });
+  }
+}
+
+async function writeRegistryFile<T>(
+  registryPath: string,
+  registry: { entries: T[] },
+): Promise<void> {
+  await fs.mkdir(SANDBOX_STATE_DIR, { recursive: true });
+  const payload = `${JSON.stringify(registry, null, 2)}\n`;
+  const registryDir = path.dirname(registryPath);
+  const tempPath = path.join(
+    registryDir,
+    `${path.basename(registryPath)}.${crypto.randomUUID()}.tmp`,
+  );
+  await fs.writeFile(tempPath, payload, "utf-8");
+  try {
+    await fs.rename(tempPath, registryPath);
+  } catch (error) {
+    await fs.rm(tempPath, { force: true });
+    throw error;
+  }
+}
+
+export async function readRegistry(): Promise<SandboxRegistry> {
+  return await readRegistryFromFile<SandboxRegistryEntry>(SANDBOX_REGISTRY_PATH, "fallback");
+}
+
+async function readRegistryForWrite(): Promise<SandboxRegistry> {
+  return await readRegistryFromFile<SandboxRegistryEntry>(SANDBOX_REGISTRY_PATH, "strict");
 }

 async function writeRegistry(registry: SandboxRegistry) {
-  await fs.mkdir(SANDBOX_STATE_DIR, { recursive: true });
-  await fs.writeFile(SANDBOX_REGISTRY_PATH, `${JSON.stringify(registry, null, 2)}\n`, "utf-8");
+  await writeRegistryFile<SandboxRegistryEntry>(SANDBOX_REGISTRY_PATH, registry);
 }

 export async function updateRegistry(entry: SandboxRegistryEntry) {
-  const registry = await readRegistry();
-  const existing = registry.entries.find((item) => item.containerName === entry.containerName);
-  const next = registry.entries.filter((item) => item.containerName !== entry.containerName);
-  next.push({
-    ...entry,
-    createdAtMs: existing?.createdAtMs ?? entry.createdAtMs,
-    image: existing?.image ?? entry.image,
-    configHash: entry.configHash ?? existing?.configHash,
+  await withRegistryLock(SANDBOX_REGISTRY_PATH, async () => {
+    const registry = await readRegistryForWrite();
+    const existing = registry.entries.find((item) => item.containerName === entry.containerName);
+    const next = registry.entries.filter((item) => item.containerName !== entry.containerName);
+    next.push({
+      ...entry,
+      createdAtMs: existing?.createdAtMs ?? entry.createdAtMs,
+      image: existing?.image ?? entry.image,
+      configHash: entry.configHash ?? existing?.configHash,
+    });
+    await writeRegistry({ entries: next });
  });
-  await writeRegistry({ entries: next });
 }

 export async function removeRegistryEntry(containerName: string) {
-  const registry = await readRegistry();
-  const next = registry.entries.filter((item) => item.containerName !== containerName);
-  if (next.length === registry.entries.length) {
-    return;
-  }
-  await writeRegistry({ entries: next });
+  await withRegistryLock(SANDBOX_REGISTRY_PATH, async () => {
+    const registry = await readRegistryForWrite();
+    const next = registry.entries.filter((item) => item.containerName !== containerName);
+    if (next.length === registry.entries.length) {
+      return;
+    }
+    await writeRegistry({ entries: next });
+  });
 }

 export async function readBrowserRegistry(): Promise<SandboxBrowserRegistry> {
-  try {
-    const raw = await fs.readFile(SANDBOX_BROWSER_REGISTRY_PATH, "utf-8");
-    const parsed = JSON.parse(raw) as SandboxBrowserRegistry;
-    if (parsed && Array.isArray(parsed.entries)) {
-      return parsed;
-    }
-  } catch {
-    // ignore
-  }
-  return { entries: [] };
-}
-
-async function writeBrowserRegistry(registry: SandboxBrowserRegistry) {
-  await fs.mkdir(SANDBOX_STATE_DIR, { recursive: true });
-  await fs.writeFile(
+  return await readRegistryFromFile<SandboxBrowserRegistryEntry>(
    SANDBOX_BROWSER_REGISTRY_PATH,
-    `${JSON.stringify(registry, null, 2)}\n`,
-    "utf-8",
+    "fallback",
  );
 }

+async function readBrowserRegistryForWrite(): Promise<SandboxBrowserRegistry> {
+  return await readRegistryFromFile<SandboxBrowserRegistryEntry>(
+    SANDBOX_BROWSER_REGISTRY_PATH,
+    "strict",
+  );
+}
+
+async function writeBrowserRegistry(registry: SandboxBrowserRegistry) {
+  await writeRegistryFile<SandboxBrowserRegistryEntry>(SANDBOX_BROWSER_REGISTRY_PATH, registry);
+}
+
 export async function updateBrowserRegistry(entry: SandboxBrowserRegistryEntry) {
-  const registry = await readBrowserRegistry();
-  const existing = registry.entries.find((item) => item.containerName === entry.containerName);
-  const next = registry.entries.filter((item) => item.containerName !== entry.containerName);
-  next.push({
-    ...entry,
-    createdAtMs: existing?.createdAtMs ?? entry.createdAtMs,
-    image: existing?.image ?? entry.image,
-    configHash: entry.configHash ?? existing?.configHash,
+  await withRegistryLock(SANDBOX_BROWSER_REGISTRY_PATH, async () => {
+    const registry = await readBrowserRegistryForWrite();
+    const existing = registry.entries.find((item) => item.containerName === entry.containerName);
+    const next = registry.entries.filter((item) => item.containerName !== entry.containerName);
+    next.push({
+      ...entry,
+      createdAtMs: existing?.createdAtMs ?? entry.createdAtMs,
+      image: existing?.image ?? entry.image,
+      configHash: entry.configHash ?? existing?.configHash,
+    });
+    await writeBrowserRegistry({ entries: next });
  });
-  await writeBrowserRegistry({ entries: next });
 }

 export async function removeBrowserRegistryEntry(containerName: string) {
-  const registry = await readBrowserRegistry();
-  const next = registry.entries.filter((item) => item.containerName !== containerName);
-  if (next.length === registry.entries.length) {
-    return;
-  }
-  await writeBrowserRegistry({ entries: next });
+  await withRegistryLock(SANDBOX_BROWSER_REGISTRY_PATH, async () => {
+    const registry = await readBrowserRegistryForWrite();
+    const next = registry.entries.filter((item) => item.containerName !== containerName);
+    if (next.length === registry.entries.length) {
+      return;
+    }
+    await writeBrowserRegistry({ entries: next });
+  });
 }