Skip to content

Commit 454d319

Browse files
eoinshaeoinsha
committed
fix: prune old build containers
1 parent 35370f4 commit 454d319

4 files changed

Lines changed: 235 additions & 3 deletions

File tree

‎src/build-container.ts‎

Lines changed: 106 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import { spawnSync } from 'node:child_process';
22

33
export const BUILDER_LABEL = 'com.fourtheorem.uv-python-lambda.builder';
4+
const BUILDER_KEY_LABEL = 'com.fourtheorem.uv-python-lambda.builder-key';
5+
const BUILDER_PID_LABEL = 'com.fourtheorem.uv-python-lambda.builder-owner-pid';
46

57
const managedBuilders = new Map<string, string>();
68

79
let cleanupRegistered = false;
10+
let cleanupInProgress = false;
811

912
interface DockerResult {
1013
readonly stdout: string;
@@ -16,6 +19,7 @@ export interface BuilderContainerOptions {
1619
readonly name: string;
1720
readonly args: string[];
1821
readonly readyLog: string;
22+
readonly builderKey?: string;
1923
readonly timeoutMs?: number;
2024
}
2125

@@ -30,6 +34,9 @@ export function ensureBuilderContainer(
3034
}
3135

3236
pruneExitedBuilderContainers();
37+
if (options.builderKey) {
38+
pruneOrphanedBuilderContainers(options.builderKey, options.name);
39+
}
3340
removeContainer(options.name);
3441

3542
const dockerRun = runDockerCommand(options.args);
@@ -57,10 +64,16 @@ export function ensureBuilderContainer(
5764
}
5865

5966
export function cleanupBuilderContainers() {
67+
if (cleanupInProgress) {
68+
return;
69+
}
70+
71+
cleanupInProgress = true;
6072
for (const containerId of managedBuilders.values()) {
6173
removeContainer(containerId);
6274
}
6375
managedBuilders.clear();
76+
cleanupInProgress = false;
6477
}
6578

6679
export function getManagedBuilderContainerNames(): string[] {
@@ -88,6 +101,42 @@ export function pruneExitedBuilderContainers() {
88101
}
89102
}
90103

104+
function pruneOrphanedBuilderContainers(builderKey: string, currentName: string) {
105+
const matching = runDockerCommand([
106+
'ps',
107+
'-aq',
108+
'--filter',
109+
`label=${BUILDER_LABEL}=true`,
110+
'--filter',
111+
`label=${BUILDER_KEY_LABEL}=${builderKey}`,
112+
]);
113+
114+
if (matching.status !== 0) {
115+
throw new Error(
116+
`Failed to list builder containers for ${builderKey}: ${matching.stderr}`,
117+
);
118+
}
119+
120+
for (const containerId of matching.stdout.split(/\s+/).filter(Boolean)) {
121+
const metadata = inspectBuilderMetadata(containerId);
122+
if (!metadata) {
123+
continue;
124+
}
125+
126+
if (metadata.name === currentName) {
127+
continue;
128+
}
129+
130+
const ownerPid =
131+
metadata.ownerPid ?? parsePidFromBuilderName(metadata.name, builderKey);
132+
if (ownerPid === undefined || isProcessAlive(ownerPid)) {
133+
continue;
134+
}
135+
136+
removeContainer(containerId);
137+
}
138+
}
139+
91140
function waitForContainerReady(
92141
containerId: string,
93142
readyLog: string,
@@ -145,6 +194,25 @@ function removeContainer(containerIdOrName: string) {
145194
runDockerCommand(['rm', '-f', containerIdOrName]);
146195
}
147196

197+
function inspectBuilderMetadata(containerId: string) {
198+
const result = runDockerCommand([
199+
'inspect',
200+
'--format',
201+
`{{.Name}}\n{{index .Config.Labels "${BUILDER_PID_LABEL}"}}`,
202+
containerId,
203+
]);
204+
205+
if (result.status !== 0) {
206+
return undefined;
207+
}
208+
209+
const [rawName = '', rawOwnerPid = ''] = result.stdout.split('\n');
210+
return {
211+
name: rawName.replace(/^\//, '').trim(),
212+
ownerPid: parseOptionalPid(rawOwnerPid),
213+
};
214+
}
215+
148216
function runDockerCommand(args: string[]): DockerResult {
149217
const result = spawnSync('docker', args, {
150218
encoding: 'utf8',
@@ -164,9 +232,12 @@ function registerCleanupHandlers() {
164232

165233
cleanupRegistered = true;
166234

167-
process.on('exit', cleanupBuilderContainers);
168-
for (const signal of ['SIGINT', 'SIGTERM'] as const) {
169-
process.on(signal, () => {
235+
for (const event of ['beforeExit', 'exit'] as const) {
236+
process.once(event, cleanupBuilderContainers);
237+
}
238+
239+
for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP'] as const) {
240+
process.once(signal, () => {
170241
cleanupBuilderContainers();
171242
process.exit(1);
172243
});
@@ -176,3 +247,35 @@ function registerCleanupHandlers() {
176247
function sleep(ms: number) {
177248
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
178249
}
250+
251+
function parseOptionalPid(value: string) {
252+
const parsed = Number.parseInt(value.trim(), 10);
253+
return Number.isInteger(parsed) && parsed > 0 ? parsed : undefined;
254+
}
255+
256+
function parsePidFromBuilderName(name: string, builderKey: string) {
257+
const prefix = `${builderKey}-`;
258+
if (!name.startsWith(prefix)) {
259+
return undefined;
260+
}
261+
262+
return parseOptionalPid(name.slice(prefix.length));
263+
}
264+
265+
function isProcessAlive(pid: number) {
266+
try {
267+
process.kill(pid, 0);
268+
return true;
269+
} catch (error) {
270+
if (
271+
typeof error === 'object' &&
272+
error !== null &&
273+
'code' in error &&
274+
error.code === 'EPERM'
275+
) {
276+
return true;
277+
}
278+
279+
return false;
280+
}
281+
}

‎src/bundling.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ export const DEFAULT_UV_VERSION = '0.5.27';
3737
const BUILDER_TOOL_DIR = '/opt/uv-python-lambda';
3838
const BUILDER_READY_LOG = 'Builder container is ready and waiting';
3939
const BUILDER_NOFILE_LIMIT = '1048576:1048576';
40+
const BUILDER_OWNER_PID_LABEL =
41+
'com.fourtheorem.uv-python-lambda.builder-owner-pid';
4042

4143
export interface BundlingProps extends BundlingOptions {
4244
/**
@@ -191,6 +193,8 @@ export class Bundling {
191193
`${BUILDER_LABEL}=true`,
192194
'--label',
193195
`com.fourtheorem.uv-python-lambda.builder-key=${this.containerBuilderKey}`,
196+
'--label',
197+
`${BUILDER_OWNER_PID_LABEL}=${process.pid}`,
194198
'--name',
195199
this.containerBuilderName,
196200
];
@@ -232,6 +236,7 @@ export class Bundling {
232236
);
233237

234238
ensureBuilderContainer({
239+
builderKey: this.containerBuilderKey,
235240
name: this.containerBuilderName,
236241
args: dockerArgs,
237242
readyLog: BUILDER_READY_LOG,

‎test/build-container.test.ts‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,118 @@ describe('build-container', () => {
122122
);
123123
});
124124

125+
test('removes stale running builders for the same key when the owner pid is gone', () => {
126+
const spawnSyncMock = jest
127+
.fn()
128+
.mockReturnValueOnce(dockerResult())
129+
.mockReturnValueOnce(dockerResult({ stdout: 'stale-container\n' }))
130+
.mockReturnValueOnce(dockerResult({ stdout: '/builder-key-123\n123\n' }))
131+
.mockReturnValueOnce(dockerResult())
132+
.mockReturnValueOnce(dockerResult())
133+
.mockReturnValueOnce(dockerResult({ stdout: 'container-id\n' }))
134+
.mockReturnValueOnce(dockerResult({ stdout: 'running' }))
135+
.mockReturnValueOnce(dockerResult({ stdout: 'ready' }));
136+
const processKillSpy = jest
137+
.spyOn(process, 'kill')
138+
.mockImplementation(((_pid: number, _signal?: number | NodeJS.Signals) => {
139+
const error = new Error('missing process') as NodeJS.ErrnoException;
140+
error.code = 'ESRCH';
141+
throw error;
142+
}) as typeof process.kill);
143+
144+
const buildContainer = loadBuildContainerModule(spawnSyncMock);
145+
146+
buildContainer.ensureBuilderContainer({
147+
name: 'builder-key-999',
148+
builderKey: 'builder-key',
149+
args: ['run', 'image'],
150+
readyLog: 'ready',
151+
});
152+
153+
expect(processKillSpy).toHaveBeenCalledWith(123, 0);
154+
expect(spawnSyncMock).toHaveBeenCalledWith(
155+
'docker',
156+
['rm', '-f', 'stale-container'],
157+
{ encoding: 'utf8' },
158+
);
159+
160+
processKillSpy.mockRestore();
161+
});
162+
163+
test('keeps running builders for the same key when the owner pid is still alive', () => {
164+
const spawnSyncMock = jest
165+
.fn()
166+
.mockReturnValueOnce(dockerResult())
167+
.mockReturnValueOnce(dockerResult({ stdout: 'live-container\n' }))
168+
.mockReturnValueOnce(dockerResult({ stdout: '/builder-key-456\n456\n' }))
169+
.mockReturnValueOnce(dockerResult())
170+
.mockReturnValueOnce(dockerResult({ stdout: 'container-id\n' }))
171+
.mockReturnValueOnce(dockerResult({ stdout: 'running' }))
172+
.mockReturnValueOnce(dockerResult({ stdout: 'ready' }));
173+
const processKillSpy = jest
174+
.spyOn(process, 'kill')
175+
.mockImplementation(
176+
((_pid: number, _signal?: number | NodeJS.Signals) =>
177+
true) as typeof process.kill,
178+
);
179+
180+
const buildContainer = loadBuildContainerModule(spawnSyncMock);
181+
182+
buildContainer.ensureBuilderContainer({
183+
name: 'builder-key-999',
184+
builderKey: 'builder-key',
185+
args: ['run', 'image'],
186+
readyLog: 'ready',
187+
});
188+
189+
expect(processKillSpy).toHaveBeenCalledWith(456, 0);
190+
expect(spawnSyncMock).not.toHaveBeenCalledWith(
191+
'docker',
192+
['rm', '-f', 'live-container'],
193+
{ encoding: 'utf8' },
194+
);
195+
196+
processKillSpy.mockRestore();
197+
});
198+
199+
test('falls back to the builder name pid when older containers lack an owner label', () => {
200+
const spawnSyncMock = jest
201+
.fn()
202+
.mockReturnValueOnce(dockerResult())
203+
.mockReturnValueOnce(dockerResult({ stdout: 'stale-container\n' }))
204+
.mockReturnValueOnce(dockerResult({ stdout: '/builder-key-123\n\n' }))
205+
.mockReturnValueOnce(dockerResult())
206+
.mockReturnValueOnce(dockerResult())
207+
.mockReturnValueOnce(dockerResult({ stdout: 'container-id\n' }))
208+
.mockReturnValueOnce(dockerResult({ stdout: 'running' }))
209+
.mockReturnValueOnce(dockerResult({ stdout: 'ready' }));
210+
const processKillSpy = jest
211+
.spyOn(process, 'kill')
212+
.mockImplementation(((_pid: number, _signal?: number | NodeJS.Signals) => {
213+
const error = new Error('missing process') as NodeJS.ErrnoException;
214+
error.code = 'ESRCH';
215+
throw error;
216+
}) as typeof process.kill);
217+
218+
const buildContainer = loadBuildContainerModule(spawnSyncMock);
219+
220+
buildContainer.ensureBuilderContainer({
221+
name: 'builder-key-999',
222+
builderKey: 'builder-key',
223+
args: ['run', 'image'],
224+
readyLog: 'ready',
225+
});
226+
227+
expect(processKillSpy).toHaveBeenCalledWith(123, 0);
228+
expect(spawnSyncMock).toHaveBeenCalledWith(
229+
'docker',
230+
['rm', '-f', 'stale-container'],
231+
{ encoding: 'utf8' },
232+
);
233+
234+
processKillSpy.mockRestore();
235+
});
236+
125237
test('times out when the builder never becomes ready', () => {
126238
const spawnSyncMock = jest
127239
.fn()
@@ -190,6 +302,8 @@ describe('build-container', () => {
190302
readyLog: 'ready',
191303
});
192304

305+
expect(listeners.has('beforeExit')).toBe(true);
306+
expect(listeners.has('exit')).toBe(true);
193307
listeners.get('SIGINT')?.();
194308

195309
expect(buildContainer.getManagedBuilderContainerNames()).toHaveLength(0);

‎test/bundling.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,12 @@ describe('Bundling', () => {
374374
expect(fromBuildSpy).toHaveBeenCalled();
375375
expect(ensureBuilderContainerMock).toHaveBeenCalledWith(
376376
expect.objectContaining({
377+
builderKey: expect.stringMatching(/^uv-bundling-/),
377378
args: expect.arrayContaining([
379+
'--label',
380+
expect.stringMatching(
381+
/^com\.fourtheorem\.uv-python-lambda\.builder-owner-pid=\d+$/,
382+
),
378383
'--user',
379384
getExpectedDockerUserArg(),
380385
'mock-image',
@@ -412,6 +417,7 @@ describe('Bundling', () => {
412417

413418
expect(ensureBuilderContainerMock).toHaveBeenCalledWith(
414419
expect.objectContaining({
420+
builderKey: expect.stringMatching(/^uv-bundling-/),
415421
args: expect.arrayContaining([
416422
'--network',
417423
'test-network',
@@ -421,6 +427,10 @@ describe('Bundling', () => {
421427
'/tmp/cache:/cache',
422428
'--volumes-from',
423429
'shared-container',
430+
'--label',
431+
expect.stringMatching(
432+
/^com\.fourtheorem\.uv-python-lambda\.builder-owner-pid=\d+$/,
433+
),
424434
'mock-image',
425435
]),
426436
}),

0 commit comments

Comments
 (0)