diff --git a/.changeset/puny-houses-arrive.md b/.changeset/puny-houses-arrive.md new file mode 100644 index 00000000..1ca13ea0 --- /dev/null +++ b/.changeset/puny-houses-arrive.md @@ -0,0 +1,5 @@ +--- +'@primer/agent-eval': patch +--- + +Prebuild and reuse a local sandbox image from the configured `--docker-image` base, and remove active sandbox containers when an evaluation process receives SIGINT or SIGTERM. diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index f762a1ab..4dc419e7 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -11,8 +11,8 @@ on: type: string docker-image: description: >- - Docker container image to use for running trials. - Must be a Debian-based Node image with apt-get and a node user (e.g. node:26.5.0-slim). + Docker base image to layer the trial environment on. + Must be a Debian-based Node image with npm, apt-get, and a node user. required: false default: 'node:26.5.0-slim' type: string diff --git a/.github/workflows/experiment.yml b/.github/workflows/experiment.yml index 43cb8956..bb8ad426 100644 --- a/.github/workflows/experiment.yml +++ b/.github/workflows/experiment.yml @@ -13,8 +13,8 @@ on: type: string docker-image: description: >- - Docker container image to use for running treatments. - Must be a Debian-based Node image with apt-get and a node user (e.g. node:26.5.0-slim). + Docker base image to layer the treatment environment on. + Must be a Debian-based Node image with npm, apt-get, and a node user. required: false default: 'node:26.5.0-slim' type: string diff --git a/packages/agent-eval/src/cli.ts b/packages/agent-eval/src/cli.ts index dfe91ac6..95b2b072 100644 --- a/packages/agent-eval/src/cli.ts +++ b/packages/agent-eval/src/cli.ts @@ -40,7 +40,7 @@ const {values} = parseArgs({ 'docker-image': { type: 'string', description: - 'The Docker container image to use for running treatments (must be a Debian-based Node image with apt-get and a node user, e.g. node:26.5.0-slim)', + 'The Docker base image to layer the treatment environment on (must be a Debian-based Node image with npm, apt-get, and a node user, default: node:26.5.0-slim)', }, experiment: { type: 'string', @@ -87,7 +87,7 @@ Options: -b, --benchmark The file name of the benchmark to run --benchmarks The directory containing local benchmark files (default: ./benchmarks) -c, --concurrency The number of treatments to run in parallel - --docker-image The Docker container image to use for running treatments (must be a Debian-based Node image with apt-get and a node user, e.g. node:26.5.0-slim) + --docker-image The Docker base image to layer the treatment environment on (must be a Debian-based Node image with npm, apt-get, and a node user; default: node:26.5.0-slim) -e, --experiment The file name of the experiment to run --experiments The directory containing local experiment files (default: ./experiments) -h, --help Learn more about the command and its options diff --git a/packages/agent-eval/src/sandbox/system.test.ts b/packages/agent-eval/src/sandbox/system.test.ts index 238ed33c..cafee00a 100644 --- a/packages/agent-eval/src/sandbox/system.test.ts +++ b/packages/agent-eval/src/sandbox/system.test.ts @@ -2,10 +2,17 @@ import Docker from 'dockerode' import {beforeEach, describe, expect, test, vi} from 'vitest' import {VirtualHost} from '../host' import {MCP_CONFIG_PATH, NODE_USER, SKILLS_DIR} from './constants' -import {createContainer, SandboxSchema, SystemSandbox} from './system' +import { + buildDockerImage, + cleanupActiveContainers, + createContainer, + getDockerImageName, + SandboxSchema, + SystemSandbox, +} from './system' import {VirtualSandbox} from './virtual' -function createSandbox(container = {remove: vi.fn()}) { +function createSandbox(container = {remove: vi.fn().mockResolvedValue(undefined)}) { // @ts-expect-error This test only exercises methods whose container operations are mocked. return new SystemSandbox(VirtualHost.create(), new Docker(), container) } @@ -24,9 +31,47 @@ describe('SandboxSchema', () => { }) describe('SystemSandbox lifecycle', () => { + test('builds the local sandbox image', async () => { + const stream = {} + const docker = { + buildImage: vi.fn().mockResolvedValue(stream), + modem: { + followProgress: vi.fn((_stream: unknown, onFinished: (error: Error | null) => void) => { + onFinished(null) + }), + }, + } + + // @ts-expect-error This test only exercises the Docker methods used to build the image. + const image = await buildDockerImage(docker, 'custom-node:local') + + expect(docker.buildImage).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + buildargs: { + BASE_IMAGE: 'custom-node:local', + COPILOT_CLI_VERSION: '1.0.82', + NPM_VERSION: '12.0.2', + }, + dockerfile: 'Dockerfile', + t: expect.stringMatching(/^agent-eval-sandbox:[a-f0-9]{16}$/), + target: 'sandbox', + }), + ) + expect(docker.modem.followProgress).toHaveBeenCalledWith(stream, expect.any(Function)) + expect(image).toMatch(/^agent-eval-sandbox:[a-f0-9]{16}$/) + }) + + test('includes the Dockerfile contents in the local image tag', () => { + const firstImage = getDockerImageName('custom-node:local', 'FROM custom-node:local\nRUN echo first') + const secondImage = getDockerImageName('custom-node:local', 'FROM custom-node:local\nRUN echo second') + + expect(firstImage).not.toBe(secondImage) + }) + test('force removes the container when disposed', async () => { const container = { - remove: vi.fn(), + remove: vi.fn().mockResolvedValue(undefined), } const sandbox = createSandbox(container) @@ -43,20 +88,63 @@ describe('SystemSandbox lifecycle', () => { } const docker = { createContainer: vi.fn().mockResolvedValue(container), - pull: vi.fn((_name: string, callback: (error: Error | null, stream: NodeJS.ReadableStream) => void) => { - callback(null, {} as NodeJS.ReadableStream) - }), - modem: { - followProgress: vi.fn((_stream: NodeJS.ReadableStream, onFinished: (error: Error | null) => void) => { - onFinished(null) - }), - }, } // @ts-expect-error This test only exercises the Docker methods used before container initialization. await expect(createContainer(docker, 'test-image')).rejects.toBe(initializationError) expect(container.remove).toHaveBeenCalledWith({force: true}) }) + + test('removes active containers when the process is terminated', async () => { + const container = { + start: vi.fn(), + remove: vi.fn().mockResolvedValue(undefined), + } + const docker = { + createContainer: vi.fn().mockResolvedValue(container), + } + const once = vi.spyOn(process, 'once') + const off = vi.spyOn(process, 'off') + + // @ts-expect-error This test only exercises the Docker methods used to create and remove the container. + const initializedContainer = await createContainer(docker, 'test-image') + + expect(once).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(once).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + + await cleanupActiveContainers() + + expect(container.remove).toHaveBeenCalledWith({force: true}) + expect(off).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(off).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + + const sandbox = new SystemSandbox(VirtualHost.create(), new Docker(), initializedContainer) + await sandbox[Symbol.asyncDispose]() + + expect(container.remove).toHaveBeenCalledTimes(1) + }) + + test('untracks containers that Docker already removed', async () => { + const notFoundError = Object.assign(new Error('No such container'), {statusCode: 404}) + const container = { + start: vi.fn(), + remove: vi.fn().mockRejectedValue(notFoundError), + } + const docker = { + createContainer: vi.fn().mockResolvedValue(container), + } + const off = vi.spyOn(process, 'off') + + // @ts-expect-error This test only exercises the Docker methods used to create and remove the container. + const initializedContainer = await createContainer(docker, 'test-image') + const sandbox = new SystemSandbox(VirtualHost.create(), new Docker(), initializedContainer) + + await expect(sandbox[Symbol.asyncDispose]()).resolves.toBeUndefined() + + expect(off).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(off).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + await expect(cleanupActiveContainers()).resolves.toBeUndefined() + }) }) describe('SystemSandbox configuration helpers', () => { diff --git a/packages/agent-eval/src/sandbox/system.ts b/packages/agent-eval/src/sandbox/system.ts index 6db12ba4..2986fb19 100644 --- a/packages/agent-eval/src/sandbox/system.ts +++ b/packages/agent-eval/src/sandbox/system.ts @@ -1,4 +1,4 @@ -import {randomUUID} from 'node:crypto' +import {createHash, randomUUID} from 'node:crypto' import path from 'node:path' import {pipeline} from 'node:stream/promises' import Docker from 'dockerode' @@ -10,9 +10,7 @@ import {McpConfigFileSchema} from '../mcp-config' import type {McpConfigFile} from '../mcp-config' import { AGENT_INSTRUCTIONS_PATH, - AGENTS_DIR, CONTAINER_WORKDIR, - COPILOT_DIR, COPILOT_PLUGIN_SOURCES_DIR, CUSTOM_AGENTS_DIR, MCP_CONFIG_PATH, @@ -47,6 +45,44 @@ import {createCapturedStream} from './captured-stream' const COPILOT_CLI_VERSION = '1.0.82' const NPM_VERSION = '12.0.2' +const DOCKERFILE = `ARG BASE_IMAGE=node:26.5.0-slim + +FROM \${BASE_IMAGE} AS base + +ARG NPM_VERSION +ARG COPILOT_CLI_VERSION + +RUN apt-get update \\ + && apt-get install -y --no-install-recommends ca-certificates chromium curl \\ + && rm -rf /var/lib/apt/lists/* + +RUN npm install --global "npm@\${NPM_VERSION}" + +RUN mkdir -p \\ + /home/sandbox/workspace \\ + /home/node/.npm-global \\ + /home/node/.copilot/agents \\ + /home/node/.agents/skills \\ + && chown -R node:node \\ + /home/sandbox \\ + /home/node/.npm-global \\ + /home/node/.copilot \\ + /home/node/.agents + +USER node + +RUN npm config set prefix /home/node/.npm-global \\ + && npm install --global "@github/copilot@\${COPILOT_CLI_VERSION}" \\ + && printf '%s\\n' '{"mcpServers":{}}' > /home/node/.copilot/mcp-config.json + +ENV PATH="/home/node/.npm-global/bin:\${PATH}" + +FROM base AS sandbox + +WORKDIR /home/sandbox/workspace + +CMD ["sleep", "infinity"] +` const DEFAULT_MCP_CONFIG: McpConfigFile = { mcpServers: {}, @@ -55,7 +91,8 @@ const DEFAULT_MCP_CONFIG: McpConfigFile = { class SystemSandbox implements Sandbox { static async create(options: SandboxCreateOptions = {}) { const docker = new Docker() - const dockerImage = options.dockerImage?.trim() || DEFAULT_DOCKER_IMAGE + const baseDockerImage = options.dockerImage?.trim() || DEFAULT_DOCKER_IMAGE + const dockerImage = await ensureDockerImage(docker, baseDockerImage) const container = await createContainer(docker, dockerImage) return new SystemSandbox(options.host ?? DefaultHost, docker, container) } @@ -71,7 +108,7 @@ class SystemSandbox implements Sandbox { } async [Symbol.asyncDispose]() { - await this.#container.remove({force: true}) + await removeContainer(this.#container) } async copy(sourcePath: string, destinationPath: string, options: CopyOptions = {}): Promise { @@ -317,14 +354,96 @@ class SystemSandbox implements Sandbox { const INITIALIZED_CONTAINER: unique symbol = Symbol('InitializedContainer') const DEFAULT_DOCKER_IMAGE = 'node:26.5.0-slim' +const activeContainers = new Set() +const containerRemovals = new WeakMap>() +const removedContainers = new WeakSet() +const dockerImageBuilds = new Map>() +let terminationCleanup: Promise | undefined + +type TerminationSignal = 'SIGINT' | 'SIGTERM' + +const terminationHandlers: Record void> = { + SIGINT() { + handleTermination('SIGINT') + }, + SIGTERM() { + handleTermination('SIGTERM') + }, +} type InitializedContainer = Docker.Container & { readonly [INITIALIZED_CONTAINER]?: true } -async function createContainer(docker: Docker, dockerImage: string): Promise { - await pullImage(docker, dockerImage) +async function ensureDockerImage(docker: Docker, baseDockerImage: string): Promise { + let build = dockerImageBuilds.get(baseDockerImage) + if (!build) { + build = buildDockerImage(docker, baseDockerImage).catch(error => { + dockerImageBuilds.delete(baseDockerImage) + throw error + }) + dockerImageBuilds.set(baseDockerImage, build) + } + + return build +} +async function buildDockerImage(docker: Docker, baseDockerImage: string): Promise { + const dockerImage = getDockerImageName(baseDockerImage) + logger.debug('Building sandbox image %s from %s...', dockerImage, baseDockerImage) + + const dockerfile = Buffer.from(DOCKERFILE) + const context = tarStream.pack() + context.entry( + { + name: 'Dockerfile', + size: dockerfile.byteLength, + }, + dockerfile, + ) + context.finalize() + + const stream = await docker.buildImage(context, { + buildargs: { + BASE_IMAGE: baseDockerImage, + COPILOT_CLI_VERSION, + NPM_VERSION, + }, + dockerfile: 'Dockerfile', + t: dockerImage, + target: 'sandbox', + }) + + await new Promise((resolve, reject) => { + docker.modem.followProgress(stream, error => { + if (error) { + reject(error) + return + } + + resolve() + }) + }) + + return dockerImage +} + +function getDockerImageName(baseDockerImage: string, dockerfile = DOCKERFILE): string { + const digest = createHash('sha256') + .update(baseDockerImage) + .update('\0') + .update(dockerfile) + .update('\0') + .update(NPM_VERSION) + .update('\0') + .update(COPILOT_CLI_VERSION) + .digest('hex') + .slice(0, 16) + + return `agent-eval-sandbox:${digest}` +} + +async function createContainer(docker: Docker, dockerImage: string): Promise { const container = await docker.createContainer({ Image: dockerImage, Cmd: ['sleep', 'infinity'], @@ -337,88 +456,7 @@ async function createContainer(docker: Docker, dockerImage: string): Promise ${MCP_CONFIG_PATH}`], - { - user: NODE_USER, - }, - ) - await execCommand(docker, container, 'mkdir', ['-p', CUSTOM_AGENTS_DIR], { - user: NODE_USER, - }) - - logger.debug('Setting up agents config...') - await execCommand(docker, container, 'mkdir', ['-p', AGENTS_DIR], { - user: 'root', - }) - await execCommand(docker, container, 'chown', ['-R', NODE_USER, AGENTS_DIR], { - user: 'root', - }) - + trackContainer(container) return container as InitializedContainer } catch (error) { try { @@ -432,6 +470,78 @@ async function createContainer(docker: Docker, dockerImage: string): Promise { + if (removedContainers.has(container)) { + return + } + + const activeRemoval = containerRemovals.get(container) + if (activeRemoval) { + return activeRemoval + } + + const removal = (async () => { + try { + await container.remove({force: true}) + } catch (error) { + if (!isDockerNotFoundError(error)) { + throw error + } + } + + removedContainers.add(container) + activeContainers.delete(container) + + if (activeContainers.size === 0) { + process.off('SIGINT', terminationHandlers.SIGINT) + process.off('SIGTERM', terminationHandlers.SIGTERM) + } + })().finally(() => { + containerRemovals.delete(container) + }) + containerRemovals.set(container, removal) + return removal +} + +function isDockerNotFoundError(error: unknown): boolean { + return error instanceof Error && 'statusCode' in error && error.statusCode === 404 +} + +async function cleanupActiveContainers(): Promise { + const results = await Promise.allSettled(Array.from(activeContainers, container => removeContainer(container))) + const errors = results.flatMap(result => { + if (result.status === 'rejected') { + return [result.reason] + } + + return [] + }) + + if (errors.length > 0) { + throw new AggregateError(errors, 'Failed to remove active sandbox containers') + } +} + +function handleTermination(signal: TerminationSignal): void { + terminationCleanup ??= cleanupActiveContainers() + .catch(error => { + logger.error({error}, 'Failed to clean up sandbox containers during termination') + }) + .then(() => { + process.exit(signal === 'SIGINT' ? 130 : 143) + }) +} + function mapCopiedHeader(header: Headers, sourceName: string, destinationName: string): Headers { const name = header.name === sourceName @@ -592,30 +702,6 @@ async function readFileFromArchive(archive: NodeJS.ReadableStream): Promise { - return new Promise((resolve, reject) => { - docker.pull(name, (error: Error | null, stream: NodeJS.ReadableStream) => { - if (error) { - reject(error) - return - } - - // Follow the pull progress - docker.modem.followProgress( - stream, - (progressError: Error | null) => { - if (progressError) { - reject(progressError) - } else { - resolve() - } - }, - () => {}, - ) - }) - }) -} - class CommandError extends Error { command: ReadonlyArray result: CommandResult @@ -696,4 +782,12 @@ const SandboxSchema = z.custom(value => { return value instanceof SystemSandbox || value instanceof VirtualSandbox }) -export {SandboxSchema, SystemSandbox, DEFAULT_DOCKER_IMAGE, createContainer} +export { + SandboxSchema, + SystemSandbox, + DEFAULT_DOCKER_IMAGE, + buildDockerImage, + cleanupActiveContainers, + createContainer, + getDockerImageName, +} diff --git a/packages/agent-eval/src/trial.ts b/packages/agent-eval/src/trial.ts index 47e876fc..b06bdf81 100644 --- a/packages/agent-eval/src/trial.ts +++ b/packages/agent-eval/src/trial.ts @@ -452,9 +452,6 @@ async function run({ const WALKTHROUGH_VIEWPORT_WIDTH = 1440 const WALKTHROUGH_VIEWPORT_HEIGHT = 900 logger.debug('%s Capturing walkthrough...', logPrefix) - await sandbox.runCommand('apt-get', ['install', '-y', 'chromium'], { - user: 'root', - }) await sandbox.runCommand('npm', ['install', '-g', '--allow-scripts=agent-browser', 'agent-browser'], { user: NODE_USER, }) diff --git a/script/run-benchmark.sh b/script/run-benchmark.sh index 687d7486..4f6b0da1 100755 --- a/script/run-benchmark.sh +++ b/script/run-benchmark.sh @@ -21,5 +21,6 @@ node "$repository_root/packages/agent-eval/bin/agent-eval" \ --benchmark "$benchmark_name" \ --benchmarks "$repository_root/benchmarks" \ --concurrency "${CONCURRENCY:-1}" \ + --docker-image "${DOCKER_IMAGE:-node:26.5.0-slim}" \ --output-dir "$run_directory" \ --scenarios "$repository_root/scenarios"