Skip to content

Fix #18834: use docker inspect exit code instead of stdout parsing #27428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
LifeJiggy wants to merge 1 commit into from
Closed

Fix #18834: use docker inspect exit code instead of stdout parsing #27428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 40 additions & 69 deletions packages/cli/src/utils/sandbox.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,18 +299,15 @@ describe('sandbox', () => {
});

// Mock image check to return true (image exists)
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess.stdout = new EventEmitter();
const mockImageCheckProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
vi.mocked(spawn).mockImplementationOnce((_cmd, args) => {
if (args && args[0] === 'images') {
if (args && args[0] === 'inspect') {
setTimeout(() => {
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess.emit('close', 0);
}, 1);
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess;
}
return new EventEmitter() as unknown as ReturnType<typeof spawn>; // fallback
});
Expand Down Expand Up @@ -425,45 +422,35 @@ describe('sandbox', () => {
image: 'missing-image',
});

// 1. Image check fails
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
// 1. Image check fails (exit code 1)
const mockImageCheckProcess1 =
new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess1.stdout = new EventEmitter();
new EventEmitter() as unknown as ReturnType<typeof spawn>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess1.emit('close', 0);
mockImageCheckProcess1.emit('close', 1);
}, 1);
return mockImageCheckProcess1 as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess1;
});

// 2. Pull image succeeds
interface MockProcessWithStdoutStderr extends EventEmitter {
stdout: EventEmitter;
stderr: EventEmitter;
}
const mockPullProcess = new EventEmitter() as MockProcessWithStdoutStderr;
mockPullProcess.stdout = new EventEmitter();
mockPullProcess.stderr = new EventEmitter();
const mockPullProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockPullProcess.emit('close', 0);
}, 1);
return mockPullProcess as unknown as ReturnType<typeof spawn>;
return mockPullProcess;
});

// 3. Image check succeeds
// 3. Image check succeeds (exit code 0)
const mockImageCheckProcess2 =
new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess2.stdout = new EventEmitter();
new EventEmitter() as unknown as ReturnType<typeof spawn>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess2.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess2.emit('close', 0);
}, 1);
return mockImageCheckProcess2 as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess2;
});

// 4. Docker run
Expand Down Expand Up @@ -494,33 +481,25 @@ describe('sandbox', () => {
image: 'missing-image',
});

// 1. Image check fails
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
// 1. Image check fails (exit code 1)
const mockImageCheckProcess1 =
new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess1.stdout = new EventEmitter();
new EventEmitter() as unknown as ReturnType<typeof spawn>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess1.emit('close', 0);
mockImageCheckProcess1.emit('close', 1);
}, 1);
return mockImageCheckProcess1 as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess1;
});

// 2. Pull image fails
interface MockProcessWithStdoutStderr extends EventEmitter {
stdout: EventEmitter;
stderr: EventEmitter;
}
const mockPullProcess = new EventEmitter() as MockProcessWithStdoutStderr;
mockPullProcess.stdout = new EventEmitter();
mockPullProcess.stderr = new EventEmitter();
// 2. Pull image fails (exit code 1)
const mockPullProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockPullProcess.emit('close', 1);
}, 1);
return mockPullProcess as unknown as ReturnType<typeof spawn>;
return mockPullProcess;
});

await expect(start_sandbox(config)).rejects.toThrow(FatalSandboxError);
Expand All @@ -535,17 +514,14 @@ describe('sandbox', () => {
vi.mocked(fs.existsSync).mockReturnValue(true); // For mount path check

// Mock image check to return true
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess.stdout = new EventEmitter();
const mockImageCheckProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess.emit('close', 0);
}, 1);
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess;
});

const mockSpawnProcess = new EventEmitter() as unknown as ReturnType<
Expand All @@ -561,11 +537,12 @@ describe('sandbox', () => {

await start_sandbox(config);

// The first call is 'docker images -q ...'
// The first call is 'docker inspect --type=image ...'
expect(spawn).toHaveBeenNthCalledWith(
1,
'docker',
expect.arrayContaining(['images', '-q']),
expect.arrayContaining(['inspect', '--type=image']),
expect.objectContaining({ stdio: 'ignore' }),
);

// The second call is 'docker run ...'
Expand Down Expand Up @@ -711,17 +688,14 @@ describe('sandbox', () => {
process.env['GOOGLE_VERTEX_BASE_URL'] = 'http://vertex.proxy';

// Mock image check to return true
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess.stdout = new EventEmitter();
const mockImageCheckProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess.emit('close', 0);
}, 1);
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess;
});

const mockSpawnProcess = new EventEmitter() as unknown as ReturnType<
Expand Down Expand Up @@ -763,17 +737,14 @@ describe('sandbox', () => {
});

// Mock image check to return true
interface MockProcessWithStdout extends EventEmitter {
stdout: EventEmitter;
}
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
mockImageCheckProcess.stdout = new EventEmitter();
const mockImageCheckProcess = new EventEmitter() as unknown as ReturnType<
typeof spawn
>;
vi.mocked(spawn).mockImplementationOnce(() => {
setTimeout(() => {
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
mockImageCheckProcess.emit('close', 0);
}, 1);
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
return mockImageCheckProcess;
});

const mockSpawnProcess = new EventEmitter() as unknown as ReturnType<
Expand Down
18 changes: 3 additions & 15 deletions packages/cli/src/utils/sandbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,15 +1080,8 @@ async function start_lxc_sandbox(
// Helper functions to ensure sandbox image is present
async function imageExists(sandbox: string, image: string): Promise<boolean> {
return new Promise((resolve) => {
const args = ['images', '-q', image];
const checkProcess = spawn(sandbox, args);

let stdoutData = '';
if (checkProcess.stdout) {
checkProcess.stdout.on('data', (data) => {
stdoutData += data.toString();
});
}
const args = ['inspect', '--type=image', image];
const checkProcess = spawn(sandbox, args, { stdio: 'ignore' });

checkProcess.on('error', (err) => {
debugLogger.warn(
Expand All @@ -1098,12 +1091,7 @@ async function imageExists(sandbox: string, image: string): Promise<boolean> {
});

checkProcess.on('close', (code) => {
// Non-zero code might indicate docker daemon not running, etc.
// The primary success indicator is non-empty stdoutData.
if (code !== 0) {
// console.warn(`'${sandbox} images -q ${image}' exited with code ${code}.`);
}
resolve(stdoutData.trim() !== '');
resolve(code === 0);
});
});
}
Expand Down