From 50eb8faa3f0b091cb871d94b4c1676246dff606f Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sun, 20 Sep 2026 23:01:09 +0800 Subject: [PATCH] fix(sandbox): respect registry owner prune policy (#153752) --- CHANGELOG/Unreleased.md | 1 + src/agents/sandbox/context.ts | 2 +- src/agents/sandbox/prune.test.ts | 152 ++++++++++++------ src/agents/sandbox/prune.ts | 97 +++++++---- src/agents/sandbox/registry.ts | 3 +- .../sandbox/runtime-reservation.test.ts | 23 ++- 6 files changed, 194 insertions(+), 84 deletions(-) diff --git a/CHANGELOG/Unreleased.md b/CHANGELOG/Unreleased.md index 92f80363a8d8..0d8d2bf3ab4d 100644 --- a/CHANGELOG/Unreleased.md +++ b/CHANGELOG/Unreleased.md @@ -3,6 +3,7 @@ ### Fixes - Codex: restore background memory narratives and isolated text completions on agent-scoped local runtimes with administrator-managed hooks, preserving managed hooks and existing native-account/proxy routing while keeping ordinary hooks and model tools isolated. (#151658) +- Sandboxes: honor each registered runtime owner's pruning policy so a stricter agent cannot evict another agent's containers or browser bridges. ### Changes diff --git a/src/agents/sandbox/context.ts b/src/agents/sandbox/context.ts index 74af7427c900..543a7b874f74 100644 --- a/src/agents/sandbox/context.ts +++ b/src/agents/sandbox/context.ts @@ -316,7 +316,7 @@ async function resolveProvisionedSandboxContext( resolved, ); if (cfg.prune.idleHours !== 0 || cfg.prune.maxAgeDays !== 0) { - await (await import("./prune.js")).maybePruneSandboxes(cfg); + await (await import("./prune.js")).maybePruneSandboxes(); } const { diff --git a/src/agents/sandbox/prune.test.ts b/src/agents/sandbox/prune.test.ts index 7447ef155dbc..4a8954553bba 100644 --- a/src/agents/sandbox/prune.test.ts +++ b/src/agents/sandbox/prune.test.ts @@ -1,8 +1,8 @@ // Sandbox prune tests cover runtime removal ordering and registry cleanup // behavior for stale sandbox entries. import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { OpenClawConfig } from "../../config/types.openclaw.js"; import type { SandboxRegistryEntry } from "./registry.js"; -import type { SandboxConfig } from "./types.js"; let maybePruneSandboxes: typeof import("./prune.js").maybePruneSandboxes; let BROWSER_BRIDGES: typeof import("./browser-bridges.js").BROWSER_BRIDGES; @@ -17,6 +17,7 @@ const backendMocks = vi.hoisted(() => ({ })); const registryMocks = vi.hoisted(() => ({ + assertSandboxBrowserRegistryEntryCurrent: vi.fn(), readBrowserRegistry: vi.fn(), readRegistry: vi.fn(), removeBrowserRegistryEntry: vi.fn(), @@ -49,68 +50,49 @@ vi.mock("./docker-backend.js", () => ({ })); vi.mock("./registry.js", () => ({ + assertSandboxBrowserRegistryEntryCurrent: registryMocks.assertSandboxBrowserRegistryEntryCurrent, readBrowserRegistry: registryMocks.readBrowserRegistry, readRegistry: registryMocks.readRegistry, removeBrowserRegistryEntry: registryMocks.removeBrowserRegistryEntry, removeRegistryEntry: registryMocks.removeRegistryEntry, + removeSandboxRegistryGeneration: ( + _kind: string, + entry: SandboxRegistryEntry, + assertCurrent: () => void, + ) => { + assertCurrent(); + return registryMocks.removeBrowserRegistryEntry(entry.containerName); + }, removeSandboxRegistryRuntime: async ( entry: SandboxRegistryEntry, removeRuntime: (current: SandboxRegistryEntry) => Promise, + options?: { shouldRemove?: (current: SandboxRegistryEntry) => boolean }, ) => { + if (options?.shouldRemove && !options.shouldRemove(entry)) { + return; + } await removeRuntime(entry); await registryMocks.removeRegistryEntry(entry.containerName); }, + withSandboxRegistryEntryLock: async ( + _entry: SandboxRegistryEntry, + operation: () => Promise, + ) => operation(), })); vi.mock("../../plugin-sdk/browser-bridge.js", () => ({ stopBrowserBridgeServer: bridgeMocks.stopBrowserBridgeServer, })); -function buildPruneConfig(): SandboxConfig { +function buildPruneConfig(): OpenClawConfig { return { - mode: "all", - backend: "docker", - scope: "session", - workspaceAccess: "none", - workspaceRoot: "/tmp/openclaw-sandboxes", - dockerTmpfsSource: "configured", - docker: { - image: "openclaw-sandbox:bookworm-slim", - containerPrefix: "openclaw-sbx-", - workdir: "/workspace", - readOnlyRoot: true, - tmpfs: [], - network: "none", - capDrop: ["ALL"], - env: {}, - }, - ssh: { - command: "ssh", - workspaceRoot: "/tmp/openclaw-sandboxes", - strictHostKeyChecking: true, - updateHostKeys: true, - }, - browser: { - enabled: true, - image: "openclaw-sandbox-browser:bookworm-slim", - containerPrefix: "openclaw-sbx-browser-", - network: "none", - cdpPort: 9222, - vncPort: 5900, - noVncPort: 6080, - headless: true, - noVncEnabled: false, - allowHostControl: false, - autoStart: true, - autoStartTimeoutMs: 1_000, - }, - tools: { - allow: [], - deny: [], - }, - prune: { - idleHours: 1, - maxAgeDays: 0, + agents: { + defaults: { + sandbox: { + mode: "all", + prune: { idleHours: 1, maxAgeDays: 0 }, + }, + }, }, }; } @@ -119,6 +101,7 @@ describe("maybePruneSandboxes", () => { beforeEach(async () => { vi.resetModules(); configMocks.getRuntimeConfig.mockReset(); + registryMocks.assertSandboxBrowserRegistryEntryCurrent.mockReset(); backendMocks.getSandboxBackendManager.mockReset().mockReturnValue(backendMocks); backendMocks.removeRuntime.mockReset(); registryMocks.readBrowserRegistry.mockReset(); @@ -135,6 +118,7 @@ describe("maybePruneSandboxes", () => { { containerName: "sandbox-1", backendId: "docker", + sessionKey: "agent:main:main", createdAtMs: Date.now() - 4 * 60 * 60 * 1000, lastUsedAtMs: Date.now() - 2 * 60 * 60 * 1000, image: "openclaw-sandbox:bookworm-slim", @@ -154,6 +138,82 @@ describe("maybePruneSandboxes", () => { expect(registryMocks.removeRegistryEntry).toHaveBeenCalledWith("sandbox-1"); }); + it("uses each registry owner's prune policy for containers and browsers", async () => { + const now = Date.now(); + vi.spyOn(Date, "now").mockReturnValue(now); + configMocks.getRuntimeConfig.mockReturnValue({ + agents: { + defaults: { + sandbox: { mode: "all", prune: { idleHours: 1, maxAgeDays: 0 } }, + }, + entries: { + work: { sandbox: { prune: { idleHours: 24, maxAgeDays: 0 } } }, + }, + }, + }); + const entry = (containerName: string, agentId: string) => ({ + containerName, + backendId: "docker", + sessionKey: `agent:${agentId}:main`, + createdAtMs: now - 4 * 60 * 60 * 1000, + lastUsedAtMs: now - 2 * 60 * 60 * 1000, + image: "openclaw-sandbox:bookworm-slim", + }); + registryMocks.readRegistry.mockResolvedValue({ + entries: [entry("main-container", "main"), entry("work-container", "work")], + }); + registryMocks.readBrowserRegistry.mockResolvedValue({ + entries: [ + { ...entry("main-browser", "main"), cdpPort: 9222 }, + { ...entry("work-browser", "work"), cdpPort: 9223 }, + ], + }); + + await maybePruneSandboxes(); + + expect( + backendMocks.removeRuntime.mock.calls.map(([params]) => params.entry.containerName), + ).toEqual(["main-container", "main-browser"]); + expect(backendMocks.removeRuntime.mock.calls.map(([params]) => params.agentId)).toEqual([ + "main", + "main", + ]); + expect(registryMocks.removeRegistryEntry).toHaveBeenCalledExactlyOnceWith("main-container"); + expect(registryMocks.removeBrowserRegistryEntry).toHaveBeenCalledExactlyOnceWith( + "main-browser", + ); + }); + + it("uses global prune policy for shared runtimes despite the caller override", async () => { + const now = Date.now(); + vi.spyOn(Date, "now").mockReturnValue(now); + configMocks.getRuntimeConfig.mockReturnValue({ + agents: { + defaults: { sandbox: { mode: "all", prune: { idleHours: 24, maxAgeDays: 0 } } }, + entries: { + main: { sandbox: { prune: { idleHours: 1, maxAgeDays: 0 } } }, + }, + }, + }); + registryMocks.readRegistry.mockResolvedValue({ + entries: [ + { + containerName: "shared-runtime", + backendId: "docker", + sessionKey: "shared", + createdAtMs: now - 4 * 60 * 60 * 1000, + lastUsedAtMs: now - 2 * 60 * 60 * 1000, + image: "openclaw-sandbox:bookworm-slim", + }, + ], + }); + registryMocks.readBrowserRegistry.mockResolvedValue({ entries: [] }); + + await maybePruneSandboxes(); + + expect(backendMocks.removeRuntime).not.toHaveBeenCalled(); + }); + it("keeps the registry entry when runtime removal fails", async () => { // The registry is the retry source; keep it until the backend confirms the // runtime was removed. @@ -174,6 +234,7 @@ describe("maybePruneSandboxes", () => { { containerName: "openshell-1", backendId: "openshell", + sessionKey: "agent:main:main", createdAtMs: Date.now() - 4 * 60 * 60 * 1000, lastUsedAtMs: Date.now() - 2 * 60 * 60 * 1000, image: "openclaw", @@ -195,6 +256,7 @@ describe("maybePruneSandboxes", () => { { containerName: "sandbox-out-of-range", backendId: "docker", + sessionKey: "agent:main:main", createdAtMs: Date.now(), lastUsedAtMs: Number.MAX_SAFE_INTEGER, image: "openclaw-sandbox:bookworm-slim", diff --git a/src/agents/sandbox/prune.ts b/src/agents/sandbox/prune.ts index 55bbffdb6243..676b88ebff17 100644 --- a/src/agents/sandbox/prune.ts +++ b/src/agents/sandbox/prune.ts @@ -5,30 +5,43 @@ import { asDateTimestampMs } from "@openclaw/normalization-core/number-coercion" * Removes stale runtime containers and browser bridges on a best-effort schedule. */ import { getRuntimeConfig } from "../../config/config.js"; +import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { defaultRuntime } from "../../runtime.js"; import { getSandboxBackendManager, usesSandboxRuntimeReservations } from "./backend.js"; import { stopCachedBrowserBridgesForContainer } from "./browser-bridges.js"; +import { resolveSandboxConfigForAgent } from "./config.js"; import { dockerSandboxBackendManager } from "./docker-backend.js"; import { + assertSandboxBrowserRegistryEntryCurrent, readBrowserRegistry, readRegistry, - removeBrowserRegistryEntry, + removeSandboxRegistryGeneration, removeSandboxRegistryRuntime, + withSandboxRegistryEntryLock, type SandboxBrowserRegistryEntry, type SandboxRegistryEntry, } from "./registry.js"; -import type { SandboxConfig } from "./types.js"; +import { resolveSandboxAgentId } from "./shared.js"; +import type { SandboxPruneConfig } from "./types.js"; let lastPruneAtMs = 0; type PruneableRegistryEntry = Pick< SandboxRegistryEntry, - "containerName" | "backendId" | "createdAtMs" | "lastUsedAtMs" + "containerName" | "backendId" | "createdAtMs" | "lastUsedAtMs" | "sessionKey" >; -function shouldPruneSandboxEntry(cfg: SandboxConfig, now: number, entry: PruneableRegistryEntry) { - const idleHours = cfg.prune.idleHours; - const maxAgeDays = cfg.prune.maxAgeDays; +function resolveEntryPruneConfig(config: OpenClawConfig, entry: PruneableRegistryEntry) { + return resolveSandboxConfigForAgent(config, resolveSandboxAgentId(entry.sessionKey)).prune; +} + +function shouldPruneSandboxEntry( + prune: SandboxPruneConfig, + now: number, + entry: PruneableRegistryEntry, +) { + const idleHours = prune.idleHours; + const maxAgeDays = prune.maxAgeDays; if (idleHours === 0 && maxAgeDays === 0) { return false; } @@ -45,7 +58,7 @@ function shouldPruneSandboxEntry(cfg: SandboxConfig, now: number, entry: Pruneab /** Removes expired registry entries and their backing runtime resources. */ async function pruneSandboxRegistryEntries(params: { - cfg: SandboxConfig; + config: OpenClawConfig; read: () => Promise<{ entries: TEntry[] }>; remove: ( entry: TEntry, @@ -53,16 +66,15 @@ async function pruneSandboxRegistryEntries( ) => Promise; }) { const now = Date.now(); - if (params.cfg.prune.idleHours === 0 && params.cfg.prune.maxAgeDays === 0) { - return; - } const registry = await params.read(); for (const entry of registry.entries) { - if (!shouldPruneSandboxEntry(params.cfg, now, entry)) { + if (!shouldPruneSandboxEntry(resolveEntryPruneConfig(params.config, entry), now, entry)) { continue; } try { - await params.remove(entry, (current) => shouldPruneSandboxEntry(params.cfg, now, current)); + await params.remove(entry, (current) => + shouldPruneSandboxEntry(resolveEntryPruneConfig(params.config, current), now, current), + ); } catch (error) { const message = error instanceof Error @@ -78,10 +90,9 @@ async function pruneSandboxRegistryEntries( } /** Prunes ordinary sandbox runtime containers from the configured backend manager. */ -async function pruneSandboxContainers(cfg: SandboxConfig) { - const config = getRuntimeConfig(); +async function pruneSandboxContainers(config: OpenClawConfig) { await pruneSandboxRegistryEntries({ - cfg, + config, read: readRegistry, remove: (entry, shouldRemove) => removeSandboxRegistryRuntime( @@ -94,7 +105,11 @@ async function pruneSandboxContainers(cfg: SandboxConfig) { `Sandbox backend "${backendId}" is unavailable; enable its plugin before removing this runtime.`, ); } - await manager.removeRuntime({ entry: current, config }); + await manager.removeRuntime({ + entry: current, + config, + agentId: resolveSandboxAgentId(current.sessionKey), + }); }, { reserveRuntime: usesSandboxRuntimeReservations(entry.backendId ?? "docker"), @@ -105,8 +120,7 @@ async function pruneSandboxContainers(cfg: SandboxConfig) { } /** Prunes browser bridge containers and closes matching in-process bridge servers. */ -async function pruneSandboxBrowsers(cfg: SandboxConfig) { - const config = getRuntimeConfig(); +async function pruneSandboxBrowsers(config: OpenClawConfig) { await pruneSandboxRegistryEntries< SandboxBrowserRegistryEntry & { backendId?: string; @@ -114,34 +128,51 @@ async function pruneSandboxBrowsers(cfg: SandboxConfig) { configLabelKind?: string; } >({ - cfg, + config, read: readBrowserRegistry, - remove: async (entry) => { - await stopCachedBrowserBridgesForContainer(entry.containerName); - await dockerSandboxBackendManager.removeRuntime({ - entry: { - ...entry, - backendId: "docker", - runtimeLabel: entry.containerName, - configLabelKind: "Image", - }, - config, + remove: async (entry, shouldRemove) => { + await withSandboxRegistryEntryLock({ ...entry, backendId: "docker" }, async () => { + const current = (await readBrowserRegistry()).entries.find( + (candidate) => candidate.containerName === entry.containerName, + ); + if (!current || !shouldRemove(current)) { + return; + } + try { + assertSandboxBrowserRegistryEntryCurrent(entry); + } catch { + return; + } + await stopCachedBrowserBridgesForContainer(current.containerName); + await dockerSandboxBackendManager.removeRuntime({ + entry: { + ...current, + backendId: "docker", + runtimeLabel: current.containerName, + configLabelKind: "Image", + }, + config, + agentId: resolveSandboxAgentId(current.sessionKey), + }); + removeSandboxRegistryGeneration("browser", current, () => + assertSandboxBrowserRegistryEntryCurrent(current), + ); }); - await removeBrowserRegistryEntry(entry.containerName); }, }); } /** Runs sandbox pruning at most once per throttle window. */ -export async function maybePruneSandboxes(cfg: SandboxConfig) { +export async function maybePruneSandboxes(config?: OpenClawConfig) { const now = Date.now(); if (now - lastPruneAtMs < 5 * 60 * 1000) { return; } lastPruneAtMs = now; try { - await pruneSandboxContainers(cfg); - await pruneSandboxBrowsers(cfg); + const currentConfig = config ?? getRuntimeConfig(); + await pruneSandboxContainers(currentConfig); + await pruneSandboxBrowsers(currentConfig); } catch (error) { const message = error instanceof Error diff --git a/src/agents/sandbox/registry.ts b/src/agents/sandbox/registry.ts index 5101c36117a6..79c47185f3ed 100644 --- a/src/agents/sandbox/registry.ts +++ b/src/agents/sandbox/registry.ts @@ -383,7 +383,8 @@ export async function removeSandboxRegistryRuntime( !current || (current.runtimeState !== "removing" && current.runtimeState !== "removing-pending") || current.backendId !== removing.backendId || - current.sessionKey !== removing.sessionKey + current.sessionKey !== removing.sessionKey || + (options.shouldRemove && !options.shouldRemove(current)) ) { return; } diff --git a/src/agents/sandbox/runtime-reservation.test.ts b/src/agents/sandbox/runtime-reservation.test.ts index 2d3ddf3d6837..7dd39e94fd15 100644 --- a/src/agents/sandbox/runtime-reservation.test.ts +++ b/src/agents/sandbox/runtime-reservation.test.ts @@ -44,6 +44,22 @@ function advancePruneTime() { vi.spyOn(Date, "now").mockReturnValue(pruneTimeMs); } +function withImmediatePrune(cfg: OpenClawConfig): OpenClawConfig { + return { + ...cfg, + agents: { + ...cfg.agents, + defaults: { + ...cfg.agents?.defaults, + sandbox: { + ...cfg.agents?.defaults?.sandbox, + prune: { idleHours: 1, maxAgeDays: 0 }, + }, + }, + }, + }; +} + beforeEach(() => { const stateDir = tempDirs.make("sandbox-reservation-"); vi.stubEnv("OPENCLAW_STATE_DIR", stateDir); @@ -269,7 +285,7 @@ describe("durable sandbox runtime generations", () => { it.each(["recreate", "prune"] as const)( "fences a legacy %s snapshot after concurrent adoption", async (operation) => { - const cfg = await seedLegacyRuntime(); + await seedLegacyRuntime(); const snapshot = await readRegistry(); const started = createDeferred(); const finish = createDeferred(); @@ -290,7 +306,7 @@ describe("durable sandbox runtime generations", () => { const removing = operation === "recreate" ? removeSandboxContainer("legacy-runtime") - : maybePruneSandboxes({ ...cfg, prune: { idleHours: 1, maxAgeDays: 0 } }); + : maybePruneSandboxes(withImmediatePrune(config)); const creating = expect(resolve()).rejects.toThrow("removed or is being removed"); await started.promise; try { @@ -419,8 +435,7 @@ describe("durable sandbox runtime generations", () => { let removing: Promise; if (operation === "prune") { advancePruneTime(); - const cfg = resolveSandboxConfigForAgent(config, "test"); - removing = maybePruneSandboxes({ ...cfg, prune: { idleHours: 1, maxAgeDays: 0 } }); + removing = maybePruneSandboxes(withImmediatePrune(config)); } else { removing = removeSandboxContainer(id); }