Every Docker client call the branch adds or moves in a test has a time limit (#189)

Review round 49: synchronous calls cannot be stopped by a Vitest timer, so each
one the branch added, moved or rewrote (removals, inspects, the foreign creates
and the ack-script run) takes CLIENT_LIMIT, 60 s with SIGKILL, as main's docker()
helper did; the supervisor's removal message names a kill by signal.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: mchwang

test/agent-container.test.ts

@@ -1,5 +1,5 @@
 import { execFileSync, spawnSync } from 'node:child_process';
-import { removeByName, removeLabelledObjects, removeRunnerObjects, requireRealDocker, TEST_FILE_LABEL, TEST_RUN_LABEL } from './fixtures/docker-sweep.ts';
+import { CLIENT_LIMIT, 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,
@@ -490,8 +490,8 @@ describe('real Docker agent isolation', () => {
       docker('rm', '--force', valid.name); containers.delete(valid.name);
     } finally {
       // The container first: a volume it mounts cannot be removed.
-      spawnSync('docker', ['rm', '--force', valid.name], { stdio: 'ignore' });
-      spawnSync('docker', ['volume', 'rm', '--force', rogue], { stdio: 'ignore' });
+      spawnSync('docker', ['rm', '--force', valid.name], { stdio: 'ignore', ...CLIENT_LIMIT });
+      spawnSync('docker', ['volume', 'rm', '--force', rogue], { stdio: 'ignore', ...CLIENT_LIMIT });
     }
   }, 60_000);
 
@@ -819,7 +819,7 @@ describe('real Docker agent isolation', () => {
       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' });
+        { encoding: 'utf8', ...CLIENT_LIMIT });
       expect(partial.status).not.toBe(0);
       expect(partial.stdout).toContain(holders[0]);
       expect(partial.stderr).toMatch(/^Error response from daemon: No such container: codeboost-gone-/m);
@@ -846,7 +846,7 @@ describe('real Docker agent isolation', () => {
       const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined);
       try {
         for (const [kind, name] of byName) removeByName(kind, name, 'test');
-        expect(byName.filter(([kind, name]) => spawnSync('docker', [kind, 'inspect', name], { stdio: 'ignore' }).status === 0)).toEqual([]);
+        expect(byName.filter(([kind, name]) => spawnSync('docker', [kind, 'inspect', name], { stdio: 'ignore', ...CLIENT_LIMIT }).status === 0)).toEqual([]);
         for (const [kind, name] of byName) removeByName(kind, name, 'test');
         expect(warn).not.toHaveBeenCalled();
       } finally { warn.mockRestore(); }
@@ -856,7 +856,7 @@ describe('real Docker agent isolation', () => {
       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');
-      const exists = (kind: string, ref: string) => spawnSync('docker', [kind, 'inspect', ref], { stdio: 'ignore' }).status === 0;
+      const exists = (kind: string, ref: string) => spawnSync('docker', [kind, 'inspect', ref], { stdio: 'ignore', ...CLIENT_LIMIT }).status === 0;
       for (const holder of holders) expect(exists('container', holder)).toBe(false);
       expect([exists('network', network), exists('network', prefixNamed), exists('volume', volume)]).toEqual([false, false, false]);
       expect(exists('container', labelled)).toBe(false);
@@ -893,7 +893,7 @@ describe('real Docker agent isolation', () => {
     // 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(spawnSync('docker', ['container', 'inspect', left.keeper], { stdio: 'ignore' }).status).not.toBe(0);
+    expect(spawnSync('docker', ['container', 'inspect', left.keeper], { stdio: 'ignore', ...CLIENT_LIMIT }).status).not.toBe(0);
     // Its volumes carry the runner label, so ordinary cleanup can remove them now that no keeper holds them.
     removeTaskFilesystems(left);
     expect(byAllocation(allocationId)).toEqual([]);
@@ -2559,7 +2559,7 @@ describe('real Docker agent isolation', () => {
     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);
+      expect(spawnSync('docker', [kind, 'inspect', ref], { stdio: 'ignore', ...CLIENT_LIMIT }).status).not.toBe(0);
   }, 180_000);
 
   it('never removes a task keeper replaced by another runner, but still removes the owned volumes', () => {
@@ -2580,7 +2580,7 @@ describe('real Docker agent isolation', () => {
       expect(spawnSync('docker', ['volume', 'inspect', filesystems.workVolume]).status).not.toBe(0);
       passed = true;
     } finally {
-      spawnSync('docker', ['rm', '--force', filesystems.keeper], { stdio: 'ignore' });
+      spawnSync('docker', ['rm', '--force', filesystems.keeper], { stdio: 'ignore', ...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 storage must release once the foreign keeper is
       // gone, so its failure fails the test. The sweep may take up to 5.5 minutes, which the test's budget allows for.

test/agent-network.test.ts

@@ -9,7 +9,7 @@ import { assertVendorNetwork, createVendorNetwork, removeVendorNetwork, VENDOR_H
   VendorNetworkCreationCleanupError,
   type VendorNetwork } from '../agents/network/network.ts';
 import { captureInvocation, type InvocationInput } from '../agents/contract.ts';
-import { removeByName, removeLabelledObjects, removeRunnerObjects, requireRealDocker, TEST_FILE_LABEL } from './fixtures/docker-sweep.ts';
+import { CLIENT_LIMIT, 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');
@@ -135,7 +135,7 @@ describe('vendor-only egress', () => {
       const made = existsSync(recorded) ? readFileSync(recorded, 'utf8').split('\n').filter(Boolean) : [];
       foreign = made[0] ?? '';
       // afterAll reads the record again: the object may land after this removal (#189).
-      if (foreign) survived = spawnSync('docker', ['network', 'inspect', foreign], { stdio: 'ignore' }).status === 0;
+      if (foreign) survived = spawnSync('docker', ['network', 'inspect', foreign], { stdio: 'ignore', ...CLIENT_LIMIT }).status === 0;
       for (const name of made) removeByName('network', name, 'finally');
       rmSync(shim, { recursive: true, force: true });
     }
@@ -191,7 +191,7 @@ describe('vendor-only egress', () => {
       const made = existsSync(recorded) ? readFileSync(recorded, 'utf8').split('\n').filter(Boolean) : [];
       foreign = made[0] ?? '';
       // afterAll reads the record again: the object may land after this removal (#189).
-      if (foreign) survived = spawnSync('docker', ['container', 'inspect', foreign], { stdio: 'ignore' }).status === 0;
+      if (foreign) survived = spawnSync('docker', ['container', 'inspect', foreign], { stdio: 'ignore', ...CLIENT_LIMIT }).status === 0;
       for (const name of made) removeByName('container', name, 'finally');
       rmSync(shim, { recursive: true, force: true });
     }
@@ -247,7 +247,7 @@ describe('vendor-only egress', () => {
       const made = existsSync(recorded) ? readFileSync(recorded, 'utf8').split('\n').filter(Boolean) : [];
       impostor = made[0] ?? '';
       // afterAll reads the record again: the object may land after this removal (#189).
-      if (impostor) survived = spawnSync('docker', ['container', 'inspect', impostor], { stdio: 'ignore' }).status === 0;
+      if (impostor) survived = spawnSync('docker', ['container', 'inspect', impostor], { stdio: 'ignore', ...CLIENT_LIMIT }).status === 0;
       for (const name of made) removeByName('container', name, 'finally');
       rmSync(shim, { recursive: true, force: true });
     }

test/agent-supervisor.test.ts

@@ -1,6 +1,6 @@
 import { randomBytes, randomUUID } from 'node:crypto';
 import { execFileSync, spawnSync } from 'node:child_process';
-import { removeByName, removeRunnerObjects, requireRealDocker } from './fixtures/docker-sweep.ts';
+import { CLIENT_LIMIT, 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';
@@ -393,7 +393,7 @@ describe('container invocation supervisor', () => {
     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' });
+        '--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/);
@@ -407,8 +407,8 @@ describe('container invocation supervisor', () => {
     } finally {
       // 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' });
-      if (passed && removed.status !== 0) throw new Error(`Removing ${current.name} failed: ${removed.error?.message ?? removed.stderr?.trim()}`);
+      const removed = spawnSync('docker', ['rm', '--force', current.name], { encoding: 'utf8', ...CLIENT_LIMIT });
+      if (passed && removed.status !== 0) throw new Error(`Removing ${current.name} failed: ${removed.error?.message ?? (removed.signal ? `killed by ${removed.signal}` : removed.stderr?.trim())}`);
     }
   }, 3 * 60_000);
 
@@ -450,7 +450,7 @@ describe('container invocation supervisor', () => {
     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' });
+        '--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);
@@ -658,7 +658,7 @@ describe('container invocation supervisor', () => {
       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' });
+        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' });

test/fixtures/docker-sweep.ts

@@ -21,6 +21,11 @@ const dockerEnv = () => {
   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.');