diff --git a/.github/workflows/agent-isolation.yml b/.github/workflows/agent-isolation.yml index 838004a6..20a6517f 100644 --- a/.github/workflows/agent-isolation.yml +++ b/.github/workflows/agent-isolation.yml @@ -9,7 +9,9 @@ permissions: jobs: real-docker: runs-on: ubuntu-latest - # Covers the 10 min image build, 2 min teardown and the per-test Docker budgets. + # Covers the typecheck, the ordinary Vitest project, the 10 min image build and ordinary run times of the + # Docker-backed files. It does not cover every test and hook at its own Vitest limit: the five afterAll hooks that + # call the shared sweep allow 46 min between them (#189), which only a stalled daemon reaches. timeout-minutes: 45 steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/live-probes.yml b/.github/workflows/live-probes.yml index 31728b85..515da1e6 100644 --- a/.github/workflows/live-probes.yml +++ b/.github/workflows/live-probes.yml @@ -13,13 +13,14 @@ concurrency: jobs: live-probes: runs-on: ubuntu-latest - # The probes' own time limits total 33 min (6, 8, 8 and 11). agent-container and agent-supervisor may each spend - # up to 10 min building the image before their probes; agent-question builds inside its probe's own 11 min. The - # teardown hooks have Vitest limits that total 9 min: agent-container 2 after each test and 2 at the end, - # agent-supervisor 3 and agent-question 2. That is 62 min, plus setup. These are only Vitest's limits: the afterAll - # hooks in agent-container and agent-question call docker through spawnSync with no timeout of their own, and - # Vitest's hook timer cannot fire while such a call blocks. The 75 min job limit caps the run. - timeout-minutes: 75 + # The probes' own time limits total 44 min (6, 8, 8, 11 and 11). agent-container and agent-supervisor may each spend + # up to 10 min building the image before their probes; agent-question and agent-planning build inside their probes' + # own 11 min. The teardown hooks have Vitest limits that total 26 min: agent-container 2 after each test and 13 at + # the end (two sweeps of up to 5.5 min each, #189), agent-supervisor 7, agent-question 2 and agent-planning 2. That + # is 90 min, plus setup. These are only Vitest's limits: Vitest's hook timer cannot fire while a synchronous docker + # call blocks, and some calls in these hooks have no timeout of their own (the sweeps' calls have 30 s each). The + # 100 min job limit caps the run. + timeout-minutes: 100 steps: # Fail at once when the secret is missing, so a missing secret never looks like a passing run. - name: Require the Claude token diff --git a/test/agent-container.test.ts b/test/agent-container.test.ts index 660c1547..b468a173 100644 --- a/test/agent-container.test.ts +++ b/test/agent-container.test.ts @@ -1,5 +1,5 @@ import { execFileSync, spawnSync } from 'node:child_process'; -import { removeRunnerObjects } from './fixtures/docker-sweep.ts'; +import { CLIENT_LIMIT, objectExists, removeByName, removeLabelledObjects, removeRunnerObjects, requireRealDocker, TEST_FILE_LABEL, TEST_RUN_LABEL } from './fixtures/docker-sweep.ts'; import { fixtureGit } from './fixtures/git.ts'; import { createHash, randomBytes, randomUUID } from 'node:crypto'; import { chmodSync, existsSync, mkdtempSync, mkdirSync, readdirSync, readFileSync, realpathSync, renameSync, rmSync, statSync, @@ -7,7 +7,7 @@ import { chmodSync, existsSync, mkdtempSync, mkdirSync, readdirSync, readFileSyn import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { pathToFileURL } from 'node:url'; -import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { afterAll, afterEach, beforeAll, describe, expect, it, onTestFinished, vi } from 'vitest'; import { captureInvocation, type InvocationInput, type Phase } from '../agents/contract.ts'; import { AGENT_IMAGE, assertBuiltAgentImage, buildAgentImage } from '../agents/container/image.ts'; import { assertContainerProfile, createContainerProfile, disposeContainerProfile, @@ -15,7 +15,7 @@ import { assertContainerProfile, createContainerProfile, disposeContainerProfile import { createValidatedContainer, disposeValidatedContainer, prepareTaskFilesystems, removeTaskFilesystems, runContainer, startValidatedContainer, hasExactOptions, keepsNoDaemonLog, validateContainer } from '../agents/container/run.ts'; import { createTaskClone } from '../git/clone.ts'; -import { hasOwnerLabels } from '../agents/labels.ts'; +import { hasOwnerLabels, RUNNER_LABEL } from '../agents/labels.ts'; import { exportTaskDiff, exportTaskPaths, importTaskPaths, isRecoveredTaskStorage, prepareTaskFilesystemsAsync, taskFilesystemOwner, EXPORT_SCRIPT, UnusableRepositoryError } from '../agents/container/storage.ts'; import { checkConflictTree, checkTaskTree, commitTaskChanges, inspectTaskChanges, manifestDigest, MAXIMUM_CHANGES, MAXIMUM_NAME_BYTES, snapshotDeclaredLinks, @@ -47,6 +47,30 @@ const testOwner = (attemptId = 'fixture') => ({ runnerOwner: TEST_RUNNER_OWNER, const roots: string[] = []; const taskFilesystems: ReturnType[] = []; const containers = new Set(); +/** + * Put, beside its test-run ID, on every object the `withoutRunnerLabel` wrapper makes and on the holder test's swept + * objects (#189): afterAll sweeps it. Other label-less objects are tracked by name (`containers`, `networks`, + * `volumes`), or found as holders of this run's volumes or of networks that afterAll still finds. (Docker 27.4 removes a + * network that a created, never started container still names, so such a container is tracked by name.) + */ +const TEST_FILE_RUN = randomUUID(); +/** Networks a test makes without this run's runner label, removed by name in afterAll as `volumes` are. */ +const networks = new Set(); +/** + * Volumes a test makes without this run's runner label, removed in afterAll if the test did not remove them (it may + * have timed out). + */ +const volumes = new Set(); +/** + * For an async test that makes objects under another runner's owner, or its own random one, which no afterAll sweep + * finds, and for `withoutRunnerLabel`'s test-run ID, so they go before the next test rather than at afterAll: removes + * what carries `label=value` when the test finishes, passed, failed or timed out (a timed-out body runs + * on, and its finally may never run), with the sweep's own budget: up to eleven synchronous Docker calls of 30 s each, + * which a timer cannot cut short, so 6 minutes rather than a hook's default 10 s (#189). Not covered: an object that + * lands after this sweep has run, from a timed-out body or a killed process's Docker client. + */ +const sweepWhenFinished = (label: string, value: string) => + onTestFinished(() => removeLabelledObjects(label, value, 'onTestFinished'), 6 * 60_000); const profiles: Awaited>[] = []; let imageId = ''; const vendorNetworks: VendorNetwork[] = []; @@ -134,21 +158,32 @@ afterEach(async () => { }, 120_000); afterAll(async () => { try { - for (const container of containers) spawnSync('docker', ['rm', '--force', container], { stdio: 'ignore' }); - for (const filesystems of taskFilesystems.reverse()) removeTaskFilesystems(filesystems); - for (const profile of profiles) await disposeContainerProfile(profile); - for (const network of vendorNetworks.splice(0).reverse()) await removeVendorNetwork(network); - for (const root of roots.reverse()) { + // By exact name: some are named like the start of another object's ID. + for (const container of containers) removeByName('container', container, 'afterAll'); + for (const network of networks) removeByName('network', network, 'afterAll'); + for (const volume of volumes) removeByName('volume', volume, 'afterAll'); + // Each step, even when an earlier one fails: storage left here is found by the sweep below only by its label, and + // every failure is reported. + const failures: unknown[] = []; + const attempt = async (step: () => unknown) => { try { await step(); } catch (error) { failures.push(error); } }; + for (const filesystems of taskFilesystems.reverse()) await attempt(() => removeTaskFilesystems(filesystems)); + for (const profile of profiles) await attempt(() => disposeContainerProfile(profile)); + for (const network of vendorNetworks.splice(0).reverse()) await attempt(() => removeVendorNetwork(network)); + for (const root of roots.reverse()) await attempt(() => { chmodSync(join(root, 'input'), 0o700); rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); - } + }); + if (failures.length) throw new AggregateError(failures, 'Test cleanup failed.'); } finally { - // The runner owner is this run's own, so nothing else would find what this run leaves behind. Objects a test - // makes under another owner or without the runner label are not found here; those tests remove their own. It may - // not run before a hook timeout. + // The runner owner is this run's own, so nothing else would find what this run leaves behind. Objects a test makes + // under another owner or without the runner label are not found here, unless they hold one of this run's volumes or + // networks: those this file labels with its own run ID are found by the file-label sweep, the try above removes + // those tracked by name (`containers`, `networks`, `volumes`), and the other tests remove their own. It may not run + // before a hook timeout. Its budget covers two sweeps of up to 5.5 minutes each and the cleanup before them. + removeLabelledObjects(TEST_FILE_LABEL, TEST_FILE_RUN, 'afterAll'); removeRunnerObjects(TEST_RUNNER_OWNER); } -}, 120_000); +}, 13 * 60_000); describe('real Docker agent isolation', () => { it.each([ @@ -442,13 +477,22 @@ describe('real Docker agent isolation', () => { await expect(validateContainer(valid.name, valid)).rejects.toThrow('unexpected external mount'); docker('rm', '--force', valid.name); containers.delete(valid.name); - const rogue = `codeboost-work-${randomUUID()}`; docker('volume', 'create', rogue); + const rogue = `codeboost-work-${randomUUID()}`; try { + // Inside the try, and tracked for afterAll should the test time out: the volume has no labels, so nothing else + // finds it (#189). + volumes.add(rogue); + docker('volume', 'create', rogue); const rogueArgs = valid.args.map(value => value.replace(data.filesystems.workVolume, rogue)); - docker(...rogueArgs); containers.add(valid.name); + // Tracked before the create: should it land late, afterAll removes it before the volume it mounts. + containers.add(valid.name); docker(...rogueArgs); await expect(validateContainer(valid.name, valid)).rejects.toThrow('captured identity'); docker('rm', '--force', valid.name); containers.delete(valid.name); - } finally { spawnSync('docker', ['volume', 'rm', '--force', rogue], { stdio: 'ignore' }); } + } finally { + // The container first: a volume it mounts cannot be removed. + spawnSync('docker', ['rm', '--force', valid.name], { stdio: 'ignore', ...CLIENT_LIMIT }); + spawnSync('docker', ['volume', 'rm', '--force', rogue], { stdio: 'ignore', ...CLIENT_LIMIT }); + } }, 60_000); it('rejects cloned profiles while sealed snapshots ignore later host changes', async () => { @@ -538,7 +582,7 @@ describe('real Docker agent isolation', () => { it('releases a killed create once its settle window passes with no container', async () => { const data = fixture(), unsettled = await profile(data, 'planning', 'noop'); const shim = join(data.root, 'docker-shim'); mkdirSync(shim); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); // The create client hangs until its deadline kills it, so the daemon outcome stays unknown. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', 'if [ "$1" = create ]; then exec sleep 30; fi', `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); @@ -557,7 +601,7 @@ describe('real Docker agent isolation', () => { const data = fixture(), unsettled = await profile(data, 'planning', 'noop'); const shim = join(data.root, 'docker-shim'); mkdirSync(shim); const created = join(shim, 'created'), failed = join(shim, 'failed'); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); // The create client hangs until killed, and the first inspect after it fails for an unrelated reason, so the // create path gives up early, inside the settle window. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', @@ -583,11 +627,13 @@ describe('real Docker agent isolation', () => { const moved = `${live.name}-moved`, replacement = live.name; // Someone renames our container and puts a same-named, same-labelled container in its place. docker('rename', live.name, moved); containers.add(moved); + // Tracked before the create, so afterAll removes it even if the create's client fails after the daemon made it. + containers.add(replacement); docker('create', '--name', replacement, '--label', `io.codeboost.invocation=${live.ownershipId}`, - '--entrypoint', 'true', imageId); containers.add(replacement); + '--entrypoint', 'true', imageId); const replacementId = docker('container', 'inspect', '--format', '{{.Id}}', replacement); await disposeValidatedContainer(live); - expect(spawnSync('docker', ['container', 'inspect', originalId], { stdio: 'ignore' }).status).not.toBe(0); + expect(objectExists('container', originalId)).toBe(false); expect(docker('container', 'inspect', '--format', '{{.Id}}', replacement)).toBe(replacementId); expect(isContainerProfileAuthentic(live)).toBe(false); }, 60_000); @@ -597,13 +643,16 @@ describe('real Docker agent isolation', () => { await createValidatedContainer(live); const blocker = `codeboost-blocker-${randomUUID()}`, replacement = live.name; // Our container goes, but a foreign endpoint keeps the network busy, so the profile cleanup fails and is retried. - docker('run', '--detach', '--name', blocker, '--network', live.network.name, '--entrypoint', 'sleep', imageId, '300'); + // Tracked before it is made: it has no labels, and once its network is gone no sweep finds it (#189). containers.add(blocker); + docker('run', '--detach', '--name', blocker, '--network', live.network.name, '--entrypoint', 'sleep', imageId, '300'); await expect(disposeValidatedContainer(live)).rejects.toThrow('did not settle'); expect(isContainerProfileAuthentic(live)).toBe(true); // A same-named, same-labelled container appears before the retry. + // Tracked before the create, so afterAll removes it even if the create's client fails after the daemon made it. + containers.add(replacement); docker('create', '--name', replacement, '--label', `io.codeboost.invocation=${live.ownershipId}`, - '--entrypoint', 'true', imageId); containers.add(replacement); + '--entrypoint', 'true', imageId); const replacementId = docker('container', 'inspect', '--format', '{{.Id}}', replacement); docker('rm', '--force', blocker); containers.delete(blocker); await disposeValidatedContainer(live); @@ -617,12 +666,14 @@ describe('real Docker agent isolation', () => { const originalId = docker('container', 'inspect', '--format', '{{.Id}}', live.name); const moved = `${live.name}-moved`, replacement = live.name; docker('rename', live.name, moved); containers.add(moved); + // Tracked before the create, so afterAll removes it even if the create's client fails after the daemon made it. + containers.add(replacement); docker('create', '--name', replacement, '--label', `io.codeboost.invocation=${live.ownershipId}`, - '--entrypoint', 'true', imageId); containers.add(replacement); + '--entrypoint', 'true', imageId); expect(await startValidatedContainer(live)).toBe('stdout-marker'); // The replacement was never started, and cleanup removed only ours. expect(docker('container', 'inspect', '--format', '{{.State.StartedAt}}', replacement)).toBe('0001-01-01T00:00:00Z'); - expect(spawnSync('docker', ['container', 'inspect', originalId], { stdio: 'ignore' }).status).not.toBe(0); + expect(objectExists('container', originalId)).toBe(false); }, 60_000); it('labels every Docker object with its runner, attempt and allocation', async () => { @@ -650,17 +701,29 @@ describe('real Docker agent isolation', () => { }, 60_000); // A docker wrapper that drops our runner label from commands starting with one of `prefixes`, so the object it - // creates is genuinely mislabelled and only the owner-label checks can catch it. + // creates is genuinely mislabelled and only the owner-label checks can catch it. Production cleanup may refuse such + // an object (task storage has no captured IDs), and afterAll's runner sweep finds such a container only while it + // holds one of this run's volumes or networks, and such a volume or network never, so the wrapper labels it with a + // test-run ID and this file's run ID instead (#189). Whatever has that ID is removed when the test finishes (Vitest + // runs `onTestFinished` after a pass, a failure or a timeout; the `it.fails` pair below tests the failure), and again + // in afterAll, for a create that lands after a timed-out test's removal (its body runs on). A timed-out test can leave + // the wrapper on PATH for the rest of the file (a later shim saves the PATH that holds it, and puts it back), so a + // later test's matching create is unlabelled too and that test fails; what it makes carries this file's run ID, so + // afterAll removes it. const withoutRunnerLabel = async (prefixes: string[][], run: () => Promise | T): Promise => { + if (!prefixes.every(prefix => prefix.includes('create'))) throw new Error('Only create commands can be unlabelled.'); + const testRun = randomUUID(); + sweepWhenFinished(TEST_RUN_LABEL, testRun); const shim = mkdtempSync(join(tmpdir(), 'codeboost-unlabelled-docker-')); roots.push(shim); mkdirSync(join(shim, 'input')); // afterAll resets this path's mode - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); writeFileSync(join(shim, 'docker'), [`#!${process.execPath}`, "const { spawnSync } = require('node:child_process');", 'const args = process.argv.slice(2);', `const strip = ${JSON.stringify(prefixes)}.some(prefix => prefix.every((word, i) => args[i] === word));`, 'const out = [];', "for (let i = 0; i < args.length; i++) { if (strip && args[i] === '--label' && String(args[i + 1]).startsWith('io.codeboost.runner=')) { i++; continue; } out.push(args[i]); }", + `if (strip) out.splice(out.indexOf('create') + 1, 0, '--label', ${JSON.stringify(`${TEST_RUN_LABEL}=${testRun}`)}, '--label', ${JSON.stringify(`${TEST_FILE_LABEL}=${TEST_FILE_RUN}`)});`, `const result = spawnSync(${JSON.stringify(realDocker)}, out, { stdio: 'inherit' });`, 'process.exit(result.status ?? 1);'].join('\n'), { mode: 0o755 }); const path = process.env.PATH; @@ -688,6 +751,11 @@ describe('real Docker agent isolation', () => { it('rejects task volumes without their runner label before the agent starts', async () => { const data = await withoutRunnerLabel([['volume', 'create']], () => fixture()); + // The wrapper labelled the volume for its removal in place of the runner label it dropped. + const labels = JSON.parse(docker('volume', 'inspect', '--format', '{{json .Labels}}', data.filesystems.workVolume)) as Record; + expect(labels).not.toHaveProperty(['io.codeboost.runner']); + expect(labels).toHaveProperty([TEST_RUN_LABEL]); + expect(labels).toHaveProperty([TEST_FILE_LABEL], TEST_FILE_RUN); const live = await profile(data, 'planning', 'must-not-run'); await expect(createValidatedContainer(live)).rejects.toThrow('bounded tmpfs allocation'); refuseThenRemove(data.filesystems, [['volume', 'rm', '--force', data.filesystems.workVolume, @@ -705,7 +773,133 @@ describe('real Docker agent isolation', () => { const data = fixture(), live = await profile(data, 'planning', 'must-not-run'); await withoutRunnerLabel([['create', '--name']], () => expect(createValidatedContainer(live)).rejects.toThrow('lockdown')); - expect(spawnSync('docker', ['container', 'inspect', live.name]).status).not.toBe(0); + expect(objectExists('container', live.name)).toBe(false); + }, 60_000); + + // #189: the sweep removes what holds a swept network or volume, even a container created and never started, and + // nothing on a network or volume it does not sweep, even one whose ID starts with a swept network's name. + it('removes the containers that hold a swept network or volume, and nothing else', () => { + // The swept objects carry this file's run ID too, for afterAll, and each object is recorded by name before it is + // made, so a failure here leaves nothing behind (#189). + const run = randomUUID(), label = `${TEST_RUN_LABEL}=${run}`, fileLabel = `${TEST_FILE_LABEL}=${TEST_FILE_RUN}`, made: { kind: 'container' | 'network' | 'volume'; ref: string }[] = []; + const make = (kind: 'container' | 'network' | 'volume', name: string, ...args: string[]) => { + made.push({ kind, ref: name }); + // For afterAll too: the daemon can still create an object after its client's 60 s limit and this test's finally. + (kind === 'container' ? containers : kind === 'network' ? networks : volumes).add(name); + return docker(...(kind === 'container' ? [args[0]!, '--name', name, ...args.slice(1)] : [kind, args[0]!, ...args.slice(1), name])); + }; + const named = (what: string) => `codeboost-${what}-${randomUUID()}`; + try { + const bystanderNetwork = make('network', named('bystander'), 'create'); + const bystanderVolume = make('volume', named('bystander'), 'create'); + const bystanders = [make('container', named('bystander'), 'run', '--detach', '--network', bystanderNetwork, '--entrypoint', 'sleep', imageId, '300'), + make('container', named('bystander'), 'create', '--mount', `type=volume,source=${bystanderVolume},target=/x`, '--entrypoint', 'true', imageId)]; + const network = make('network', named('swept'), 'create', '--label', label, '--label', fileLabel); + // Named like the start of the bystander network's ID: Docker's network filter also matches that prefix. + const prefixNamed = make('network', bystanderNetwork.slice(0, 12), 'create', '--label', label, '--label', fileLabel); + const volume = make('volume', named('swept'), 'create', '--label', label, '--label', fileLabel); + // And a bystander named like the start of the swept network's ID: removing by a short ID would remove it. + const lookalike = make('network', network.slice(0, 12), 'create'); + const holders = [make('container', named('holder'), 'create', '--network', network, '--entrypoint', 'true', imageId), + make('container', named('holder'), 'run', '--detach', '--network', network, '--entrypoint', 'sleep', imageId, '300'), + make('container', named('holder'), 'create', '--mount', `type=volume,source=${volume},target=/x`, '--entrypoint', 'true', imageId)]; + // The same for containers: Docker resolves a name before an ID prefix. A holder and a swept container each have one. + const holderLookalike = make('container', holders[0]!.slice(0, 12), 'create', '--entrypoint', 'true', imageId); + const labelled = make('container', named('swept'), 'create', '--label', label, '--label', fileLabel, '--entrypoint', 'true', imageId); + const labelledLookalike = make('container', labelled.slice(0, 12), 'create', '--entrypoint', 'true', imageId); + // Unlabelled and free, for removeByName below: one of each kind, by its exact name. + const byName = [['container', named('by-name')], ['network', named('by-name')], ['volume', named('by-name')]] as const; + for (const [kind, name] of byName) + make(kind, name, 'create', ...(kind === 'container' ? ['--entrypoint', 'true', imageId] : [])); + // The traps are live on this daemon, so the checks below test something: the network filter matches the start of + // a network's ID, and a name is resolved before an ID prefix, for networks and containers alike. + expect(docker('ps', '--all', '--quiet', '--no-trunc', '--filter', `network=${bystanderNetwork.slice(0, 12)}`).split('\n')) + .toContain(bystanders[0]); + expect(docker('container', 'inspect', '--format', '{{.Id}}', holders[0]!.slice(0, 12))).toBe(holderLookalike); + expect(docker('network', 'inspect', '--format', '{{.Id}}', network.slice(0, 12))).toBe(lookalike); + // And an inspect naming a missing container exits non-zero yet prints the others, the answer the shim below gives. + const partial = spawnSync('docker', ['container', 'inspect', '--format', '{{.Id}}', holders[0]!, `codeboost-gone-${randomUUID()}`], + { encoding: 'utf8', ...CLIENT_LIMIT }); + expect(partial.status).toBe(1); + expect(partial.stdout).toContain(holders[0]); + expect(partial.stderr).toMatch(/^Error response from daemon: No such container: codeboost-gone-/m); + // The holders' inspect also names a container that does not exist, as one gone since the listing would be: Docker + // 27.4 prints the others and exits 1, and the sweep must still remove them. + const shim = mkdtempSync(join(tmpdir(), 'codeboost-sweep-docker-')); roots.push(shim); + mkdirSync(join(shim, 'input')); // afterAll resets this path's mode + const realDocker = requireRealDocker(); + // It leaves a marker, so the test knows the sweep's holder read went through it. + const marker = join(shim, 'gone-added'); + writeFileSync(join(shim, 'docker'), ['#!/bin/sh', + `if [ "$1 $2" = "container inspect" ]; then : > '${marker}'; exec '${realDocker}' "$@" codeboost-gone-${randomUUID()}; fi`, + `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); + // And it says nothing of that gone holder: Docker's own answer is not a failure. + const sweepWarn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + // Copied before the restore, which clears the calls. + let sweepCalls: unknown[][] = []; + try { + removeLabelledObjects(TEST_RUN_LABEL, run, 'test', join(shim, 'docker')); + sweepCalls = [...sweepWarn.mock.calls]; + } finally { sweepWarn.mockRestore(); } + expect(existsSync(marker), 'the sweep read its holders through the shim').toBe(true); + // Only the sweep's own lines: what it removes. Any failure or unreadable answer is a different line. + expect(sweepCalls.map(call => String(call[0])).filter(line => !/^test removes /.test(line))).toEqual([]); + expect(sweepCalls.some(call => String(call[0]).startsWith('test removes the containers that hold them: '))).toBe(true); + // afterAll removes tracked names with removeByName, as here: an object of exactly that name goes, and a name that + // is gone says nothing. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + try { + for (const [kind, name] of byName) removeByName(kind, name, 'test'); + expect(byName.filter(([kind, name]) => objectExists(kind, name))).toEqual([]); + for (const [kind, name] of byName) removeByName(kind, name, 'test'); + expect(warn).not.toHaveBeenCalled(); + } finally { warn.mockRestore(); } + // A missing name that Docker resolves to a live object's ID, as a tracked name gone since would be: removeByName + // must leave that object alone. The network has no endpoints, so Docker would remove it if asked. + expect(docker('network', 'inspect', '--format', '{{.Id}}', lookalike.slice(0, 12))).toBe(lookalike); + removeByName('network', lookalike.slice(0, 12), 'test'); + expect(docker('container', 'inspect', '--format', '{{.Id}}', bystanders[0]!.slice(0, 12))).toBe(bystanders[0]); + removeByName('container', bystanders[0]!.slice(0, 12), 'test'); + for (const holder of holders) expect(objectExists('container', holder)).toBe(false); + expect([objectExists('network', network), objectExists('network', prefixNamed), objectExists('volume', volume)]).toEqual([false, false, false]); + expect(objectExists('container', labelled)).toBe(false); + for (const bystander of [...bystanders, holderLookalike, labelledLookalike]) expect(objectExists('container', bystander)).toBe(true); + expect([objectExists('network', bystanderNetwork), objectExists('volume', bystanderVolume), objectExists('network', lookalike)]) + .toEqual([true, true, true]); + } finally { + // By exact name: some are named like the start of another object's ID. Containers first, as they hold the rest. + for (const { kind, ref } of [...made.filter(object => object.kind === 'container'), ...made.filter(object => object.kind !== 'container')]) + removeByName(kind, ref, 'finally'); + } + // The budget covers the sweep's worst case (5.5 minutes) and ordinary Docker calls, removals by name included, not + // every call at its own limit. + }, 8 * 60_000); + + // #189: these two run in order (run alone, the second fails: it checks what the first left). The first stops after + // making a keeper without its runner label, before the removal that "rejects a keeper without its runner label" + // ends with; the second checks that the keeper went when the first finished. `it.fails` passes on any failure, so the + // first records the storage only once its checks passed, just before its deliberate stop. + let stoppedEarly: ReturnType | undefined; + it.fails('stops after making a keeper without its runner label (expected to fail)', async () => { + const data = await withoutRunnerLabel([['create', '--name']], () => fixture()); + const keeper = JSON.parse(docker('container', 'inspect', data.filesystems.keeper))[0] as + { Config: { Labels: Record } }; + expect(keeper.Config.Labels).not.toHaveProperty('io.codeboost.runner'); + expect(keeper.Config.Labels).toHaveProperty(TEST_RUN_LABEL); + stoppedEarly = data; + throw new Error('This test stops before it removes the keeper.'); + }, 60_000); + + it('removed the keeper that a test without its runner label left when it stopped', () => { + expect(stoppedEarly, 'the test before did not reach its deliberate stop').toBeDefined(); + const left = stoppedEarly!.filesystems, allocationId = taskFilesystemOwner(left).allocationId; + // First, so afterAll does not refuse a keeper this test finds left: its file-label sweep removes that keeper, then + // its runner sweep the volumes. + taskFilesystems.splice(taskFilesystems.indexOf(left), 1); + expect(objectExists('container', left.keeper)).toBe(false); + // Its volumes carry the runner label, so ordinary cleanup can remove them now that no keeper holds them. + removeTaskFilesystems(left); + expect(byAllocation(allocationId)).toEqual([]); }, 60_000); it('refuses an allocation ID that still labels Docker objects, as after a restart, before creating any storage', () => { @@ -721,7 +915,7 @@ describe('real Docker agent isolation', () => { const withDockerShim = async (prefix: string[], script: string, run: () => Promise | T): Promise => { const shim = mkdtempSync(join(tmpdir(), 'codeboost-shim-docker-')); roots.push(shim); mkdirSync(join(shim, 'input')); // afterAll resets this path's mode - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); writeFileSync(join(shim, 'docker'), [`#!${process.execPath}`, "const { spawnSync } = require('node:child_process');", 'const args = process.argv.slice(2);', @@ -737,16 +931,19 @@ describe('real Docker agent isolation', () => { try { return await run(); } finally { process.env.PATH = path; } }; // After our create, another process creates a volume carrying the same allocation ID: both passed the check first. - const racer = (name: string) => [ + // Only for this allocation's create: should the test time out with the shim still on PATH, other creates pass through. + const racer = (name: string, allocationId: string) => [ 'result = run(args);', - "const allocation = args.find(arg => arg.startsWith('io.codeboost.allocation='));", - `run(['volume', 'create', '--label', 'io.codeboost.runner=${'f'.repeat(32)}', '--label', allocation, '${name}']);`, + `if (args.includes('io.codeboost.allocation=${allocationId}'))`, + ` run(['volume', 'create', '--label', 'io.codeboost.runner=${'f'.repeat(32)}', '--label', 'io.codeboost.allocation=${allocationId}', '${name}']);`, ].join('\n'); it('backs out of task storage when a concurrent process claims the same allocation ID', async () => { const data = fixture(), owner = testOwner('storage-race'), rival = `codeboost-race-${randomUUID()}`; + // Tracked for afterAll should the test time out: the rival carries another runner's label (#189). + volumes.add(rival); try { - await withDockerShim(['volume', 'create'], racer(rival), () => expect(() => prepareTaskFilesystems(data.clone, + await withDockerShim(['volume', 'create'], racer(rival, owner.allocationId), () => expect(() => prepareTaskFilesystems(data.clone, { workBytes: 16 * 1024 * 1024, workInodes: 512, metadataBytes: 16 * 1024 * 1024, metadataInodes: 512 }, imageId, owner)).toThrow('still labels a Docker object')); expect(byAllocation(owner.allocationId)).toEqual([rival]); @@ -755,8 +952,10 @@ describe('real Docker agent isolation', () => { it('backs out of a vendor network when a concurrent process claims the same allocation ID', async () => { const data = fixture(), allocationId = randomUUID(), rival = `codeboost-race-${randomUUID()}`; + // Tracked for afterAll should the test time out: the rival carries another runner's label (#189). + volumes.add(rival); try { - await withDockerShim(['network', 'create'], racer(rival), () => expect(createVendorNetwork( + await withDockerShim(['network', 'create'], racer(rival, allocationId), () => expect(createVendorNetwork( invocation(data.clone, 'planning'), imageId, allocationId)).rejects.toThrow('still labels a Docker object')); expect(byAllocation(allocationId)).toEqual([rival]); } finally { spawnSync('docker', ['volume', 'rm', '--force', rival], { stdio: 'ignore' }); } @@ -764,32 +963,44 @@ describe('real Docker agent isolation', () => { it('never removes a network found by name after a killed create when its runner label differs', async () => { const data = fixture(), allocationId = randomUUID(); - // The create lands with another runner's label, and its client is killed before it reports the ID. + // The network carries another runner's label, so no sweep of this run finds it: removed by its allocation (#189). + sweepWhenFinished('io.codeboost.allocation', allocationId); + // The create lands with another runner's label, and its client is killed before it reports the ID. Only this + // allocation's: should the test time out with the shim still on PATH, other tests' creates pass through. const foreign = [ + `if (!args.includes('io.codeboost.allocation=${allocationId}')) result = run(args); else {`, `const at = args.findIndex(arg => arg.startsWith('io.codeboost.runner='));`, `args[at] = 'io.codeboost.runner=${'f'.repeat(32)}';`, 'result = run(args);', - "process.kill(process.pid, 'SIGKILL');", + "process.kill(process.pid, 'SIGKILL'); }", ].join('\n'); const created = await withDockerShim(['network', 'create'], foreign, () => createVendorNetwork(invocation(data.clone, 'planning'), imageId, allocationId).then(() => undefined, error => error)); - const left = docker('network', 'ls', '--quiet', '--filter', `label=io.codeboost.allocation=${allocationId}`); - try { - expect(created).toBeInstanceOf(VendorNetworkCreationCleanupError); - const cleanup = (created as VendorNetworkCreationCleanupError).errors[1] as AggregateError; - expect(cleanup.errors.map(error => (error as Error).message)).toEqual(['Refused to remove unowned vendor network.']); - expect(left).not.toBe(''); - } finally { if (left) spawnSync('docker', ['network', 'rm', ...left.split('\n')], { stdio: 'ignore' }); } + const listArgs = ['network', 'ls', '--quiet', '--filter', `label=io.codeboost.allocation=${allocationId}`]; + const listing = spawnSync('docker', listArgs, { encoding: 'utf8', ...CLIENT_LIMIT }); + // A listing that failed fails here with the command and its cause, as main's helper did. + expect(listing.status, `docker ${listArgs.join(' ')} failed: ` + + (listing.error?.message ?? (listing.signal ? `killed by ${listing.signal}` : listing.stderr.trim()))).toBe(0); + const left = listing.stdout.trim(); + expect(created).toBeInstanceOf(VendorNetworkCreationCleanupError); + const cleanup = (created as VendorNetworkCreationCleanupError).errors[1] as AggregateError; + expect(cleanup.errors.map(error => (error as Error).message)).toEqual(['Refused to remove unowned vendor network.']); + expect(left).not.toBe(''); }, 60_000); it('never removes an agent container found by name after a killed create when a label is missing', async () => { const data = fixture(), live = await profile(data, 'planning', 'must-not-run'); - // The create lands without the runner label, and its client is killed before it reports the ID. + // Tracked for afterAll should the test time out: it has no runner label, and until it goes the task volumes it + // mounts cannot be removed (#189). + containers.add(live.name); + // The create lands without the runner label, and its client is killed before it reports the ID. Only this + // container's: should the test time out with the shim still on PATH, other tests' creates pass through. const unlabelled = [ + `if (args[2] !== ${JSON.stringify(live.name)}) result = run(args); else {`, "const at = args.findIndex(arg => arg.startsWith('io.codeboost.runner='));", 'args.splice(at - 1, 2);', 'result = run(args);', - "process.kill(process.pid, 'SIGKILL');", + "process.kill(process.pid, 'SIGKILL'); }", ].join('\n'); try { await withDockerShim(['create', '--name'], unlabelled, () => @@ -802,6 +1013,8 @@ describe('real Docker agent isolation', () => { it('removes a task keeper by the ID it inspected, never a same-named replacement created after the inspect', async () => { const data = fixture(), filesystems = data.filesystems, swapped = join(data.root, 'swapped'); taskFilesystems.splice(taskFilesystems.indexOf(filesystems), 1); + // Tracked before the shim runs: the stand-in has no labels, so afterAll's removal by name is the only other one (#189). + containers.add(filesystems.keeper); // Right after cleanup inspects the keeper, its name is taken over by another container. const swap = [ 'result = run(args);', @@ -2168,32 +2381,37 @@ describe('real Docker agent isolation', () => { it('keeps a failed allocation whose cleanup did not settle live, so recovery in this process refuses its runner', async () => { const data = fixture(), runnerOwner = randomBytes(16).toString('hex'); const owner = { runnerOwner, attemptId: 'unsettled-allocation', allocationId: randomUUID() }; + // Another runner's objects, so no sweep of this run finds them: removed by allocation when the test finishes (#189). + // After a pass, a listing that fails fails the test, as on main, and now nothing of the allocation may be left once + // that sweep has run either; this check, registered first, runs after the sweep (Vitest runs onTestFinished hooks + // in reverse order). + let passed = false; + // Its budget covers byAllocation's three listings at their 60 s limits, plus a minute. + onTestFinished(() => { if (passed) expect(byAllocation(owner.allocationId)).toEqual([]); }, 4 * 60_000); + sweepWhenFinished('io.codeboost.allocation', owner.allocationId); // The seeder is refused, and every volume removal fails, so the allocation's cleanup does not settle. const failing = [ "if (args[0] === 'run' && args[1] === '--rm') result = { status: 1, stdout: '', stderr: 'refused' };", "else if (args[0] === 'volume' && args[1] === 'rm') result = { status: 1, stdout: '', stderr: 'volume is in use' };", 'else result = run(args);', ].join('\n'); - try { - await withDockerShim([], failing, () => expect(() => prepareTaskFilesystems(data.clone, storageLimits, imageId, - owner)).toThrow('cleanup did not settle')); - await expect(recoverLeftovers(runnerOwner)).rejects.toThrow('still holds task storage'); - } finally { - for (const name of docker('ps', '--all', '--quiet', '--filter', `label=io.codeboost.allocation=${owner.allocationId}`) - .split('\n').filter(Boolean)) spawnSync('docker', ['rm', '--force', name], { stdio: 'ignore' }); - for (const name of docker('volume', 'ls', '--quiet', '--filter', `label=io.codeboost.allocation=${owner.allocationId}`) - .split('\n').filter(Boolean)) spawnSync('docker', ['volume', 'rm', '--force', name], { stdio: 'ignore' }); - } + await withDockerShim([], failing, () => expect(() => prepareTaskFilesystems(data.clone, storageLimits, imageId, + owner)).toThrow('cleanup did not settle')); + await expect(recoverLeftovers(runnerOwner)).rejects.toThrow('still holds task storage'); + passed = true; }, 60_000); it('recovers only its own runner after a restart, keeping its storage for removal by handle', async () => { const data = fixture(), runners = [randomBytes(16).toString('hex'), randomBytes(16).toString('hex')] as const; + // Both runners are this test's own random owners, so no sweep of this run finds what an undo step could not remove + // or never recorded: each is swept when the test finishes (#189). + for (const runner of runners) sweepWhenFinished(RUNNER_LABEL, runner); // Every object is registered for cleanup as soon as it exists, so a failure anywhere in setup leaks nothing. // Cleanup runs in reverse: containers go before the networks and volumes they use. const undo: (() => unknown)[] = []; const quietly = (...args: string[]) => () => spawnSync('docker', args, { stdio: 'ignore' }); - const exists = (kind: 'container' | 'volume' | 'network', ref: string) => - spawnSync('docker', [kind, 'inspect', ref], { stdio: 'ignore' }).status === 0; + // Only Docker's missing answer counts as gone (#189). + const exists = objectExists; const label = (labels: Record) => Object.entries(labels).flatMap(([key, value]) => ['--label', `${key}=${value}`]); // What a crashed process leaves for the first runner, made with the labels and names D writes, so nothing in this @@ -2260,6 +2478,8 @@ describe('real Docker agent isolation', () => { const mine = crashed(runners[0]), theirs = await live(runners[1]); // An object from a build before runner labels. const legacy = `codeboost-work-legacy-${randomUUID()}`; + // Tracked for afterAll too, should the test time out: it has no runner label (#189). + volumes.add(legacy); undo.push(quietly('volume', 'rm', '--force', legacy)); docker('volume', 'create', '--label', `io.codeboost.allocation=${randomUUID()}`, legacy); // A runner with live storage in this process is refused: its agents would be removed under it. @@ -2302,6 +2522,8 @@ describe('real Docker agent isolation', () => { it('exports through a recovery handle after a restart, then removes the storage by that handle', async () => { const data = fixture({ hostile: agentBase }); const owner = { runnerOwner: randomBytes(16).toString('hex'), attemptId: `restart-${randomUUID()}`, allocationId: randomUUID() }; + // Its own random runner, so no sweep of this run finds what it leaves: removed by allocation when it finishes (#189). + sweepWhenFinished('io.codeboost.allocation', owner.allocationId); const staging = join(data.root, 'restart-staging'); mkdirSync(staging); // An earlier process clones and allocates task storage, then exits without releasing it, as a crash does: this // process never held the allocator value. @@ -2319,68 +2541,90 @@ describe('real Docker agent isolation', () => { "process.stdout.write('\\n' + JSON.stringify(prepareTaskFilesystems(clone, limits, imageId, owner)));", ].join('\n')], { encoding: 'utf8', timeout: 120_000, env: { ...process.env, EARLIER: JSON.stringify({ source: data.source, staging, head: data.clone.head, owner }) } }); - try { - if (earlier.status !== 0) throw new Error(`The earlier process failed (${earlier.status}): ${earlier.stderr}`); - const left = JSON.parse(earlier.stdout.trim().split('\n').pop()!) as ReturnType; - agentChanges(left); - const report = await recoverLeftovers(owner.runnerOwner); - expect(report.removed).toEqual([]); - expect(report.storage).toEqual([{ ...owner, workVolume: left.workVolume, metadataVolume: left.metadataVolume, - keeper: left.keeper }]); - const handle = report.storage[0]!; - // D keeps no baseline across a restart: the export needs the one F recorded, and checks the metadata against it. - await expect(exportTaskDiff(handle, { base: data.clone.head, imageId })).rejects.toThrow('needs the metadataBaseline'); - await expect(exportTaskDiff(handle, { base: data.clone.head, imageId, metadataBaseline: 'c'.repeat(64) })) - .rejects.toThrow('metadata changed'); - const before = storageSnapshot(left); - const exported = await exportTaskDiff(handle, { base: data.clone.head, imageId, metadataBaseline: left.metadataBaseline }); - expect(storageSnapshot(left)).toBe(before); - const text = exported.diff.toString('utf8'); - expect(exported.truncated).toBe(false); - for (const expected of ['+committed', '+changed', '+staged only', '+brand new', 'GIT binary patch']) - expect(text).toContain(expected); - // Every export, refused or not, removed its container: only the keeper is left before release. - expect(docker('ps', '--all', '--quiet', '--no-trunc', '--filter', `label=io.codeboost.allocation=${owner.allocationId}`)) - .toBe(docker('container', 'inspect', '--format', '{{.Id}}', left.keeper)); - // The handle still releases the storage after the export, once. - removeTaskFilesystems(handle); - expect(isRecoveredTaskStorage(handle)).toBe(false); - for (const [kind, ref] of [['container', left.keeper], ['volume', left.workVolume], ['volume', left.metadataVolume]] as const) - expect(spawnSync('docker', [kind, 'inspect', ref], { stdio: 'ignore' }).status).not.toBe(0); - } finally { - // Never throws, so a failure above is the one reported. - const labelled = (...args: string[]) => spawnSync('docker', [...args, '--quiet', '--filter', - `label=io.codeboost.allocation=${owner.allocationId}`], { encoding: 'utf8' }).stdout?.split('\n').filter(Boolean) ?? []; - for (const name of labelled('ps', '--all')) spawnSync('docker', ['rm', '--force', name], { stdio: 'ignore' }); - for (const name of labelled('volume', 'ls')) spawnSync('docker', ['volume', 'rm', '--force', name], { stdio: 'ignore' }); - } + if (earlier.status !== 0) throw new Error(`The earlier process failed (${earlier.status}): ${earlier.stderr}`); + const left = JSON.parse(earlier.stdout.trim().split('\n').pop()!) as ReturnType; + agentChanges(left); + const report = await recoverLeftovers(owner.runnerOwner); + expect(report.removed).toEqual([]); + expect(report.storage).toEqual([{ ...owner, workVolume: left.workVolume, metadataVolume: left.metadataVolume, + keeper: left.keeper }]); + const handle = report.storage[0]!; + // D keeps no baseline across a restart: the export needs the one F recorded, and checks the metadata against it. + await expect(exportTaskDiff(handle, { base: data.clone.head, imageId })).rejects.toThrow('needs the metadataBaseline'); + await expect(exportTaskDiff(handle, { base: data.clone.head, imageId, metadataBaseline: 'c'.repeat(64) })) + .rejects.toThrow('metadata changed'); + const before = storageSnapshot(left); + const exported = await exportTaskDiff(handle, { base: data.clone.head, imageId, metadataBaseline: left.metadataBaseline }); + expect(storageSnapshot(left)).toBe(before); + const text = exported.diff.toString('utf8'); + expect(exported.truncated).toBe(false); + for (const expected of ['+committed', '+changed', '+staged only', '+brand new', 'GIT binary patch']) + expect(text).toContain(expected); + // Every export, refused or not, removed its container: only the keeper is left before release. + expect(docker('ps', '--all', '--quiet', '--no-trunc', '--filter', `label=io.codeboost.allocation=${owner.allocationId}`)) + .toBe(docker('container', 'inspect', '--format', '{{.Id}}', left.keeper)); + // The handle still releases the storage after the export, once. + removeTaskFilesystems(handle); + expect(isRecoveredTaskStorage(handle)).toBe(false); + for (const [kind, ref] of [['container', left.keeper], ['volume', left.workVolume], ['volume', left.metadataVolume]] as const) + expect(objectExists(kind, ref)).toBe(false); }, 180_000); it('never removes a task keeper replaced by another runner, but still removes the owned volumes', () => { const data = fixture(), filesystems = data.filesystems, owner = taskFilesystemOwner(filesystems); taskFilesystems.splice(taskFilesystems.indexOf(filesystems), 1); docker('rm', '--force', filesystems.keeper); - docker('run', '--detach', '--name', filesystems.keeper, '--label', `io.codeboost.runner=${'f'.repeat(32)}`, - '--label', `io.codeboost.attempt=${owner.attemptId}`, '--label', `io.codeboost.allocation=${owner.allocationId}`, - '--network=none', '--entrypoint', 'sleep', imageId, 'infinity'); + // Tracked for afterAll too: its create may land after the finally (#189). + containers.add(filesystems.keeper); + let passed = false; try { + // Inside the try: this keeper carries another runner's label, so no sweep of this run finds it; the finally below + // removes it, and afterAll by name (#189). + docker('run', '--detach', '--name', filesystems.keeper, '--label', `io.codeboost.runner=${'f'.repeat(32)}`, + '--label', `io.codeboost.attempt=${owner.attemptId}`, '--label', `io.codeboost.allocation=${owner.allocationId}`, + '--network=none', '--entrypoint', 'sleep', imageId, 'infinity'); expect(() => removeTaskFilesystems(filesystems)).toThrow('did not settle'); expect(docker('container', 'inspect', '--format', '{{.State.Running}}', filesystems.keeper)).toBe('true'); - expect(spawnSync('docker', ['volume', 'inspect', filesystems.workVolume]).status).not.toBe(0); + expect(objectExists('volume', filesystems.workVolume)).toBe(false); + passed = true; } finally { - docker('rm', '--force', filesystems.keeper); - removeTaskFilesystems(filesystems); + const keeperRemoved = spawnSync('docker', ['rm', '--force', filesystems.keeper], { encoding: 'utf8', ...CLIENT_LIMIT }); + // A failure above stays the one reported: what the removal leaves, the allocation sweep below removes, the keeper + // with another runner's label included (#189). After a pass, the foreign keeper's removal and then the storage's + // release must succeed, as on main, so either failure fails the test, the keeper's first. The sweep may take up to + // 5.5 minutes, which the test's budget allows for. + let released: unknown; + try { removeTaskFilesystems(filesystems); } catch (error) { released = error; console.warn('cleanup: removing the storage failed:', error); } + removeLabelledObjects('io.codeboost.allocation', owner.allocationId, 'finally'); + const keeperFailure = keeperRemoved.status === 0 ? undefined : `docker rm --force ${filesystems.keeper} failed: ` + + (keeperRemoved.error?.message ?? (keeperRemoved.signal ? `killed by ${keeperRemoved.signal}` : keeperRemoved.stderr?.trim())); + if (keeperFailure && !passed) console.warn('cleanup:', keeperFailure); + if (keeperFailure && passed) throw new Error(keeperFailure); + if (passed && released) throw released; } - }, 60_000); + }, 7 * 60_000); it('refuses task storage that belongs to another runner', async () => { - const data = fixture(); - const foreign = prepareTaskFilesystems(data.clone, { workBytes: 16 * 1024 * 1024, workInodes: 512, - metadataBytes: 16 * 1024 * 1024, metadataInodes: 512 }, imageId, - { runnerOwner: 'f'.repeat(32), attemptId: 'other-runner', allocationId: randomUUID() }); - taskFilesystems.push(foreign); - await expect(createContainerProfile({ ...await governed(invocation(data.clone, 'planning')), filesystems: foreign, - inputDirectory: data.input, codexAuthFile: data.fakeAuth, imageId })).rejects.toThrow('another runner'); + const data = fixture(), allocationId = randomUUID(); + let foreign: ReturnType | undefined; + // It carries another runner's label, so no sweep of this run finds it: removed here, and whatever still carries its + // allocation when the test finishes, should its creation or removal not settle or the test time out (#189). + sweepWhenFinished('io.codeboost.allocation', allocationId); + let passed = false; + try { + foreign = prepareTaskFilesystems(data.clone, { workBytes: 16 * 1024 * 1024, workInodes: 512, + metadataBytes: 16 * 1024 * 1024, metadataInodes: 512 }, imageId, + { runnerOwner: 'f'.repeat(32), attemptId: 'other-runner', allocationId }); + await expect(createContainerProfile({ ...await governed(invocation(data.clone, 'planning')), filesystems: foreign, + inputDirectory: data.input, codexAuthFile: data.fakeAuth, imageId })).rejects.toThrow('another runner'); + passed = true; + } finally { + // A failure above stays the one reported, and the sweep removes whatever the removal left. After a pass, the + // storage must release by its handle, so its failure fails the test. + let released: unknown; + try { if (foreign) removeTaskFilesystems(foreign); } catch (error) { released = error; console.warn('cleanup: removing the storage failed:', error); } + if (passed && released) throw released; + } }, 60_000); it('refuses to seed a clone whose staging directory was replaced after creation', async () => { @@ -2402,9 +2646,13 @@ describe('real Docker agent isolation', () => { }, 60_000); it('startup probe refuses to exec when seccomp filtering is disabled', async () => { - const result = spawnSync('docker', ['run', '--rm', '--read-only', '--user', '10001:10001', '--cap-drop=ALL', + // Named and tracked before it is made: it has no labels, so if `--rm` never applies (its client killed before the + // start), afterAll's removal by name is the only one (#189). + const name = `codeboost-seccomp-probe-${randomUUID()}`; + containers.add(name); + const result = spawnSync('docker', ['run', '--rm', '--name', name, '--read-only', '--user', '10001:10001', '--cap-drop=ALL', '--security-opt=no-new-privileges', '--security-opt=seccomp=unconfined', '--network=none', imageId, 'true'], - { encoding: 'utf8', timeout: 60_000 }); + { encoding: 'utf8', ...CLIENT_LIMIT }); expect(result.status).toBe(78); expect(result.stderr).toContain('seccomp syscall filter must be enforced'); }, 60_000); @@ -2418,7 +2666,7 @@ describe('real Docker agent isolation', () => { '--filter', `label=io.codeboost.runner=${TEST_RUNNER_OWNER}`).split('\n').filter(Boolean)); const before = keepers(); const shim = join(data.root, 'docker-shim'); mkdirSync(shim); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); // The keeper's create client hangs until killed, and the real create lands in the daemon afterwards. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', `if [ "$1" = create ] && [ "$2" = --name ]; then ( sleep 10; exec '${realDocker}' "$@" ) >/dev/null 2>&1 { it('rejects a new endpoint attached to the invocation network before launch', async () => { const data = fixture(), valid = await profile(data, 'planning', 'must-not-run'); const rogue = `codeboost-rogue-${randomUUID()}`; + // Tracked for afterAll: its create may land after the finally, and once the network is gone it is no holder (#189). + containers.add(rogue); try { docker('run', '--detach', '--name', rogue, `--network=${valid.network.name}`, '--entrypoint', 'node', imageId, '-e', 'setInterval(()=>{},1000)'); await expect(createValidatedContainer(valid)).rejects.toThrow('cleanup did not settle'); - const absent = spawnSync('docker', ['container', 'inspect', valid.name], { encoding: 'utf8' }); - expect(absent.status).not.toBe(0); + expect(objectExists('container', valid.name)).toBe(false); } finally { spawnSync('docker', ['rm', '--force', rogue], { stdio: 'ignore' }); await disposeContainerProfile(valid); diff --git a/test/agent-gate.test.ts b/test/agent-gate.test.ts index 1bd77fdd..eae872f0 100644 --- a/test/agent-gate.test.ts +++ b/test/agent-gate.test.ts @@ -51,8 +51,9 @@ const writableMetadata = tmpfs('/work/.git', 'size=16m,nr_inodes=512'); const boundedScratch = tmpfs('/tmp', 'size=32m,nr_inodes=4096'); beforeAll(() => { imageId = buildAgentImage(); }, 10 * 60_000); -// The file has no other cleanup: a probe container whose client timed out may still be running. -afterAll(() => removeRunnerObjects(TEST_RUNNER_OWNER), 4 * 60_000); +// The file has no other cleanup: a probe container whose client timed out may still be running. The budget is the +// sweep's worst case (eleven calls of 30 s) and half a minute. +afterAll(() => removeRunnerObjects(TEST_RUNNER_OWNER), 6 * 60_000); describe('isolation gate detects breaches', () => { it.each([ diff --git a/test/agent-network.test.ts b/test/agent-network.test.ts index ee2b4ed2..39cc50c1 100644 --- a/test/agent-network.test.ts +++ b/test/agent-network.test.ts @@ -1,6 +1,6 @@ import { execFileSync, spawnSync } from 'node:child_process'; import { randomBytes, randomUUID } from 'node:crypto'; -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -9,19 +9,25 @@ import { assertVendorNetwork, createVendorNetwork, removeVendorNetwork, VENDOR_H VendorNetworkCreationCleanupError, type VendorNetwork } from '../agents/network/network.ts'; import { captureInvocation, type InvocationInput } from '../agents/contract.ts'; -import { removeRunnerObjects } from './fixtures/docker-sweep.ts'; +import { CLIENT_LIMIT, objectExists, removeByName, removeLabelledObjects, removeRunnerObjects, requireRealDocker, TEST_FILE_LABEL } from './fixtures/docker-sweep.ts'; // Each run has its own runner owner, so the sweep after this file removes only what this run left behind. Network and // proxy names are random, so the fixed attempt IDs below reach only labels, where this owner tells runs apart. const TEST_RUNNER_OWNER = randomBytes(16).toString('hex'); +/** + * On the unnamed `run --rm` containers that join a vendor network (#189): a client killed after the create but before + * the start leaves a container `--rm` never removes, which is no holder once its network is gone (Docker 27.4 removes + * a network a created, never started container still names). afterAll sweeps this label. + */ +const TEST_FILE_RUN = randomUUID(), fileLabel = ['--label', `${TEST_FILE_LABEL}=${TEST_FILE_RUN}`]; let imageId = '', network: VendorNetwork, invocation: InvocationInput; const docker = (...args: string[]) => execFileSync('docker', args, { encoding: 'utf8', timeout: 60_000, stdio: ['ignore', 'pipe', 'pipe'], }).trim(); -const curl = (url: string, direct = false) => spawnSync('docker', ['run', '--rm', `--network=${network.name}`, +const curl = (url: string, direct = false) => spawnSync('docker', ['run', '--rm', ...fileLabel, `--network=${network.name}`, '--env', `HTTPS_PROXY=${network.proxyUrl}`, ...(direct ? ['--env', 'NO_PROXY=*'] : []), '--entrypoint', 'curl', imageId, '--silent', '--show-error', '--output', '/dev/null', '--write-out', '%{http_code}', - '--max-time', '15', url], { encoding: 'utf8', timeout: 30_000, stdio: ['ignore', 'pipe', 'pipe'] }); + '--max-time', '15', url], { encoding: 'utf8', timeout: 30_000, killSignal: 'SIGKILL', stdio: ['ignore', 'pipe', 'pipe'] }); beforeAll(async () => { imageId = buildAgentImage(); @@ -33,21 +39,45 @@ beforeAll(async () => { }); network = await createVendorNetwork(invocation, imageId, randomUUID()); }, 10 * 60_000); +/** + * Objects a test makes without this run's runner label, by name or by the file its shim records the name in, so + * afterAll removes them should the test time out before its own removal runs (#189). The record files live in + * `records`, not in the shims, so a name a late client adds after the test's own removal is still read here. + */ +const unlabelled: { kind: 'container' | 'network'; name?: string; file?: string }[] = []; +// Made by the first test that records, so a run that selects none of this file's tests leaves no directory. +let records = ''; +const recordFile = () => join(records ||= mkdtempSync(join(tmpdir(), 'codeboost-records-')), randomUUID()); // If setup failed there is no network, and a teardown error would hide the setup failure. afterAll(async () => { - try { if (network) await removeVendorNetwork(network); } + try { + for (const { kind, name, file } of unlabelled) { + // A record holds one name per line: should a test time out with its shim still on PATH, later creates add theirs. + // A record that cannot be read is reported, and the rest still run. + let refs: string[] = []; + try { refs = name ? [name] : file && existsSync(file) ? readFileSync(file, 'utf8').split('\n').filter(Boolean) : []; } + catch (error) { console.warn('afterAll: reading a record failed:', error); } + for (const ref of refs) removeByName(kind, ref, 'afterAll'); + } + if (network) await removeVendorNetwork(network); + } finally { // Then remove whatever this run still owns, even when a test failed or the removal above threw. Objects a test - // makes without the runner label are not found here; those tests remove their own. It may not run before a hook - // timeout. + // makes without the runner label are found here only while they hold one of this run's networks or volumes, or + // carry this file's run ID; the tests remove their own, and the try above removes those registered in + // `unlabelled`. It may not run before a hook timeout. The hook's 13 minutes leave two over the worst case of these + // two sweeps (eleven calls of 30 s each), for the cleanup before them. + // The records were read above; removed first, so a hook timeout during the sweeps does not leave them. + try { if (records) rmSync(records, { recursive: true, force: true }); } catch (error) { console.warn('afterAll: removing the records failed:', error); } + removeLabelledObjects(TEST_FILE_LABEL, TEST_FILE_RUN, 'afterAll'); removeRunnerObjects(TEST_RUNNER_OWNER); } -}, 4 * 60_000); +}, 13 * 60_000); describe('vendor-only egress', () => { it('keeps failed allocation and its cleanup inside the caller deadline', async () => { const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); // Every network operation hangs, so both setup and cleanup can only end by deadline. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', 'if [ "$1" = network ]; then exec sleep 30; fi', `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); @@ -60,7 +90,7 @@ describe('vendor-only egress', () => { it('removes a network that lands in the daemon after its create client was killed', async () => { const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), requested = join(shim, 'network-name'); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); // The create client hangs until killed, and the real create lands after that, inside the cleanup reserve. // The shim records this test's network name, since other suites may create egress networks concurrently. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', @@ -77,42 +107,60 @@ describe('vendor-only egress', () => { name = readFileSync(requested, 'utf8'); } finally { process.env.PATH = path; rmSync(shim, { recursive: true, force: true }); } execFileSync('sleep', ['3']); - const orphaned = spawnSync('docker', ['network', 'inspect', name], { stdio: 'ignore' }).status === 0; - if (orphaned) docker('network', 'rm', name); + // A network landing later still carries this run's runner label (the shim passes production's arguments on), so + // afterAll's runner sweep removes it. + // First, so a shim that recorded something else is reported as itself. expect(name).toMatch(/^codeboost-egress-/); + const orphaned = objectExists('network', name); + if (orphaned) docker('network', 'rm', name); expect(orphaned).toBe(false); }, 60_000); it('leaves a same-named network alone when the daemon refuses the create', async () => { - const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = join(shim, 'foreign'); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); - // The name is taken by someone else's network just before our create, so the daemon refuses it. + const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = recordFile(); + unlabelled.push({ kind: 'network', file: recorded }); + const realDocker = requireRealDocker(); + // The name is taken by someone else's network just before our create, so the daemon refuses it. The refusal is + // Docker 27.4's own text, and the shim makes the network with the real docker. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', `if [ "$1" = network ] && [ "$2" = create ]; then for arg; do last="$arg"; done; ` - + `'${realDocker}' network create "$last" >/dev/null; printf %s "$last" > '${recorded}'; ` + + `printf '%s\\n' "$last" >> '${recorded}'; '${realDocker}' network create "$last" >/dev/null; ` + `echo "Error response from daemon: network with name $last already exists" >&2; exit 1; fi`, `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); const refused = captureInvocation({ ...invocation, attemptId: `refused-network-${randomUUID()}`, deadline: Date.now() + 60_000 }); const path = process.env.PATH; process.env.PATH = `${shim}:${path}`; - let foreign = '', error: unknown; + let foreign = '', error: unknown, survived: boolean | Error = false; try { try { await createVendorNetwork(refused, imageId, randomUUID()); } catch (caught) { error = caught; } - foreign = readFileSync(recorded, 'utf8'); - } finally { process.env.PATH = path; rmSync(shim, { recursive: true, force: true }); } - const survived = spawnSync('docker', ['network', 'inspect', foreign], { stdio: 'ignore' }).status === 0; - spawnSync('docker', ['network', 'rm', foreign], { stdio: 'ignore' }); + } finally { + process.env.PATH = path; + // Recorded before it is made, and removed here: the network has no labels, so no sweep finds it (#189). + // A record that cannot be read is reported, not thrown, so the body's own failure stays the one reported. + let made: string[] = []; + try { made = existsSync(recorded) ? readFileSync(recorded, 'utf8').split('\n').filter(Boolean) : []; } + catch (caught) { console.warn('finally: reading the record failed:', caught); } + foreign = made[0] ?? ''; + // afterAll reads the record again: the object may land after this removal (#189). + // Only Docker's own missing answer counts as gone; any other failure is kept and reported below, after the body's. + if (foreign) try { survived = objectExists('network', foreign); } catch (caught) { survived = caught as Error; } + for (const name of made) removeByName('network', name, 'finally'); + rmSync(shim, { recursive: true, force: true }); + } // The refused create is reported as itself, with no cleanup left to retry, and the foreign network survives. + // First, so a shim that no longer matches is reported as itself. + expect(foreign, 'the shim recorded no foreign network').not.toBe(''); expect(error).toBeInstanceOf(Error); expect(error).not.toBeInstanceOf(VendorNetworkCreationCleanupError); expect(String(error)).toMatch(/already exists/); + if (survived instanceof Error) throw survived; expect(survived).toBe(true); }, 60_000); it('removes a proxy that was created even though its start failed', async () => { const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = join(shim, 'proxy'); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); // The proxy container is created, then its start fails: a container this allocation owns by ID. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', `if [ "$1" = start ] && [ "$2" != --attach ]; then printf %s "$2" > '${recorded}'; ` @@ -127,35 +175,50 @@ describe('vendor-only egress', () => { await expect(createVendorNetwork(failing, imageId, randomUUID())).rejects.toThrow('failed to start'); proxyId = readFileSync(recorded, 'utf8'); } finally { process.env.PATH = path; rmSync(shim, { recursive: true, force: true }); } - const leaked = spawnSync('docker', ['container', 'inspect', proxyId], { stdio: 'ignore' }).status === 0; - if (leaked) spawnSync('docker', ['rm', '--force', proxyId], { stdio: 'ignore' }); + // First, so a shim that recorded something else is reported as itself. expect(proxyId).toMatch(/^[0-9a-f]{64}$/); + const leaked = objectExists('container', proxyId); + if (leaked) spawnSync('docker', ['rm', '--force', proxyId], { stdio: 'ignore', ...CLIENT_LIMIT }); expect(leaked).toBe(false); }, 60_000); it('neither reports nor removes a same-named proxy when the daemon refuses the create', async () => { - const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = join(shim, 'foreign'); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = recordFile(); + unlabelled.push({ kind: 'container', file: recorded }); + const realDocker = requireRealDocker(); // Someone else's container takes the proxy name just before our create, so the daemon refuses it. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', `if [ "$1" = create ] && [ "$2" = --name ] && case "$3" in codeboost-proxy-*) true;; *) false;; esac; then ` - + `'${realDocker}' create --name "$3" --entrypoint true ${imageId} >/dev/null; printf %s "$3" > '${recorded}'; fi`, + + `printf '%s\\n' "$3" >> '${recorded}'; '${realDocker}' create --name "$3" --entrypoint true ${imageId} >/dev/null; fi`, `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); const refused = captureInvocation({ ...invocation, attemptId: `refused-proxy-${randomUUID()}`, deadline: Date.now() + 60_000 }); const path = process.env.PATH; process.env.PATH = `${shim}:${path}`; - let foreign = '', error: unknown; + let foreign = '', error: unknown, survived: boolean | Error = false; try { try { await createVendorNetwork(refused, imageId, randomUUID()); } catch (caught) { error = caught; } - foreign = readFileSync(recorded, 'utf8'); - } finally { process.env.PATH = path; rmSync(shim, { recursive: true, force: true }); } - const survived = spawnSync('docker', ['container', 'inspect', foreign], { stdio: 'ignore' }).status === 0; - spawnSync('docker', ['rm', '--force', foreign], { stdio: 'ignore' }); + } finally { + process.env.PATH = path; + // Recorded before it is made, and removed here: the container has no labels, so no sweep finds it (#189). + // A record that cannot be read is reported, not thrown, so the body's own failure stays the one reported. + let made: string[] = []; + try { made = existsSync(recorded) ? readFileSync(recorded, 'utf8').split('\n').filter(Boolean) : []; } + catch (caught) { console.warn('finally: reading the record failed:', caught); } + foreign = made[0] ?? ''; + // afterAll reads the record again: the object may land after this removal (#189). + // Only Docker's own missing answer counts as gone; any other failure is kept and reported below, after the body's. + if (foreign) try { survived = objectExists('container', foreign); } catch (caught) { survived = caught as Error; } + for (const name of made) removeByName('container', name, 'finally'); + rmSync(shim, { recursive: true, force: true }); + } // The refused create is reported as itself: our network was removed, nothing is left to retry or report. + // First, so a shim that no longer matches is reported as itself. + expect(foreign, 'the shim recorded no foreign proxy').not.toBe(''); expect(error).toBeInstanceOf(Error); expect(error).not.toBeInstanceOf(VendorNetworkCreationCleanupError); expect(String(error)).toMatch(/already in use|Conflict/); + if (survived instanceof Error) throw survived; expect(survived).toBe(true); }, 60_000); @@ -177,27 +240,45 @@ describe('vendor-only egress', () => { }, 60_000); it('does not delete a same-named stand-in when setup fails after the proxy exists', async () => { - const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = join(shim, 'impostor'); - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); - // The readiness exec swaps the proxy for a same-named, same-labelled stand-in, then fails setup. + const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')), recorded = recordFile(); + unlabelled.push({ kind: 'container', file: recorded }); + const realDocker = requireRealDocker(); + // The readiness exec swaps the proxy for a same-named, same-labelled stand-in, then fails setup. Only an exec in a + // proxy: any other call passes through, so the shim never makes a stand-in with an empty, random name. Its exit 1 + // with no stderr is invented, as an exec that failed: the code branches only on the status, and stderr goes only into + // the error message, which the test does not check. writeFileSync(join(shim, 'docker'), ['#!/bin/sh', 'if [ "$1" = exec ]; then', - ` name=$('${realDocker}' inspect -f '{{.Name}}' "$2" | sed 's#^/##')`, + ` name=$('${realDocker}' inspect -f '{{.Name}}' "$2" 2>/dev/null | sed 's#^/##')`, + ' case "$name" in codeboost-proxy-*)', ` label=$('${realDocker}' inspect -f '{{index .Config.Labels "io.codeboost.egress"}}' "$2")`, ` '${realDocker}' rm --force "$2" >/dev/null`, + // Recorded before the stand-in exists, so the test can remove it even if the run is cut short. + ` printf '%s\\n' "$name" >> '${recorded}'`, ` '${realDocker}' run --detach --name "$name" --label "io.codeboost.egress=$label" --entrypoint sleep ${imageId} 300 >/dev/null`, - ` printf %s "$name" > '${recorded}'; exit 1`, 'fi', `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); + ' exit 1;;', ' esac', 'fi', `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); const failing = captureInvocation({ ...invocation, attemptId: `failed-setup-${randomUUID()}`, deadline: Date.now() + 60_000 }); const path = process.env.PATH; process.env.PATH = `${shim}:${path}`; - let impostor = ''; + let impostor = '', survived: boolean | Error = false; try { await expect(createVendorNetwork(failing, imageId, randomUUID())).rejects.toThrow(); - impostor = readFileSync(recorded, 'utf8'); - } finally { process.env.PATH = path; rmSync(shim, { recursive: true, force: true }); } - const survived = spawnSync('docker', ['container', 'inspect', impostor], { stdio: 'ignore' }).status === 0; - spawnSync('docker', ['rm', '--force', impostor], { stdio: 'ignore' }); + } finally { + process.env.PATH = path; + // The stand-in has no runner label, so no sweep finds it: remove it here, however the test ends (#189). + // A record that cannot be read is reported, not thrown, so the body's own failure stays the one reported. + let made: string[] = []; + try { made = existsSync(recorded) ? readFileSync(recorded, 'utf8').split('\n').filter(Boolean) : []; } + catch (caught) { console.warn('finally: reading the record failed:', caught); } + impostor = made[0] ?? ''; + // afterAll reads the record again: the object may land after this removal (#189). + // Only Docker's own missing answer counts as gone; any other failure is kept and reported below, after the body's. + if (impostor) try { survived = objectExists('container', impostor); } catch (caught) { survived = caught as Error; } + for (const name of made) removeByName('container', name, 'finally'); + rmSync(shim, { recursive: true, force: true }); + } expect(impostor).toMatch(/^codeboost-proxy-/); + if (survived instanceof Error) throw survived; expect(survived).toBe(true); }, 60_000); @@ -205,12 +286,13 @@ describe('vendor-only egress', () => { const partial = captureInvocation({ ...invocation, attemptId: `partial-removal-${randomUUID()}`, deadline: Date.now() + 60_000 }); const created = await createVendorNetwork(partial, imageId, randomUUID()), blocker = `codeboost-blocker-${randomUUID()}`; + unlabelled.push({ kind: 'container', name: blocker }); const [proxy, net] = vendorNetworkResources(created); expect(proxy).toMatchObject({ kind: 'container', name: created.proxyContainer }); expect(net).toMatchObject({ kind: 'network', name: created.name }); // A foreign endpoint keeps the network busy, so the proxy is removed but `network rm` fails. - docker('run', '--detach', '--name', blocker, '--network', created.name, '--entrypoint', 'sleep', imageId, '300'); try { + docker('run', '--detach', '--name', blocker, '--network', created.name, '--entrypoint', 'sleep', imageId, '300'); await expect(removeVendorNetwork(created, 10_000)).rejects.toThrow('did not settle'); expect(vendorNetworkResources(created)).toEqual([net]); } finally { spawnSync('docker', ['rm', '--force', blocker], { stdio: 'ignore' }); } @@ -256,12 +338,12 @@ describe('vendor-only egress', () => { }, 60_000); it('does not forward arbitrary DNS even when the embedded resolver is addressed directly', async () => { - const result = spawnSync('docker', ['run', '--rm', `--network=${network.name}`, '--dns=127.0.0.1', + const result = spawnSync('docker', ['run', '--rm', ...fileLabel, `--network=${network.name}`, '--dns=127.0.0.1', '--entrypoint', 'node', imageId, '-e', [ "const dns=require('node:dns');dns.setServers(['127.0.0.11']);", "dns.resolve4('example.com',(error)=>process.exit(error?0:1));", 'setTimeout(()=>process.exit(0),3000);', - ].join('')], { encoding: 'utf8', timeout: 10_000, stdio: ['ignore', 'pipe', 'pipe'] }); + ].join('')], { encoding: 'utf8', timeout: 10_000, killSignal: 'SIGKILL', stdio: ['ignore', 'pipe', 'pipe'] }); expect(result.status).toBe(0); }, 30_000); @@ -278,12 +360,14 @@ describe('vendor-only egress', () => { const otherInvocation = captureInvocation({ ...invocation, attemptId: 'concurrent-network-probe', deadline: Date.now() + 60_000 }); const other = await createVendorNetwork(otherInvocation, imageId, randomUUID()), peer = `codeboost-peer-${randomUUID()}`; + // Tracked for afterAll: its create may land after the finally, and once the network is gone it is no holder (#189). + unlabelled.push({ kind: 'container', name: peer }); try { docker('run', '--detach', '--name', peer, `--network=${network.name}`, '--network-alias', 'codeboost-peer', '--entrypoint', 'node', imageId, '-e', "require('node:net').createServer(()=>{}).listen(4567,'0.0.0.0');setInterval(()=>{},1000)"); - const result = spawnSync('docker', ['run', '--rm', `--network=${other.name}`, '--entrypoint', 'node', imageId, + const result = spawnSync('docker', ['run', '--rm', ...fileLabel, `--network=${other.name}`, '--entrypoint', 'node', imageId, '-e', "const s=require('node:net').connect(4567,'codeboost-peer');s.on('connect',()=>process.exit(0));s.on('error',()=>process.exit(1));setTimeout(()=>process.exit(2),3000)"], - { encoding: 'utf8', timeout: 10_000, stdio: ['ignore', 'pipe', 'pipe'] }); + { encoding: 'utf8', timeout: 10_000, killSignal: 'SIGKILL', stdio: ['ignore', 'pipe', 'pipe'] }); expect(result.status).not.toBe(0); } finally { spawnSync('docker', ['rm', '--force', peer], { stdio: 'ignore' }); @@ -312,6 +396,8 @@ describe('vendor-only egress', () => { { Config: { Labels: Record }; NetworkSettings: { Networks: Record } }; const allocation = inspected.Config.Labels['io.codeboost.egress']; const proxyIp = inspected.NetworkSettings.Networks[replacement.name]!.IPAddress; + // Tracked for afterAll: the stand-in has no runner label, and its create may land after the finally (#189). + unlabelled.push({ kind: 'container', name: replacement.proxyContainer }); try { docker('rm', '--force', replacement.proxyContainer); // Same name, label, image, lockdown and IP as the original, so only the extra option and the object ID differ. diff --git a/test/agent-supervisor.test.ts b/test/agent-supervisor.test.ts index 1db2faa0..85bc3134 100644 --- a/test/agent-supervisor.test.ts +++ b/test/agent-supervisor.test.ts @@ -1,11 +1,11 @@ import { randomBytes, randomUUID } from 'node:crypto'; import { execFileSync, spawnSync } from 'node:child_process'; -import { removeRunnerObjects } from './fixtures/docker-sweep.ts'; +import { CLIENT_LIMIT, objectExists, removeByName, removeRunnerObjects, requireRealDocker } from './fixtures/docker-sweep.ts'; import { fixtureGit } from './fixtures/git.ts'; import { chmodSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, onTestFinished } from 'vitest'; import { startClaudeInvocation } from '../agents/adapters/claude.ts'; import { readCodexOutput } from '../agents/adapters/codex.ts'; import { ACKNOWLEDGEMENT_SCRIPT, isInvocationActive, readBoundedContainerFile, retainSetupCleanup, @@ -30,6 +30,11 @@ const testOwner = (attemptId = 'fixture') => ({ runnerOwner: TEST_RUNNER_OWNER, const roots: string[] = [], profiles: ContainerProfile[] = []; const allocations: ReturnType[] = []; +/** + * Containers without this run's runner label, by name, removed in afterAll should their create land after their test's + * finally (#189). + */ +const unlabelled = new Set(); let imageId = ''; const git = fixtureGit; @@ -70,6 +75,7 @@ async function profile(data: ReturnType, probe: IsolationProbe, beforeAll(async () => { imageId = buildAgentImage(); }, 10 * 60_000); afterAll(async () => { try { + for (const name of unlabelled) removeByName('container', name, 'afterAll'); for (const profile of profiles) await disposeContainerProfile(profile); for (const allocation of allocations.reverse()) removeTaskFilesystems(allocation); for (const root of roots.reverse()) { @@ -79,9 +85,11 @@ afterAll(async () => { } finally { // Then remove whatever this run still owns, even when a test failed or the cleanup above threw: an agent that // ignores SIGTERM, or storage whose removal did not settle. It may not run before a hook timeout. + // The hook's 7 minutes leave a minute and a half over this sweep's worst case (eleven calls of 30 s), for the + // cleanup before it. removeRunnerObjects(TEST_RUNNER_OWNER); } -}, 3 * 60_000); +}, 7 * 60_000); describe('container invocation supervisor', () => { it('preserves the first cancellation reason while retained setup cleanup settles', async () => { @@ -330,7 +338,7 @@ describe('container invocation supervisor', () => { const hangingDocker = (match: string) => { const shim = mkdtempSync(join(tmpdir(), 'codeboost-slow-docker-')); roots.push(shim); mkdirSync(join(shim, 'input')); // afterAll resets this path's mode - const realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const realDocker = requireRealDocker(); writeFileSync(join(shim, 'docker'), ['#!/bin/sh', `case "$*" in ${match}) exec sleep 60;; esac`, `exec '${realDocker}' "$@"`].join('\n'), { mode: 0o755 }); return `${shim}:${process.env.PATH}`; @@ -385,20 +393,30 @@ describe('container invocation supervisor', () => { it('releases its own resources, and leaves a same-named container another invocation owns', async () => { const current = await profile(fixture(), 'finite-output', 'foreign-name'); - execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=foreign', - '--entrypoint', 'true', imageId], { stdio: 'ignore' }); + // Removed by name when the test finishes, timeouts included: its label value is shared across runs (#189). Tracked + // for afterAll too, should its create land after that. + unlabelled.add(current.name); + onTestFinished(() => removeByName('container', current.name, 'onTestFinished'), 90_000); // two calls of up to 30 s + let passed = false; try { + // Inside the try, so its removal runs even if the create's client fails after the daemon made it (#189). + execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=foreign', + '--entrypoint', 'true', imageId], { stdio: 'ignore', ...CLIENT_LIMIT }); const result = await startProfileInvocation(current).settled; expect(result.stopReason).toBe('capture-failure'); expect(result.stderr).toMatch(/already in use|Conflict/); expect(result.unreleased).toBeUndefined(); expect(isInvocationActive(attempt('foreign-name'))).toBe(false); expect(isContainerProfileAuthentic(current)).toBe(false); - expect(spawnSync('docker', ['network', 'inspect', current.network.name], { stdio: 'ignore' }).status).not.toBe(0); + expect(objectExists('network', current.network.name)).toBe(false); expect(execFileSync('docker', ['container', 'inspect', '--format', '{{index .Config.Labels "io.codeboost.invocation"}}', current.name], { encoding: 'utf8' }).trim()).toBe('foreign'); + passed = true; } finally { - execFileSync('docker', ['rm', '--force', current.name], { stdio: 'ignore' }); + // A failure above stays the one reported, whatever exit status this Docker version gives for a missing name. After + // a pass the container exists, and its removal must succeed, as on main. + const removed = spawnSync('docker', ['rm', '--force', current.name], { encoding: 'utf8', ...CLIENT_LIMIT }); + if (passed && removed.status !== 0) throw new Error(`docker rm --force ${current.name} failed: ${removed.error?.message ?? (removed.signal ? `killed by ${removed.signal}` : removed.stderr?.trim())}`); } }, 3 * 60_000); @@ -436,9 +454,13 @@ describe('container invocation supervisor', () => { it('releases only the profile when rejecting before creation, even if its name is held elsewhere', async () => { const current = await profile(fixture(), 'noop'); // A foreign container occupies the deterministic name, so container-level cleanup could never settle. - execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=someone-else', - '--entrypoint', 'true', imageId], { stdio: 'ignore' }); + // Removed by name when the test finishes, timeouts included: its label value is shared across runs (#189). Tracked + // for afterAll too, should its create land after that. + unlabelled.add(current.name); + onTestFinished(() => removeByName('container', current.name, 'onTestFinished'), 90_000); // two calls of up to 30 s try { + execFileSync('docker', ['create', '--name', current.name, '--label', 'io.codeboost.invocation=someone-else', + '--entrypoint', 'true', imageId], { stdio: 'ignore', ...CLIENT_LIMIT }); await expectRejected(startProfileInvocation(current, { timeoutMs: 10 * 60_000 + 1 }), 'ceiling'); expect(isContainerProfileAuthentic(current)).toBe(false); expect(spawnSync('docker', ['container', 'inspect', current.name], { stdio: 'ignore' }).status).toBe(0); @@ -641,11 +663,15 @@ describe('container invocation supervisor', () => { it('writes the deferred output acknowledgement idempotently, repairing a partial one', () => { const token = randomUUID(), file = `/run/codeboost-control/collected-${token}`; const name = `codeboost-ack-script-${randomUUID()}`; - execFileSync('docker', ['run', '-d', '--rm', '--name', name, '--network', 'none', '--cap-drop=ALL', - '--security-opt=no-new-privileges', '--user', '10001:10001', '--entrypoint', 'sleep', - '--tmpfs', '/run/codeboost-control:rw,nosuid,nodev,noexec,size=65536,nr_inodes=16,uid=0,gid=0,mode=0711', - imageId, '120'], { stdio: 'ignore' }); + // Tracked before it is made: it has no labels, and a create that lands after its client was killed is never started, + // so `--rm` does not apply and only removal by name finds it (#189). + unlabelled.add(name); try { + // Inside the try: the container has no labels, so the finally below removes it, and afterAll by name (#189). + execFileSync('docker', ['run', '-d', '--rm', '--name', name, '--network', 'none', '--cap-drop=ALL', + '--security-opt=no-new-privileges', '--user', '10001:10001', '--entrypoint', 'sleep', + '--tmpfs', '/run/codeboost-control:rw,nosuid,nodev,noexec,size=65536,nr_inodes=16,uid=0,gid=0,mode=0711', + imageId, '120'], { stdio: 'ignore', ...CLIENT_LIMIT }); const acknowledge = () => spawnSync('docker', ['exec', '--user', '0', name, 'node', '-e', ACKNOWLEDGEMENT_SCRIPT, token], { encoding: 'utf8' }); const content = () => execFileSync('docker', ['exec', name, 'cat', file], { encoding: 'utf8' }); diff --git a/test/docker-sweep.test.ts b/test/docker-sweep.test.ts new file mode 100644 index 00000000..313fa767 --- /dev/null +++ b/test/docker-sweep.test.ts @@ -0,0 +1,414 @@ +import { randomUUID } from 'node:crypto'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, relative } from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { afterEach, expect, it, vi } from 'vitest'; +import { removeByName, removeLabelledObjects, TEST_RUN_LABEL } from './fixtures/docker-sweep.ts'; +import type * as Sweep from './fixtures/docker-sweep.ts'; + +// The test sweep's and `removeByName`'s handling of answers they cannot use (#189), each test with its own fake +// `docker`, described beside it. The sweep reads only stdout, the exit status, whether a signal killed the call and +// whether it could start, and stderr to report a failure, and, for its read of holders and its removal of networks, to +// tell one gone since the listing ("No such container", "network not found") from a failure. `removeByName` reads +// the same, and also an inspect's stdout (the ID and name) and its stderr, to tell a missing object from a failure, and +// a network removal's stderr, to tell a network gone since its inspect. Against the daemon, agent-container's holder +// test covers the sweep and removeByName's removal, its silence on a missing name, and its refusal of an object found +// by an ID prefix. +const roots: string[] = []; +afterEach(() => { + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); + vi.restoreAllMocks(); + vi.unstubAllEnvs(); +}); + +// The fake answers as no real daemon does for `network ls`: a line that is not JSON, a JSON null and one network +// without a name. To every other call it answers nothing and exit 0, as Docker 27.4 does for `ls` with no match. +it('skips and reports output that is not a network, and never throws', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'); + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' 'not json' 'null' '{"ID":"abc"}'; fi`, 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + expect(() => removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker'))).not.toThrow(); + expect(warn.mock.calls).toEqual([['test: listing networks: docker printed a line that is not JSON:', 'not json'], + ['test: listing networks: docker printed a line that is not a network:', 'null'], + ['test: listing networks: docker printed a line that is not a network:', '{"ID":"abc"}']]); + // Nothing it could not read is removed or looked up as a holder. + expect(readFileSync(calls, 'utf8').split('\n').filter(line => /^(ps|rm|network rm|volume rm|container rm|container inspect) /.test(line))).toEqual([]); +}); + +it('reads nothing from a docker call that was killed, whose output may be cut short', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'); + // A well-formed network line, then the client is killed by a signal before it exits by itself. The line holds only + // the fields the sweep reads (Docker 27.4 also prints CreatedAt, Driver, IPv6, Internal, Labels and Scope); every + // other call gets no output and exit 0, as Docker 27.4 gives for `ls` with no match. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"${'a'.repeat(64)}","Name":"swept"}'; kill -9 $$; fi`, 'exit 0'].join('\n'), + { mode: 0o755 }); + // The fake does what this test needs: a network line on stdout, then death by a signal. + const direct = spawnSync(join(root, 'docker'), ['network', 'ls'], { encoding: 'utf8' }); + expect([direct.status, direct.signal, direct.stdout]).toEqual([null, 'SIGKILL', `{"ID":"${'a'.repeat(64)}","Name":"swept"}\n`]); + rmSync(calls); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + expect(readFileSync(calls, 'utf8').split('\n').filter(line => /^(ps|network rm) /.test(line))).toEqual([]); + expect(warn.mock.calls).toContainEqual([`test: listing networks: docker network ls --no-trunc --format {{json .}} --filter label=${TEST_RUN_LABEL}=x failed:`, + 'killed by SIGKILL']); +}); + +it('removes by name only an object of exactly that name, and reports failures other than no such object', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'), id = 'b'.repeat(64), other = 'c'.repeat(64), raced = 'd'.repeat(64); + // Answers as Docker 27.4 does for `inspect --format '{{json .Id}} {{json .Name}}'` (a volume: '{{json .Name}}') and + // `rm`, in the parts removeByName reads: stdout of an inspect that succeeded, stderr and the exit status. A missing + // object, an object found under another name (by an ID prefix), a refused removal, an inspect killed by a signal, and + // a failure before the daemon (an unknown context; its meta.json path shortened). Also a network gone between its + // inspect and its removal, which 27.4 answers "network not found", and inspect answers that are not JSON or not + // an ID and a name, which 27.4 is not known to give. The stdout of a failed call (a bare newline for a missing object + // in 27.4, nothing for the others) is not read. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + 'for arg; do name="$arg"; done', + 'if [ "$2" = inspect ]; then case "$1 $name" in', + " 'container gone') echo; echo 'Error response from daemon: No such container: gone' >&2; exit 1;;", + " 'network gone') echo; echo 'Error response from daemon: network gone not found' >&2; exit 1;;", + " 'volume gone') echo; echo 'Error response from daemon: get gone: no such volume' >&2; exit 1;;", + ` 'container prefix') printf '%s\\n' '"${id}" "/someone-else"';;`, + ` 'network prefix') printf '%s\\n' '"${other}" "someone-else"';;`, + ` 'container busy') printf '%s\\n' '"${id}" "/busy"';;`, + ` 'network busy') printf '%s\\n' '"${other}" "busy"';;`, + ` 'volume busy') printf '%s\\n' '"busy"';;`, + ` 'network raced') printf '%s\\n' '"${raced}" "raced"';;`, + " 'container junk') echo 'not json';;", + " 'container odd') echo '1 2';;", + " 'container nospace') echo '\"nospace\"';;", + " 'container optid') echo '\"--help\" \"/optid\"';;", + " 'container killed') kill -9 $$;;", + " 'container lost') echo 'Failed to initialize: unable to resolve docker endpoint: context \"x\": context not found: open /x/meta.json: no such file or directory' >&2; exit 1;;", + 'esac; exit 0; fi', + `if [ "$1" = rm ]; then echo 'Error response from daemon: removal of container ${id} is already in progress' >&2; exit 1; fi`, + `if [ "$1 $2 $3" = 'network rm ${raced}' ]; then echo 'Error response from daemon: network ${raced} not found' >&2; exit 1; fi`, + `if [ "$1 $2" = 'network rm' ]; then echo 'Error response from daemon: error while removing network: network busy id ${other} has active endpoints' >&2; exit 1; fi`, + `if [ "$1 $2" = 'volume rm' ]; then echo 'Error response from daemon: remove busy: volume is in use - [${id}]' >&2; exit 1; fi`, + 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const objects = [['container', 'gone'], ['network', 'gone'], ['volume', 'gone'], ['container', 'prefix'], ['network', 'prefix'], + ['container', 'busy'], ['network', 'busy'], ['volume', 'busy'], ['container', 'killed'], ['container', 'lost'], + ['network', 'raced'], ['container', 'junk'], ['container', 'odd'], ['container', 'nospace'], ['container', 'optid'], + ['container', '--help'], ['volume', '-v'], ['container', '']] as const; + expect(() => { for (const [kind, name] of objects) removeByName(kind, name, 'test', join(root, 'docker')); }).not.toThrow(); + // Each missing name reached its inspect, so the quiet answer was Docker's missing one. + for (const kind of ['container', 'network'] as const) + expect(readFileSync(calls, 'utf8').split('\n')).toContain(`${kind} inspect --format {{json .Id}} {{json .Name}} gone`); + expect(readFileSync(calls, 'utf8').split('\n')).toContain('volume inspect --format {{json .Name}} gone'); + // Only the exact names are removed, by full ID (a volume's name is its ID). + expect(readFileSync(calls, 'utf8').split('\n').filter(line => / rm |^rm /.test(line))) + .toEqual([`rm --force ${id}`, `network rm ${other}`, 'volume rm --force busy', `network rm ${raced}`]); + expect(warn.mock.calls).toEqual([ + [`test: removing container busy: docker rm --force ${id} failed:`, + `Error response from daemon: removal of container ${id} is already in progress`], + [`test: removing network busy: docker network rm ${other} failed:`, + `Error response from daemon: error while removing network: network busy id ${other} has active endpoints`], + ['test: removing volume busy: docker volume rm --force busy failed:', `Error response from daemon: remove busy: volume is in use - [${id}]`], + ['test: removing container killed: docker container inspect --format {{json .Id}} {{json .Name}} killed failed:', 'killed by SIGKILL'], + ['test: removing container lost: docker container inspect --format {{json .Id}} {{json .Name}} lost failed:', + 'Failed to initialize: unable to resolve docker endpoint: context "x": context not found: open /x/meta.json: no such file or directory'], + ['test: removing container junk: docker printed what is not JSON:', 'not json\n'], + ['test: removing container odd: docker printed no ID and name:', '1 2\n'], + ['test: removing container nospace: docker printed no ID and name:', '"nospace"\n'], + ['test: removing container optid: docker printed an ID that is not a full ID:', '"--help" "/optid"\n'], + ['test: removing container --help: refused a name that is empty or could read as an option'], + ['test: removing volume -v: refused a name that is not a volume name Docker allows'], + ['test: removing container : refused a name that is empty or could read as an option'], + ]); +}); + +it('talks to the daemon set when it loaded, though a test changed DOCKER_HOST or DOCKER_CONTEXT since', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const seen = join(root, 'seen'); + // Records the address each call is given, and answers as Docker 27.4 does for a missing container and an empty `ls`. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s|%s\\n' "\${DOCKER_HOST-unset}" "\${DOCKER_CONTEXT-unset}" >> '${seen}'`, + 'if [ "$2" = inspect ]; then echo; echo "Error response from daemon: No such container: x" >&2; exit 1; fi', 'exit 0'].join('\n'), + { mode: 0o755 }); + const loaded = `${process.env.DOCKER_HOST ?? 'unset'}|${process.env.DOCKER_CONTEXT ?? 'unset'}`; + vi.stubEnv('DOCKER_HOST', 'unix:///codeboost-missing.sock'); + vi.stubEnv('DOCKER_CONTEXT', 'codeboost-missing'); + try { + removeByName('container', 'x', 'test', join(root, 'docker')); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + } finally { vi.unstubAllEnvs(); } + const lines = readFileSync(seen, 'utf8').split('\n').filter(Boolean); + expect(lines.length).toBeGreaterThan(1); + expect(new Set(lines)).toEqual(new Set([loaded])); +}); + +it('takes the daemon set at load, not the default, when one was set, for every call', async () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const seen = join(root, 'seen'), net = 'f'.repeat(64), holder = 'a'.repeat(64), named = 'b'.repeat(64), labelled = 'c'.repeat(64); + const volumeHolder = 'e'.repeat(64); + // Records the address and the whole command of each call. Answers as Docker 27.4 does, in the parts the code reads, + // for one labelled container, one swept network and one swept volume, each held by one container, and for a container + // named x; every other call gets no output and exit 0, and the removals' stdout is not read. The network line holds + // only the fields the sweep reads, and the holder's inspect an empty endpoint for its network. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', 'for arg; do last="$arg"; done', + `printf '%s|%s|%s\\n' "\${DOCKER_HOST-unset}" "\${DOCKER_CONTEXT-unset}" "$*" >> '${seen}'`, + `if [ "$1 $2" = "container ls" ]; then printf '%s\\n' ${labelled}; exit 0; fi`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"${net}","Name":"swept"}'; exit 0; fi`, + `if [ "$1 $2" = "volume ls" ]; then printf '%s\\n' swept-volume; exit 0; fi`, + `if [ "$1" = ps ] && [ "$last" = volume=swept-volume ]; then printf '%s\\n' ${volumeHolder}; exit 0; fi`, + `if [ "$1" = ps ]; then printf '%s\\n' ${holder}; exit 0; fi`, + `if [ "$1 $2" = "container inspect" ] && [ "$last" = x ]; then printf '%s\\n' '"${named}" "/x"'; exit 0; fi`, + `if [ "$1 $2" = "container inspect" ]; then printf '%s\\n' '"${holder}" {"swept":{}}'; exit 0; fi`, + 'exit 0'].join('\n'), + { mode: 0o755 }); + // A fresh copy of the module, loaded while a daemon is set, as on a machine with Colima or rootless Docker. + vi.stubEnv('DOCKER_HOST', 'unix:///codeboost-loaded.sock'); + vi.stubEnv('DOCKER_CONTEXT', 'codeboost-loaded'); + vi.resetModules(); + const fresh = await import('./fixtures/docker-sweep.ts') as typeof Sweep; + vi.stubEnv('DOCKER_HOST', 'unix:///codeboost-changed.sock'); + vi.stubEnv('DOCKER_CONTEXT', 'codeboost-changed'); + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + try { + fresh.removeByName('container', 'x', 'test', join(root, 'docker')); + fresh.removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + } finally { vi.unstubAllEnvs(); } + const lines = readFileSync(seen, 'utf8').split('\n').filter(Boolean).map(line => line.split('|')); + const commands = lines.map(line => line[2]); + // Every call the two make, each checked by its own command line. + for (const command of [ + `container inspect --format {{json .Id}} {{json .Name}} x`, `rm --force ${named}`, + `container ls --all --quiet --no-trunc --filter label=${TEST_RUN_LABEL}=x`, `container rm --force ${labelled}`, + `network ls --no-trunc --format {{json .}} --filter label=${TEST_RUN_LABEL}=x`, + 'ps --all --quiet --no-trunc --filter network=swept', + `container inspect --format {{json .Id}} {{json .NetworkSettings.Networks}} ${holder}`, `rm --force ${holder}`, + `network rm ${net}`, + `volume ls --quiet --filter label=${TEST_RUN_LABEL}=x`, 'ps --all --quiet --no-trunc --filter volume=swept-volume', + `rm --force ${volumeHolder}`, 'volume rm --force swept-volume', + ]) expect(commands).toContain(command); + expect(new Set(lines.map(line => `${line[0]}|${line[1]}`))).toEqual(new Set(['unix:///codeboost-loaded.sock|codeboost-loaded'])); +}); + +it('says nothing of a network gone since the listing, and reports any other failure to remove one', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const net = 'd'.repeat(64), answer = join(root, 'answer'), calls = join(root, 'calls'); + // Answers as Docker 27.4 does for one swept network with no holder, whose removal gives the stderr in `answer`; every + // other call gets no output and exit 0, as for `ls` with no match. The network line holds only the fields the sweep + // reads. The second answer is invented, as a failure 27.4 might give. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"${net}","Name":"swept"}'; exit 0; fi`, + `if [ "$1 $2" = "network rm" ]; then cat '${answer}' >&2; exit 1; fi`, 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + const failures = () => warn.mock.calls.filter(call => String(call[0]).endsWith('failed:')); + writeFileSync(answer, `Error response from daemon: network ${net} not found\n`); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + // The removal ran and gave that answer, so the silence is the gone answer's. + expect(readFileSync(calls, 'utf8').split('\n')).toContain(`network rm ${net}`); + expect(failures()).toEqual([]); + writeFileSync(answer, 'Error response from daemon: permission denied\n'); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + expect(failures()).toEqual([[`test: removing networks: docker network rm ${net} failed:`, 'Error response from daemon: permission denied']]); +}); + +it.each([ + ['says nothing of a holder gone since the listing', `Error response from daemon: No such container: ${'b'.repeat(64)}`, []], + ['reports any other failure to read the holders', 'permission denied', [['test: reading holders: docker container inspect --format {{json .Id}} {{json .NetworkSettings.Networks}} HOLDERS failed:', 'permission denied']]], +])('%s, and still removes the holders it read', (_label, stderr, warnings) => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'), net = 'f'.repeat(64), a = 'a'.repeat(64), b = 'b'.repeat(64); + // Answers as Docker 27.4 does for one swept network with two candidate holders, one gone before the inspect: the + // inspect prints the other and exits 1. Every other call gets no output and exit 0, as for `ls` with no match; the + // removals' stdout (the removed IDs or names in 27.4) is not read. The second case's answer is invented: it stands in + // for an unexpected inspect failure that still printed some holders, which 27.4 is not known to give. The network + // line holds only the fields the sweep reads (27.4 also prints CreatedAt, Driver, IPv6, Internal, Labels and Scope), + // and the inspect an empty endpoint for the network, where 27.4 gives its full settings: the sweep reads only the + // network names. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"${net}","Name":"swept"}'; exit 0; fi`, + `if [ "$1" = ps ]; then printf '%s\\n' ${a} ${b}; exit 0; fi`, + `if [ "$1 $2" = "container inspect" ]; then printf '%s\\n' '"${a}" {"swept":{}}'; echo '${stderr}' >&2; exit 1; fi`, + 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + expect(warn.mock.calls.filter(call => String(call[0]).endsWith('failed:'))) + .toEqual(warnings.map(([line, cause]) => [line!.replace('HOLDERS', `${a} ${b}`), cause])); + expect(readFileSync(calls, 'utf8').split('\n')).toContain(`rm --force ${a}`); +}); + +it('finds the real docker once, at load, uses it by default, and refuses a shim when none was found', async () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + // Two directories, each with a docker and the sh the lookup runs, and one with only sh. Each docker records its calls + // and answers as Docker 27.4 does for a missing container and for an `ls` with no match; nothing else is asked of it. + const bin = (name: string, docker: boolean) => { + const dir = join(root, name); mkdirSync(dir); + symlinkSync(spawnSync('sh', ['-c', 'command -v sh'], { encoding: 'utf8' }).stdout.trim(), join(dir, 'sh')); + if (docker) writeFileSync(join(dir, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${join(dir, 'calls')}'`, + 'if [ "$2" = inspect ]; then echo; echo "Error response from daemon: No such container: x" >&2; exit 1; fi', 'exit 0'].join('\n'), + { mode: 0o755 }); + return dir; + }; + const first = bin('first', true), second = bin('second', true), none = bin('none', false); + // A relative PATH entry, resolved at load. + vi.stubEnv('PATH', relative(process.cwd(), first)); + vi.resetModules(); + const loaded = await import('./fixtures/docker-sweep.ts') as typeof Sweep; + // A shim put on PATH later does not change it, and the removals use it when given no docker. + vi.stubEnv('PATH', second); + expect(loaded.REAL_DOCKER).toBe(join(first, 'docker')); + expect(loaded.requireRealDocker()).toBe(join(first, 'docker')); + loaded.removeByName('container', 'x', 'test'); + loaded.removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test'); + expect(readFileSync(join(first, 'calls'), 'utf8').split('\n').filter(Boolean).length).toBeGreaterThan(1); + expect(existsSync(join(second, 'calls'))).toBe(false); + vi.stubEnv('PATH', none); + vi.resetModules(); + const missing = await import('./fixtures/docker-sweep.ts') as typeof Sweep; + expect(() => missing.requireRealDocker()).toThrow('No docker on PATH when the test files loaded.'); +}); + +it('reports a holder line that is not an ID and networks, and removes no holder from it', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'), net = 'd'.repeat(64); + // One swept network with one candidate holder, whose inspect line parses but is not an ID and networks, which Docker + // 27.4 is not known to print, one of them not JSON. Every other call gets no output and exit 0, as for `ls` with no + // match, and the removals' stdout is not read. The network line holds only the fields the sweep reads. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"${net}","Name":"swept"}'; exit 0; fi`, + `if [ "$1" = ps ]; then printf '%s\\n' ${'a'.repeat(64)}; exit 0; fi`, + `if [ "$1 $2" = "container inspect" ]; then printf '%s\\n' '123 null' '"--all" {"swept":{}}' '"${'a'.repeat(64)}"' 'unquoted {}'; exit 0; fi`, 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + // Each line reported once: the one that is not JSON only as that. + expect(warn.mock.calls.filter(call => String(call[0]).startsWith('test: reading holders:'))).toEqual([ + ['test: reading holders: docker printed a line that is not an ID and networks:', '123 null'], + ['test: reading holders: docker printed a line that is not an ID and networks:', '"--all" {"swept":{}}'], + ['test: reading holders: docker printed a line that is not an ID and networks:', `"${'a'.repeat(64)}"`], + ['test: reading holders: docker printed a line that is not JSON:', 'unquoted']]); + expect(readFileSync(calls, 'utf8').split('\n').filter(line => line.startsWith('rm '))).toEqual([]); +}); + +it('reports and skips listed lines that are not a container ID, a network or a volume name, so none becomes an option', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'); + // Lines Docker 27.4 is not known to print for these listings; every other call gets no output and exit 0, as for + // `ls` with no match. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "container ls" ]; then printf '%s\\n' '--all'; exit 0; fi`, + `if [ "$1 $2" = "volume ls" ]; then printf '%s\\n' '-v'; exit 0; fi`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"--help","Name":"x"}'; exit 0; fi`, 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + expect(warn.mock.calls).toEqual([['test: listing containers: docker printed a line that is not a container ID:', '--all'], + ['test: listing networks: docker printed a line that is not a network:', '{"ID":"--help","Name":"x"}'], + ['test: listing volumes: docker printed a line that is not a volume name:', '-v']]); + expect(readFileSync(calls, 'utf8').split('\n').filter(line => / rm |^rm |^ps /.test(line))).toEqual([]); +}); + +it('counts an object gone only on Docker\'s missing answer, and throws on any other failure, at the loaded daemon', async () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const bin = join(root, 'bin'), seen = join(root, 'seen'); mkdirSync(bin); + symlinkSync(spawnSync('sh', ['-c', 'command -v sh'], { encoding: 'utf8' }).stdout.trim(), join(bin, 'sh')); + // Answers as Docker 27.4 does for `inspect` of an object that exists, of a missing container, network and volume, + // and for an unknown context (its meta.json path shortened); one call is killed by a signal. Each call records its + // daemon address. Only the exit status and stderr are read: the stdout is invented (27.4 prints the objects' JSON, or + // `[]` for a missing one). + writeFileSync(join(bin, 'docker'), ['#!/bin/sh', `printf '%s|%s\\n' "\${DOCKER_HOST-unset}" "\${DOCKER_CONTEXT-unset}" >> '${seen}'`, + 'case "$1 $3" in', + " 'container there') echo '[]';;", + " 'container gone') echo; echo 'Error response from daemon: No such container: gone' >&2; exit 1;;", + " 'network gone') echo; echo 'Error response from daemon: network gone not found' >&2; exit 1;;", + " 'volume gone') echo; echo 'Error response from daemon: get gone: no such volume' >&2; exit 1;;", + " 'container killed') kill -9 $$;;", + " 'container lost') echo 'Failed to initialize: unable to resolve docker endpoint: context \"x\": context not found: open /x/meta.json: no such file or directory' >&2; exit 1;;", + " 'container odd') echo 'Error response from daemon: No such container: odd' >&2; exit 2;;", + 'esac; exit 0'].join('\n'), { mode: 0o755 }); + vi.stubEnv('PATH', bin); + vi.stubEnv('DOCKER_HOST', 'unix:///codeboost-loaded.sock'); + vi.stubEnv('DOCKER_CONTEXT', 'codeboost-loaded'); + vi.resetModules(); + const fresh = await import('./fixtures/docker-sweep.ts') as typeof Sweep; + vi.stubEnv('DOCKER_HOST', 'unix:///codeboost-changed.sock'); + vi.stubEnv('DOCKER_CONTEXT', 'codeboost-changed'); + expect(fresh.objectExists('container', 'there')).toBe(true); + for (const kind of ['container', 'network', 'volume'] as const) expect(fresh.objectExists(kind, 'gone')).toBe(false); + expect(() => fresh.objectExists('container', 'killed')).toThrow('docker container inspect killed failed: killed by SIGKILL'); + expect(() => fresh.objectExists('container', 'lost')).toThrow('docker container inspect lost failed: Failed to initialize'); + // The missing answer with another exit status is not Docker's missing answer (invented: 27.4 exits 1). + expect(() => fresh.objectExists('container', 'odd')).toThrow('docker container inspect odd failed: Error response from daemon: No such container: odd'); + expect(() => fresh.objectExists('container', '--help')).toThrow('Refused to inspect container "--help"'); + expect(() => fresh.objectExists('network', '')).toThrow('Refused to inspect network ""'); + expect(new Set(readFileSync(seen, 'utf8').split('\n').filter(Boolean))).toEqual(new Set(['unix:///codeboost-loaded.sock|codeboost-loaded'])); +}); + +it('gives every docker call its time limit, killed with SIGKILL: 30 s for the removals, 60 s for objectExists', async () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const net = 'f'.repeat(64), holder = 'a'.repeat(64), named = 'b'.repeat(64), labelled = 'c'.repeat(64), volumeHolder = 'e'.repeat(64); + // The loaded-daemon test's answers, so every call the two make runs (13), plus an inspect of a missing container for + // objectExists; only the options each call is given are checked. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', 'for arg; do last="$arg"; done', + `if [ "$1 $2 $3" = "container inspect x" ]; then echo; echo 'Error response from daemon: No such container: x' >&2; exit 1; fi`, + `if [ "$1 $2" = "container ls" ]; then printf '%s\\n' ${labelled}; exit 0; fi`, + `if [ "$1 $2" = "network ls" ]; then printf '%s\\n' '{"ID":"${net}","Name":"swept"}'; exit 0; fi`, + `if [ "$1 $2" = "volume ls" ]; then printf '%s\\n' swept-volume; exit 0; fi`, + `if [ "$1" = ps ] && [ "$last" = volume=swept-volume ]; then printf '%s\\n' ${volumeHolder}; exit 0; fi`, + `if [ "$1" = ps ]; then printf '%s\\n' ${holder}; exit 0; fi`, + `if [ "$1 $2" = "container inspect" ] && [ "$last" = x ]; then printf '%s\\n' '"${named}" "/x"'; exit 0; fi`, + `if [ "$1 $2" = "container inspect" ]; then printf '%s\\n' '"${holder}" {"swept":{}}'; exit 0; fi`, + 'exit 0'].join('\n'), + { mode: 0o755 }); + const options: { timeout?: number; killSignal?: unknown }[] = []; + vi.doMock('node:child_process', async original => { + const actual = await original(); + return { ...actual, spawnSync: (command: string, args: string[], given: { timeout?: number; killSignal?: unknown }) => { + if (!['sh'].includes(command)) options.push(given); + return actual.spawnSync(command, args, given as never); + } }; + }); + vi.stubEnv('PATH', `${root}:${process.env.PATH}`); + vi.resetModules(); + try { + const fresh = await import('./fixtures/docker-sweep.ts') as typeof Sweep; + vi.spyOn(console, 'warn').mockImplementation(() => undefined); + fresh.removeByName('container', 'x', 'test', join(root, 'docker')); + fresh.removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + const removals = options.length; + expect(fresh.objectExists('container', 'x')).toBe(false); + // removeByName's inspect and removal, and the sweep's eleven calls. + expect(removals).toBe(13); + expect(options.slice(0, removals).every(given => given.timeout === 30_000 && given.killSignal === 'SIGKILL')).toBe(true); + expect(options.slice(removals)).toMatchObject([{ timeout: 60_000, killSignal: 'SIGKILL' }]); + } finally { vi.doUnmock('node:child_process'); vi.resetModules(); } +}); + +it('reports and skips a holder search line that is not a container ID, so none becomes an option', () => { + const root = mkdtempSync(join(tmpdir(), 'codeboost-sweep-')); roots.push(root); + const calls = join(root, 'calls'); + // One swept volume, whose holder search prints a line Docker 27.4 is not known to print; every other call gets no + // output and exit 0, as for `ls` with no match, and the removals' stdout is not read. + writeFileSync(join(root, 'docker'), ['#!/bin/sh', `printf '%s\\n' "$*" >> '${calls}'`, + `if [ "$1 $2" = "volume ls" ]; then printf '%s\\n' swept-volume; exit 0; fi`, + `if [ "$1" = ps ]; then printf '%s\\n' '--all'; exit 0; fi`, 'exit 0'].join('\n'), + { mode: 0o755 }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', join(root, 'docker')); + expect(warn.mock.calls).toContainEqual(['test: finding holders: docker printed a line that is not a container ID:', '--all']); + expect(readFileSync(calls, 'utf8').split('\n').filter(line => line.startsWith('rm '))).toEqual([]); +}); + +it('reports a docker that cannot start, and never throws', () => { + const missing = join(tmpdir(), `codeboost-no-docker-${randomUUID()}`, 'docker'); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); + expect(() => removeByName('container', 'x', 'test', missing)).not.toThrow(); + expect(() => removeLabelledObjects(TEST_RUN_LABEL, 'x', 'test', missing)).not.toThrow(); + const reports = warn.mock.calls.map(call => `${String(call[0])} ${String(call[1])}`); + expect(reports[0]).toMatch(/^test: removing container x: docker container inspect .* x failed: .*ENOENT/); + expect(reports.slice(1).length).toBe(3); + for (const report of reports.slice(1)) expect(report).toMatch(/^test: listing (containers|networks|volumes): docker .* failed: .*ENOENT/); +}); diff --git a/test/fixtures/docker-sweep.ts b/test/fixtures/docker-sweep.ts index 97c0bf0e..46b76e60 100644 --- a/test/fixtures/docker-sweep.ts +++ b/test/fixtures/docker-sweep.ts @@ -1,25 +1,198 @@ -import { spawnSync } from 'node:child_process'; +import { spawnSync, type SpawnSyncReturns } from 'node:child_process'; +import { isAbsolute, resolve } from 'node:path'; import { RUNNER_LABEL } from '../../agents/labels.ts'; /** - * Removes the containers, networks and volumes labelled with one runner owner, containers first, as they hold the - * others in use. Containers and volumes go by force; a network that another container still uses stays. It reports - * what it finds and any Docker call that fails, and never throws, so a test file's afterAll can call it in a finally - * block without hiding an error from its ordinary cleanup. Each call has a 30 s limit, so all six can take up to - * 3 minutes. - */ -export function removeRunnerObjects(runnerOwner: string): void { - const run = (args: string[]) => { - const result = spawnSync('docker', args, { encoding: 'utf8', timeout: 30_000, killSignal: 'SIGKILL' }); - if (result.status !== 0) console.warn(`afterAll: docker ${args.slice(0, 2).join(' ')} failed:`, - result.error?.message ?? result.stderr.trim()); - return result.status === 0 ? result.stdout.split('\n').filter(Boolean) : []; + * The real `docker`, found once when this module loads, before any test puts a shim on PATH. A sweep that runs while a + * shim is still on PATH (after a test timed out inside one) would otherwise wait on the shim's own child, which the + * call's SIGKILL does not reach. + */ +const FOUND_DOCKER = spawnSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).stdout?.trim(); +// Resolved, as a PATH entry may be relative. +export const REAL_DOCKER = FOUND_DOCKER ? resolve(FOUND_DOCKER) : 'docker'; +/** + * The daemon `docker` talks to when this module loads. A test that times out may leave DOCKER_HOST or DOCKER_CONTEXT + * changed (agent-supervisor points DOCKER_HOST at a missing socket until its handle settles), so every call here + * runs with the values they had when this module loaded. + */ +const LOADED_ENV = { DOCKER_HOST: process.env.DOCKER_HOST, DOCKER_CONTEXT: process.env.DOCKER_CONTEXT }; +const dockerEnv = () => { + const env = { ...process.env }; + for (const [name, value] of Object.entries(LOADED_ENV)) if (value === undefined) delete env[name]; else env[name] = value; + return env; +}; +/** + * A Docker client call's own limit, for a test's synchronous calls (#189): no Vitest timer can stop one, so a stalled + * daemon would otherwise hold the whole suite. + */ +export const CLIENT_LIMIT = { timeout: 60_000, killSignal: 'SIGKILL' as const }; +/** The real `docker` for a shim to call; refuses when none was found, since a shim calling `docker` would call itself. */ +export function requireRealDocker(): string { + if (!isAbsolute(REAL_DOCKER)) throw new Error('No docker on PATH when the test files loaded.'); + return REAL_DOCKER; +} + +/** + * The label a test (most often through its Docker shim) adds to an object it makes without the runner label (#189), + * so the test can still find and remove that object however the test ends. No codeboost code writes or reads it. + */ +export const TEST_RUN_LABEL = 'io.codeboost.test-run'; +/** Beside it, one value per test file run, so afterAll removes them all with one sweep. */ +export const TEST_FILE_LABEL = 'io.codeboost.test-file'; + +/** Docker 27.4's answer to an inspect of a missing object, which names it. */ +const missingAnswer = (kind: 'container' | 'network' | 'volume', ref: string) => + ({ container: `No such container: ${ref}`, network: `network ${ref} not found`, volume: `get ${ref}: no such volume` })[kind]; +/** + * Whether the container, network or volume `ref` exists, for a test's checks: only Docker's own missing answer counts as + * gone, and any other failure (a killed call, a daemon that cannot be reached) throws, naming the command. + */ +export function objectExists(kind: 'container' | 'network' | 'volume', ref: string): boolean { + // The ref is passed on: one that is empty or could read as an option is refused, naming it. + if (!ref || ref.startsWith('-')) throw new Error(`Refused to inspect ${kind} ${JSON.stringify(ref)}: it is empty or could read as an option.`); + const args = [kind, 'inspect', ref]; + const found = spawnSync(REAL_DOCKER, args, { encoding: 'utf8', ...CLIENT_LIMIT, env: dockerEnv() }); + if (found.status === 0) return true; + if (found.status === 1 && found.stderr.includes(missingAnswer(kind, ref))) return false; + throw new Error(`docker ${args.join(' ')} failed: ` + + (found.error?.message ?? (found.signal ? `killed by ${found.signal}` : found.stderr.trim()))); +} + +/** + * Removes the container, network or volume named exactly `name`, by its full ID (a volume by its name, which is its + * ID). Docker would read a missing name as an ID prefix, so an object found under another name is left alone. It + * reports, prefixed with `when`, a failed call and an answer it cannot read, but not an object that is gone: an inspect + * that finds no such object, or a network gone between its inspect and its removal. It never throws; each call has a + * 30 s limit. + */ +export function removeByName(kind: 'container' | 'network' | 'volume', name: string, when: string, docker = REAL_DOCKER): void { + const options = { encoding: 'utf8' as const, timeout: 30_000, killSignal: 'SIGKILL' as const, env: dockerEnv() }; + const failed = (result: SpawnSyncReturns, args: string[]) => console.warn(`${when}: removing ${kind} ${name}: docker ${args.join(' ')} failed:`, + result.error?.message ?? (result.signal ? `killed by ${result.signal}` : result.stderr.trim())); + // The name is passed on too: refused if it could read as an option (a volume's must have the shape Docker allows). + if (kind === 'volume' ? !VOLUME_NAME.test(name) : name.startsWith('-') || !name) { + console.warn(`${when}: removing ${kind} ${name}: refused a name that ${kind === 'volume' ? 'is not a volume name Docker allows' : 'is empty or could read as an option'}`); + return; + } + // As JSON: a network's name may hold spaces. Only a container's name comes with a leading slash. + const inspect = [kind, 'inspect', '--format', kind === 'volume' ? '{{json .Name}}' : '{{json .Id}} {{json .Name}}', name]; + const found = spawnSync(docker, inspect, options); + // Docker 27.4's answers for a missing object, each naming it, so a failure before the daemon (an unknown context's + // "context not found: … no such file or directory") is still reported. + const missing = missingAnswer(kind, name); + if (found.error || found.status !== 0) { if (found.status === null || !found.stderr.includes(missing)) failed(found, inspect); return; } + let id: unknown, actual: unknown; + try { + const at = found.stdout.indexOf(' '); + if (kind !== 'volume' && at < 0) { + console.warn(`${when}: removing ${kind} ${name}: docker printed no ID and name:`, found.stdout); return; + } + [id, actual] = kind === 'volume' ? [name, JSON.parse(found.stdout)] : [JSON.parse(found.stdout.slice(0, at)), JSON.parse(found.stdout.slice(at + 1))]; + } catch { console.warn(`${when}: removing ${kind} ${name}: docker printed what is not JSON:`, found.stdout); return; } + if (typeof id !== 'string' || typeof actual !== 'string') { + console.warn(`${when}: removing ${kind} ${name}: docker printed no ID and name:`, found.stdout); return; + } + // A container's or network's ID is checked too, as it is passed on as an argument. + if (kind !== 'volume' && !OBJECT_ID.test(id)) { + console.warn(`${when}: removing ${kind} ${name}: docker printed an ID that is not a full ID:`, found.stdout); return; + } + if ((kind === 'container' ? actual.replace(/^\//, '') : actual) !== name) return; + const remove = kind === 'container' ? ['rm', '--force', id] : [kind, 'rm', ...(kind === 'volume' ? ['--force'] : []), id]; + const removed = spawnSync(docker, remove, options); + // A network gone since the inspect is gone: Docker 27.4 answers "network not found" (rm --force of a missing + // container or volume exits 0). + const raced = kind === 'network' && removed.status === 1 && removed.stderr.includes(`network ${id} not found`); + if ((removed.error || removed.status !== 0) && !raced) failed(removed, remove); +} + +/** + * Removes the containers, networks and volumes labelled with one runner owner, and every container that holds one of + * those networks or volumes, whatever its labels (see `removeLabelledObjects`). A test file's afterAll calls it after + * its other cleanup, and a test that makes objects under its own runner owner sweeps them, by owner (here, or with + * `removeLabelledObjects(RUNNER_LABEL, owner)`) or by allocation with `removeLabelledObjects`. + */ +export function removeRunnerObjects(runnerOwner: string, when = 'afterAll'): void { + removeLabelledObjects(RUNNER_LABEL, runnerOwner, when); +} + +// A container's or network's full ID, as `--no-trunc` and inspect give it, and the names Docker 27.4 allows a volume +// (two characters at least). +// Every value the code passes on as an argument has one of these shapes, so none can read as an option. +const OBJECT_ID = /^[0-9a-f]{64}$/, VOLUME_NAME = /^[a-zA-Z0-9][a-zA-Z0-9_.-]+$/; +/** + * Removes the containers, networks and volumes that carry `label=value`, containers first, as they hold the others in + * use. Before it removes a network or a volume it also removes every container attached to it or mounting it, labelled + * or not: a test can make a network or volume with one owner and the container that holds it with another, or without + * one. Containers and volumes go by force; a network another container joins meanwhile stays. It reports what it finds + * and any Docker call that fails (but not one that fails only because what it names has gone since the listing: a + * holder it reads, or a network it removes), prefixed with `when`, and never throws, so cleanup can call it without + * hiding an earlier error. Each call has a 30 s limit, so all eleven can take up to 5.5 minutes. It runs `docker`, the + * real one by default (`REAL_DOCKER`). + */ +export function removeLabelledObjects(label: string, value: string, when: string, docker = REAL_DOCKER): void { + const run = (stage: string, args: string[]) => { + const result = spawnSync(docker, args, { encoding: 'utf8', timeout: 30_000, killSignal: 'SIGKILL', env: dockerEnv() }); + // A holder gone since the listing is expected: Docker 27.4 answers "No such container" for it, and prints the rest. + // So is a network gone since the listing (removed by its own test meanwhile): "network not found". + const errors = result.stderr?.trim().split('\n') ?? []; + const gone = result.status === 1 && (stage === 'reading holders' + ? errors.every(line => line.startsWith('Error response from daemon: No such container: ')) + : stage === 'removing networks' && errors.every(line => /^Error response from daemon: network [0-9a-f]{64} not found$/.test(line))); + if (result.status !== 0 && !gone) console.warn(`${when}: ${stage}: docker ${args.join(' ')} failed:`, + result.error?.message ?? (result.signal ? `killed by ${result.signal}` : result.stderr.trim())); + // An inspect of several objects exits 1 if one is gone, yet prints the rest (Docker 27.4), so the output of a call + // that exited by itself is read whatever its status. A killed call's output may be cut short, so it is not. + return result.error || result.status === null ? [] : result.stdout.split('\n').filter(Boolean); + }; + // A container ID line or a volume name line of another shape is reported and skipped: it would be passed on as an + // argument, where a leading dash would read as an option. + const shaped = (stage: string, pattern: RegExp) => (line: string) => { + if (pattern.test(line)) return true; + console.warn(`${when}: ${stage}: docker printed a line that is not ${pattern === OBJECT_ID ? 'a container ID' : 'a volume name'}:`, line); + return false; + }; + // A line Docker printed that is not JSON is reported and skipped, so the sweep never throws. + const parsed = (stage: string, text: string): T[] => { + try { return [JSON.parse(text) as T]; } catch { console.warn(`${when}: ${stage}: docker printed a line that is not JSON:`, text); return []; } }; for (const kind of ['container', 'network', 'volume'] as const) { - const ids = run([kind, 'ls', ...(kind === 'container' ? ['--all'] : []), '--quiet', '--filter', - `label=${RUNNER_LABEL}=${runnerOwner}`]); + // Networks with their names: `ps --filter network=` finds a created, never started container by the name only + // (Docker 27.4). Each is removed by its full ID, which no other network's name can be mistaken for. + // As JSON: a network's name may hold spaces. Only a container's name comes with a leading slash. + const listed = run(`listing ${kind}s`, [kind, 'ls', ...(kind === 'container' ? ['--all'] : []), ...(kind === 'network' + ? ['--no-trunc', '--format', '{{json .}}'] : kind === 'container' ? ['--quiet', '--no-trunc'] : ['--quiet']), + '--filter', `label=${label}=${value}`]) + .flatMap(line => kind === 'network' ? parsed<{ ID?: unknown; Name?: unknown } | null>('listing networks', line) + .filter((object): object is { ID: string; Name: string } => { + if (typeof object?.ID === 'string' && OBJECT_ID.test(object.ID) && typeof object.Name === 'string') return true; + console.warn(`${when}: listing networks: docker printed a line that is not a network:`, line); return false; + }) + : shaped(`listing ${kind}s`, kind === 'container' ? OBJECT_ID : VOLUME_NAME)(line) ? [{ ID: line, Name: line }] : []); + const ids = listed.map(object => object.ID), names = listed.map(object => object.Name); if (!ids.length) continue; - console.warn(`afterAll removes this run's leftover ${kind}s: ${ids.join(' ')}`); - run([kind, 'rm', ...(kind === 'network' ? [] : ['--force']), ...ids]); + console.warn(`${when} removes leftover ${kind}s labelled ${label}=${value}: ${names.join(' ')}`); + if (kind !== 'container') { + // Filters with one key match any of their values. The network filter also matches the start of another network's + // ID, so a container counts only if it is on one of these networks by name (Docker refuses a second local network + // of one name). Containers go by full ID: Docker resolves a name before an ID prefix. + const candidates = run('finding holders', ['ps', '--all', '--quiet', '--no-trunc', + ...names.flatMap(name => ['--filter', `${kind}=${name}`])]).filter(shaped('finding holders', OBJECT_ID)); + const holders = kind === 'volume' || !candidates.length ? candidates : run('reading holders', ['container', 'inspect', '--format', + '{{json .Id}} {{json .NetworkSettings.Networks}}', ...candidates]).flatMap(line => { + const at = line.indexOf(' '); + if (at < 0) { console.warn(`${when}: reading holders: docker printed a line that is not an ID and networks:`, line); return []; } + const id = parsed('reading holders', line.slice(0, at)), on = parsed>('reading holders', line.slice(at + 1)); + if (typeof id[0] === 'string' && OBJECT_ID.test(id[0]) && typeof on[0] === 'object' && on[0] !== null) + return [{ id: id[0], on: Object.keys(on[0]) }]; + // A line already reported as not JSON is not reported again. + if (id.length && on.length) console.warn(`${when}: reading holders: docker printed a line that is not an ID and networks:`, line); + return []; + }).filter(holder => holder.on.some(name => names.includes(name))).map(holder => holder.id); + if (holders.length) { + console.warn(`${when} removes the containers that hold them: ${holders.join(' ')}`); + run('removing holders', ['rm', '--force', ...holders]); + } + } + run(`removing ${kind}s`, [kind, 'rm', ...(kind === 'network' ? [] : ['--force']), ...ids]); } } diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts index edf7bdcf..9983f7d1 100644 --- a/test/runner-workspace.test.ts +++ b/test/runner-workspace.test.ts @@ -4,7 +4,7 @@ import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSy import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { promisify } from 'node:util'; -import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, onTestFinished } from 'vitest'; import { buildAgentImage } from '../agents/container/image.ts'; import { pathIdentityProbeArgs, probeTaskStoragePathIdentity, type TaskFilesystems } from '../agents/container/storage.ts'; import { recoverLeftovers } from '../agents/recovery.ts'; @@ -15,7 +15,7 @@ import { attemptRef, openRunnerRepository } from '../runner/runner-repository.ts import { ReviewService } from '../runner/review.ts'; import { Store } from '../runner/store.ts'; import { createTaskWorkspace } from '../runner/workspace.ts'; -import { removeRunnerObjects } from './fixtures/docker-sweep.ts'; +import { CLIENT_LIMIT, removeRunnerObjects, requireRealDocker } from './fixtures/docker-sweep.ts'; import { fixtureGit as git } from './fixtures/git.ts'; // F2b against the real lane D (#87): each item runs in fresh task storage cloned from the runner-owned repository, an @@ -36,12 +36,15 @@ afterAll(() => { for (const store of stores) store.close(); for (const root of roots) rmSync(root, { recursive: true, force: true }); } finally { - // Then remove the task storage and path identity probes this run still owns, even when a test failed or the cleanup - // above threw. It may not run before a hook timeout. + // Then remove the task storage and path identity probes this run still owns, and every container that holds one of + // their volumes or networks, even when a test failed or the cleanup above threw. It may not run before a hook + // timeout. The hook's 7 minutes leave a minute and a half over this sweep's worst case (eleven calls of 30 s), for + // the cleanup before it. removeRunnerObjects(RUNNER_OWNER); } -}, 4 * 60_000); -const docker = (...args: string[]) => execFileSync('docker', args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim(); +}, 7 * 60_000); +const docker = (...args: string[]) => execFileSync('docker', args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], + ...CLIENT_LIMIT }).trim(); async function setup(agent: Record) { const root = mkdtempSync(join(tmpdir(), 'runner-workspace-')); roots.push(root); @@ -163,7 +166,7 @@ describe('task storage path identity (#162)', () => { // run from another checkout) or a container a dead runner left behind does not count. async function watchedProbe() { const shim = mkdtempSync(join(tmpdir(), 'docker-shim-')); roots.push(shim); - const names = join(shim, 'names'), realDocker = execFileSync('sh', ['-c', 'command -v docker'], { encoding: 'utf8' }).trim(); + const names = join(shim, 'names'), realDocker = requireRealDocker(); // Not a fake: it records the argument after each separate `--name` (the form the probe uses; if that changed, no name // is recorded and the test fails) and runs the real docker with the same arguments, so every exit status, stdout // and stderr the probe reads is Docker's own. @@ -232,31 +235,30 @@ describe('task storage path identity (#162)', () => { expect(probe.volume).toMatch(/^[0-9a-f]{64}$/); expect(docker('volume', 'inspect', '--format', '{{json .Options}}', probe.volume)).toContain('size=1048576,nr_inodes=64,'); } finally { if (probe) docker('rm', '--force', '--volumes', probe.name); } - expect(() => docker('volume', 'inspect', probe.volume)).toThrow(); + expect(() => docker('volume', 'inspect', probe.volume)).toThrow(/no such volume/i); }, 120_000); it('removes a probe a killed runner left, with its volume, in the next startup\'s recovery, and no other runner\'s (#181)', async () => { // Tokens of their own: recovery removes every agent container of its runner, and this file's may have some. const mine = randomBytes(16).toString('hex'), theirs = randomBytes(16).toString('hex'); - try { - const left = leftoverProbe(mine), other = leftoverProbe(theirs); - // Both parts carry the runner's token and the probe kind. - for (const labels of [docker('container', 'inspect', '--format', '{{json .Config.Labels}}', left.name), - docker('volume', 'inspect', '--format', '{{json .Labels}}', left.volume)]) - expect(JSON.parse(labels)).toMatchObject({ 'io.codeboost.runner': mine, 'io.codeboost.probe': 'path-identity' }); - // A container of this runner, named like a probe, without the probe kind label: reported, never removed. - const unmarked = `codeboost-path-identity-${randomUUID()}`; - docker('create', '--name', unmarked, '--label', `io.codeboost.runner=${mine}`, '--entrypoint', 'true', imageId); - const report = await recoverLeftovers(mine, 120_000, { unowned: false }); - expect(report.removed.map(resource => resource.name).sort()).toEqual([left.name, left.volume].sort()); - expect(() => docker('container', 'inspect', left.name)).toThrow(/No such container/); - expect(() => docker('volume', 'inspect', left.volume)).toThrow(/no such volume/i); - expect(report.unowned).toEqual([expect.objectContaining({ name: unmarked, reason: 'unknown-kind' })]); - for (const ref of [other.name, unmarked]) expect(docker('container', 'inspect', '--format', '{{.State.Status}}', ref)).toBe('created'); - expect(docker('volume', 'inspect', '--format', '{{.Name}}', other.volume)).toBe(other.volume); - } finally { - removeRunnerObjects(mine); - removeRunnerObjects(theirs); - } + // This test's own random runners, so no sweep of this run finds them: each is swept when the test finishes, however + // it ends (a timed-out body runs on, and its finally may never run), with the sweep's own budget (#189). + for (const runner of [mine, theirs]) + onTestFinished(() => removeRunnerObjects(runner, 'onTestFinished'), 6 * 60_000); + const left = leftoverProbe(mine), other = leftoverProbe(theirs); + // Both parts carry the runner's token and the probe kind. + for (const labels of [docker('container', 'inspect', '--format', '{{json .Config.Labels}}', left.name), + docker('volume', 'inspect', '--format', '{{json .Labels}}', left.volume)]) + expect(JSON.parse(labels)).toMatchObject({ 'io.codeboost.runner': mine, 'io.codeboost.probe': 'path-identity' }); + // A container of this runner, named like a probe, without the probe kind label: reported, never removed. + const unmarked = `codeboost-path-identity-${randomUUID()}`; + docker('create', '--name', unmarked, '--label', `io.codeboost.runner=${mine}`, '--entrypoint', 'true', imageId); + const report = await recoverLeftovers(mine, 120_000, { unowned: false }); + expect(report.removed.map(resource => resource.name).sort()).toEqual([left.name, left.volume].sort()); + expect(() => docker('container', 'inspect', left.name)).toThrow(/No such container/); + expect(() => docker('volume', 'inspect', left.volume)).toThrow(/no such volume/i); + expect(report.unowned).toEqual([expect.objectContaining({ name: unmarked, reason: 'unknown-kind' })]); + for (const ref of [other.name, unmarked]) expect(docker('container', 'inspect', '--format', '{{.State.Status}}', ref)).toBe('created'); + expect(docker('volume', 'inspect', '--format', '{{.Name}}', other.volume)).toBe(other.volume); }, 120_000); it('runs only the image the trusted builder produced', async () => { await expect(probeTaskStoragePathIdentity(`sha256:${'0'.repeat(64)}`, RUNNER_OWNER)).rejects.toThrow('Agent image was not produced by the trusted validated builder.'); diff --git a/test/vitest-config.test.ts b/test/vitest-config.test.ts index d010c604..3ea47599 100644 --- a/test/vitest-config.test.ts +++ b/test/vitest-config.test.ts @@ -317,7 +317,7 @@ describe('CI workflows', () => { const job = live.jobs['live-probes']!; // No `defaults`, `env`, `strategy` or `container`: any of them could change what the steps run. expect(Object.keys(job).toSorted()).toEqual(['runs-on', 'steps', 'timeout-minutes']); - expect(job).toMatchObject({ 'runs-on': 'ubuntu-latest', 'timeout-minutes': 75 }); + expect(job).toMatchObject({ 'runs-on': 'ubuntu-latest', 'timeout-minutes': 100 }); const token = { CLAUDE_CODE_OAUTH_TOKEN: '${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}' }; expect(job.steps.map(step => Object.keys(step).toSorted())).toEqual([['env', 'name', 'run'], ['uses'], ['uses', 'with'], ['run'], ['env', 'name', 'run'], ['name', 'run']]);