From 770e52445458319a10780858b736fb9eba34a3d6 Mon Sep 17 00:00:00 2001 From: Ying Xiang Date: Mon, 26 Oct 2026 17:21:59 +0800 Subject: [PATCH 1/4] refactor(sandbox): derive container names from the key; drop test-only linkAbortSignal and VaultManager.list ProvisionOptions.containerName had one caller, which passed the default name, so the resolver's returned-name check could never fail. --- src/harness/execution-resolver.ts | 12 ++------ src/sandbox/provisioner.ts | 34 +++++++++-------------- src/sandbox/utils.ts | 10 ------- src/test/commands-utils.test.ts | 1 - src/test/commands.test.ts | 1 - src/test/provisioner.test.ts | 12 ++++---- src/test/sandbox-utils.test.ts | 46 +------------------------------ src/test/vault-disabled.test.ts | 1 - src/test/vault.test.ts | 13 --------- src/types.ts | 1 - src/vault/index.ts | 17 ------------ src/vault/types.ts | 1 - 12 files changed, 21 insertions(+), 128 deletions(-) diff --git a/src/harness/execution-resolver.ts b/src/harness/execution-resolver.ts index 8ac82bfe..c1233da8 100644 --- a/src/harness/execution-resolver.ts +++ b/src/harness/execution-resolver.ts @@ -112,11 +112,8 @@ export class ActorExecutionResolver { } return async () => { - const expected = plan.sandboxConfig.type === "container" ? plan.sandboxConfig.container : ""; - let actual: string | undefined; try { - actual = await this.provisioner?.provision(plan.resourceKey, { - containerName: expected, + await this.provisioner?.provision(plan.resourceKey, { mounts: plan.mounts, conversationId, }); @@ -131,17 +128,12 @@ export class ActorExecutionResolver { conversationId, credentialKey: plan.credentialKey, resourceKey: plan.resourceKey, - expectedContainer: expected, + expectedContainer: DockerContainerManager.containerName(plan.resourceKey), hasVault: Boolean(plan.env || plan.mounts.length > 0), }, }); throw err; } - if (actual && actual !== expected) { - throw new Error( - `Provisioner returned container "${actual}" for resource key "${plan.resourceKey}", expected "${expected}"`, - ); - } }; } diff --git a/src/sandbox/provisioner.ts b/src/sandbox/provisioner.ts index d7b56a5c..2f246c42 100644 --- a/src/sandbox/provisioner.ts +++ b/src/sandbox/provisioner.ts @@ -15,7 +15,6 @@ type DriftReason = "binds" | "mount-content" | "network" | "image"; interface ContainerState { status: ContainerStatus; lastUsed: number; - containerName: string; } import type { @@ -103,8 +102,7 @@ export class DockerContainerManager { } private async provisionInner(containerKey: string, options: ProvisionOptions): Promise { - const containerName = - options.containerName ?? DockerContainerManager.containerName(containerKey); + const containerName = DockerContainerManager.containerName(containerKey); const mounts = options.mounts ?? []; const status = await this.inspectStatus(containerName); @@ -131,8 +129,8 @@ export class DockerContainerManager { throw err; } - this.setState(containerKey, "running", containerName); - await this.applyResourceLimits(containerKey, containerName); + this.setState(containerKey, "running"); + await this.applyResourceLimits(containerKey); return containerName; } @@ -145,7 +143,7 @@ export class DockerContainerManager { this.boostedKeys.add(containerKey); const state = this.state.get(containerKey); if (state?.status === "running") { - await this.applyResourceLimits(containerKey, state.containerName); + await this.applyResourceLimits(containerKey); } return this.getLimitStatus(containerKey); } @@ -155,7 +153,7 @@ export class DockerContainerManager { this.overrideLimits.set(containerKey, { ...this.limits, ...limits }); const state = this.state.get(containerKey); if (state?.status === "running") { - await this.applyResourceLimits(containerKey, state.containerName); + await this.applyResourceLimits(containerKey); } return this.getLimitStatus(containerKey); } @@ -200,10 +198,10 @@ export class DockerContainerManager { } private async stopInner(containerKey: string): Promise { - const containerName = this.getContainerName(containerKey); + const containerName = DockerContainerManager.containerName(containerKey); try { await this.run(["stop", containerName]); - this.setState(containerKey, "stopped", containerName); + this.setState(containerKey, "stopped"); this.boostedKeys.delete(containerKey); this.overrideLimits.delete(containerKey); log.logInfo(`Container ${containerName} stopped (idle)`); @@ -217,7 +215,7 @@ export class DockerContainerManager { } private async removeInner(containerKey: string): Promise { - const containerName = this.getContainerName(containerKey); + const containerName = DockerContainerManager.containerName(containerKey); const networkName = DockerContainerManager.networkName(containerKey); const removed = await this.forceRemoveContainer( @@ -273,7 +271,7 @@ export class DockerContainerManager { } const status: ContainerStatus = details.running ? "running" : "stopped"; const lastUsed = details.startedAtMs ?? Date.now(); - this.state.set(containerKey, { status, lastUsed, containerName }); + this.state.set(containerKey, { status, lastUsed }); } const running = Array.from(this.state.values()).filter((s) => s.status === "running").length; @@ -283,15 +281,8 @@ export class DockerContainerManager { ); } - private setState(containerKey: string, status: ContainerStatus, containerName: string): void { - this.state.set(containerKey, { status, lastUsed: Date.now(), containerName }); - } - - private getContainerName(containerKey: string): string { - return ( - this.state.get(containerKey)?.containerName ?? - DockerContainerManager.containerName(containerKey) - ); + private setState(containerKey: string, status: ContainerStatus): void { + this.state.set(containerKey, { status, lastUsed: Date.now() }); } private mountArgs(mounts: ContainerMount[]): string[] { @@ -376,9 +367,10 @@ export class DockerContainerManager { return args; } - private async applyResourceLimits(containerKey: string, containerName: string): Promise { + private async applyResourceLimits(containerKey: string): Promise { const limitArgs = this.resourceLimitArgs(this.effectiveLimits(containerKey)); if (limitArgs.length === 0) return; + const containerName = DockerContainerManager.containerName(containerKey); const args = ["update", ...limitArgs, containerName]; try { await this.run(args); diff --git a/src/sandbox/utils.ts b/src/sandbox/utils.ts index 5f1df9fc..1b80e068 100644 --- a/src/sandbox/utils.ts +++ b/src/sandbox/utils.ts @@ -21,16 +21,6 @@ export function execSimple(cmd: string, args: string[]): Promise { }); } -export function linkAbortSignal(signal: AbortSignal | undefined, onAbort: () => void): () => void { - if (!signal) return () => {}; - if (signal.aborted) { - onAbort(); - return () => {}; - } - signal.addEventListener("abort", onAbort, { once: true }); - return () => signal.removeEventListener("abort", onAbort); -} - export function createMountedRuntimePathContext( hostWorkspaceRoot: string, runtimeWorkspaceRoot: string, diff --git a/src/test/commands-utils.test.ts b/src/test/commands-utils.test.ts index ab9db344..fa2cef57 100644 --- a/src/test/commands-utils.test.ts +++ b/src/test/commands-utils.test.ts @@ -42,7 +42,6 @@ function fakeVaultManager(): VaultManager { return { hasEntry: () => false, resolve: () => undefined, - list: () => [], isEnabled: () => false, upsertEnv: () => {}, deleteEnvKey: () => false, diff --git a/src/test/commands.test.ts b/src/test/commands.test.ts index 00a974cd..59325a27 100644 --- a/src/test/commands.test.ts +++ b/src/test/commands.test.ts @@ -80,7 +80,6 @@ function fakeVaultManager(): VaultManager & { entries: Set } { entries, hasEntry: (key) => entries.has(key), resolve: () => undefined, - list: () => [], isEnabled: () => true, upsertEnv: (key) => { entries.add(key); diff --git a/src/test/provisioner.test.ts b/src/test/provisioner.test.ts index 4b4dd107..7a64ff7b 100644 --- a/src/test/provisioner.test.ts +++ b/src/test/provisioner.test.ts @@ -300,12 +300,11 @@ describe("DockerContainerManager", () => { ]); }); - test("provisions custom container names with extra vault mounts", async () => { + test("provisions a container with extra vault mounts", async () => { const engine = new FakeEngine(); const sandbox = manager(engine); await sandbox.provision("alice", { - containerName: "alice-box", mounts: [{ source: "/tmp/vaults/alice/.ssh", target: "/root/.ssh" }], conversationId: "D123", }); @@ -328,7 +327,7 @@ describe("DockerContainerManager", () => { "run", "-d", "--name", - "alice-box", + "mikan-sandbox-alice", "--network", "mikan-sandbox-net-alice", "--cap-drop", @@ -359,7 +358,7 @@ describe("DockerContainerManager", () => { "sleep", "infinity", ]); - expect(engine.callsOf("stop")).toEqual([["stop", "alice-box"]]); + expect(engine.callsOf("stop")).toEqual([["stop", "mikan-sandbox-alice"]]); }); test("checks for the per-office network by name and creates it only when absent", async () => { @@ -470,17 +469,16 @@ describe("DockerContainerManager", () => { test("replaces an existing container from the image when vault mounts change", async () => { const engine = new FakeEngine(); - engine.seed("alice-box", "alice", { + engine.seed("mikan-sandbox-alice", "alice", { mounts: [{ source: "/tmp/vaults/alice/.ssh", target: "/root/.ssh" }], }); await manager(engine).provision("alice", { - containerName: "alice-box", mounts: [{ source: "/tmp/vaults/alice/.kube", target: "/root/.kube" }], conversationId: "D123", }); - expect(engine.callsOf("rm")).toEqual([["rm", "-f", "alice-box"]]); + expect(engine.callsOf("rm")).toEqual([["rm", "-f", "mikan-sandbox-alice"]]); const runArgs = engine.callsOf("run")[0]; expect(runArgs).toContain("/tmp/vaults/alice/.kube:/root/.kube"); expect(runArgs).not.toContain("/tmp/vaults/alice/.ssh:/root/.ssh"); diff --git a/src/test/sandbox-utils.test.ts b/src/test/sandbox-utils.test.ts index 0133f729..ea4dba6b 100644 --- a/src/test/sandbox-utils.test.ts +++ b/src/test/sandbox-utils.test.ts @@ -1,49 +1,5 @@ import { describe, expect, test } from "vitest"; -import { SandboxError, execSimple, linkAbortSignal } from "../sandbox/utils.js"; - -describe("linkAbortSignal", () => { - test("is inert without a signal", () => { - let fired = 0; - const unlink = linkAbortSignal(undefined, () => fired++); - expect(fired).toBe(0); - expect(() => unlink()).not.toThrow(); - expect(fired).toBe(0); - }); - - test("fires immediately for an already-aborted signal", () => { - const controller = new AbortController(); - controller.abort(); - let fired = 0; - - const unlink = linkAbortSignal(controller.signal, () => fired++); - - expect(fired).toBe(1); - expect(() => unlink()).not.toThrow(); - expect(fired).toBe(1); - }); - - test("forwards an abort that arrives later", () => { - const controller = new AbortController(); - let fired = 0; - - linkAbortSignal(controller.signal, () => fired++); - expect(fired).toBe(0); - - controller.abort(); - expect(fired).toBe(1); - }); - - test("unlinking drops the listener, so a later abort is ignored", () => { - const controller = new AbortController(); - let fired = 0; - - const unlink = linkAbortSignal(controller.signal, () => fired++); - unlink(); - controller.abort(); - - expect(fired).toBe(0); - }); -}); +import { SandboxError, execSimple } from "../sandbox/utils.js"; describe("SandboxError", () => { test("formats the message alone when there are no details", () => { diff --git a/src/test/vault-disabled.test.ts b/src/test/vault-disabled.test.ts index f827dce9..c4ac1cdd 100644 --- a/src/test/vault-disabled.test.ts +++ b/src/test/vault-disabled.test.ts @@ -6,7 +6,6 @@ describe("disabledVaultManager", () => { expect(disabledVaultManager.isEnabled()).toBe(false); expect(disabledVaultManager.hasEntry("anyone")).toBe(false); expect(disabledVaultManager.resolve("anyone")).toBeUndefined(); - expect(disabledVaultManager.list()).toEqual([]); expect(disabledVaultManager.listSharedVaults()).toEqual([]); expect(disabledVaultManager.deleteSharedVault("shared")).toBe(false); expect(disabledVaultManager.copySharedVaultTo("shared", "user")).toEqual({ diff --git a/src/test/vault.test.ts b/src/test/vault.test.ts index eb916a21..10149029 100644 --- a/src/test/vault.test.ts +++ b/src/test/vault.test.ts @@ -71,17 +71,6 @@ describe("FileVaultManager", () => { expect(new FileVaultManager(tmpDir).isEnabled()).toBe(false); }); - test("list() skips reserved namespaces (shared profiles, extension secrets)", () => { - mkdirSync(join(vaultsDir, "U123"), { recursive: true }); - writeFileSync(join(vaultsDir, "U123", "env"), "TOKEN=x\n"); - mkdirSync(join(vaultsDir, "shared", "team-login"), { recursive: true }); - mkdirSync(join(vaultsDir, "extensions", "agent-pm"), { recursive: true }); - writeFileSync(join(vaultsDir, "extensions", "agent-pm", "env"), "LINEAR_TOKEN=y\n"); - - const vaults = new FileVaultManager(tmpDir).list(); - expect(vaults).toEqual([{ env: { TOKEN: "x" }, mounts: [] }]); - }); - test("resolves a vault from directory contents", () => { const userDir = join(vaultsDir, "U123"); mkdirSync(join(userDir, ".ssh"), { recursive: true }); @@ -566,7 +555,6 @@ describe("ActorExecutionResolver image mode", () => { expect(ran.ok ? "ran" : ran.error.message).toContain("provisioning stops the call"); expect(provision).toHaveBeenCalledWith(D123_OFFICE, { - containerName: `mikan-sandbox-${D123_OFFICE}`, conversationId: "D123", mounts: [ { source: join(tmpDir, D123_OFFICE), target: `/workspace/${D123_OFFICE}` }, @@ -605,7 +593,6 @@ describe("ActorExecutionResolver image mode", () => { expect(ran.ok ? "ran" : ran.error.message).toContain("provisioning stops the call"); expect(provision).toHaveBeenCalledWith(D123_OFFICE, { - containerName: `mikan-sandbox-${D123_OFFICE}`, conversationId: "D123", mounts: [ { source: join(tmpDir, D123_OFFICE), target: `/workspace/${D123_OFFICE}` }, diff --git a/src/types.ts b/src/types.ts index fe4fe48c..74f8f852 100644 --- a/src/types.ts +++ b/src/types.ts @@ -306,7 +306,6 @@ export interface SandboxResourceController { } export interface ProvisionOptions { - containerName?: string; mounts?: ContainerMount[]; conversationId?: string; } diff --git a/src/vault/index.ts b/src/vault/index.ts index c60af3e7..fc748ae6 100644 --- a/src/vault/index.ts +++ b/src/vault/index.ts @@ -11,7 +11,6 @@ import type { PlatformTrustModel } from "../types.js"; const PRIVATE_DIR_MODE = 0o700; const SHARED_VAULT_DIR = "shared"; const MOUNT_TARGETS_DIR = "vault-mount-targets"; -const RESERVED_VAULT_DIRS = new Set([SHARED_VAULT_DIR, "extensions"]); export function normalizeSharedVaultName(name: string): string | undefined { const trimmed = name.trim(); @@ -120,21 +119,6 @@ export class FileVaultManager implements VaultManager { return this.buildResolved(userId); } - list(): ResolvedVault[] { - if (!existsSync(this.vaultsDir)) return []; - const keys = new Set(); - for (const entry of readdirSync(this.vaultsDir, { withFileTypes: true })) { - if ( - entry.isDirectory() && - !RESERVED_VAULT_DIRS.has(entry.name) && - isSafeVaultKey(entry.name) - ) { - keys.add(entry.name); - } - } - return Array.from(keys, (key) => this.buildResolved(key)); - } - upsertEnv(key: string, env: Record): void { if (!isSafeVaultKey(key)) throw new Error(`vault: invalid vault key: ${key}`); const dir = join(this.vaultsDir, key); @@ -500,7 +484,6 @@ export const disabledVaultManager: VaultManager = { isEnabled: () => false, hasEntry: () => false, resolve: () => undefined, - list: () => [], listSharedVaults: () => [], deleteSharedVault: () => false, copySharedVaultTo: () => ({ filesCopied: 0, envKeysCopied: 0 }), diff --git a/src/vault/types.ts b/src/vault/types.ts index 7ead8dec..037e576c 100644 --- a/src/vault/types.ts +++ b/src/vault/types.ts @@ -16,7 +16,6 @@ export interface VaultInjection { export interface VaultManager { hasEntry(key: string): boolean; resolve(userId: string): ResolvedVault | undefined; - list(): ResolvedVault[]; isEnabled(): boolean; upsertEnv(key: string, env: Record): void; deleteEnvKey(key: string, envKey: string): boolean; From 12537b9a3296f39ff93b307035b0a47f79ccd5ba Mon Sep 17 00:00:00 2001 From: Ying Xiang Date: Mon, 26 Oct 2026 17:22:23 +0800 Subject: [PATCH 2/4] refactor(settings): one rewriteSettingsFile for the three settings writers noPlatformsMessage now reuses envSummaryLines. --- src/env-manifest.ts | 11 ++++------ src/settings/index.ts | 48 +++++++++++++++++-------------------------- 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/src/env-manifest.ts b/src/env-manifest.ts index 47a7b39d..49d9c0ed 100644 --- a/src/env-manifest.ts +++ b/src/env-manifest.ts @@ -253,19 +253,16 @@ function platformRecipe(group: EnvGroup): string { return `${group.title}: ${required.join(" + ")}`; } -export function noPlatformsMessage(): string { - const recipes = ENV_MANIFEST.filter((group) => group.kind === "platform") - .map((group) => ` ${platformRecipe(group)}`) - .join("\n"); - return `No platform tokens found. Set one of:\n${recipes}`; -} - export function envSummaryLines(): string[] { return ENV_MANIFEST.filter((group) => group.kind === "platform").map( (group) => ` ${platformRecipe(group)}`, ); } +export function noPlatformsMessage(): string { + return `No platform tokens found. Set one of:\n${envSummaryLines().join("\n")}`; +} + export function envReport(env: EnvLookup = readEnv): string { const lines: string[] = []; for (const group of ENV_MANIFEST) { diff --git a/src/settings/index.ts b/src/settings/index.ts index 5bd58868..314f312c 100644 --- a/src/settings/index.ts +++ b/src/settings/index.ts @@ -359,7 +359,7 @@ function patchSettingsConfig( existing: SettingsFileConfig, config: Partial, ): SettingsFileConfig { - const patched: SettingsFileConfig = { + return { ...existing, llm: { ...existing.llm, @@ -375,7 +375,6 @@ function patchSettingsConfig( office: existing.office, ...(config.mcpServers !== undefined ? { mcpServers: config.mcpServers } : {}), }; - return compactSettingsConfig(patched); } function loadSettingsFileForUpdate( @@ -394,25 +393,29 @@ function loadSettingsFileForUpdate( } } -function updateSettingsFile( +function rewriteSettingsFile( settingsPath: string, - patch: Partial, defaultSettings: SettingsFileConfig, + change: (existing: SettingsFileConfig) => SettingsFileConfig, ): void { const existing = loadSettingsFileForUpdate(settingsPath, defaultSettings); ensurePrivateDirExists(dirname(settingsPath)); atomicWritePrivateFile( settingsPath, - JSON.stringify(patchSettingsConfig(existing, patch), null, 2), + JSON.stringify(compactSettingsConfig(change(existing)), null, 2), ); } export function updateGlobalSettings(stateDir: string, patch: Partial): void { - updateSettingsFile(globalSettingsPath(stateDir), patch, ONBOARD_SETTINGS); + rewriteSettingsFile(globalSettingsPath(stateDir), ONBOARD_SETTINGS, (existing) => + patchSettingsConfig(existing, patch), + ); } export function updateConversationSettings(office: Office, patch: Partial): void { - updateSettingsFile(conversationSettingsPath(office), patch, {}); + rewriteSettingsFile(conversationSettingsPath(office), {}, (existing) => + patchSettingsConfig(existing, patch), + ); } export function loadSkillPatterns(office: Office): SkillPatterns { @@ -431,18 +434,13 @@ export function updateSkillPatterns( scope === "conversation" ? conversationSettingsPath(office) : globalSettingsPath(office.workspace.stateDir); - const existing = loadSettingsFileForUpdate( + rewriteSettingsFile( settingsPath, scope === "conversation" ? {} : ONBOARD_SETTINGS, - ); - ensurePrivateDirExists(dirname(settingsPath)); - atomicWritePrivateFile( - settingsPath, - JSON.stringify( - compactSettingsConfig({ ...existing, skills: skills.length > 0 ? [...skills] : undefined }), - null, - 2, - ), + (existing) => ({ + ...existing, + skills: skills.length > 0 ? [...skills] : undefined, + }), ); } @@ -461,16 +459,8 @@ export function loadOfficeVisibilityOverride(office: Office): "private" | null { } export function setOfficeVisibilityOverride(office: Office, visibility: "private" | null): void { - const settingsPath = conversationSettingsPath(office); - const existing = loadSettingsFileForUpdate(settingsPath, {}); - const { office: _previous, ...rest } = existing; - ensurePrivateDirExists(dirname(settingsPath)); - atomicWritePrivateFile( - settingsPath, - JSON.stringify( - compactSettingsConfig({ ...rest, office: visibility ? { visibility } : undefined }), - null, - 2, - ), - ); + rewriteSettingsFile(conversationSettingsPath(office), {}, (existing) => ({ + ...existing, + office: visibility ? { visibility } : undefined, + })); } From c39e94cd77a3759c912fe02f468ab805c348e1d0 Mon Sep 17 00:00:00 2001 From: Ying Xiang Date: Mon, 26 Oct 2026 17:22:47 +0800 Subject: [PATCH 3/4] refactor(observability): move Slack breadcrumb names into the Slack update diagnostics caller --- src/adapters/slack/update-diagnostics.ts | 26 ++++++++++++++++-------- src/observability/index.ts | 22 +++++++------------- src/observability/types.ts | 7 +++++++ 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/adapters/slack/update-diagnostics.ts b/src/adapters/slack/update-diagnostics.ts index 1dd9c897..bd550bdd 100644 --- a/src/adapters/slack/update-diagnostics.ts +++ b/src/adapters/slack/update-diagnostics.ts @@ -127,13 +127,23 @@ export function recordSlackUpdate( } else { failures?.delete(key); } - recordDiagnosticEvent("error" in outcome ? "slack.update.rejected" : "slack.update.recovered", { - operation: "chat.update", - channelId: channel, - responseMessageId: ts, - failedAttempts: attempts, - ...shape(source, payload), - ...("error" in outcome ? validation(outcome.error) : {}), - }); + const rejected = "error" in outcome; + recordDiagnosticEvent( + rejected ? "slack.update.rejected" : "slack.update.recovered", + { + operation: "chat.update", + channelId: channel, + responseMessageId: ts, + failedAttempts: attempts, + ...shape(source, payload), + ...(rejected ? validation(outcome.error) : {}), + }, + { + message: rejected ? "Slack update rejected" : "Slack update recovered", + category: "slack.update", + level: rejected ? "warning" : "info", + log: !rejected, + }, + ); } catch {} } diff --git a/src/observability/index.ts b/src/observability/index.ts index 35b0b4e0..ff736bd9 100644 --- a/src/observability/index.ts +++ b/src/observability/index.ts @@ -12,6 +12,7 @@ import { } from "./sentry.js"; import { isOpenTelemetryMetricsEnabled, shutdownOpenTelemetry } from "./otel.js"; import type { + DiagnosticBreadcrumb, JevOutcomeReport, ObservabilityAttributes, ReportUserFacingErrorOptions, @@ -168,7 +169,11 @@ export function addLifecycleEvent( addSentryBreadcrumb(name, safeAttributes); } -export function recordDiagnosticEvent(name: string, data: Record): void { +export function recordDiagnosticEvent( + name: string, + data: Record, + breadcrumb: DiagnosticBreadcrumb, +): void { const spanAttributes = metricAttributes( Object.fromEntries( Object.entries(data).filter((entry): entry is [string, string | number | boolean] => { @@ -178,20 +183,7 @@ export function recordDiagnosticEvent(name: string, data: Record; export type SentryAttributionAttributes = ObservabilityAttributes; From 49d09f5d215a68ca0a425b597663ad2142555109 Mon Sep 17 00:00:00 2001 From: Ying Xiang Date: Mon, 26 Oct 2026 17:26:12 +0800 Subject: [PATCH 4/4] refactor(sandbox): read drift state with one inspect template provision read status, binds, and each label with separate inspect calls (up to five per run). One template returns them all, so a steady-state provision costs one inspect instead of four, and a stopped one one instead of five. The fake engine now evaluates templates field by field. --- src/sandbox/provisioner.ts | 134 +++++++++++++++++------------------ src/test/provisioner.test.ts | 27 ++++--- 2 files changed, 83 insertions(+), 78 deletions(-) diff --git a/src/sandbox/provisioner.ts b/src/sandbox/provisioner.ts index 2f246c42..05dbc6ce 100644 --- a/src/sandbox/provisioner.ts +++ b/src/sandbox/provisioner.ts @@ -9,7 +9,7 @@ import { reportUserFacingError } from "../observability/index.js"; const execFileAsync = promisify(execFile); -type ContainerStatus = "running" | "stopped" | "missing"; +type ContainerStatus = "running" | "stopped"; type DriftReason = "binds" | "mount-content" | "network" | "image"; interface ContainerState { @@ -17,6 +17,15 @@ interface ContainerState { lastUsed: number; } +interface ContainerSnapshot { + running: boolean; + image: string; + mountSignature?: string; + network?: string; + imageId?: string; + mountsJson: string; +} + import type { ContainerMount, DockerContainerManagerOptions, @@ -104,23 +113,20 @@ export class DockerContainerManager { private async provisionInner(containerKey: string, options: ProvisionOptions): Promise { const containerName = DockerContainerManager.containerName(containerKey); const mounts = options.mounts ?? []; - const status = await this.inspectStatus(containerName); + const snapshot = await this.inspectContainer(containerName); try { - const drift = - status === "missing" - ? undefined - : await this.runtimeDrift(containerKey, containerName, mounts, status); - if (drift) { + const drift = snapshot ? await this.runtimeDrift(containerKey, snapshot, mounts) : undefined; + if (snapshot && drift) { log.logInfo(`Container ${containerName} is out of date (${drift}); replacing container`); - const leftBehind = await this.imageLeftBehind(containerName); + const leftBehind = await this.imageLeftBehind(snapshot); await this.replaceContainer(containerKey, containerName, mounts, options); log.logInfo(`Container ${containerName} replaced from image ${this.image}`); if (leftBehind) await this.removeImageIfUnused(leftBehind); - } else if (status === "stopped") { + } else if (snapshot && !snapshot.running) { await this.run(["start", containerName]); log.logInfo(`Container ${containerName} started`); - } else if (status === "missing") { + } else if (!snapshot) { await this.runContainer(containerKey, containerName, mounts, options); log.logInfo(`Container ${containerName} created`); } @@ -397,19 +403,28 @@ export class DockerContainerManager { private async runtimeDrift( containerKey: string, - containerName: string, + snapshot: ContainerSnapshot, mounts: ContainerMount[], - status: ContainerStatus, ): Promise { - if (this.hasBindMountDrift(await this.inspectBindMounts(containerName), mounts)) return "binds"; - if (await this.hasMountSignatureDrift(containerName, mounts)) return "mount-content"; - if (await this.hasNetworkModeDrift(containerKey, containerName)) return "network"; - if (status === "stopped" && (await this.hasImageDrift(containerName))) return "image"; + if (this.hasBindMountDrift(containerKey, snapshot, mounts)) return "binds"; + if (mounts.length > 0 && snapshot.mountSignature !== this.mountSignature(mounts)) { + return "mount-content"; + } + if (snapshot.network !== DockerContainerManager.networkName(containerKey)) return "network"; + if (!snapshot.running && (await this.hasImageDrift(snapshot))) return "image"; return undefined; } - private hasBindMountDrift(binds: readonly string[], mounts: ContainerMount[]): boolean { - return !this.sameBinds(this.expectedBinds(mounts), [...binds]); + private hasBindMountDrift( + containerKey: string, + snapshot: ContainerSnapshot, + mounts: ContainerMount[], + ): boolean { + const containerName = DockerContainerManager.containerName(containerKey); + const binds = parseBindMounts(snapshot.mountsJson, containerName) + .map((mount) => this.toBindSpec(mount)) + .toSorted(); + return !this.sameBinds(this.expectedBinds(mounts), binds); } private expectedBinds(mounts: ContainerMount[]): string[] { @@ -427,18 +442,6 @@ export class DockerContainerManager { return expected.every((bind, index) => bind === actual[index]); } - private async hasMountSignatureDrift( - containerName: string, - mounts: ContainerMount[], - ): Promise { - if (mounts.length === 0) return false; - const expected = this.mountSignature(mounts); - return ( - (await this.label(containerName, DockerContainerManager.MOUNT_SIGNATURE_LABEL_KEY)) !== - expected - ); - } - private mountSignature(mounts: ContainerMount[]): string { const payload = mounts .map((mount) => ({ @@ -464,27 +467,10 @@ export class DockerContainerManager { } } - private async label(containerName: string, key: string): Promise { - const { stdout } = await this.run([ - "inspect", - "-f", - `{{index .Config.Labels "${key}"}}`, - containerName, - ]); - return this.normalizeDockerValue(stdout.trim()); - } - - private async inspectBindMounts(containerName: string): Promise { - const { stdout } = await this.run(["inspect", "-f", "{{json .Mounts}}", containerName]); - return parseBindMounts(stdout, containerName) - .map((mount) => `${mount.source}:${mount.target}${mount.readOnly ? ":ro" : ""}`) - .toSorted(); - } - - private async hasImageDrift(containerName: string): Promise { + private async hasImageDrift(snapshot: ContainerSnapshot): Promise { const desired = await this.localImageId(); if (!desired) return false; - return (await this.label(containerName, DockerContainerManager.IMAGE_ID_LABEL_KEY)) !== desired; + return snapshot.imageId !== desired; } private async localImageId(): Promise { @@ -513,15 +499,8 @@ export class DockerContainerManager { } } - private async imageLeftBehind(containerName: string): Promise { - const { stdout } = await this.run([ - "inspect", - "-f", - `{{.Config.Image}}\t{{index .Config.Labels "${DockerContainerManager.IMAGE_ID_LABEL_KEY}"}}`, - containerName, - ]); - const [reference = "", label] = stdout.trim().split("\t"); - const imageId = this.normalizeDockerValue(label); + private async imageLeftBehind(snapshot: ContainerSnapshot): Promise { + const { image: reference, imageId } = snapshot; if (imageId && imageId === (await this.localImageId())) return undefined; if (reference !== this.image && isPublishedSandboxImage(reference)) return reference; if (imageId && (await this.isUnnamedImage(imageId))) return imageId; @@ -571,11 +550,6 @@ export class DockerContainerManager { await this.runContainer(containerKey, containerName, mounts, options); } - private async hasNetworkModeDrift(containerKey: string, containerName: string): Promise { - const expected = DockerContainerManager.networkName(containerKey); - return (await this.label(containerName, DockerContainerManager.NETWORK_LABEL_KEY)) !== expected; - } - private async ensureNetwork(containerKey: string): Promise { const networkName = DockerContainerManager.networkName(containerKey); const { stdout } = await this.run([ @@ -615,10 +589,32 @@ export class DockerContainerManager { return this.parseNameLines(stdout).includes(containerName); } - private async inspectStatus(containerName: string): Promise { - if (!(await this.containerExists(containerName))) return "missing"; - const { stdout } = await this.run(["inspect", "-f", "{{.State.Running}}", containerName]); - return stdout.trim() === "true" ? "running" : "stopped"; + private async inspectContainer(containerName: string): Promise { + if (!(await this.containerExists(containerName))) return undefined; + const { stdout } = await this.run([ + "inspect", + "-f", + [ + "{{.State.Running}}", + "{{.Config.Image}}", + labelField(DockerContainerManager.MOUNT_SIGNATURE_LABEL_KEY), + labelField(DockerContainerManager.NETWORK_LABEL_KEY), + labelField(DockerContainerManager.IMAGE_ID_LABEL_KEY), + "{{json .Mounts}}", + ].join("\t"), + containerName, + ]); + const [running, image = "", mountSignature, network, imageId, ...mounts] = stdout + .trim() + .split("\t"); + return { + running: running === "true", + image, + mountSignature: this.normalizeDockerValue(mountSignature), + network: this.normalizeDockerValue(network), + imageId: this.normalizeDockerValue(imageId), + mountsJson: mounts.join("\t"), + }; } private async listContainerNamesByLabel(): Promise { @@ -705,6 +701,10 @@ export class DockerContainerManager { } } +function labelField(key: string): string { + return `{{index .Config.Labels "${key}"}}`; +} + const CPU_PERIOD_MICROSECONDS = 100_000; function cpuQuotaArgs(cpus: string): string[] { diff --git a/src/test/provisioner.test.ts b/src/test/provisioner.test.ts index 7a64ff7b..e5513712 100644 --- a/src/test/provisioner.test.ts +++ b/src/test/provisioner.test.ts @@ -161,17 +161,22 @@ class FakeEngine { Propagation: "rprivate", })), ); - if (template === "{{.State.Running}}") return String(container.running); - if (template.startsWith("{{.Config.Image}}\t")) { - return `${container.image ?? ""}\t${container.labels["mikan.image-id"] ?? ""}`; - } - if (template === "{{json .Mounts}}") return mounts; - if (template.startsWith("{{.State.Running}}\t")) { - return `${container.running}\t${container.startedAt}\t${mounts}`; - } - const label = /index \.Config\.Labels "([^"]+)"/.exec(template)?.[1]; - if (label) return container.labels[label] ?? ""; - throw new Error(`unexpected inspect template ${template}`); + const fields: Record = { + "{{.State.Running}}": String(container.running), + "{{.State.StartedAt}}": container.startedAt, + "{{.Config.Image}}": container.image ?? "", + "{{json .Mounts}}": mounts, + }; + return template + .split("\t") + .map((field) => { + const label = /^\{\{index \.Config\.Labels "([^"]+)"\}\}$/.exec(field)?.[1]; + if (label) return container.labels[label] ?? ""; + const value = fields[field]; + if (value === undefined) throw new Error(`unexpected inspect field ${field}`); + return value; + }) + .join("\t"); } private network(args: string[]): string {