Skip to content

Commit c0b8bf0

Browse files
mchwangclaude
andauthored
Give the network, gate and workspace Docker tests per-run isolation (#188)
* Give the network, gate and workspace Docker tests per-run isolation Each file now has its own random runner owner and removes what that owner still holds after its ordinary cleanup, as #185 did for the supervisor and container tests. - runner-workspace: the path identity probe test counted probe containers and probe-option volumes across the whole daemon, so a leftover probe container or another runner's probe failed it. It now follows only this probe's container, by the name a PATH shim records (the shim runs the real docker unchanged), and the volumes Docker's events show mounted in it. - agent-gate: broken-isolation containers carry the runner label, and the client is killed with SIGKILL at its timeout. SIGTERM was passed to the container's shell, which ignores it, so the client waited for the container to exit. - agent-network: network and proxy names are random, so the fixed attempt IDs reach only labels; the per-run owner and sweep are preventive. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Fail a gate case whose probe client is killed at its timeout A SIGKILLed client has no exit status, which the cases' not.toBe(0) checks accepted, so a probe that hung would have passed. Found in independent review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent a56bbdc commit c0b8bf0

3 files changed

Lines changed: 106 additions & 30 deletions

File tree

‎test/agent-gate.test.ts‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,14 @@
11
import { spawnSync } from 'node:child_process';
2-
import { beforeAll, describe, expect, it } from 'vitest';
2+
import { randomBytes } from 'node:crypto';
3+
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
34
import { captureInvocation, type Phase } from '../agents/contract.ts';
45
import { buildAgentImage } from '../agents/container/image.ts';
6+
import { RUNNER_LABEL } from '../agents/labels.ts';
57
import { createIsolationProbeCommand, createPhasePolicy, type IsolationProbe } from '../agents/policy.ts';
6-
const TEST_RUNNER_OWNER = '0123456789abcdef0123456789abcdef';
8+
import { removeRunnerObjects } from './fixtures/docker-sweep.ts';
9+
// Each run has its own runner owner, and its probe containers carry it, so the sweep after this file removes this
10+
// run's leftovers and never another run's.
11+
const TEST_RUNNER_OWNER = randomBytes(16).toString('hex');
712

813
// The gate is only meaningful if its probes fail when isolation breaks. Each case runs the exact production probe
914
// script in a container deliberately built without one protection, bypassing the profile and its validator, and
@@ -25,18 +30,29 @@ const probeScript = (phase: Phase, probe: IsolationProbe) => {
2530
// A committed repository whose metadata lives on its own filesystem, as in a real task container.
2631
const seedRepository = 'git init -q /work && git -C /work -c user.name=gate -c user.email=gate@example.com '
2732
+ 'commit -q --allow-empty -m seed';
28-
const runBroken = (mounts: readonly string[], script: string, env: readonly string[] = []) => spawnSync('docker', ['run',
29-
'--rm', '--network=none', '--user', '10001:10001', '--env', 'HOME=/home/codeboost', ...env, '--workdir', '/work', ...mounts,
30-
...(mounts.some(mount => mount.startsWith('/home/codeboost:')) ? []
31-
: ['--tmpfs', '/home/codeboost:rw,size=1048576,nr_inodes=128,uid=10001,gid=10001,mode=0700']),
32-
'--entrypoint', 'sh', imageId, '-c', `${seedRepository} && ${script}`],
33-
{ encoding: 'utf8', timeout: 120_000, stdio: ['ignore', 'pipe', 'pipe'] });
33+
// A client killed by SIGTERM passes the signal to the container's shell, which ignores it, and waits; SIGKILL ends the
34+
// client at the timeout instead, and leaves the container to the sweep, which finds it by its runner label. A killed
35+
// client has no exit status, which the cases' `not.toBe(0)` checks would accept, so it throws: a hung probe fails.
36+
const runBroken = (mounts: readonly string[], script: string, env: readonly string[] = []) => {
37+
const result = spawnSync('docker', ['run', '--rm', '--label', `${RUNNER_LABEL}=${TEST_RUNNER_OWNER}`,
38+
'--network=none', '--user', '10001:10001', '--env', 'HOME=/home/codeboost', ...env, '--workdir', '/work', ...mounts,
39+
...(mounts.some(mount => mount.startsWith('/home/codeboost:')) ? []
40+
: ['--tmpfs', '/home/codeboost:rw,size=1048576,nr_inodes=128,uid=10001,gid=10001,mode=0700']),
41+
'--entrypoint', 'sh', imageId, '-c', `${seedRepository} && ${script}`],
42+
{ encoding: 'utf8', timeout: 120_000, killSignal: 'SIGKILL', stdio: ['ignore', 'pipe', 'pipe'] });
43+
if (result.error || result.status === null)
44+
throw new Error(`docker run did not exit: ${result.error?.message ?? `signal ${result.signal}`}`,
45+
{ cause: result.error });
46+
return result;
47+
};
3448
const tmpfs = (target: string, options: string) => ['--tmpfs', `${target}:rw,uid=10001,gid=10001,${options}`];
3549
const bounded = tmpfs('/work', 'size=16m,nr_inodes=512');
3650
const writableMetadata = tmpfs('/work/.git', 'size=16m,nr_inodes=512');
3751
const boundedScratch = tmpfs('/tmp', 'size=32m,nr_inodes=4096');
3852

3953
beforeAll(() => { imageId = buildAgentImage(); }, 10 * 60_000);
54+
// The file has no other cleanup: a probe container whose client timed out may still be running.
55+
afterAll(() => removeRunnerObjects(TEST_RUNNER_OWNER), 4 * 60_000);
4056

4157
describe('isolation gate detects breaches', () => {
4258
it.each([

‎test/agent-network.test.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { execFileSync, spawnSync } from 'node:child_process';
2-
import { randomUUID } from 'node:crypto';
2+
import { randomBytes, randomUUID } from 'node:crypto';
33
import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
44
import { tmpdir } from 'node:os';
55
import { join } from 'node:path';
@@ -9,7 +9,10 @@ import { assertVendorNetwork, createVendorNetwork, removeVendorNetwork, VENDOR_H
99
VendorNetworkCreationCleanupError,
1010
type VendorNetwork } from '../agents/network/network.ts';
1111
import { captureInvocation, type InvocationInput } from '../agents/contract.ts';
12-
const TEST_RUNNER_OWNER = '0123456789abcdef0123456789abcdef';
12+
import { removeRunnerObjects } from './fixtures/docker-sweep.ts';
13+
// Each run has its own runner owner, so the sweep after this file removes only what this run left behind. Network and
14+
// proxy names are random, so the fixed attempt IDs below reach only labels, where this owner tells runs apart.
15+
const TEST_RUNNER_OWNER = randomBytes(16).toString('hex');
1316

1417
let imageId = '', network: VendorNetwork, invocation: InvocationInput;
1518
const docker = (...args: string[]) => execFileSync('docker', args, {
@@ -31,7 +34,15 @@ beforeAll(async () => {
3134
network = await createVendorNetwork(invocation, imageId, randomUUID());
3235
}, 10 * 60_000);
3336
// If setup failed there is no network, and a teardown error would hide the setup failure.
34-
afterAll(async () => { if (network) await removeVendorNetwork(network); }, 60_000);
37+
afterAll(async () => {
38+
try { if (network) await removeVendorNetwork(network); }
39+
finally {
40+
// Then remove whatever this run still owns, even when a test failed or the removal above threw. Objects a test
41+
// makes without the runner label are not found here; those tests remove their own. It may not run before a hook
42+
// timeout.
43+
removeRunnerObjects(TEST_RUNNER_OWNER);
44+
}
45+
}, 4 * 60_000);
3546

3647
describe('vendor-only egress', () => {
3748
it('keeps failed allocation and its cleanup inside the caller deadline', async () => {

‎test/runner-workspace.test.ts‎

Lines changed: 68 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
import { randomUUID } from 'node:crypto';
2-
import { execFile, execFileSync } from 'node:child_process';
3-
import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs';
1+
import { randomBytes, randomUUID } from 'node:crypto';
2+
import { execFile, execFileSync, spawn } from 'node:child_process';
3+
import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
44
import { tmpdir } from 'node:os';
55
import { join } from 'node:path';
66
import { promisify } from 'node:util';
@@ -14,11 +14,13 @@ import { attemptRef, openRunnerRepository } from '../runner/runner-repository.ts
1414
import { ReviewService } from '../runner/review.ts';
1515
import { Store } from '../runner/store.ts';
1616
import { createTaskWorkspace } from '../runner/workspace.ts';
17+
import { removeRunnerObjects } from './fixtures/docker-sweep.ts';
1718
import { fixtureGit as git } from './fixtures/git.ts';
1819

1920
// F2b against the real lane D (#87): each item runs in fresh task storage cloned from the runner-owned repository, an
2021
// agent edits the work volume, and the runner's audited commit is taken into that repository for the next item.
21-
const RUNNER_OWNER = '0123456789abcdef0123456789abcdef';
22+
// Each run has its own runner owner, so the sweep after this file removes only what this run left behind.
23+
const RUNNER_OWNER = randomBytes(16).toString('hex');
2224
const identity = { repositoryId: 'repo', taskId: 'task', planId: 'plan' };
2325
const plan: Plan = { schema_version: 1, issue: 1, revision: 1, summary: 'Two items', questions: [], items: [
2426
{ id: 'P1', title: 'First', intent: 'Change a', files: [{ path: 'a.ts', kind: 'edit', renamed_from: null, change: 'x' }], acceptance: [{ type: 'check', text: 'a' }], depends_on: [] },
@@ -29,9 +31,15 @@ let imageId = '';
2931
const roots: string[] = [], stores: Store[] = [];
3032
beforeAll(() => { imageId = buildAgentImage(); }, 10 * 60_000);
3133
afterAll(() => {
32-
for (const store of stores) store.close();
33-
for (const root of roots) rmSync(root, { recursive: true, force: true });
34-
});
34+
try {
35+
for (const store of stores) store.close();
36+
for (const root of roots) rmSync(root, { recursive: true, force: true });
37+
} finally {
38+
// Then remove the task storage this run still owns, even when a test failed or the cleanup above threw. The path
39+
// identity probe carries no owner labels, so it is not found here. It may not run before a hook timeout.
40+
removeRunnerObjects(RUNNER_OWNER);
41+
}
42+
}, 4 * 60_000);
3543
const docker = (...args: string[]) => execFileSync('docker', args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
3644

3745
async function setup(agent: Record<string, string>) {
@@ -148,22 +156,63 @@ describe('real task workspace (#87)', () => {
148156
});
149157

150158
describe('task storage path identity (#162)', () => {
151-
// Only volumes made with the probe's own tmpfs options count, so other users of the daemon do not change the result.
152-
// Precondition: no other probe runs on this daemon during the test (another runner's startup, or this file run from
153-
// another checkout); its short-lived container and volume would match too. The docker project runs files one at a
154-
// time, and no other file probes.
155-
const probeVolumes = () => new Set(docker('volume', 'ls', '-q').split('\n').filter(Boolean).filter(volume => {
156-
try { return docker('volume', 'inspect', '--format', '{{json .Options}}', volume).includes('size=1048576,nr_inodes=64,'); }
157-
catch { return false; } // removed between the listing and the inspect
158-
}));
159+
// The probe names its container itself and gives it an anonymous volume, with no labels. This test finds both from
160+
// the name the probe gave: a shim on PATH records it, and the daemon's events give that container's ID and the
161+
// volumes mounted in it. Another probe on the daemon (another runner's startup, or this file run from another
162+
// checkout) or a container a dead runner left behind does not count.
163+
async function watchedProbe() {
164+
const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')); roots.push(shim);
165+
const names = join(shim, 'names'), realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim();
166+
// Not a fake: it records the argument after each separate `--name` (the form the probe uses; if that changed, no name
167+
// is recorded and the test fails) and runs the real docker with the same arguments, so every exit status, stdout
168+
// and stderr the probe reads is Docker's own.
169+
writeFileSync(join(shim, 'docker'), ['#!/bin/sh', 'prev=',
170+
`for arg; do if [ "$prev" = --name ]; then printf '%s\\n' "$arg" >> '${names}'; fi; prev="$arg"; done`,
171+
`exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 });
172+
// --since replays from the daemon's event buffer, so nothing between this call and the stream's start is lost. The
173+
// minute of margin covers a daemon clock behind this one; it is harmless, as events are matched by the probe's own
174+
// name and IDs.
175+
const events: { Type: string; Action: string; Actor: { ID: string; Attributes: Record<string, string> } }[] = [];
176+
const stream = spawn(realDocker, ['events', '--since', String(Math.floor(Date.now() / 1000) - 60), '--format',
177+
'{{json .}}', '--filter', 'type=container', '--filter', 'type=volume'], { stdio: ['ignore', 'pipe', 'inherit'] });
178+
let buffered = '';
179+
stream.stdout.setEncoding('utf8').on('data', (chunk: string) => {
180+
const lines = (buffered + chunk).split('\n'); buffered = lines.pop()!;
181+
for (const line of lines) if (line) events.push(JSON.parse(line));
182+
});
183+
const path = process.env.PATH;
184+
process.env.PATH = `${shim}:${path}`;
185+
try {
186+
const identity = await probeTaskStoragePathIdentity(imageId);
187+
const recorded = readFileSync(names, 'utf8').split('\n').filter(Boolean);
188+
expect(recorded).toEqual([expect.stringMatching(/^codeboost-path-identity-[0-9a-f-]{36}$/)]);
189+
const container = () => events.find(event => event.Type === 'container' && event.Action === 'create'
190+
&& event.Actor.Attributes.name === recorded[0])?.Actor.ID;
191+
const volumes = () => [...new Set(events.filter(event => event.Type === 'volume' && event.Action === 'mount'
192+
&& event.Actor.Attributes.container === container()).map(event => event.Actor.ID))];
193+
// Wait for the daemon to report the container and its volumes destroyed, for up to 10 s; absence is still
194+
// proved by inspect below, so a destroy that never comes fails there.
195+
const destroyed = (type: string, id: string | undefined) =>
196+
events.some(event => event.Type === type && event.Action === 'destroy' && event.Actor.ID === id);
197+
for (const until = performance.now() + 10_000; performance.now() < until;) {
198+
if (container() && volumes().length && destroyed('container', container())
199+
&& volumes().every(volume => destroyed('volume', volume))) break;
200+
await new Promise(resolve => setTimeout(resolve, 100));
201+
}
202+
return { identity, container: container(), volumes: volumes() };
203+
} finally { process.env.PATH = path; stream.kill(); }
204+
}
159205
it('measures the tmpfs the agent edits, whatever the host folds, and leaves no container or volume behind', async () => {
160-
const before = probeVolumes();
206+
const probe = await watchedProbe();
161207
// Task storage is a tmpfs in the daemon's Linux kernel: case-sensitive, and names are compared as bytes. A macOS
162208
// host folder would report the reverse, which is why the probe does not read the host.
163-
expect(await probeTaskStoragePathIdentity(imageId)).toEqual({ caseSensitive: true, unicodeNormalization: 'none' });
209+
expect(probe.identity).toEqual({ caseSensitive: true, unicodeNormalization: 'none' });
210+
// The events found the probe's container and its one volume, so the absence checks below look at real objects.
211+
expect(probe.container).toMatch(/^[0-9a-f]{64}$/);
212+
expect(probe.volumes).toEqual([expect.stringMatching(/^[0-9a-f]{64}$/)]);
164213
// In any state: a container created but never started would outlive --rm.
165-
expect(docker('ps', '-aq', '--filter', 'name=codeboost-path-identity-')).toBe('');
166-
expect([...probeVolumes()].filter(volume => !before.has(volume))).toEqual([]);
214+
expect(() => docker('container', 'inspect', probe.container!)).toThrow(/No such container/);
215+
expect(() => docker('volume', 'inspect', probe.volumes[0]!)).toThrow(/no such volume/i);
167216
}, 120_000);
168217
it('removes a never-started probe container and its volume with the cleanup\'s own rm', () => {
169218
// The failure path's claim: a container created but never started (a killed client) loses its anonymous volume to

0 commit comments

Comments
 (0)