Handle concurrent removal (microsoft/vscode-remote-release#6509)

Author: chrmarti

src/spec-node/dockerCompose.ts

@@ -11,7 +11,7 @@ import { ContainerProperties, setupInContainer, ResolverProgress } from '../spec
 import { ContainerError } from '../spec-common/errors';
 import { Workspace } from '../spec-utils/workspaces';
 import { equalPaths, parseVersion, isEarlierVersion, CLIHost } from '../spec-common/commonUtils';
-import { ContainerDetails, inspectContainer, listContainers, DockerCLIParameters, dockerCLI, dockerComposeCLI, dockerComposePtyCLI, PartialExecParameters, DockerComposeCLI, ImageDetails, toExecParameters, toPtyExecParameters } from '../spec-shutdown/dockerUtils';
+import { ContainerDetails, inspectContainer, listContainers, DockerCLIParameters, dockerComposeCLI, dockerComposePtyCLI, PartialExecParameters, DockerComposeCLI, ImageDetails, toExecParameters, toPtyExecParameters, removeContainer } from '../spec-shutdown/dockerUtils';
 import { DevContainerFromDockerComposeConfig, getDockerComposeFilePaths } from '../spec-configuration/configuration';
 import { Log, LogLevel, makeLog, terminalEscapeSequences } from '../spec-utils/log';
 import { getExtendImageBuildInfo, updateRemoteUserUID } from './containerFeatures';
@@ -54,7 +54,7 @@ async function _openDockerComposeDevContainer(params: DockerResolverParameters,
 		if (container && (params.removeOnStartup === true || params.removeOnStartup === container.Id)) {
 			const text = 'Removing existing container.';
 			const start = common.output.start(text);
-			await dockerCLI(params, 'rm', '-f', container.Id);
+			await removeContainer(params, container.Id);
 			common.output.stop(text, start);
 			container = undefined;
 		}

src/spec-node/singleContainer.ts

@@ -7,7 +7,7 @@
 import { createContainerProperties, startEventSeen, ResolverResult, getTunnelInformation, getDockerfilePath, getDockerContextPath, DockerResolverParameters, isDockerFileConfig, uriToWSLFsPath, WorkspaceConfiguration, getFolderImageName, inspectDockerImage, logUMask, SubstitutedConfig, checkDockerSupportForGPU, isBuildKitImagePolicyError } from './utils';
 import { ContainerProperties, setupInContainer, ResolverProgress, ResolverParameters } from '../spec-common/injectHeadless';
 import { ContainerError, toErrorText } from '../spec-common/errors';
-import { ContainerDetails, listContainers, DockerCLIParameters, inspectContainers, dockerCLI, dockerPtyCLI, toPtyExecParameters, ImageDetails, toExecParameters } from '../spec-shutdown/dockerUtils';
+import { ContainerDetails, listContainers, DockerCLIParameters, inspectContainers, dockerCLI, dockerPtyCLI, toPtyExecParameters, ImageDetails, toExecParameters, removeContainer } from '../spec-shutdown/dockerUtils';
 import { DevContainerConfig, DevContainerFromDockerfileConfig, DevContainerFromImageConfig } from '../spec-configuration/configuration';
 import { LogLevel, Log, makeLog } from '../spec-utils/log';
 import { extendImage, getExtendImageBuildInfo, updateRemoteUserUID } from './containerFeatures';
@@ -299,7 +299,7 @@ export async function findExistingContainer(params: DockerResolverParameters, la
 	if (container && (params.removeOnStartup === true || params.removeOnStartup === container.Id)) {
 		const text = 'Removing Existing Container';
 		const start = common.output.start(text);
-		await dockerCLI(params, 'rm', '-f', container.Id);
+		await removeContainer(params, container.Id);
 		common.output.stop(text, start);
 		container = undefined;
 	}

src/spec-node/utils.ts

@@ -15,7 +15,7 @@ import { CommonDevContainerConfig, ContainerProperties, getContainerProperties,
 import { Workspace } from '../spec-utils/workspaces';
 import { URI } from 'vscode-uri';
 import { ShellServer } from '../spec-common/shellServer';
-import { inspectContainer, inspectImage, getEvents, ContainerDetails, DockerCLIParameters, dockerExecFunction, dockerPtyCLI, dockerPtyExecFunction, toDockerImageName, DockerComposeCLI, ImageDetails, dockerCLI } from '../spec-shutdown/dockerUtils';
+import { inspectContainer, inspectImage, getEvents, ContainerDetails, DockerCLIParameters, dockerExecFunction, dockerPtyCLI, dockerPtyExecFunction, toDockerImageName, DockerComposeCLI, ImageDetails, dockerCLI, removeContainer } from '../spec-shutdown/dockerUtils';
 import { getRemoteWorkspaceFolder } from './dockerCompose';
 import { findGitRootFolder } from '../spec-common/git';
 import { parentURI, uriToFsPath } from '../spec-configuration/configurationCommonUtils';
@@ -564,7 +564,7 @@ export async function findContainerAndIdLabels(params: DockerResolverParameters
 					container = undefined;
 				} else if (removeContainerWithOldLabels === true || removeContainerWithOldLabels === container.Id) {
 					// Remove container, so it will be rebuilt with new labels.
-					await dockerCLI(params, 'rm', '-f', container.Id);
+					await removeContainer(params, container.Id);
 					container = undefined;
 				}
 			}

src/spec-shutdown/dockerUtils.ts

@@ -9,6 +9,7 @@ import * as ptyType from 'node-pty';
 import { Log, makeLog } from '../spec-utils/log';
 import { Event } from '../spec-utils/event';
 import { escapeRegExCharacters } from '../spec-utils/strings';
+import { delay } from '../spec-common/async';
 
 export interface ContainerDetails {
 	Id: string;
@@ -168,15 +169,52 @@ export async function listContainers(params: DockerCLIParameters | PartialExecPa
 		.filter(s => !!s);
 }
 
-export async function getEvents(params: DockerResolverParameters, filters?: Record<string, string[]>) {
+export async function removeContainer(params: DockerCLIParameters | PartialExecParameters | DockerResolverParameters, nameOrId: string) {
+	let eventsProcess: Exec | undefined;
+	let removedSeenP: Promise<void> | undefined;
+	try {
+		for (let i = 0, n = 4; i < n; i++) {
+			try {
+				await dockerCLI(params, 'rm', '-f', nameOrId);
+				return;
+			} catch (err) {
+				// https://github.com/microsoft/vscode-remote-release/issues/6509
+				const stderr: string = err?.stderr?.toString().toLowerCase() || '';
+				if (i === n - 1 || !stderr.includes('already in progress')) {
+					throw err;
+				}
+				if (!removedSeenP) {
+					eventsProcess = await getEvents(params, {
+						container: [nameOrId],
+						event: ['destroy'],
+					});
+					removedSeenP = new Promise<void>(resolve => {
+						eventsProcess!.stdout.on('data', () => {
+							resolve();
+							eventsProcess!.terminate();
+							removedSeenP = new Promise(() => {}); // safeguard in case we see the 'removal already in progress' error again
+						});
+					});
+				}
+				await Promise.race([removedSeenP, delay(1000)]);
+			}
+		}
+	} finally {
+		if (eventsProcess) {
+			eventsProcess.terminate();
+		}
+	}
+}
+
+export async function getEvents(params: DockerCLIParameters | PartialExecParameters | DockerResolverParameters, filters?: Record<string, string[]>) {
 	const { exec, cmd, args, env, output } = toExecParameters(params);
 	const filterArgs = [];
 	for (const filter in filters) {
 		for (const value of filters[filter]) {
 			filterArgs.push('--filter', `${filter}=${value}`);
 		}
 	}
-	const format = params.isPodman ? 'json' : '{{json .}}'; // https://github.com/containers/libpod/issues/5981
+	const format = 'isPodman' in params && params.isPodman ? 'json' : '{{json .}}'; // https://github.com/containers/libpod/issues/5981
 	const combinedArgs = (args || []).concat(['events', '--format', format, ...filterArgs]);
 
 	const p = await exec({

src/test/dockerUtils.test.ts

@@ -6,6 +6,8 @@
 import { createPlainLog, LogLevel, makeLog } from '../spec-utils/log';
 import { inspectImageInRegistry, qualifyImageName } from '../spec-node/utils';
 import assert from 'assert';
+import { dockerCLI, listContainers, PartialExecParameters, removeContainer, toExecParameters } from '../spec-shutdown/dockerUtils';
+import { createCLIParams } from './testUtils';
 
 export const output = makeLog(createPlainLog(text => process.stdout.write(text), () => LogLevel.Trace));
 
@@ -46,4 +48,26 @@ describe('Docker utils', function () {
 		assert.strictEqual(qualifyImageName('random/image'), 'docker.io/random/image');
 		assert.strictEqual(qualifyImageName('foo/random/image'), 'foo/random/image');
 	});
+
+	it('protects against concurrent removal', async () => {
+		const params = await createCLIParams(__dirname);
+		const verboseParams = { ...toExecParameters(params), output: makeLog(output, LogLevel.Info), print: 'continuous' as 'continuous' };
+		const { stdout } = await dockerCLI(verboseParams, 'run', '-d', 'ubuntu:latest', 'sleep', 'inf');
+		const containerId = stdout.toString().trim();
+		const start = Date.now();
+		await Promise.all([
+			testRemoveContainer(verboseParams, containerId),
+			testRemoveContainer(verboseParams, containerId),
+			testRemoveContainer(verboseParams, containerId),
+		]);
+		console.log('removal took', Date.now() - start, 'ms');
+	});
 });
+
+async function testRemoveContainer(params: PartialExecParameters, nameOrId: string) {
+	await removeContainer(params, nameOrId);
+	const all = await listContainers(params, true);
+	if (all.some(shortId => nameOrId.startsWith(shortId))) {
+		throw new Error('container still exists');
+	}
+}