From c865381b9782a7c999ccc5ac9c96695fe27470e4 Mon Sep 17 00:00:00 2001 From: Son Hyunbin <147061193+SickofU@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:08:17 +0900 Subject: [PATCH] fix(toolkit-lib): clean up the diff change set when early validation fails When a diff change set fails early validation (for example a resource that already exists), `waitAndThrowOnProblem` threw before the change-set and empty-stack cleanup ran, orphaning the change set and leaving a new stack stuck in REVIEW_IN_PROGRESS, which then blocked subsequent change-set creation on that stack. Run the cleanup on the failure path too (best-effort, without masking the original error) so a failed diff no longer leaks the change set or the empty review stack. Fixes #1767 --- .../lib/api/deployments/cfn-api.ts | 62 ++++++++++++------- .../toolkit-lib/test/actions/diff.test.ts | 38 +++++++++++- 2 files changed, 77 insertions(+), 23 deletions(-) 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 47e482e23..0e851a396 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 @@ -223,33 +223,51 @@ async function createChangeSetAndCleanup( }); await ioHelper.defaults.debug(format('Initiated creation of changeset: %s; waiting for it to finish creating...', changeSet.Id)); - // Fetching all pages if we'll execute, so we can have the correct change count when monitoring. - const createdChangeSet = await new ChangeSetDescriber({ - cfn: options.cfn, - ioHelper, - stackNameOrArn: changeSet.StackId ?? options.stack.stackName, - changeSetNameOrArn: changeSet.Id ?? options.changeSetName, - }).waitAndThrowOnProblem({ - diagnoser: options.diagnoser, - }); - await cleanupOldChangeset( - options.cfn, - ioHelper, - changeSet.Id ?? options.changeSetName, - changeSet.StackId ?? options.stack.stackName, - ); + const changeSetId = changeSet.Id ?? options.changeSetName; + const stackId = changeSet.StackId ?? options.stack.stackName; + + // Remove the change set (and, for a brand new stack, the empty stack that a + // CREATE change set leaves in REVIEW_IN_PROGRESS). This has to run whether the + // change set succeeds or fails validation: otherwise a change set that fails + // early validation is orphaned and leaves the stack stuck in REVIEW_IN_PROGRESS, + // which then blocks subsequent change set creation. + const cleanup = async () => { + await cleanupOldChangeset(options.cfn, ioHelper, changeSetId, stackId); + + if (!options.exists) { + await ioHelper.defaults.debug(format('Deleting empty stack created by diff changeset: %s', stackId)); + await options.cfn.deleteStack({ + StackName: stackId, + ClientRequestToken: randomUUID(), + }); + } + }; - // 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(), + let createdChangeSet: ChangeSetReport; + try { + // Fetching all pages if we'll execute, so we can have the correct change count when monitoring. + createdChangeSet = await new ChangeSetDescriber({ + cfn: options.cfn, + ioHelper, + stackNameOrArn: stackId, + changeSetNameOrArn: changeSetId, + }).waitAndThrowOnProblem({ + diagnoser: options.diagnoser, }); + } catch (e) { + // Best-effort cleanup so a failed change set doesn't leak; don't let a + // cleanup failure mask the original creation/validation error. + try { + await cleanup(); + } catch (cleanupError) { + await ioHelper.defaults.debug(format('Failed to clean up change set after a creation error: %s', cleanupError)); + } + throw e; } + await cleanup(); + 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 421f14dfd..1f08698a9 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, DeleteStackCommand, DescribeChangeSetCommand, DescribeStacksCommand, GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; +import { CreateChangeSetCommand, DeleteChangeSetCommand, DeleteStackCommand, DescribeChangeSetCommand, DescribeStacksCommand, GetTemplateCommand, ListStacksCommand } from '@aws-sdk/client-cloudformation'; import { GetParameterCommand } from '@aws-sdk/client-ssm'; import chalk from 'chalk'; import { DiffMethod } from '../../lib/actions/diff'; @@ -443,6 +443,42 @@ describe('diff', () => { })).rejects.toThrow(/Could not create a change set, and '--method=change-set' was specified/); }); + test('ChangeSet diff cleans up the failed change set and empty stack when validation fails', async () => { + // GIVEN - a new stack whose CREATE change set fails early validation + // (e.g. a resource that already exists), which would otherwise leave the + // change set orphaned and the stack stuck in REVIEW_IN_PROGRESS. + 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-change-set/abc', + StackId: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1/def', + }); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'FAILED', + StatusReason: "Resource of type 'AWS::S3::Bucket' with identifier 'mybucket' already exists.", + ExecutionStatus: 'UNAVAILABLE', + Changes: [], + }); + + // WHEN - the diff falls back to a template diff (default fallbackToTemplate = true) + const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); + await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet(), + }); + + // THEN - the failed change set is deleted and the empty stack is cleaned up + const deleteChangeSetCalls = mockCloudFormationClient.commandCalls(DeleteChangeSetCommand); + expect(deleteChangeSetCalls.length).toBeGreaterThan(0); + expect(deleteChangeSetCalls[0].args[0].input).toEqual(expect.objectContaining({ + ChangeSetName: expect.stringContaining('cdk-diff-change-set'), + })); + + const deleteStackCalls = mockCloudFormationClient.commandCalls(DeleteStackCommand); + expect(deleteStackCalls.length).toBeGreaterThan(0); + }); + test('ChangeSet diff method creates changeset for new stacks when fallBackToTemplate = false', async () => { // GIVEN - stack doesn't exist jest.spyOn(deployments.Deployments.prototype, 'stackExists').mockResolvedValue(false);