From 4ea7e96eccba7903fe54229e3982798f5c06f992 Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Wed, 20 May 2026 10:21:42 +0100 Subject: [PATCH] fix(toolkit-lib): diff leaves empty stack in REVIEW_IN_PROGRESS When `cdk diff --method=change-set` is run against a stack that doesn't exist, a CREATE changeset is made which puts the stack into REVIEW_IN_PROGRESS state. After describing the changeset, only the changeset was deleted but the empty stack was left behind. This change deletes the stack after cleaning up the changeset when the stack didn't previously exist. Also treats DELETE_IN_PROGRESS as non-existent to handle the race condition of running diff twice in quick succession. Closes #1503 --- ...e-stack-in-review-in-progress.integtest.ts | 15 +++++ .../lib/api/deployments/cfn-api.ts | 15 ++++- .../toolkit-lib/test/actions/diff.test.ts | 57 ++++++++++++++++++- 3 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-does-not-leave-stack-in-review-in-progress.integtest.ts diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-does-not-leave-stack-in-review-in-progress.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-does-not-leave-stack-in-review-in-progress.integtest.ts new file mode 100644 index 000000000..fc1f80a2a --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-does-not-leave-stack-in-review-in-progress.integtest.ts @@ -0,0 +1,15 @@ +import { integTest, withDefaultFixture } from '../../../lib'; + +integTest( + 'cdk diff --method=change-set does not leave stack in REVIEW_IN_PROGRESS', + withDefaultFixture(async (fixture) => { + const stackName = fixture.fullStackName('test-1'); + + // WHEN - diff against a stack that has not been deployed + await fixture.cdk(['diff', '--method=change-set', stackName]); + + // THEN - the stack should be deleted or deleting (not stuck in REVIEW_IN_PROGRESS) + const status = await fixture.aws.stackStatus(stackName); + expect(status).not.toBe('REVIEW_IN_PROGRESS'); + }), +); diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts index b30942162..9a541db93 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts @@ -1,4 +1,5 @@ -import { format } from 'util'; +import { randomUUID } from 'node:crypto'; +import { format } from 'node:util'; import type { FileManifestEntry } from '@aws-cdk/cdk-assets-lib'; import { AssetManifest } from '@aws-cdk/cdk-assets-lib'; import * as cxapi from '@aws-cdk/cloud-assembly-api'; @@ -277,7 +278,7 @@ async function uploadBodyParameterAndCreateChangeSet( const stack = await CloudFormationStack.lookup(cfn, options.stack.stackName, false); // A stack in REVIEW_IN_PROGRESS was created by a previous CREATE changeset // that was never executed. Treat it as non-existent for changeset purposes. - const exists = stack.exists && stack.stackStatus.name !== 'REVIEW_IN_PROGRESS'; + const exists = stack.exists && stack.stackStatus.name !== 'REVIEW_IN_PROGRESS' && stack.stackStatus.name !== 'DELETE_IN_PROGRESS'; const executionRoleArn = await env.replacePlaceholders(options.stack.cloudFormationExecutionRoleArn); await ioHelper.defaults.info( @@ -384,6 +385,16 @@ async function createChangeSetAndCleanup( changeSet.StackId ?? options.stack.stackName, ); + // If the stack didn't exist before, creating a CREATE changeset will have + // put it in REVIEW_IN_PROGRESS state. Delete the empty stack to clean up. + if (!options.exists) { + await ioHelper.defaults.debug(format('Deleting empty stack created by diff changeset: %s', changeSet.StackId ?? options.stack.stackName)); + await options.cfn.deleteStack({ + StackName: changeSet.StackId ?? options.stack.stackName, + ClientRequestToken: randomUUID(), + }); + } + return createdChangeSet; } diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts index 83843cd2d..68a664842 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts @@ -1,5 +1,5 @@ import * as path from 'path'; -import { CreateChangeSetCommand, DescribeChangeSetCommand, DescribeStacksCommand, GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; +import { CreateChangeSetCommand, DeleteStackCommand, DescribeChangeSetCommand, DescribeStacksCommand, GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; import { GetParameterCommand } from '@aws-sdk/client-ssm'; import * as chalk from 'chalk'; import { DiffMethod } from '../../lib/actions/diff'; @@ -465,6 +465,61 @@ describe('diff', () => { ChangeSetType: 'CREATE', })); }); + + test('ChangeSet diff deletes stack created in REVIEW_IN_PROGRESS for new stacks', async () => { + // GIVEN - stack doesn't exist + jest.spyOn(deployments.Deployments.prototype, 'stackExists').mockResolvedValue(false); + mockCloudFormationClient.on(DescribeStacksCommand).resolves({ Stacks: [] }); + mockSSMClient.on(GetParameterCommand).resolves({ Parameter: { Value: '99' } }); + mockCloudFormationClient.on(CreateChangeSetCommand).resolves({ + Id: 'arn:aws:cloudformation:us-east-1:123456789012:changeSet/cdk-diff', + StackId: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1/fake-id', + }); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'CREATE_COMPLETE', + Changes: [], + }); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); + await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), + }); + + // THEN - the stack is deleted after the changeset is cleaned up + expect(mockCloudFormationClient).toHaveReceivedCommandWith(DeleteStackCommand, { + StackName: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1/fake-id', + }); + }); + + test('ChangeSet diff treats DELETE_IN_PROGRESS stack as non-existent', async () => { + // GIVEN - stack is in DELETE_IN_PROGRESS (from a previous diff cleanup) + jest.spyOn(deployments.Deployments.prototype, 'stackExists').mockResolvedValue(true); + mockCloudFormationClient.on(DescribeStacksCommand).resolves({ + Stacks: [{ StackName: 'Stack1', StackStatus: 'DELETE_IN_PROGRESS', CreationTime: new Date() }], + }); + mockSSMClient.on(GetParameterCommand).resolves({ Parameter: { Value: '99' } }); + mockCloudFormationClient.on(CreateChangeSetCommand).resolves({ Id: 'arn:aws:cloudformation:us-east-1:123456789012:changeSet/cdk-diff' }); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'CREATE_COMPLETE', + Changes: [], + }); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); + await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), + }); + + // THEN - a CREATE changeset was made (not UPDATE) + const createCalls = mockCloudFormationClient.commandCalls(CreateChangeSetCommand); + expect(createCalls).toHaveLength(1); + expect(createCalls[0].args[0].input).toEqual(expect.objectContaining({ + ChangeSetType: 'CREATE', + })); + }); }); describe('DiffMethod.LocalFile', () => {