Allow target fetch before current kill if target service is new

Signed-off-by: Christina Ying Wang <[email protected]>

src/compose/app.ts

@@ -572,6 +572,8 @@ export class App {
 			const dependenciesMetForKill = this.dependenciesMetForServiceKill(
 				context.targetApp,
 				context.availableImages,
+				context.servicePairs,
+				strategy,
 			);
 
 			return getStepsFromStrategy(strategy, {
@@ -660,7 +662,10 @@ export class App {
 		volumePairs: Array<ChangingPair<Volume>>,
 		servicePairs: Array<ChangingPair<Service>>,
 	): CompositionStep | undefined {
-		if (needsDownload && this.dependenciesMetForServiceFetch(servicePairs)) {
+		if (
+			needsDownload &&
+			this.dependenciesMetForServiceFetch(target, servicePairs)
+		) {
 			// We know the service name exists as it always does for targets
 			return generateStep('fetch', {
 				image: imageManager.imageFromService(target),
@@ -681,19 +686,29 @@ export class App {
 	}
 
 	private dependenciesMetForServiceFetch(
+		target: Service,
 		servicePairs: Array<ChangingPair<Service>>,
 	) {
-		// Current services should be killed before target
-		// service fetch depending on update strategy
-		const servicePairsWithCurrent = servicePairs.filter(
+		const [servicePairsWithCurrent, servicePairsWithoutCurrent] = _.partition(
+			servicePairs,
 			(pair) => pair.current != null,
 		);
-		for (const { current, target } of servicePairsWithCurrent) {
+
+		// Target services not in current can be safely fetched
+		for (const pair of servicePairsWithoutCurrent) {
+			if (target.serviceName === pair.target!.serviceName) {
+				return true;
+			}
+		}
+
+		// Current services should be killed before target
+		// service fetch depending on update strategy
+		for (const pair of servicePairsWithCurrent) {
 			// Prefer target's update strategy if target service exists
-			const strategy = getStrategyFromService(target ?? current!);
+			const strategy = getStrategyFromService(pair.target ?? pair.current!);
 			if (
 				['kill-then-download', 'delete-then-download'].includes(strategy) &&
-				current!.config.running
+				pair.current!.config.running
 			) {
 				return false;
 			}
@@ -754,7 +769,7 @@ export class App {
 		}
 
 		// do not start until all images have been downloaded
-		return this.targetImagesReady(targetApp, availableImages);
+		return this.targetImagesReady(targetApp.services, availableImages);
 	}
 
 	// Unless the update strategy requires an early kill (i.e kill-then-download,
@@ -764,13 +779,30 @@ export class App {
 	private dependenciesMetForServiceKill(
 		targetApp: App,
 		availableImages: Image[],
+		servicePairs: Array<ChangingPair<Service>>,
+		strategy: string,
 	) {
-		// Don't kill any services before all images have been downloaded
-		return this.targetImagesReady(targetApp, availableImages);
+		// if kill|delete-then-download, don't kill any services
+		// before new images in target have been downloaded.
+		// Services are safe to kill if they are in both current and target
+		// even if the target image isn't yet ready.
+		if (['kill-then-download', 'delete-then-download'].includes(strategy)) {
+			const servicePairsWithOnlyTarget = servicePairs
+				.filter((pair) => pair.current == null && pair.target != null)
+				.map((pair) => pair.target) as Service[];
+			return this.targetImagesReady(
+				servicePairsWithOnlyTarget,
+				availableImages,
+			);
+		}
+		return this.targetImagesReady(targetApp.services, availableImages);
 	}
 
-	private targetImagesReady(targetApp: App, availableImages: Image[]) {
-		return targetApp.services.every((service) =>
+	private targetImagesReady(
+		targetServices: Service[],
+		availableImages: Image[],
+	) {
+		return targetServices.every((service) =>
 			availableImages.some(
 				(image) =>
 					image.dockerImageId === service.config.image ||

src/compose/update-strategies.ts

@@ -28,11 +28,15 @@ export function getStepsFromStrategy(
 				// We only kill when dependencies are already met, so that we minimize downtime
 				return generateStep('kill', { current: context.current });
 			} else {
-				return { action: 'noop' };
+				return generateStep('noop', {});
 			}
 		case 'kill-then-download':
 		case 'delete-then-download':
-			return generateStep('kill', { current: context.current });
+			if (context.dependenciesMetForKill) {
+				return generateStep('kill', { current: context.current });
+			} else {
+				return generateStep('noop', {});
+			}
 		case 'hand-over':
 			if (context.needsDownload && context.target) {
 				return generateStep('fetch', {
@@ -47,7 +51,7 @@ export function getStepsFromStrategy(
 					target: context.target,
 				});
 			} else {
-				return { action: 'noop' };
+				return generateStep('noop', {});
 			}
 		default:
 			throw new InternalInconsistencyError(