Skip to content
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.

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

Feature/prevent zombie containers #53

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions cli/src/cmd/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export async function testApp({
// build a temp image for test
spinner.start('Building app docker image for test...\n');
const imageId = await dockerBuild({
tag: 'iapp',
abbesBenayache marked this conversation as resolved.
Show resolved Hide resolved
isForTest: true,
progressCallback: (msg) => {
spinner.text = spinner.text + color.comment(msg);
Expand Down
144 changes: 91 additions & 53 deletions cli/src/execDocker/docker.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@ import os from 'os';

const docker = new Docker();

const abortController = new AbortController();
const { signal } = abortController;

process.on('SIGINT', () => {
abortController.abort();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registering the listener in the module scope and never cleaning it is problematic because it prevents SIGINT from stopping the process in the whole app.

This creates this bug:
image
I cannot cancel the command while a long async step is running (in the example TEE transformation step)

We need to register the SIGINT listener just before calling docker and clean it as soon as the docker interaction ends (successfully or with an error).


export async function checkDockerDaemon() {
try {
await docker.ping();
Expand Down Expand Up @@ -39,63 +46,81 @@ export async function dockerBuild({
pull: true, // docker store does not support multi platform image, this can cause issues when switching build target platform, pulling ensures the right image is used
});

const imageId = await new Promise((resolve, reject) => {
docker.modem.followProgress(buildImageStream, onFinished, onProgress);

function onFinished(err, output) {
/**
* expected output format for image id
* ```
* {
* aux: {
* ID: 'sha256:e994101ce877e9b42f31f1508e11bbeb8fa5096a1fb2d0c650a6a26797b1906b'
* }
* },
* ```
*/
const builtImageId = output?.find((row) => row?.aux?.ID)?.aux?.ID;
let imageId = null;

/**
* 3 kind of error possible, we want to catch each of them:
* - stream error
* - build error
* - no image id (should not happen)
*
* expected output format for build error
* ```
* {
* errorDetail: {
* code: 1,
* message: "The command '/bin/sh -c npm ci' returned a non-zero code: 1"
* },
* error: "The command '/bin/sh -c npm ci' returned a non-zero code: 1"
* }
* ```
*/
const errorOrErrorMessage =
err || // stream error
output.find((row) => row?.error)?.error || // build error message
(!builtImageId && 'Failed to retrieve generated image ID'); // no image id -> error message
try {
const imageId = await new Promise((resolve, reject) => {
// Handle abort signal
if (signal) {
signal.addEventListener('abort', () => {
buildImageStream.destroy();
reject(
new Error('Docker build process was unexpectedly terminated.')
abbesBenayache marked this conversation as resolved.
Show resolved Hide resolved
);
});
}
abbesBenayache marked this conversation as resolved.
Show resolved Hide resolved

if (errorOrErrorMessage) {
const error =
errorOrErrorMessage instanceof Error
? errorOrErrorMessage
: Error(errorOrErrorMessage);
reject(error);
} else {
resolve(builtImageId);
docker.modem.followProgress(buildImageStream, onFinished, onProgress);

function onFinished(err, output) {
/**
* expected output format for image id
* ```
* {
* aux: {
* ID: 'sha256:e994101ce877e9b42f31f1508e11bbeb8fa5096a1fb2d0c650a6a26797b1906b'
* }
* },
* ```
*/
const builtImageId = output?.find((row) => row?.aux?.ID)?.aux?.ID;

/**
* 3 kind of error possible, we want to catch each of them:
* - stream error
* - build error
* - no image id (should not happen)
*
* expected output format for build error
* ```
* {
* errorDetail: {
* code: 1,
* message: "The command '/bin/sh -c npm ci' returned a non-zero code: 1"
* },
* error: "The command '/bin/sh -c npm ci' returned a non-zero code: 1"
* }
* ```
*/
const errorOrErrorMessage =
err || // stream error
output.find((row) => row?.error)?.error || // build error message
(!builtImageId && 'Failed to retrieve generated image ID'); // no image id -> error message

if (errorOrErrorMessage) {
const error =
errorOrErrorMessage instanceof Error
? errorOrErrorMessage
: Error(errorOrErrorMessage);
reject(error);
} else {
resolve(builtImageId);
}
}
}

function onProgress(event) {
if (event?.stream) {
progressCallback(event.stream);
function onProgress(event) {
if (event?.stream) {
progressCallback(event.stream);
}
}
});
return imageId;
} catch (error) {
if (imageId) {
await docker.getImage(imageId).remove();
}
});

return imageId;
throw error;
}
}

// Function to push a Docker image
Expand All @@ -116,8 +141,15 @@ export async function pushDockerImage({
password: dockerhubAccessToken,
},
});

await new Promise((resolve, reject) => {
// Handle abort signal
if (signal) {
signal.addEventListener('abort', () => {
imagePushStream.destroy();
reject(new Error('Docker push process was unexpectedly terminated.'));
});
}
abbesBenayache marked this conversation as resolved.
Show resolved Hide resolved

docker.modem.followProgress(imagePushStream, onFinished, onProgress);

function onFinished(err, output) {
Expand Down Expand Up @@ -176,9 +208,15 @@ export async function runDockerContainer({
},
Env: env,
});
// Handle abort signal
if (signal) {
signal.addEventListener('abort', async () => {
await container.kill();
logsCallback('Container stopped unexpectedly during execution.');
abbesBenayache marked this conversation as resolved.
Show resolved Hide resolved
});
}

// Start the container
// TODO we should handle abort signal to stop the container and avoid containers running after command is interrupted
await container.start();

// get the logs stream
Expand Down