Skip to content

Commit 9c228ad

Browse files
mchwangclaude
andauthored
Sweep leftover path identity probes in startup recovery (#181) (#192)
* Sweep leftover path identity probes in startup recovery (#181) The probe's container and anonymous volume now carry the database's runner token and `io.codeboost.probe=path-identity`, a label shape for runner-scoped objects. Startup recovery, under the runner lock, removes this runner's probe containers (by full ID) and then their volumes (by name), and leaves anything of another runner, anything without the probe kind, and anything with attempt, allocation or other kind labels. A probe without a runner label is reported. Closes #181 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Answer volume removal as Docker 27.4 does; exact probe label wording (#181) Review round 1: the fake daemon exits 0 for a missing volume under --force and refuses a volume a container mounts; tests for a refused probe container and a malformed runner token; docs state the probe exception to the scoped recovery rule and what an orphaned client can do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep a probe volume still in use; recheck the runner label by name (#181) Review round 2: a probe volume that a container recovery leaves still mounts is kept for a later start rather than failing every start; a probe volume counts only with this runner's label, since it is removed by name; a lone leftover volume is tested; doc wording. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Skip objects relabelled before the inspect; fake rm --force as Docker 27.4 (#181) Review round 3: an object listed under this runner but inspected with another's is neither removed nor reported; the fake's rm --force of a missing container exits 0, as Docker 27.4 does; the kept in-use volume's wording says it goes once its holder is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Say exactly when an orphaned probe is kept or missed; keep --rm in the fixture (#181) Review round 4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 46567ea commit 9c228ad

11 files changed

Lines changed: 320 additions & 86 deletions

‎agents/container/storage.ts‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { MAXIMUM_TIMER_MS, runInProcessGroup, type ProcessGroup } from '../proce
1212
import { runTrackedProcess, type ProcessGroupLifecycle } from '../tracked-docker.ts';
1313
import { MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES, TREE_SCRIPT } from './tree-script.ts';
1414
import { ALLOCATION_IN_USE, allocationListCommands, assertResourceOwner, claimAllocationId, hasOwnerLabels,
15-
ownerLabelArgs, releaseAllocationId, type ResourceOwner, UUID_V4 } from '../labels.ts';
15+
ownerLabelArgs, releaseAllocationId, type ResourceOwner, runnerProbeLabels, UUID_V4 } from '../labels.ts';
1616

1717
export interface TaskFilesystems {
1818
readonly keeper: string;
@@ -415,24 +415,37 @@ export const PROBE_VOLUME_MOUNT = ['type=volume', 'target=/work', 'volume-driver
415415
...TASK_VOLUME_OPTIONS(1024 * 1024, 64).map(option => `volume-opt=${option}`)]
416416
// A field with a comma in it is quoted, as Docker's CSV parser reads --mount.
417417
.map(field => field.includes(',') ? `"${field}"` : field).join(',');
418+
/** The probe container's name: a fixed prefix and a UUID v4. Recovery removes only containers named so (#181). */
419+
export const PATH_IDENTITY_PROBE_NAME = new RegExp(`^codeboost-path-identity-${UUID_V4}$`);
420+
/**
421+
* The probe's `docker run` arguments. The container and its anonymous volume carry the runner's token and the probe kind
422+
* (#181), so startup recovery finds what a failed or interrupted probe left.
423+
*/
424+
export function pathIdentityProbeArgs(imageId: string, runnerOwner: string, name: string): string[] {
425+
if (!PATH_IDENTITY_PROBE_NAME.test(name)) throw new Error('The path identity probe needs a name D generates.');
426+
const labels = Object.entries(runnerProbeLabels(runnerOwner, 'path-identity')).map(([key, value]) => `${key}=${value}`);
427+
return ['run', '--rm', '--name', name, ...labels.flatMap(label => ['--label', label]), '--read-only', '--user', '10001:10001',
428+
'--network=none', '--cap-drop=ALL', '--security-opt=no-new-privileges', '--security-opt=seccomp=builtin', '--runtime=runc',
429+
'--pids-limit=32', '--memory=128m', '--cpus=.25',
430+
'--mount', [PROBE_VOLUME_MOUNT, ...labels.map(label => `volume-label=${label}`)].join(','),
431+
'--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT, 'probe', '/work'];
432+
}
418433
/**
419434
* The path identity of task storage (#162): how the filesystem the agent edits, where the runner inspects task changes, tells two
420435
* spellings of a path apart. It is measured, not assumed, on a volume made with the same options as task storage, in a
421436
* container hardened like the keeper. The volume is anonymous and the container `--rm`, so once the container has
422437
* started the daemon removes both when it exits, even if this process does not live to see it. A client stopped before
423438
* then (its deadline) may leave the container created but never started, which `--rm` does not remove, so a failed
424-
* probe removes its container, and that volume, by the unique name it gave it. Not covered: a create that lands after
425-
* the settle window, or a runner that dies before the removal; the probe carries no owner labels, so recovery does not
426-
* find that container (`docker ps -a --filter name=codeboost-path-identity-` does). Nor is the volume's removal
427-
* confirmed: `--rm` and `rm --volumes` remove it, but Docker only logs a failure to. A recovery sweep is #181.
439+
* probe removes its container, and that volume, by the unique name it gave it. What this cannot remove (a create that
440+
* lands after the settle window, a runner that dies before the removal, a volume Docker failed to remove, since it
441+
* only logs that) carries `runnerOwner` and the probe kind, and the next startup's recovery removes it (#181).
428442
*/
429-
export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs = 60_000, cleanupMs = PROBE_CLEANUP_MS): Promise<PathIdentity> {
443+
export async function probeTaskStoragePathIdentity(imageId: string, runnerOwner: string, timeoutMs = 60_000,
444+
cleanupMs = PROBE_CLEANUP_MS): Promise<PathIdentity> {
430445
if (!/^sha256:[0-9a-f]{64}$/.test(imageId)) throw new Error('The path identity probe requires the immutable built image ID.');
431446
assertBuiltAgentImage(imageId);
432447
const name = `codeboost-path-identity-${randomUUID()}`;
433-
const args = ['run', '--rm', '--name', name, '--read-only', '--user', '10001:10001', '--network=none', '--cap-drop=ALL',
434-
'--security-opt=no-new-privileges', '--security-opt=seccomp=builtin', '--runtime=runc', '--pids-limit=32', '--memory=128m',
435-
'--cpus=.25', '--mount', PROBE_VOLUME_MOUNT, '--entrypoint', 'sh', imageId, '-c', PATH_IDENTITY_SCRIPT, 'probe', '/work'];
448+
const args = pathIdentityProbeArgs(imageId, runnerOwner, name);
436449
const outcome = await runInProcessGroup('docker', args, { env: dockerEnvironment(), timeoutMs });
437450
if (outcome.status !== 0) {
438451
const failure = new DockerError(args, outcome);
@@ -445,7 +458,7 @@ export async function probeTaskStoragePathIdentity(imageId: string, timeoutMs =
445458
const refused = outcome.status !== null && /Error response from daemon/.test(outcome.stderr);
446459
try { await removeProbeContainer(name, refused ? 0 : PROBE_SETTLE_MS, cleanupMs); }
447460
catch (error) {
448-
throw new Error(`${failure.message} The probe container ${name} may remain; remove it with docker rm --force --volumes ${name}.`,
461+
throw new Error(`${failure.message} The probe container ${name} may remain; the next start's recovery removes it, or remove it now with docker rm --force --volumes ${name}.`,
449462
{ cause: new AggregateError([failure, error]) });
450463
}
451464
throw failure;

‎agents/labels.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,3 +87,23 @@ export const ownerLabels = (owner: ResourceOwner): Readonly<Record<string, strin
8787
export const hasOwnerLabels = (labels: Readonly<Record<string, string>> | null | undefined, owner: ResourceOwner) =>
8888
labels?.[RUNNER_LABEL] === owner.runnerOwner && labels?.[ATTEMPT_LABEL] === owner.attemptId
8989
&& labels?.[ALLOCATION_LABEL] === owner.allocationId;
90+
91+
/**
92+
* The kind label of a runner-scoped probe (#181): an object that belongs to the runner as a whole, not to an attempt or
93+
* an allocation, so it carries only the runner label and this one. Startup recovery, under the runner lock, removes
94+
* this runner's leftover probes: only one process holds a runner token at a time, so a probe found then is an earlier
95+
* process's. A Docker client that process left may still be creating one: if the create lands between recovery's
96+
* container and volume listings, recovery keeps its volume, which the new container holds; if it lands after them,
97+
* recovery does not see it. Either way the next start removes both.
98+
*/
99+
export const PROBE_LABEL = 'io.codeboost.probe';
100+
/** The probes D runs: `path-identity` measures task storage's path identity at runner startup (#162). */
101+
export type RunnerProbe = 'path-identity';
102+
export const RUNNER_PROBES: readonly RunnerProbe[] = Object.freeze(['path-identity']);
103+
104+
/** The labels on a runner-scoped probe's container and volume. Validates the runner token before it is written. */
105+
export function runnerProbeLabels(runnerOwner: string, probe: RunnerProbe): Readonly<Record<string, string>> {
106+
if (!isRunnerOwner(runnerOwner)) throw new Error('runnerOwner must be 32 lowercase hex characters.');
107+
if (!RUNNER_PROBES.includes(probe)) throw new Error('Unknown runner probe.');
108+
return Object.freeze({ [RUNNER_LABEL]: runnerOwner, [PROBE_LABEL]: probe });
109+
}

‎agents/recovery.ts‎

Lines changed: 46 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import { DOCKER_ID } from './client-outcome.ts';
22
import { runDocker, type DockerOutcome } from './docker.ts';
3-
import { ALLOCATION_LABEL, ATTEMPT_LABEL, isAllocationId, isLabelAttemptId, isRunnerOwner,
3+
import { ALLOCATION_LABEL, ATTEMPT_LABEL, isAllocationId, isLabelAttemptId, isRunnerOwner, PROBE_LABEL,
44
RUNNER_LABEL } from './labels.ts';
5-
import { adoptRecoveredTaskStorage, hasLiveTaskStorage, RecoveredStorageRejected, type RecoveredTaskStorage,
6-
type TaskStorageParts } from './container/storage.ts';
5+
import { adoptRecoveredTaskStorage, hasLiveTaskStorage, PATH_IDENTITY_PROBE_NAME, RecoveredStorageRejected,
6+
type RecoveredTaskStorage, type TaskStorageParts } from './container/storage.ts';
77

88
/** A Docker object recovery found, with the labels it carries. */
99
export interface RecoveredResource {
@@ -26,7 +26,7 @@ export interface UnownedResource extends RecoveredResource {
2626
export interface RecoveryReport {
2727
/**
2828
* Agent containers, egress proxies, seeders, export, inspection and commit containers and networks of this runner,
29-
* now confirmed gone.
29+
* and its path identity probes' containers and volumes (#181), now confirmed gone.
3030
*/
3131
readonly removed: readonly RecoveredResource[];
3232
/** One handle per task-storage allocation of this runner, kept whole for export and `removeTaskFilesystems`. */
@@ -48,7 +48,9 @@ const EGRESS_LABEL = 'io.codeboost.egress';
4848
const STORAGE_LABEL = 'io.codeboost.task-storage';
4949
// Every label a codeboost object of any build carries at least one of; used to find objects without a runner label.
5050
// Each Docker type is scanned for all of them, so an object with a label D never puts on its type is still found.
51-
const KIND_LABELS = [INVOCATION_LABEL, EGRESS_LABEL, STORAGE_LABEL, ALLOCATION_LABEL] as const;
51+
const KIND_LABELS = [INVOCATION_LABEL, EGRESS_LABEL, STORAGE_LABEL, ALLOCATION_LABEL, PROBE_LABEL] as const;
52+
// The name Docker gives an anonymous volume, as the path identity probe's is.
53+
const ANONYMOUS_VOLUME = /^[0-9a-f]{64}$/;
5254
const DIAGNOSTIC_LIMIT = 1_000;
5355
// Objects per `docker inspect`, so a daemon with many codeboost objects cannot exceed the argument-size limit.
5456
const INSPECT_BATCH = 200;
@@ -111,19 +113,27 @@ const inspectBatch = async (kind: Kind, refs: readonly string[], remaining: () =
111113
};
112114
const unique = (values: readonly string[]) => [...new Set(values)];
113115

114-
// Remove one object by its full ID, and confirm it is gone.
116+
/** Docker refused to remove a volume because a container mounts it. */
117+
class VolumeInUse extends Error {}
118+
// Remove one object by its full ID (a volume by its name, which is its identity), and confirm it is gone.
115119
const removeById = async (resource: RecoveredResource, remaining: () => number) => {
116-
const args = resource.kind === 'network' ? ['network', 'rm', resource.id!] : ['rm', '--force', resource.id!];
120+
const ref = resource.kind === 'volume' ? resource.name : resource.id!;
121+
const args = resource.kind === 'network' ? ['network', 'rm', ref]
122+
: resource.kind === 'volume' ? ['volume', 'rm', '--force', ref] : ['rm', '--force', ref];
117123
const result = await runDocker(args, { timeoutMs: remaining() });
118124
if (result.status === 0) return;
119-
if (!absent(await runDocker([resource.kind, 'inspect', resource.id!], { timeoutMs: remaining() })))
125+
// Docker 27.4: "remove <name>: volume is in use - [<container IDs>]".
126+
if (resource.kind === 'volume' && /volume is in use/.test(result.stderr))
127+
throw new VolumeInUse(`Volume ${resource.name} is in use.`);
128+
if (!absent(await runDocker([resource.kind, 'inspect', ref], { timeoutMs: remaining() })))
120129
throw new Error(`Could not remove ${resource.kind} ${resource.name}: ${(result.error?.message ?? result.stderr).trim()}`);
121130
};
122131

123132
/**
124133
* Crash recovery for one database (#51 item 4). Acts only on objects whose `io.codeboost.runner` label is
125134
* `runnerOwner`: it removes agent containers, egress proxies, seeders, export, inspection and commit containers and
126-
* networks, and resolves only once they are gone. It keeps task storage whole (volumes and keeper) and returns a recovery handle per
135+
* networks, and the containers and anonymous volumes path identity probes left (#181), and resolves only once they are
136+
* gone. It keeps task storage whole (volumes and keeper) and returns a recovery handle per
127137
* allocation. Objects from older builds without a runner label, and anything it does not recognise, are reported and
128138
* never touched.
129139
*
@@ -151,7 +161,10 @@ export async function recoverLeftovers(runnerOwner: string, timeoutMs = 120_000,
151161
network: [] as RecoveredResource[] };
152162
const unowned: UnownedResource[] = [];
153163
for (const kind of ['container', 'volume', 'network'] as const) {
154-
owned[kind] = await inspect(kind, await list(kind, `${RUNNER_LABEL}=${runnerOwner}`, remaining), remaining);
164+
// An object inspected with another runner's label replaced the one listed under the same name: it is not this
165+
// runner's, so recovery neither removes nor reports it.
166+
owned[kind] = (await inspect(kind, await list(kind, `${RUNNER_LABEL}=${runnerOwner}`, remaining), remaining))
167+
.filter(resource => resource.labels[RUNNER_LABEL] === runnerOwner);
155168
if (options.unowned === false) continue;
156169
// Docker cannot filter on a missing label, so list every codeboost object of this kind and keep those without one.
157170
const candidates = unique((await Promise.all(KIND_LABELS.map(label => list(kind, label, remaining)))).flat());
@@ -173,10 +186,15 @@ export async function recoverLeftovers(runnerOwner: string, timeoutMs = 120_000,
173186
storage.set(allocationId, group);
174187
};
175188
// D writes exactly one kind label on each object, with complete owner labels (an egress object's egress label is its
176-
// allocation ID). Recovery has no creation-time ID to prove ownership, so any other combination is not D's and is
177-
// left alone.
189+
// allocation ID). A runner-scoped probe carries the probe label and no attempt or allocation. Recovery has no
190+
// creation-time ID to prove ownership, so any other combination is not D's and is left alone.
178191
const kindOf = (resource: RecoveredResource) => {
179192
const labels = resource.labels;
193+
if (PROBE_LABEL in labels) {
194+
const scoped = [ATTEMPT_LABEL, ALLOCATION_LABEL, INVOCATION_LABEL, EGRESS_LABEL, STORAGE_LABEL]
195+
.some(label => label in labels);
196+
return !scoped && labels[PROBE_LABEL] === 'path-identity' ? 'probe:path-identity' : undefined;
197+
}
180198
if (!isLabelAttemptId(labels[ATTEMPT_LABEL]) || !isAllocationId(labels[ALLOCATION_LABEL])) return undefined;
181199
const present = [INVOCATION_LABEL, EGRESS_LABEL, STORAGE_LABEL].filter(label => label in labels);
182200
if (present.length !== 1) return undefined;
@@ -196,12 +214,17 @@ export async function recoverLeftovers(runnerOwner: string, timeoutMs = 120_000,
196214
|| kind === 'storage:inspect' || kind === 'storage:commit'
197215
|| kind === 'agent' || kind === 'egress')
198216
remove.push(resource);
217+
// Only one process holds this runner's token, and it probes after recovery, so a probe found now is an earlier
218+
// process's. One that process's orphaned client creates during the scan holds its volume, which is then kept.
219+
else if (kind === 'probe:path-identity' && PATH_IDENTITY_PROBE_NAME.test(resource.name)) remove.push(resource);
199220
else unknown(resource);
200221
}
222+
const probeVolumes: RecoveredResource[] = [];
201223
for (const resource of owned.volume) {
202224
const kind = kindOf(resource);
203225
if (kind === 'storage:work') keep(resource, 'workVolume');
204226
else if (kind === 'storage:metadata') keep(resource, 'metadataVolume');
227+
else if (kind === 'probe:path-identity' && ANONYMOUS_VOLUME.test(resource.name)) probeVolumes.push(resource);
205228
else unknown(resource);
206229
}
207230
const networks = owned.network.filter(resource => {
@@ -210,14 +233,20 @@ export async function recoverLeftovers(runnerOwner: string, timeoutMs = 120_000,
210233
return false;
211234
});
212235

213-
// Containers first: a network cannot be removed while an agent or proxy is still attached to it.
236+
// Containers first: a network cannot be removed while an agent or proxy is still attached to it, nor a probe's
237+
// volume while its container still exists.
214238
const failures: unknown[] = [], confirmed: RecoveredResource[] = [];
215-
for (const resource of [...remove, ...networks]) {
216-
if (resource.kind === 'network' && failures.length) break;
239+
for (const resource of [...remove, ...probeVolumes, ...networks]) {
240+
if (resource.kind !== 'container' && failures.length) break;
217241
try {
218242
await removeById(resource, remaining);
219243
confirmed.push(resource);
220-
} catch (error) { failures.push(error); }
244+
} catch (error) {
245+
// Every container recovery removes is gone by now, so the holder is one it leaves: one it reports as unowned, one
246+
// without codeboost labels, or a probe an earlier process's orphaned client created between the container and
247+
// volume listings. The volume is kept, and a start after its holder is gone removes it.
248+
if (!(error instanceof VolumeInUse)) failures.push(error);
249+
}
221250
}
222251
if (failures.length) {
223252
const detail = failures.map(error => (error as Error).message).join('; ');
@@ -242,6 +271,6 @@ export async function recoverLeftovers(runnerOwner: string, timeoutMs = 120_000,
242271
for (const resource of group.resources) unowned.push(Object.freeze({ ...resource, reason: 'inconsistent-storage' }));
243272
}
244273
}
245-
return Object.freeze({ removed: Object.freeze([...remove, ...networks]), storage: Object.freeze(handles),
274+
return Object.freeze({ removed: Object.freeze(confirmed), storage: Object.freeze(handles),
246275
unowned: Object.freeze(unowned) });
247276
}

0 commit comments

Comments
 (0)