From 9a9caf7f0551b1dd19f0dc4de7f118adc08d3a8f Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Fri, 18 Sep 2026 04:20:20 +0000 Subject: [PATCH 01/14] fix(toolkit-lib): restore the replacement guard for Express Mode deployments CloudFormation rejects replacement-type updates while rollback is disabled. Express Mode disables rollback unless --rollback is passed, so `cdk deploy --express` submits replacements CloudFormation refuses, and the stack is left in UPDATE_FAILED with no rollback available. #1745 removed the express half of the guard condition (and relaxed the test covering it), #1785 restructured what was left into `if (!this.options.express)`. Derive the condition once in `rollbackDisabled()` and apply the guard in both modes, returning `replacement-requires-rollback` so the existing confirm-and-retry-with-rollback prompt engages. Fix the same condition in the failure diagnostic, which reported rollback as enabled under --express and advised users to re-run with --no-rollback. `--method=direct` is deliberately not refused up front, because redeploying the previous configuration that way is how a stuck stack is unwedged; instead the rejection is detected after the fact and the user is routed to `--express --rollback`. That routing reads the activity monitor's errors, so the monitor is now flushed before they are read. Fixes #1931 --- .../toolkit-lib/docs/message-registry.md | 1 + .../lib/api/deployments/deploy-stack.ts | 204 +++++- .../lib/api/io/private/messages.ts | 7 +- .../toolkit-lib/lib/payloads/deploy.ts | 55 ++ .../toolkit-lib/lib/toolkit/toolkit.ts | 4 +- .../_helpers/fake-aws/fake-cloudformation.md | 14 +- .../_helpers/fake-aws/fake-cloudformation.ts | 51 +- .../toolkit-lib/test/_helpers/test-io-host.ts | 10 +- .../toolkit-lib/test/actions/deploy.test.ts | 65 ++ .../deploy-stack-express-replacement.test.ts | 606 ++++++++++++++++++ .../test/api/deployments/deploy-stack.test.ts | 28 +- 11 files changed, 1015 insertions(+), 30 deletions(-) create mode 100644 packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts diff --git a/packages/@aws-cdk/toolkit-lib/docs/message-registry.md b/packages/@aws-cdk/toolkit-lib/docs/message-registry.md index 22e921d10..828891ca7 100644 --- a/packages/@aws-cdk/toolkit-lib/docs/message-registry.md +++ b/packages/@aws-cdk/toolkit-lib/docs/message-registry.md @@ -117,6 +117,7 @@ Please let us know by [opening an issue](https://github.com/aws/aws-cdk-cli/issu | `CDK_TOOLKIT_I5900` | Deployment results on success | `result` | {@link SuccessfulDeployStackResult} | | `CDK_TOOLKIT_I5901` | Generic deployment success messages | `info` | n/a | | `CDK_TOOLKIT_W5902` | Express Mode deployment completed with resources still stabilizing | `warn` | n/a | +| `CDK_TOOLKIT_W5903` | Deployment includes a replacement that CloudFormation does not support while rollback is disabled | `warn` | n/a | | `CDK_TOOLKIT_W5400` | Hotswap disclosure message | `warn` | n/a | | `CDK_TOOLKIT_E5001` | No stacks found | `error` | n/a | | `CDK_TOOLKIT_E5500` | Stack Monitoring error | `error` | {@link ErrorPayload} | diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index 36e00a616..0e44c28fe 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -27,6 +27,7 @@ import { determineAllowCrossAccountAssetPublishing } from './checks'; import type { DeployStackResult, SuccessfulDeployStackResult } from './deployment-result'; import type { ChangeSetDeployment, DeploymentMethod, DirectDeployment, ExecuteChangeSetDeployment } from '../../actions/deploy'; import { DEFAULT_DEPLOY_CHANGE_SET_NAME } from '../../actions/deploy/private/deployment-method'; +import type { ReplacedResource } from '../../payloads/deploy'; import { DeploymentError, DeploymentErrorCodes, ToolkitError } from '../../toolkit/toolkit-error'; import type { StabilizingResource } from '../../toolkit/types'; import { formatErrorMessage } from '../../util'; @@ -43,6 +44,7 @@ import { HotswapPropertyOverrides, ICON, createHotswapPropertyOverrides } from ' import { tryHotswapDeployment } from '../hotswap/hotswap-deployments'; import { invalidateHotswapTemplateCache, readHotswapTemplateCache } from '../hotswap/hotswap-template-cache'; import type { IoHelper } from '../io/private'; +import { IO } from '../io/private'; import type { ResourcesToImport } from '../resource-import'; import { StackActivityMonitor } from '../stack-events'; import type { ResourceErrors } from '../stack-events/resource-errors'; @@ -541,10 +543,29 @@ class FullCloudFormationDeployment { return this.checkAndExecuteChangeSet(changeSetReport); } + /** + * Whether CloudFormation will have rollback disabled for this deployment. + * + * Express Mode disables rollback unless it is explicitly requested; standard mode only disables it when + * `--no-rollback` was passed. Both the replacement guard and `deployConfig()` derive from this, so they cannot + * disagree about whether rollback ends up disabled server-side. + * + * Note this is deliberately NOT the predicate used by `commonExecuteOptions()`. That one decides whether to send + * `DisableRollback: true` on the API call, which express deployments must never do - they express it through + * `DeploymentConfig` instead. + */ + private rollbackDisabled(): boolean { + return this.options.express ? this.options.rollback !== true : this.options.rollback === false; + } + private deployConfig(): DeploymentConfig { + if (!this.options.express) { + return { Mode: 'STANDARD' }; + } + return { - Mode: this.options.express ? 'EXPRESS' : 'STANDARD', - ...(this.options.express && this.options.rollback == true ? { DisableRollback: false } : undefined), + Mode: 'EXPRESS', + ...(this.rollbackDisabled() ? undefined : { DisableRollback: false }), }; } @@ -552,21 +573,41 @@ class FullCloudFormationDeployment { * Check rollback/replacement constraints and execute the change set if all checks pass. */ private async checkAndExecuteChangeSet(changeSetReport: ChangeSetReport): Promise { - const replacement = hasReplacement(changeSetReport); + const replacements = findReplacements(changeSetReport); const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; // For express mode deployments, don't check paused and failed, since express mode stacks cannot use rollback API if (!this.options.express) { - if (isPausedFailState && replacement) { + if (isPausedFailState && replacements.length > 0) { return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name }; } if (isPausedFailState && rollback) { return { type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: this.cloudFormationStack.stackStatus.name }; } - if (!rollback && replacement) { - return { type: 'replacement-requires-rollback' }; + } + + // CloudFormation rejects replacement-type updates while rollback is disabled. Standard mode only disables rollback + // for `--no-rollback`, but Express Mode disables it by default - which is why this condition must not be scoped to + // non-express deployments. #1745 dropped the express half of it, #1785 restructured what was left, and #1931 is the + // resulting SEV: the update is submitted, CloudFormation refuses it, and the express stack is left in UPDATE_FAILED + // with no rollback available. + // + // Shelf life: CloudFormation has a server-side fix with a tentative ECD of 2026-11-15. Once that is confirmed in + // all regions, the express half of this guard - and CDK_TOOLKIT_W5903 - can be deleted. + if (replacements.length > 0 && this.rollbackDisabled()) { + if (this.options.express) { + await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg( + replacementRoutingMessage({ rejected: false, needsUnwedge: isPausedFailState }), + { + stackName: this.stackName, + changeSetId: changeSetReport.changeSet.ChangeSetId, + replacements, + detectedBy: 'change-set', + }, + )); } + return { type: 'replacement-requires-rollback' }; } const changeSet = changeSetReport.changeSet; @@ -726,6 +767,18 @@ class FullCloudFormationDeployment { await monitor.start(); let finalState: CloudFormationStack; + let monitorStopped = false; + + // `monitor.stop()` performs a final poll, and that poll is what fills `monitor.errors` with the resource-level + // failures CloudFormation reported. Everything that reads those errors has to run after it. `stop()` is not + // idempotent (it emits a completion message and polls again), so it must run exactly once. + const stopMonitor = async () => { + if (!monitorStopped) { + monitorStopped = true; + await monitor.stop(); + } + }; + try { const successStack = await waitForStackDeploy(this.cfn, this.ioHelper, stackArn, this.options.stackEventPollingInterval); @@ -735,6 +788,10 @@ class FullCloudFormationDeployment { } finalState = successStack; } catch (e: any) { + await stopMonitor(); + + await this.routeReplacementRejectedWithRollbackDisabled(e, monitor.errors); + // Deployment errors get replaced by a diagnosis of the underlying resource failures, which says more. // Any other error, and any failure to diagnose, leaves `e` to propagate as it is. if (ToolkitError.isDeploymentError(e)) { @@ -743,7 +800,7 @@ class FullCloudFormationDeployment { throw e; } finally { - await monitor.stop(); + await stopMonitor(); } await this.ioHelper.defaults.debug(format('Stack %s has completed updating', this.stackName)); return { @@ -756,6 +813,51 @@ class FullCloudFormationDeployment { }; } + /** + * Tell the user how to perform a replacement when CloudFormation rejected one because rollback was disabled. + * + * The `--method=direct` path has no change set to inspect, so it cannot be gated up front the way the change set + * path is; a replacement there is only discovered from the failure CloudFormation reports. We deliberately do not + * refuse `--express --method=direct` up front either, because redeploying the previous configuration that way is + * the documented way to unwedge a stack that is already stuck. + * + * The original error is left to propagate untouched, so a genuinely failing replacement still reports its real + * underlying service error. + */ + private async routeReplacementRejectedWithRollbackDisabled(error: any, errors: ResourceErrors): Promise { + if (!this.rollbackDisabled()) { + return; + } + + const rejected = errors.all.filter((e) => mentionsReplacementRejection(e.message)); + const matched = rejected.length > 0 || mentionsReplacementRejection(error?.message ?? ''); + + if (!matched) { + // CloudFormation owns the wording we match on and has a change landing around 2026-11-15. If it is reworded, + // this is the branch that will start being taken - log what we did see so that shows up in a debug log instead + // of arriving as a second SEV. + const reported = errors.allErrorMessages.filter((m) => m.trim() !== ''); + await this.ioHelper.defaults.debug(format( + 'Deployment failed with rollback disabled but no reported error mentioned %j, so no replacement guidance was emitted. Reported reasons: %s', + CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON, + reported.length > 0 ? reported.join(' | ') : '(none)', + )); + return; + } + + await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg( + replacementRoutingMessage({ rejected: true, needsUnwedge: true }), + { + stackName: this.stackName, + replacements: rejected.map((e) => ({ + logicalId: e.logicalId ?? this.stackName, + resourceType: e.resourceType, + })), + detectedBy: 'service-error', + }, + )); + } + /** * Throw a `DeploymentError` describing why the deployment failed, if we can establish that * @@ -776,7 +878,7 @@ class FullCloudFormationDeployment { } const diagnosis = await this.diagnoser.diagnoseFromErrorCollection(errors, deployedState, true, { - rollbackEnabled: this.options.rollback !== false, + rollbackEnabled: !this.rollbackDisabled(), }); diagnosis.throwOnError(); } @@ -803,6 +905,8 @@ class FullCloudFormationDeployment { * deployed everywhere yet. */ private commonExecuteOptions(): Partial> { + // Not `rollbackDisabled()`: express deployments also run with rollback disabled, but they must express that + // through `DeploymentConfig` rather than by sending `DisableRollback` on the call. const shouldDisableRollback = this.options.rollback === false; return { @@ -1015,9 +1119,85 @@ function arrayEquals(a: any[], b: any[]): boolean { return a.every((item) => b.includes(item)) && b.every((item) => a.includes(item)); } -function hasReplacement(report: ChangeSetReport) { - return (report.changeSet.Changes ?? []).some(c => { - const a = c.ResourceChange?.PolicyAction; - return a === 'ReplaceAndDelete' || a === 'ReplaceAndRetain' || a === 'ReplaceAndSnapshot'; +/** + * Find the resource changes in a change set that CloudFormation would perform by replacement + */ +function findReplacements(report: ChangeSetReport): ReplacedResource[] { + return (report.changeSet.Changes ?? []).flatMap((c) => { + const change = c.ResourceChange; + const policyAction = change?.PolicyAction; + const replacesResource = policyAction === 'ReplaceAndDelete' + || policyAction === 'ReplaceAndRetain' + || policyAction === 'ReplaceAndSnapshot'; + + if (!change || !replacesResource) { + return []; + } + + return [{ + logicalId: change.LogicalResourceId ?? '', + resourceType: change.ResourceType, + replacement: change.Replacement, + policyAction, + }]; }); } + +/** + * The reason CloudFormation reports when it refuses a replacement because rollback is disabled. + * + * CloudFormation surfaces this as a resource status reason with no structured error code attached (`extractErrorCode` + * finds no `HandlerErrorCode:`/`Error Code:` prefix in it), so matching this text is the only trigger available. That + * makes it fragile: CloudFormation owns the string and has a change landing around 2026-11-15. A miss is logged at + * debug level and only costs the extra guidance - the underlying CloudFormation error is reported either way. + */ +export const CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON = 'Replacement type updates not supported on stack with disable-rollback'; + +function mentionsReplacementRejection(message: string): boolean { + return message.toLowerCase().includes(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON.toLowerCase()); +} + +/** + * Explain how to deploy a replacement when rollback is disabled. + * + * Replacements are supported in Express Mode; they are not supported while rollback is disabled, which Express Mode + * does by default. So this routes the user to what actually works instead of just refusing. + * + * When we stopped before submitting anything and the stack is healthy, this stays deliberately short and does NOT tell + * the user to run anything: the caller is about to offer to retry with rollback enabled (which `--force` accepts + * automatically), so an instruction to run the deployment by hand would be contradicted by what happens next. + * + * TODO: point at a CloudFormation User Guide anchor for Express Mode rollback behaviour once one exists. + */ +function replacementRoutingMessage(opts: { rejected: boolean; needsUnwedge: boolean }): string { + const withRollback = chalk.blue('cdk deploy --express --rollback'); + const direct = chalk.blue('cdk deploy --express --method=direct'); + + const headline = opts.rejected + ? [ + 'CloudFormation refused a replacement because rollback is disabled for this stack.', + 'Express Mode disables rollback by default; replacements themselves are supported.', + ] + : [ + 'This deployment replaces a resource, which CloudFormation does not support while rollback is disabled.', + 'Express Mode disables rollback unless you ask for it with --rollback; replacements themselves are supported.', + ]; + + if (!opts.needsUnwedge) { + return headline.join('\n'); + } + + // Deploying with rollback enabled cannot update a stack that is already in a failed state - CloudFormation answers + // "This stack is currently in a non-terminal [UPDATE_FAILED] state" (verified against CloudFormation). The previous + // configuration has to be replayed first, so say that rather than sending the user into a second failure. + return [ + ...headline, + '', + `${opts.rejected ? 'The stack may now be' : 'This stack is'} in a failed state, which ${withRollback} cannot update. To recover:`, + ' 1. Revert your change so your app matches the last configuration that deployed successfully.', + ` 2. Run ${direct} - this should replay that configuration as a no-op`, + ' and return the stack to a terminal state.', + ` 3. Re-apply your change and deploy it with ${withRollback}.`, + ].join('\n'); +} + diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts index 6001cbca9..e7a41179d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts @@ -6,7 +6,7 @@ import type { ValidateResult } from '../../../actions/validate'; import type { StackDiff, DiffResult } from '../../../payloads'; import type { BootstrapEnvironmentProgress } from '../../../payloads/bootstrap-environment-progress'; import type { MissingContext, UpdatedContext } from '../../../payloads/context'; -import type { BuildAsset, DeployConfirmationRequest, PublishAsset, PublishAssetEvent, StackDeployProgress, SuccessfulDeployStackResult } from '../../../payloads/deploy'; +import type { BuildAsset, DeployConfirmationRequest, PublishAsset, PublishAssetEvent, ReplacementRequiresRollback, StackDeployProgress, SuccessfulDeployStackResult } from '../../../payloads/deploy'; import type { StackDestroy, StackDestroyProgress } from '../../../payloads/destroy'; import type { DriftResultPayload } from '../../../payloads/drift'; import type { FeatureFlagChangeRequest } from '../../../payloads/flags'; @@ -334,6 +334,11 @@ export const IO = { code: 'CDK_TOOLKIT_W5902', description: 'Express Mode deployment completed with resources still stabilizing', }), + CDK_TOOLKIT_W5903: make.warn({ + code: 'CDK_TOOLKIT_W5903', + description: 'Deployment includes a replacement that CloudFormation does not support while rollback is disabled', + interface: 'ReplacementRequiresRollback', + }), CDK_TOOLKIT_W5400: make.warn({ code: 'CDK_TOOLKIT_W5400', description: 'Hotswap disclosure message', diff --git a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts index a63eeb5dc..6c9a73f0a 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts @@ -75,3 +75,58 @@ export interface PublishAssetEvent { */ readonly asset?: IManifestEntry; } + +/** + * A resource that CloudFormation reported it would replace + */ +export interface ReplacedResource { + /** + * Logical ID of the resource being replaced + */ + readonly logicalId: string; + + /** + * CloudFormation resource type, when known + */ + readonly resourceType?: string; + + /** + * The `Replacement` field CloudFormation reported for this change, when known + */ + readonly replacement?: string; + + /** + * The `PolicyAction` CloudFormation reported for this change, when known + */ + readonly policyAction?: string; +} + +/** + * A deployment includes a replacement that CloudFormation will not perform while rollback is disabled + */ +export interface ReplacementRequiresRollback { + /** + * The stack being deployed + */ + readonly stackName: string; + + /** + * The change set the replacement was found in, if it was found in one + */ + readonly changeSetId?: string; + + /** + * The resources that would be replaced + * + * Empty when CloudFormation reported the rejection without naming a resource. + */ + readonly replacements: ReplacedResource[]; + + /** + * How the replacement was detected + * + * `change-set` means the pre-flight check found it and nothing was submitted. `service-error` means we only found + * out from CloudFormation's failure, after the update had already been submitted. + */ + readonly detectedBy: 'change-set' | 'service-error'; +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 93c154586..04297806e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -1032,7 +1032,9 @@ export class Toolkit extends CloudAssemblySourceBuilder { } case 'replacement-requires-rollback': { - const motivation = 'Change includes a replacement which cannot be deployed with "--no-rollback"'; + const motivation = options.express + ? 'Change includes a replacement, which CloudFormation does not support while rollback is disabled (the default for Express Mode)' + : 'Change includes a replacement which cannot be deployed with "--no-rollback"'; const question = `${motivation}. Perform a deployment with rollback enabled`; const confirmed = await ioHelper.requestResponse(IO.CDK_TOOLKIT_I5050.req(question, { diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.md b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.md index 46bef874c..92a926975 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.md +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.md @@ -68,9 +68,19 @@ Tests that use the fake should use fake timers to advance time. 3. API returns `{ StackId }` immediately. 4. Stack status: `UPDATE_IN_PROGRESS`. 5. After delay: - - If any resource has `Fail: true` → `UPDATE_FAILED` (if - `DisableRollback`) or `UPDATE_ROLLBACK_IN_PROGRESS` → + - If any resource has `Fail: true` → `UPDATE_FAILED` (if rollback is + disabled) or `UPDATE_ROLLBACK_IN_PROGRESS` → `UPDATE_ROLLBACK_COMPLETE`. + - Rollback counts as disabled when `DisableRollback: true` is passed, **or** + when `DeploymentConfig.Mode === 'EXPRESS'` and the call did not explicitly + re-enable it with `DeploymentConfig.DisableRollback: false`. This mirrors + Express Mode having rollback disabled server-side by default, and is what + makes a failed express update strand the stack in `UPDATE_FAILED`. + - Every failing resource that also has `FailReason: '...'` emits + resource-level `UPDATE_IN_PROGRESS` and `UPDATE_FAILED` events, with that + string as the `ResourceStatusReason` (this is where real CloudFormation + reports why an operation failed). Without `FailReason`, only stack-level + events are emitted. `ExecuteChangeSet` failures do the same. - Otherwise → `UPDATE_COMPLETE`. ### DeleteStack diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts index 3a208bd4d..f2be9c6d3 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts @@ -10,6 +10,7 @@ import { type DeleteChangeSetCommandOutput, type DeleteStackCommandInput, type DeleteStackCommandOutput, + type DeploymentConfig, type DescribeChangeSetCommandInput, type DescribeChangeSetCommandOutput, type DescribeEventsCommandInput, @@ -119,6 +120,7 @@ interface InMemoryChangeSet { capabilities: string[]; description?: string; changes: Change[]; + deploymentConfig?: DeploymentConfig; creationTime: Date; changeSetFailureEvents: OperationEvent[]; earlyValidationErrors: EarlyValidationErrorPrime[]; @@ -323,7 +325,7 @@ export class FakeCloudFormation { const operationId = randomUUID(); const { id, stack, template } = this.initCreateStack(input, operationId); this.scheduleAsync(() => { - this.finalizeCreateStack(stack, template, input.DisableRollback, operationId); + this.finalizeCreateStack(stack, template, this.rollbackIsDisabled(input), operationId); }); return { StackId: id, $metadata: {} }; } @@ -347,7 +349,8 @@ export class FakeCloudFormation { this.scheduleAsync(() => { if (this.shouldFail(template)) { - if (input.DisableRollback) { + this.addFailedUpdateResourceEvents(stack, template, operationId); + if (this.rollbackIsDisabled(input)) { this.transitionStack(stack, 'UPDATE_FAILED', 'Resource update failed', operationId); } else { this.transitionStack(stack, 'UPDATE_ROLLBACK_IN_PROGRESS', 'Resource update failed', operationId); @@ -550,7 +553,8 @@ export class FakeCloudFormation { if (this.shouldFail(cs.template)) { const failedStatus = isCreate ? 'CREATE_FAILED' : 'UPDATE_FAILED'; - if (input.DisableRollback) { + this.addFailedUpdateResourceEvents(stack, cs.template, operationId, isCreate ? 'CREATE' : 'UPDATE'); + if (this.rollbackIsDisabled({ ...input, DeploymentConfig: cs.deploymentConfig })) { this.transitionStack(stack, failedStatus, 'Resource operation failed', operationId); } else { const rollbackStatus = isCreate ? 'ROLLBACK_IN_PROGRESS' : 'UPDATE_ROLLBACK_IN_PROGRESS'; @@ -865,6 +869,7 @@ export class FakeCloudFormation { capabilities: (input.Capabilities as string[]) ?? [], description: input.Description, changes: [], + deploymentConfig: input.DeploymentConfig, creationTime: new Date(), changeSetFailureEvents: [], earlyValidationErrors: [], @@ -1153,7 +1158,24 @@ export class FakeCloudFormation { }); } - private addResourceEvent(stack: InMemoryStack, logicalId: string, resourceType: string, status: string, operationId?: string) { + /** + * Whether CloudFormation would have rollback disabled for this operation. + * + * Standard deployments say so with `DisableRollback` on the call. Express Mode has rollback disabled server-side by + * default, and re-enables it by sending `DeploymentConfig.DisableRollback: false` - so express operations strand the + * stack in `*_FAILED` unless they explicitly opt back into rollback. + */ + private rollbackIsDisabled(input: { DisableRollback?: boolean; DeploymentConfig?: DeploymentConfig }): boolean { + if (input.DisableRollback) { + return true; + } + if (input.DeploymentConfig?.Mode === 'EXPRESS') { + return input.DeploymentConfig.DisableRollback !== false; + } + return false; + } + + private addResourceEvent(stack: InMemoryStack, logicalId: string, resourceType: string, status: string, operationId?: string, reason?: string) { stack.events.unshift({ StackId: stack.id, StackName: stack.name, @@ -1162,11 +1184,32 @@ export class FakeCloudFormation { PhysicalResourceId: `fake-${logicalId}-${uid()}`, ResourceType: resourceType, ResourceStatus: status as any, + ResourceStatusReason: reason, OperationId: operationId, Timestamp: new Date(), }); } + /** + * Emit resource-level failure events for every failing resource that declares a `FailReason`. + * + * Real CloudFormation reports why an operation failed on a resource event, not on the stack event. Resources that + * only set `Fail: true` keep the old behaviour of producing stack-level events only. + */ + private addFailedUpdateResourceEvents(stack: InMemoryStack, template: Record, operationId?: string, verb: 'UPDATE' | 'CREATE' = 'UPDATE') { + for (const [logicalId, res] of Object.entries(templateResources(template))) { + const r = res as any; + const reason = r.Properties?.FailReason; + if (reason === undefined) { + continue; + } + if (this.alwaysFailResources || r.Properties?.Fail === true) { + this.addResourceEvent(stack, logicalId, r.Type, `${verb}_IN_PROGRESS`, operationId); + this.addResourceEvent(stack, logicalId, r.Type, `${verb}_FAILED`, operationId, reason); + } + } + } + private toStackDescription(stack: InMemoryStack): Stack { return { StackName: stack.name, diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts index d52d4898f..a09c76c59 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts @@ -68,14 +68,22 @@ export class TestIoHost implements IIoHost { return spyResponse ?? msg.defaultResponse; } - public expectMessage(m: { containing: string; level?: IoMessageLevel }) { + public expectMessage(m: { containing: string; level?: IoMessageLevel; code?: IoMessageCode }) { expect(this.messages).toContainEqual(expect.objectContaining({ ...m.level ? { level: m.level } : undefined, + ...m.code ? { code: m.code } : undefined, // Can be a partial string as well message: expect.stringContaining(m.containing), })); } + /** + * Return all messages emitted with a given code + */ + public messagesWithCode(code: IoMessageCode): Array> { + return this.messages.filter((m) => m.code === code); + } + /** * Mocks the response for a given message code. * diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts index e601943b9..fec70d984 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts @@ -620,6 +620,71 @@ IAM Statement Changes // THEN successfulDeployment(); }); + + // The confirm-and-retry-with-rollback prompt is the entire user-visible recovery path for + // aws/aws-cdk-cli#1931, so pin both what the user is told and what we do when they agree. + test('replacement-requires-rollback under --express explains that rollback is disabled, and retries with it enabled', async () => { + // GIVEN + mockDeployStack.mockImplementation(async (params) => { + if (params.rollback === true) { + return { + type: 'did-deploy-stack', + stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack', + outputs: {}, + noOp: false, + deleteFailures: [], + stabilizingResources: [], + } satisfies DeployStackResult; + } + return { type: 'replacement-requires-rollback' } satisfies DeployStackResult; + }); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-role'); + await toolkit.deploy(cx, { express: true }); + + // THEN + expect(ioHost.requestSpy).toHaveBeenCalledWith(expect.objectContaining({ + code: 'CDK_TOOLKIT_I5050', + data: expect.objectContaining({ + motivation: 'Change includes a replacement, which CloudFormation does not support while rollback is disabled (the default for Express Mode)', + }), + })); + + // ... and we retried the deployment ourselves with rollback enabled + expect(mockDeployStack).toHaveBeenCalledWith(expect.objectContaining({ express: true, rollback: true })); + successfulDeployment(); + }); + + test('replacement-requires-rollback without --express keeps the --no-rollback wording', async () => { + // GIVEN + mockDeployStack.mockImplementation(async (params) => { + if (params.rollback === true) { + return { + type: 'did-deploy-stack', + stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack', + outputs: {}, + noOp: false, + deleteFailures: [], + stabilizingResources: [], + } satisfies DeployStackResult; + } + return { type: 'replacement-requires-rollback' } satisfies DeployStackResult; + }); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-role'); + await toolkit.deploy(cx, { rollback: false }); + + // THEN + expect(ioHost.requestSpy).toHaveBeenCalledWith(expect.objectContaining({ + code: 'CDK_TOOLKIT_I5050', + data: expect.objectContaining({ + motivation: 'Change includes a replacement which cannot be deployed with "--no-rollback"', + }), + })); + successfulDeployment(); + }); }); test('deploy returns stack information', async () => { diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts new file mode 100644 index 000000000..b1aff6653 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -0,0 +1,606 @@ +import type { CloudFormationStackArtifact } from '@aws-cdk/cloud-assembly-api'; +import type { Change, CreateChangeSetCommandInput, Stack } from '@aws-sdk/client-cloudformation'; +import { + CreateChangeSetCommand, + DescribeChangeSetCommand, + ExecuteChangeSetCommand, + StackStatus, + UpdateStackCommand, +} from '@aws-sdk/client-cloudformation'; +import type { DeployStackOptions as DeployStackApiOptions } from '../../../lib/api/deployments/deploy-stack'; +import { CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON, deployStack } from '../../../lib/api/deployments/deploy-stack'; +import { CloudFormationStackDiagnoser } from '../../../lib/api/diagnosing/stack-diagnoser'; +import { NoBootstrapStackEnvironmentResources } from '../../../lib/api/environment'; +import { StackArtifactSourceTracer } from '../../../lib/api/source-tracing/private/stack-source-tracing'; +import { testStack } from '../../_helpers/assembly'; +import { FakeCloudFormation } from '../../_helpers/fake-aws/fake-cloudformation'; +import { advanceTime } from '../../_helpers/fake-time'; +import { + mockCloudFormationClient, + mockResolvedEnvironment, + MockSdk, + MockSdkProvider, + restoreSdkMocksToDefault, +} from '../../_helpers/mock-sdk'; +import { TestIoHost } from '../../_helpers/test-io-host'; + +const W5903 = 'CDK_TOOLKIT_W5903'; + +let ioHost = new TestIoHost('debug', true); +let ioHelper = ioHost.asHelper('deploy'); + +function testDeployStack(options: DeployStackApiOptions) { + return advanceTime(deployStack(options, ioHelper)); +} + +jest.mock('../../../lib/api/deployments/checks', () => ({ + determineAllowCrossAccountAssetPublishing: jest.fn().mockResolvedValue(true), +})); + +function startTemplate() { + return { + Description: 'Start template in deploy-stack-express-replacement.test.ts', + Resources: { + MyResource: { + Type: 'Test::Resource::Type', + Properties: { Foo: 'Foo' }, + }, + }, + }; +} + +function targetTemplate() { + return { + Description: 'Start template in deploy-stack-express-replacement.test.ts', + Resources: { + MyResource: { + Type: 'Test::Resource::Type', + Properties: { Bar: 'Bar' }, + }, + }, + }; +} + +/** + * A template whose update fails the way CloudFormation fails a replacement on a rollback-disabled stack + */ +function templateRejectingReplacement() { + return { + Resources: { + MyResource: { + Type: 'Test::Resource::Type', + Properties: { + Bar: 'Bar', + Fail: true, + // CloudFormation appends a period; we match on a substring, so keep the fixture faithful to the service. + FailReason: `${CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON}.`, + }, + }, + }, + }; +} + +const FAKE_STACK = testStack({ + stackName: 'withouterrors', + template: targetTemplate(), +}); + +const FAKE_STACK_REJECTING_REPLACEMENT = testStack({ + stackName: 'withouterrors', + template: templateRejectingReplacement(), +}); + +const baseResponse = { + StackName: 'mock-stack-name', + StackId: 'mock-stack-id', + CreationTime: new Date(), + StackStatus: StackStatus.CREATE_COMPLETE, + EnableTerminationProtection: false, +}; + +let sdk: MockSdk; +let sdkProvider: MockSdkProvider; +const fakeCfn = new FakeCloudFormation(); + +beforeEach(() => { + fakeCfn.reset(); + + ioHost = new TestIoHost('debug', true); + ioHelper = ioHost.asHelper('deploy'); + + sdkProvider = new MockSdkProvider(); + sdk = new MockSdk(); + sdk.getUrlSuffix = () => Promise.resolve('amazonaws.com'); + jest.resetAllMocks(); + + restoreSdkMocksToDefault(); + fakeCfn.installUsingAwsMock(mockCloudFormationClient); + + jest.useFakeTimers(); +}); + +afterEach(() => { + jest.useRealTimers(); +}); + +function standardDeployStackArguments(stack: CloudFormationStackArtifact = FAKE_STACK): DeployStackApiOptions { + const resolvedEnvironment = mockResolvedEnvironment(); + return { + stack, + sdk, + sdkProvider, + resolvedEnvironment, + envResources: new NoBootstrapStackEnvironmentResources(resolvedEnvironment, sdk, ioHelper), + diagnoser: new CloudFormationStackDiagnoser({ + sdk, + sourceTracer: new StackArtifactSourceTracer(stack), + ioHelper, + topLevelStackHierarchicalId: stack.hierarchicalId, + }), + }; +} + +function givenStackExists(overrides: Partial & { StackName?: string } = {}) { + const stackName = overrides.StackName ?? 'withouterrors'; + fakeCfn.createStackSync({ + ...baseResponse, + StackName: stackName, + ...overrides, + }); + fakeCfn.accessStack(stackName).template = startTemplate(); +} + +/** + * The shape CloudFormation actually emitted for the change in aws/aws-cdk-cli#1931. + * + * A replacement of a resource with a default deletion policy carries BOTH a `ReplaceAndDelete` policy action and + * `Replacement: 'True'` - verified against a real `DescribeChangeSet` response for the reproduction stack. + */ +function policyActionReplacementChange(logicalId = 'TaskDef54694570'): Change { + return { + Type: 'Resource', + ResourceChange: { + PolicyAction: 'ReplaceAndDelete', + Action: 'Modify', + LogicalResourceId: logicalId, + ResourceType: 'AWS::ECS::TaskDefinition', + Replacement: 'True', + Scope: ['Properties'], + Details: [ + { + Target: { Attribute: 'Properties', Name: 'Memory', RequiresRecreation: 'Always' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }, + ], + }, + }; +} + +function updateChange(logicalId = 'Queue4A7E3555'): Change { + return { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: logicalId, + ResourceType: 'AWS::SQS::Queue', + Replacement: 'False', + }, + }; +} + +/** + * `CDKMetadata` reports `Replacement: 'Conditional'` on essentially every real CDK deployment (its `Analytics` + * property is `RequiresRecreation: 'Conditionally'`), so `Conditional` must never be treated as a replacement. + */ +function conditionalChange(logicalId = 'CDKMetadata'): Change { + return { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: logicalId, + ResourceType: 'AWS::CDK::Metadata', + Replacement: 'Conditional', + Scope: ['Properties'], + Details: [ + { + Target: { Attribute: 'Properties', Name: 'Analytics', RequiresRecreation: 'Conditionally' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }, + ], + }, + }; +} + +/** + * The change shapes the guard must recognise. + */ +const REPLACEMENT_FIXTURES: Array<[string, (logicalId?: string) => Change]> = [ + ['policy action with Replacement=True', policyActionReplacementChange], +]; + +/** + * Make any attempt to actually mutate the stack a hard test failure. + * + * The point of the guard is that we stop *before* submitting the update, so a test that only asserted on the returned + * result type would still pass if the deployment happened anyway. + */ +function failOnAnyStackMutation() { + mockCloudFormationClient.on(ExecuteChangeSetCommand).callsFake(() => { + throw new Error('ExecuteChangeSet must not be called when the replacement guard trips'); + }); + mockCloudFormationClient.on(UpdateStackCommand).callsFake(() => { + throw new Error('UpdateStack must not be called when the replacement guard trips'); + }); +} + +function expectNoStackMutation() { + expect(mockCloudFormationClient).not.toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(mockCloudFormationClient).not.toHaveReceivedCommand(UpdateStackCommand); +} + +describe.each(REPLACEMENT_FIXTURES)('change set path, replacement reported as %s', (_shape, replacementChange) => { + test('express with rollback disabled is gated before ExecuteChangeSet', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [replacementChange()]; + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('replacement-requires-rollback'); + expectNoStackMutation(); + + ioHost.expectMessage({ + level: 'warn', + code: W5903, + containing: 'does not support while rollback is disabled', + }); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'unless you ask for it with --rollback' }); + + // From a healthy stack the caller is about to offer to retry with rollback enabled, so we must NOT tell the user + // to run a deployment themselves, and the recovery runbook does not apply yet. + expect(ioHost.messagesWithCode(W5903)[0].message).not.toContain('To recover'); + expect(ioHost.messagesWithCode(W5903)[0].message).not.toContain('cdk deploy --express --rollback'); + + // Nothing was submitted, so we know this from the change set rather than from a failure + expect(ioHost.messagesWithCode(W5903)[0].data).toEqual(expect.objectContaining({ + stackName: 'withouterrors', + detectedBy: 'change-set', + replacements: [expect.objectContaining({ + logicalId: 'TaskDef54694570', + resourceType: 'AWS::ECS::TaskDefinition', + })], + })); + }); + + test('express with rollback enabled deploys the replacement with DisableRollback: false', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [replacementChange()]; + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + rollback: true, + forceDeployment: true, + }); + + // THEN - this is the route users are sent to, so pin both halves of it + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(mockCloudFormationClient).toHaveReceivedCommandWith(CreateChangeSetCommand, { + ...expect.anything, + DeploymentConfig: { + Mode: 'EXPRESS', + DisableRollback: false, + }, + } as CreateChangeSetCommandInput); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + + test('non-express --no-rollback is gated, without the express-specific guidance', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [replacementChange()]; + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + rollback: false, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('replacement-requires-rollback'); + expectNoStackMutation(); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + + test('non-express paused fail state still requires a rollback first', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); + fakeCfn.overrideChangeSetChanges = [replacementChange()]; + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + rollback: false, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('failpaused-need-rollback-first'); + expectNoStackMutation(); + }); + + // Verified against CloudFormation: retrying with rollback enabled from a stack that is already in a failed state is + // answered with "This stack is currently in a non-terminal [UPDATE_FAILED] state", so telling the user to deploy with + // `--rollback` and nothing else would send them into a second failure. The previous configuration must be replayed + // first, so an already-failed stack gets the recovery steps up front. + test('express from an already-failed stack explains that it has to be unwedged first', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); + fakeCfn.overrideChangeSetChanges = [replacementChange()]; + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('replacement-requires-rollback'); + expectNoStackMutation(); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'in a failed state' }); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'Revert your change' }); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'cdk deploy --express --method=direct' }); + }); +}); + +describe('change set path', () => { + test('express with rollback disabled and no replacement deploys unchanged', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [updateChange()]; + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(mockCloudFormationClient).toHaveReceivedCommandWith(CreateChangeSetCommand, { + ...expect.anything, + DeploymentConfig: { Mode: 'EXPRESS' }, + } as CreateChangeSetCommandInput); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + + test('a replacement on the second page of DescribeChangeSet results is still gated', async () => { + // GIVEN - one change per page, with the replacement on page two + fakeCfn.reset({ pageSize: 1 }); + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [updateChange('First'), policyActionReplacementChange('Second')]; + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('replacement-requires-rollback'); + expectNoStackMutation(); + expect(mockCloudFormationClient.commandCalls(DescribeChangeSetCommand).length).toBeGreaterThan(1); + }); + + test('a plain deployment with a replacement deploys unchanged', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [policyActionReplacementChange()]; + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + }); + + // This is why the routing hook lives in `monitorDeployment` rather than in `directDeployment`: a change set can + // contain a replacement the pre-flight check cannot see (CloudFormation reports `Conditional`), so the change set + // path needs the after-the-fact guidance too. + test('a replacement the change set did not declare is still routed after CloudFormation rejects it', async () => { + // GIVEN - the change set only reports a Conditional change, so the gate does not fire... + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [conditionalChange()]; + + // WHEN - ...but executing it fails the way CloudFormation fails a rejected replacement + await expect(testDeployStack({ + ...standardDeployStackArguments(FAKE_STACK_REJECTING_REPLACEMENT), + express: true, + forceDeployment: true, + })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); + + // THEN + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'CloudFormation refused a replacement' }); + expect(ioHost.messagesWithCode(W5903)[0].data).toEqual(expect.objectContaining({ + detectedBy: 'service-error', + })); + }); +}); + +/** + * The express replacement guard has been deleted once and restructured once without the suite noticing: + * + * - aws/aws-cdk-cli#1745 removed `expressNoRollback` from the guard condition and relaxed the test that covered it, + * which shipped the regression in CLI 2.1133.0. + * - aws/aws-cdk-cli#1785 then restructured what was left into `if (!this.options.express) { ... }`. + * - aws/aws-cdk-cli#1931 is the resulting SEV: `cdk deploy --express` submits a replacement while CloudFormation has + * rollback disabled, CloudFormation rejects it, and the stack is stranded in UPDATE_FAILED. + * + * These cases exist to fail loudly if the guard is removed again, or if its scope is changed so that express + * deployments stop consulting it. They deliberately assert the *negative* (nothing was submitted to CloudFormation) + * rather than that a message was printed. + */ +describe('REGRESSION aws/aws-cdk-cli#1931: the express replacement guard must not be removed or rescoped', () => { + test.each([ + ['rollback not specified (express disables rollback by default)', undefined], + ['rollback explicitly disabled', false], + ] satisfies Array<[string, boolean | undefined]>)('%s', async (_name, rollback) => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [policyActionReplacementChange()]; + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + rollback, + forceDeployment: true, + }); + + // THEN - the result type `cdk-toolkit` turns into a confirm-and-retry-with-rollback prompt + expect(result).toEqual({ type: 'replacement-requires-rollback' }); + expectNoStackMutation(); + expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_COMPLETE); + }); + + test('rollback: true is the one express case that is allowed through', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [policyActionReplacementChange()]; + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + rollback: true, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + }); +}); + +describe('direct path', () => { + test('a rejected replacement strands the stack, and the user is routed to --express --rollback', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + + // WHEN + await expect(testDeployStack({ + ...standardDeployStackArguments(FAKE_STACK_REJECTING_REPLACEMENT), + deploymentMethod: { method: 'direct' }, + express: true, + forceDeployment: true, + })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); + + // THEN - this is the SEV: rollback is disabled server-side, so the stack cannot roll itself back + expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_FAILED); + + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'CloudFormation refused a replacement' }); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'Revert your change' }); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'cdk deploy --express --method=direct' }); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'cdk deploy --express --rollback' }); + expect(ioHost.messagesWithCode(W5903)[0].data).toEqual(expect.objectContaining({ + stackName: 'withouterrors', + detectedBy: 'service-error', + replacements: [expect.objectContaining({ logicalId: 'MyResource' })], + })); + }); + + test('with rollback enabled the stack rolls back and no guidance is emitted', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + + // WHEN + await expect(testDeployStack({ + ...standardDeployStackArguments(FAKE_STACK_REJECTING_REPLACEMENT), + deploymentMethod: { method: 'direct' }, + express: true, + rollback: true, + forceDeployment: true, + })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); + + // THEN - a genuinely failing replacement under `--express --rollback` recovers on its own + expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_ROLLBACK_COMPLETE); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + + // `cdk deploy --express --method=direct` with the previous configuration is the documented way to unwedge a stack + // that is already stranded in UPDATE_FAILED, so it must stay a plain no-op and must never be refused up front. + test('an empty-diff replay from a stranded stack is not blocked', async () => { + // GIVEN - the stranded stack already has the template we are about to deploy + givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); + fakeCfn.accessStack('withouterrors').template = targetTemplate(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'direct' }, + express: true, + forceDeployment: true, + }); + + // THEN + expect(result).toEqual(expect.objectContaining({ type: 'did-deploy-stack', noOp: true })); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + + // CloudFormation owns the prose we match on and has a change landing around 2026-11-15. When it stops matching we + // want a breadcrumb in the debug log rather than silence. + test('a failure that does not mention the rejection logs what it did see', async () => { + // GIVEN - a failing update whose reason is not the replacement rejection + const otherFailure = testStack({ + stackName: 'withouterrors', + template: { + Resources: { + MyResource: { + Type: 'Test::Resource::Type', + Properties: { Bar: 'Bar', Fail: true, FailReason: 'Some other service error' }, + }, + }, + }, + }); + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + + // WHEN + await expect(testDeployStack({ + ...standardDeployStackArguments(otherFailure), + deploymentMethod: { method: 'direct' }, + express: true, + forceDeployment: true, + })).rejects.toThrow('Some other service error'); + + // THEN + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + ioHost.expectMessage({ level: 'debug', containing: 'no reported error mentioned' }); + ioHost.expectMessage({ level: 'debug', containing: 'Some other service error' }); + }); +}); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts index 7325f66b4..75de4fa7a 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts @@ -1603,13 +1603,19 @@ test.each([ // CloudFormation's RollbackStack API is not supported for stacks last deployed // with express mode, so `cdk deploy --express` (without an explicit `--rollback`) -// must never route through the rollback path. It always fix-forwards via the -// change set / UpdateStack and lets CloudFormation surface any error (including a -// rejected replacement on a disable-rollback stack). `--express --rollback` -// explicitly opts back into the rollback-enabled path. +// must never route through the rollback path. +// +// It must, however, still refuse to submit a replacement while rollback is disabled: +// CloudFormation rejects those and leaves the stack in UPDATE_FAILED, from which an +// express stack cannot be rolled back. `--express --rollback` re-enables rollback +// server-side (DisableRollback: false) and is the route users are sent to. +// +// See aws/aws-cdk-cli#1931. The `express, no explicit rollback` expectation below was +// changed from 'replacement-requires-rollback' to 'did-deploy-stack' by #1745, which is +// why the regression shipped unnoticed; #1785 then restructured the guard it removed. test.each([ - // --express alone (rollback defaults off): always fix-forward, never divert to rollback - ['express, no explicit rollback', { express: true } as Partial, 'did-deploy-stack'], + // --express alone (rollback disabled server-side): a replacement must not be submitted + ['express, no explicit rollback', { express: true } as Partial, 'replacement-requires-rollback'], // --express --rollback: rollback is explicitly enabled, so the replacement deploys directly ['express with rollback=true', { express: true, rollback: true } as Partial, 'did-deploy-stack'], ] satisfies Array<[string, Partial, string]>)( @@ -1636,11 +1642,15 @@ test.each([ // A stack last deployed with express mode that is in a paused fail state // (UPDATE_FAILED) cannot be recovered via the RollbackStack API. `cdk deploy -// --express` must therefore always fix-forward via createChangeSet/UpdateStack -// instead of routing to the rollback path. +// --express` must therefore never return 'failpaused-need-rollback-first'; it +// fix-forwards via createChangeSet/UpdateStack instead. +// +// A replacement while rollback is disabled is still refused (with the +// retry-with-rollback result, not the rollback-first result), because submitting it +// would only deepen the wedge. See aws/aws-cdk-cli#1931. test.each([ ['express, no explicit rollback, no-replacement', { express: true } as Partial, 'no-replacement', 'did-deploy-stack'], - ['express, no explicit rollback, replacement', { express: true } as Partial, 'replacement', 'did-deploy-stack'], + ['express, no explicit rollback, replacement', { express: true } as Partial, 'replacement', 'replacement-requires-rollback'], ['express with rollback=true, no-replacement', { express: true, rollback: true } as Partial, 'no-replacement', 'did-deploy-stack'], ['express with rollback=true, replacement', { express: true, rollback: true } as Partial, 'replacement', 'did-deploy-stack'], ] satisfies Array<[string, Partial, 'replacement' | 'no-replacement', string]>)( From 2346f147ef075e51de8c15dcd5c9aa99eaae5902 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 18 Sep 2026 06:07:24 +0000 Subject: [PATCH 02/14] chore: self mutation Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- packages/@aws-cdk/toolkit-lib/docs/message-registry.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/toolkit-lib/docs/message-registry.md b/packages/@aws-cdk/toolkit-lib/docs/message-registry.md index 828891ca7..7e4e850db 100644 --- a/packages/@aws-cdk/toolkit-lib/docs/message-registry.md +++ b/packages/@aws-cdk/toolkit-lib/docs/message-registry.md @@ -117,7 +117,7 @@ Please let us know by [opening an issue](https://github.com/aws/aws-cdk-cli/issu | `CDK_TOOLKIT_I5900` | Deployment results on success | `result` | {@link SuccessfulDeployStackResult} | | `CDK_TOOLKIT_I5901` | Generic deployment success messages | `info` | n/a | | `CDK_TOOLKIT_W5902` | Express Mode deployment completed with resources still stabilizing | `warn` | n/a | -| `CDK_TOOLKIT_W5903` | Deployment includes a replacement that CloudFormation does not support while rollback is disabled | `warn` | n/a | +| `CDK_TOOLKIT_W5903` | Deployment includes a replacement that CloudFormation does not support while rollback is disabled | `warn` | {@link ReplacementRequiresRollback} | | `CDK_TOOLKIT_W5400` | Hotswap disclosure message | `warn` | n/a | | `CDK_TOOLKIT_E5001` | No stacks found | `error` | n/a | | `CDK_TOOLKIT_E5500` | Stack Monitoring error | `error` | {@link ErrorPayload} | From 041a6dbd3e5c957a35732c07582a3c404ff37b33 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Fri, 18 Sep 2026 19:57:03 +0000 Subject: [PATCH 03/14] docs(toolkit-lib): record why the direct path and Conditional are not gated Move three rationales into the code, where they are durable: - --express --method=direct is deliberately not refused up front, because replaying the previous configuration that way is the only exit from a stack already stranded in UPDATE_FAILED. - Replacement: 'Conditional' is deliberately excluded, because CDKMetadata reports it on essentially every CDK deployment. - The CloudFormation reason match should be replaced with a structured discriminator if one ever becomes available. --- .../lib/api/deployments/deploy-stack.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index 0e44c28fe..c5a41e923 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -817,9 +817,11 @@ class FullCloudFormationDeployment { * Tell the user how to perform a replacement when CloudFormation rejected one because rollback was disabled. * * The `--method=direct` path has no change set to inspect, so it cannot be gated up front the way the change set - * path is; a replacement there is only discovered from the failure CloudFormation reports. We deliberately do not - * refuse `--express --method=direct` up front either, because redeploying the previous configuration that way is - * the documented way to unwedge a stack that is already stuck. + * path is; a replacement there is only discovered from the failure CloudFormation reports. + * + * `--express --method=direct` is deliberately NOT refused up front, and that gap should not be "fixed": replaying the + * previous configuration that way is the only exit from a stack already stranded in UPDATE_FAILED, so refusing the + * combination would strand users permanently. * * The original error is left to propagate untouched, so a genuinely failing replacement still reports its real * underlying service error. @@ -1121,6 +1123,11 @@ function arrayEquals(a: any[], b: any[]): boolean { /** * Find the resource changes in a change set that CloudFormation would perform by replacement + * + * `Replacement: 'Conditional'` is deliberately excluded: `CDKMetadata` reports it on essentially every CDK deployment + * (its `Analytics` property is `RequiresRecreation: 'Conditionally'`), so gating on it would gate almost every express + * deployment. A `Conditional` change that does turn out to replace is caught after the fact by + * `routeReplacementRejectedWithRollbackDisabled`. */ function findReplacements(report: ChangeSetReport): ReplacedResource[] { return (report.changeSet.Changes ?? []).flatMap((c) => { @@ -1148,8 +1155,9 @@ function findReplacements(report: ChangeSetReport): ReplacedResource[] { * * CloudFormation surfaces this as a resource status reason with no structured error code attached (`extractErrorCode` * finds no `HandlerErrorCode:`/`Error Code:` prefix in it), so matching this text is the only trigger available. That - * makes it fragile: CloudFormation owns the string and has a change landing around 2026-11-15. A miss is logged at - * debug level and only costs the extra guidance - the underlying CloudFormation error is reported either way. + * makes it fragile: CloudFormation owns the string and has a change landing around 2026-11-15. Replace this match with + * a structured discriminator if CloudFormation ever exposes one. A miss is logged at debug level and only costs the + * extra guidance - the underlying CloudFormation error is reported either way. */ export const CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON = 'Replacement type updates not supported on stack with disable-rollback'; From c5ee8b970085cdbe89325de2fccda5b21bd44ebb Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Fri, 18 Sep 2026 20:34:35 +0000 Subject: [PATCH 04/14] fix(toolkit-lib): scope the replacement routing to express, and fail terminally when already wedged Two defects in the replacement guard. The direct-path routing was gated on rollbackDisabled() alone, which is also true for a standard-mode `--no-rollback` deployment. Since the guidance names Express Mode flags, a wedged standard-mode user was told to switch to Express Mode, which is sticky and gives up `cdk rollback` - a strictly worse position for a problem a plain `cdk deploy` fixes. The notify is now gated on `express`, matching the change-set arm. On express with a replacement against an already-failed stack, the guard printed that `--express --rollback` cannot update the stack and then returned `replacement-requires-rollback`, so the toolkit offered exactly that deployment. The confirmation defaults to yes, so a non-interactive caller ran it and hit `ValidationError: non-terminal [UPDATE_FAILED]`. That case now throws `ReplacementRequiresUnwedge` carrying the unwedge steps, so it is reported once and CI cannot auto-run a deployment that is known to fail. Also: `ReplacedResource.logicalId` is optional and both fabricated fallbacks ('' and the stack name) are gone, so `replacements` can be genuinely empty as documented; `shouldDisableRollback` renamed to `shouldSendDisableRollbackFlag`; and the comment claiming express cannot send a top-level `DisableRollback` is corrected (`--express --no-rollback` does send it). --- .../lib/api/deployments/deploy-stack.ts | 51 +++++++---- .../toolkit-lib/lib/payloads/deploy.ts | 4 +- .../deploy-stack-express-replacement.test.ts | 87 +++++++++++++++---- .../test/api/deployments/deploy-stack.test.ts | 8 +- 4 files changed, 110 insertions(+), 40 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index c5a41e923..85d2c94f7 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -597,15 +597,22 @@ class FullCloudFormationDeployment { // all regions, the express half of this guard - and CDK_TOOLKIT_W5903 - can be deleted. if (replacements.length > 0 && this.rollbackDisabled()) { if (this.options.express) { - await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg( - replacementRoutingMessage({ rejected: false, needsUnwedge: isPausedFailState }), - { - stackName: this.stackName, - changeSetId: changeSetReport.changeSet.ChangeSetId, - replacements, - detectedBy: 'change-set', - }, - )); + const guidance = replacementRoutingMessage({ rejected: false, needsUnwedge: isPausedFailState }); + + // A stack that is already in a failed state cannot be updated with rollback enabled either - CloudFormation + // answers "This stack is currently in a non-terminal [UPDATE_FAILED] state". Returning + // `replacement-requires-rollback` here would make the toolkit offer exactly that deployment, and because the + // confirmation defaults to yes, a non-interactive caller would run it and fail a second time. Report once. + if (isPausedFailState) { + throw new ToolkitError('ReplacementRequiresUnwedge', guidance); + } + + await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg(guidance, { + stackName: this.stackName, + changeSetId: changeSetReport.changeSet.ChangeSetId, + replacements, + detectedBy: 'change-set', + })); } return { type: 'replacement-requires-rollback' }; } @@ -827,7 +834,10 @@ class FullCloudFormationDeployment { * underlying service error. */ private async routeReplacementRejectedWithRollbackDisabled(error: any, errors: ResourceErrors): Promise { - if (!this.rollbackDisabled()) { + // Express only: the guidance below names Express Mode flags, and switching a standard-mode deployment to Express + // Mode would be actively harmful (the mode is sticky and gives up `cdk rollback`). A standard `--no-rollback` + // deployment that hits this recovers with a plain `cdk deploy`, which the README already documents. + if (!this.options.express || !this.rollbackDisabled()) { return; } @@ -851,10 +861,12 @@ class FullCloudFormationDeployment { replacementRoutingMessage({ rejected: true, needsUnwedge: true }), { stackName: this.stackName, - replacements: rejected.map((e) => ({ - logicalId: e.logicalId ?? this.stackName, - resourceType: e.resourceType, - })), + replacements: rejected + .filter((e) => e.logicalId !== undefined) + .map((e) => ({ + logicalId: e.logicalId, + resourceType: e.resourceType, + })), detectedBy: 'service-error', }, )); @@ -907,13 +919,14 @@ class FullCloudFormationDeployment { * deployed everywhere yet. */ private commonExecuteOptions(): Partial> { - // Not `rollbackDisabled()`: express deployments also run with rollback disabled, but they must express that - // through `DeploymentConfig` rather than by sending `DisableRollback` on the call. - const shouldDisableRollback = this.options.rollback === false; + // Deliberately not `rollbackDisabled()`, which is also true for plain `--express`. This flag tracks the explicit + // `--no-rollback` request only, so that plain `--express` does not start sending a `DisableRollback` it never + // asked for. `--express --no-rollback` sends both this flag and `DeploymentConfig`, which is intended. + const shouldSendDisableRollbackFlag = this.options.rollback === false; return { StackName: this.stackName, - ...(shouldDisableRollback ? { DisableRollback: true } : undefined), + ...(shouldSendDisableRollbackFlag ? { DisableRollback: true } : undefined), }; } } @@ -1142,7 +1155,7 @@ function findReplacements(report: ChangeSetReport): ReplacedResource[] { } return [{ - logicalId: change.LogicalResourceId ?? '', + logicalId: change.LogicalResourceId, resourceType: change.ResourceType, replacement: change.Replacement, policyAction, diff --git a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts index 6c9a73f0a..8298dfff4 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts @@ -82,8 +82,10 @@ export interface PublishAssetEvent { export interface ReplacedResource { /** * Logical ID of the resource being replaced + * + * Absent when CloudFormation reported the change or failure without naming a resource. */ - readonly logicalId: string; + readonly logicalId?: string; /** * CloudFormation resource type, when known diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index b1aff6653..569777539 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -214,11 +214,22 @@ function conditionalChange(logicalId = 'CDKMetadata'): Change { } /** - * The change shapes the guard must recognise. + * A replacement CloudFormation reports only via `Replacement: 'True'`, with no policy action attached. + * + * Not gated up front today - see the negative test below and #1971. */ -const REPLACEMENT_FIXTURES: Array<[string, (logicalId?: string) => Change]> = [ - ['policy action with Replacement=True', policyActionReplacementChange], -]; +function replacementOnlyChange(logicalId = 'TaskDef54694570'): Change { + return { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: logicalId, + ResourceType: 'AWS::ECS::TaskDefinition', + Replacement: 'True', + Scope: ['Properties'], + }, + }; +} /** * Make any attempt to actually mutate the stack a hard test failure. @@ -240,7 +251,8 @@ function expectNoStackMutation() { expect(mockCloudFormationClient).not.toHaveReceivedCommand(UpdateStackCommand); } -describe.each(REPLACEMENT_FIXTURES)('change set path, replacement reported as %s', (_shape, replacementChange) => { +describe('change set path, replacement reported as policy action with Replacement=True', () => { + const replacementChange = policyActionReplacementChange; test('express with rollback disabled is gated before ExecuteChangeSet', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -344,29 +356,32 @@ describe.each(REPLACEMENT_FIXTURES)('change set path, replacement reported as %s expectNoStackMutation(); }); - // Verified against CloudFormation: retrying with rollback enabled from a stack that is already in a failed state is - // answered with "This stack is currently in a non-terminal [UPDATE_FAILED] state", so telling the user to deploy with - // `--rollback` and nothing else would send them into a second failure. The previous configuration must be replayed - // first, so an already-failed stack gets the recovery steps up front. - test('express from an already-failed stack explains that it has to be unwedged first', async () => { + // Verified against CloudFormation: a stack already in a failed state cannot be updated with rollback enabled either - + // it answers "This stack is currently in a non-terminal [UPDATE_FAILED] state". Returning + // `replacement-requires-rollback` would make the toolkit offer that deployment, and since the confirmation defaults + // to yes, a non-interactive caller would run it and fail again. So this must be a terminal error, not a prompt. + test('express from an already-failed stack fails terminally instead of offering a doomed retry', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); fakeCfn.overrideChangeSetChanges = [replacementChange()]; failOnAnyStackMutation(); // WHEN - const result = await testDeployStack({ + const deployment = testDeployStack({ ...standardDeployStackArguments(), express: true, forceDeployment: true, }); - // THEN - expect(result.type).toEqual('replacement-requires-rollback'); + // THEN - thrown, so there is no result the toolkit could turn into a retry prompt + await expect(deployment).rejects.toThrow(/in a failed state/); + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresUnwedge' })); + await expect(deployment).rejects.toThrow(/Revert your change/); + await expect(deployment).rejects.toThrow(/cdk deploy --express --method=direct/); expectNoStackMutation(); - ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'in a failed state' }); - ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'Revert your change' }); - ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'cdk deploy --express --method=direct' }); + + // ... and it is reported once, by the error, not also as a warning + expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); }); @@ -393,6 +408,27 @@ describe('change set path', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); + // Pins today's behaviour for #1971: detection keys on `PolicyAction`, so a replacement CloudFormation reports only + // via `Replacement: 'True'` is NOT gated up front. It is still caught after the fact on the failure path. If #1971 is + // fixed by widening detection, this test should flip to expecting the gate. + test('a replacement reported without a policy action is not gated up front (#1971)', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + fakeCfn.overrideChangeSetChanges = [replacementOnlyChange()]; + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + express: true, + forceDeployment: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + test('a replacement on the second page of DescribeChangeSet results is still gated', async () => { // GIVEN - one change per page, with the replacement on page two fakeCfn.reset({ pageSize: 1 }); @@ -509,6 +545,25 @@ describe('REGRESSION aws/aws-cdk-cli#1931: the express replacement guard must no }); describe('direct path', () => { + // Standard mode also runs with rollback disabled under `--no-rollback`, and CloudFormation rejects replacements the + // same way - but the guidance names Express Mode flags, and Express Mode is sticky and gives up `cdk rollback`. A + // wedged standard-mode user recovers with a plain `cdk deploy`, so they must NOT be pushed towards `--express`. + test('a non-express --no-rollback rejection is not routed towards Express Mode', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + + // WHEN + await expect(testDeployStack({ + ...standardDeployStackArguments(FAKE_STACK_REJECTING_REPLACEMENT), + deploymentMethod: { method: 'direct' }, + rollback: false, + forceDeployment: true, + })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); + + // THEN - the real CloudFormation error, and no Express Mode guidance + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + test('a rejected replacement strands the stack, and the user is routed to --express --rollback', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts index 75de4fa7a..2f376b40d 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts @@ -1645,12 +1645,12 @@ test.each([ // --express` must therefore never return 'failpaused-need-rollback-first'; it // fix-forwards via createChangeSet/UpdateStack instead. // -// A replacement while rollback is disabled is still refused (with the -// retry-with-rollback result, not the rollback-first result), because submitting it -// would only deepen the wedge. See aws/aws-cdk-cli#1931. +// The replacement case is deliberately absent here: it does not return a result at +// all, it throws, because deploying with rollback enabled cannot update an +// already-failed stack either. It is covered by +// deploy-stack-express-replacement.test.ts. See aws/aws-cdk-cli#1931. test.each([ ['express, no explicit rollback, no-replacement', { express: true } as Partial, 'no-replacement', 'did-deploy-stack'], - ['express, no explicit rollback, replacement', { express: true } as Partial, 'replacement', 'replacement-requires-rollback'], ['express with rollback=true, no-replacement', { express: true, rollback: true } as Partial, 'no-replacement', 'did-deploy-stack'], ['express with rollback=true, replacement', { express: true, rollback: true } as Partial, 'replacement', 'did-deploy-stack'], ] satisfies Array<[string, Partial, 'replacement' | 'no-replacement', string]>)( From c1602b5e274b6db16e09279f4656c3a5cb020098 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Sun, 20 Sep 2026 05:30:56 +0000 Subject: [PATCH 05/14] fix(toolkit-lib): derive rollback safety from the change set, not the current invocation A change set carries its own rollback policy. CloudFormation persists DeploymentConfig on CreateChangeSet, returns it from DescribeChangeSet, and ExecuteChangeSet has no DeploymentConfig field, so it cannot override it. The guard read the current invocation's --express/--rollback flags instead, which is only correct when one invocation both creates and executes the change set. Split across invocations - `--method=change-set --no-execute` then `--method=execute-change-set`, or an explicit execute-change-set retry - the second command's flags decided nothing. Passing --rollback, or simply omitting --express, made the guard conclude rollback was enabled and execute a still-rollback-disabled change set containing a replacement, putting the prohibited combination back in front of CloudFormation and reaching #1931 again. Rollback state now comes from the persisted DeploymentConfig when it pins the answer (Express only; standard mode decides at execute time via the DisableRollback flag, so current options stay authoritative there). A requested policy that conflicts with the persisted one is refused with ChangeSetRollbackPolicyMismatch telling the user to create a new change set, rather than silently doing the opposite of what was asked. The fake could not express any of this because its DescribeChangeSet dropped DeploymentConfig; it now returns it, and createChangeSetSync accepts one. Also restricts the replay recovery guidance to UPDATE_FAILED. isRollbackable also covers CREATE_FAILED and UPDATE_ROLLBACK_FAILED, and a stack that never deployed successfully has no previous configuration to replay, so those states get their own guidance instead of impossible instructions. --- .../lib/api/deployments/deploy-stack.ts | 142 ++++++++++++-- .../_helpers/fake-aws/fake-cloudformation.ts | 5 + .../deploy-stack-express-replacement.test.ts | 176 +++++++++++++++++- 3 files changed, 302 insertions(+), 21 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index 85d2c94f7..7407d6e27 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -543,6 +543,28 @@ class FullCloudFormationDeployment { return this.checkAndExecuteChangeSet(changeSetReport); } + /** + * Which recovery advice applies to the stack's current state. + * + * Only `UPDATE_FAILED` is known to be recoverable by replaying the previously deployed configuration - that is the + * case verified against CloudFormation. `isRollbackable` is broader (it also covers `CREATE_FAILED` and + * `UPDATE_ROLLBACK_FAILED`), and a stack that never deployed successfully has no configuration to replay, so those + * must not be given replay instructions. + */ + private replacementRecovery(): ReplacementRecovery { + if (!this.cloudFormationStack.stackStatus.isRollbackable) { + return 'none'; + } + switch (this.cloudFormationStack.stackStatus.name) { + case 'UPDATE_FAILED': + return 'replay'; + case 'CREATE_FAILED': + return 'recreate'; + default: + return 'resolve-state'; + } + } + /** * Whether CloudFormation will have rollback disabled for this deployment. * @@ -587,6 +609,24 @@ class FullCloudFormationDeployment { } } + // A change set carries its own rollback policy. CloudFormation persists `DeploymentConfig` when the change set is + // created and `ExecuteChangeSet` cannot override it, so when we are executing a change set that already pins the + // answer it - not this invocation's flags - decides what CloudFormation will do. Reading the current options here + // would let `--rollback` (or simply omitting `--express`) appear to enable rollback on a change set that was + // created with it disabled, which is how the replacement below would reach CloudFormation anyway. + const persistedRollbackDisabled = expressRollbackDisabled(changeSetReport.changeSet.DeploymentConfig); + const requestedRollbackDisabled = this.rollbackDisabled(); + + if (persistedRollbackDisabled !== undefined && persistedRollbackDisabled !== requestedRollbackDisabled) { + throw new ToolkitError( + 'ChangeSetRollbackPolicyMismatch', + changeSetPolicyMismatchMessage(changeSetReport.changeSet.ChangeSetName, persistedRollbackDisabled), + ); + } + + const rollbackWillBeDisabled = persistedRollbackDisabled ?? requestedRollbackDisabled; + const isExpress = this.options.express || changeSetReport.changeSet.DeploymentConfig?.Mode === 'EXPRESS'; + // CloudFormation rejects replacement-type updates while rollback is disabled. Standard mode only disables rollback // for `--no-rollback`, but Express Mode disables it by default - which is why this condition must not be scoped to // non-express deployments. #1745 dropped the express half of it, #1785 restructured what was left, and #1931 is the @@ -595,9 +635,13 @@ class FullCloudFormationDeployment { // // Shelf life: CloudFormation has a server-side fix with a tentative ECD of 2026-11-15. Once that is confirmed in // all regions, the express half of this guard - and CDK_TOOLKIT_W5903 - can be deleted. - if (replacements.length > 0 && this.rollbackDisabled()) { - if (this.options.express) { - const guidance = replacementRoutingMessage({ rejected: false, needsUnwedge: isPausedFailState }); + if (replacements.length > 0 && rollbackWillBeDisabled) { + if (isExpress) { + const guidance = replacementRoutingMessage({ + rejected: false, + recovery: this.replacementRecovery(), + status: this.cloudFormationStack.stackStatus.name, + }); // A stack that is already in a failed state cannot be updated with rollback enabled either - CloudFormation // answers "This stack is currently in a non-terminal [UPDATE_FAILED] state". Returning @@ -858,7 +902,12 @@ class FullCloudFormationDeployment { } await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg( - replacementRoutingMessage({ rejected: true, needsUnwedge: true }), + replacementRoutingMessage({ + rejected: true, + // The failure just happened, so the cached stack status predates it. An update had a previous configuration to + // replay; a failed create never did. + recovery: this.update ? 'replay' : 'recreate', + }), { stackName: this.stackName, replacements: rejected @@ -1178,6 +1227,40 @@ function mentionsReplacementRejection(message: string): boolean { return message.toLowerCase().includes(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON.toLowerCase()); } +/** + * Whether a persisted change set `DeploymentConfig` pins the rollback choice, and if so which way. + * + * Only Express Mode records a rollback choice in `DeploymentConfig`, and `ExecuteChangeSet` cannot override it. Standard + * mode carries none: rollback there is decided at execute time by the `DisableRollback` flag, so the current + * invocation's options stay authoritative and this returns `undefined`. + */ +function expressRollbackDisabled(config: DeploymentConfig | undefined): boolean | undefined { + if (config?.Mode !== 'EXPRESS') { + return undefined; + } + return config.DisableRollback !== false; +} + +/** + * Explain that an existing change set's rollback policy cannot be changed by executing it differently + */ +function changeSetPolicyMismatchMessage(changeSetName: string | undefined, persistedRollbackDisabled: boolean): string { + const named = changeSetName ? ` ${chalk.blue(changeSetName)}` : ''; + const persisted = persistedRollbackDisabled ? 'disabled' : 'enabled'; + const requested = persistedRollbackDisabled ? 'enabled' : 'disabled'; + const recreateWith = chalk.blue(`cdk deploy --express${persistedRollbackDisabled ? ' --rollback' : ''}`); + + return [ + `Change set${named} was created with rollback ${persisted}, but this deployment asks for rollback ${requested}.`, + 'CloudFormation fixes that choice when the change set is created and executing it cannot change it, so this', + 'deployment would silently do the opposite of what you asked for.', + '', + `Create a new change set with the flags you want rather than executing this one: ${recreateWith}`, + ].join('\n'); +} + +type ReplacementRecovery = 'none' | 'replay' | 'recreate' | 'resolve-state'; + /** * Explain how to deploy a replacement when rollback is disabled. * @@ -1190,7 +1273,7 @@ function mentionsReplacementRejection(message: string): boolean { * * TODO: point at a CloudFormation User Guide anchor for Express Mode rollback behaviour once one exists. */ -function replacementRoutingMessage(opts: { rejected: boolean; needsUnwedge: boolean }): string { +function replacementRoutingMessage(opts: { rejected: boolean; recovery: ReplacementRecovery; status?: string }): string { const withRollback = chalk.blue('cdk deploy --express --rollback'); const direct = chalk.blue('cdk deploy --express --method=direct'); @@ -1204,21 +1287,40 @@ function replacementRoutingMessage(opts: { rejected: boolean; needsUnwedge: bool 'Express Mode disables rollback unless you ask for it with --rollback; replacements themselves are supported.', ]; - if (!opts.needsUnwedge) { - return headline.join('\n'); - } - // Deploying with rollback enabled cannot update a stack that is already in a failed state - CloudFormation answers - // "This stack is currently in a non-terminal [UPDATE_FAILED] state" (verified against CloudFormation). The previous - // configuration has to be replayed first, so say that rather than sending the user into a second failure. - return [ - ...headline, - '', - `${opts.rejected ? 'The stack may now be' : 'This stack is'} in a failed state, which ${withRollback} cannot update. To recover:`, - ' 1. Revert your change so your app matches the last configuration that deployed successfully.', - ` 2. Run ${direct} - this should replay that configuration as a no-op`, - ' and return the stack to a terminal state.', - ` 3. Re-apply your change and deploy it with ${withRollback}.`, - ].join('\n'); + // "This stack is currently in a non-terminal [UPDATE_FAILED] state" (verified against CloudFormation). What to do + // instead depends on whether a previously deployed configuration exists to go back to. + switch (opts.recovery) { + case 'none': + return headline.join('\n'); + + case 'replay': + return [ + ...headline, + '', + `${opts.rejected ? 'The stack may now be' : 'This stack is'} in a failed state, which ${withRollback} cannot update. To recover:`, + ' 1. Revert your change so your app matches the last configuration that deployed successfully.', + ` 2. Run ${direct} - this should replay that configuration as a no-op`, + ' and return the stack to a terminal state.', + ` 3. Re-apply your change and deploy it with ${withRollback}.`, + ].join('\n'); + + case 'recreate': + return [ + ...headline, + '', + `This stack never completed a deployment, so there is no previous configuration to replay and ${withRollback}`, + 'cannot update it either. Delete the stack and deploy again.', + ].join('\n'); + + case 'resolve-state': + return [ + ...headline, + '', + `This stack is in ${opts.status ?? 'a failed state'}, which ${withRollback} cannot update, and it is not a state`, + 'this command can recover from. Resolve it in CloudFormation first, then deploy the replacement with rollback', + 'enabled.', + ].join('\n'); + } } diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts index f2be9c6d3..5eb767c26 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts @@ -438,6 +438,7 @@ export class FakeCloudFormation { Tags?: Tag[]; Capabilities?: string[]; Description?: string; + DeploymentConfig?: DeploymentConfig; }): CreateChangeSetCommandOutput { const stackName = input.StackName; const stack = this.requireStack(stackName); @@ -473,6 +474,7 @@ export class FakeCloudFormation { capabilities: input.Capabilities ?? [], description: input.Description, changes: changes ?? [], + deploymentConfig: input.DeploymentConfig, creationTime: new Date(), changeSetFailureEvents: [], earlyValidationErrors: [], @@ -514,6 +516,9 @@ export class FakeCloudFormation { Capabilities: cs.capabilities as any, Description: cs.description, CreationTime: cs.creationTime, + // The real API returns the DeploymentConfig that was persisted by CreateChangeSet. It matters because + // ExecuteChangeSet cannot override it, so code executing an existing change set has to read it from here. + ...(cs.deploymentConfig ? { DeploymentConfig: cs.deploymentConfig } : undefined), NextToken: nextToken, $metadata: {}, }; diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 569777539..8e5509e24 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -610,7 +610,12 @@ describe('direct path', () => { // `cdk deploy --express --method=direct` with the previous configuration is the documented way to unwedge a stack // that is already stranded in UPDATE_FAILED, so it must stay a plain no-op and must never be refused up front. - test('an empty-diff replay from a stranded stack is not blocked', async () => { + // + // NOTE: the stranded-but-replayable state is hand-seeded here. The fake assigns the failed change set's template to + // the stack and does not restore the previous template the way a real resource rollback does, so this pins that we + // handle such a state correctly - not that a simulated failed replacement naturally arrives at it. That a real + // failed express replacement does leave the stack replayable was established by deploys against CloudFormation. + test('a no-op replay against a hand-seeded stranded stack is not blocked', async () => { // GIVEN - the stranded stack already has the template we are about to deploy givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); fakeCfn.accessStack('withouterrors').template = targetTemplate(); @@ -659,3 +664,172 @@ describe('direct path', () => { ioHost.expectMessage({ level: 'debug', containing: 'Some other service error' }); }); }); + +/** + * A change set carries the rollback policy it was created with. CloudFormation persists `DeploymentConfig` on + * `CreateChangeSet`, returns it from `DescribeChangeSet`, and `ExecuteChangeSet` cannot override it (its input has no + * `DeploymentConfig` field). So when create and execute happen in separate invocations - `--method=change-set + * --no-execute` then `--method=execute-change-set`, or the toolkit's own retry - the second invocation's flags do not + * decide what CloudFormation does. Deriving rollback safety from the current options there would let the SEV in #1931 + * back in: the guard would believe rollback is enabled and execute a rollback-disabled change set containing a + * replacement. + */ +describe('executing a change set created by an earlier invocation', () => { + function givenExpressChangeSetExists(opts: { rollbackDisabled: boolean; changes: Change[] }) { + fakeCfn.createChangeSetSync({ + StackName: 'withouterrors', + ChangeSetName: 'prepared', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + Changes: opts.changes, + DeploymentConfig: opts.rollbackDisabled + ? { Mode: 'EXPRESS' } + : { Mode: 'EXPRESS', DisableRollback: false }, + }); + } + + const executePrepared: Partial = { + deploymentMethod: { method: 'execute-change-set', changeSetName: 'prepared' }, + forceDeployment: true, + }; + + test('a rollback-disabled change set is not executed just because this invocation passes --rollback', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: true, changes: [updateChange()] }); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + rollback: true, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + await expect(deployment).rejects.toThrow(/created with rollback disabled/); + expectNoStackMutation(); + }); + + // The SEV path: the change set contains a replacement and was created with rollback disabled. Executing it would put + // the prohibited combination in front of CloudFormation and strand the stack. + test('a rollback-disabled change set containing a replacement is never executed', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: true, changes: [policyActionReplacementChange()] }); + failOnAnyStackMutation(); + + // WHEN - this is what the toolkit's retry does: same change set, rollback flipped on + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + rollback: true, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + expectNoStackMutation(); + expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_COMPLETE); + }); + + test('omitting --express does not make a persisted Express change set look safe', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: true, changes: [policyActionReplacementChange()] }); + failOnAnyStackMutation(); + + // WHEN - the invocation looks like standard mode, but the change set is still Express + rollback disabled + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + expectNoStackMutation(); + }); + + test('the opposite mismatch is refused too: rollback-enabled change set executed as rollback-disabled', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: false, changes: [updateChange()] }); + failOnAnyStackMutation(); + + // WHEN - plain `--express` requests rollback disabled, but the change set was created with it enabled + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + await expect(deployment).rejects.toThrow(/created with rollback enabled/); + expectNoStackMutation(); + }); + + // The matching case still has to be gated on the replacement itself, using the persisted policy. + test('a matching rollback-disabled change set with a replacement is gated, not executed', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: true, changes: [policyActionReplacementChange()] }); + failOnAnyStackMutation(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + expect(result.type).toEqual('replacement-requires-rollback'); + expectNoStackMutation(); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'does not support while rollback is disabled' }); + }); + + // `isRollbackable` also covers CREATE_FAILED, where there is no previously deployed configuration to replay. Telling + // such a user to "revert your change and redeploy the last configuration that deployed successfully" would be + // impossible advice, so that state gets its own guidance. + test('a failed initial create is not told to replay a configuration that never existed', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.CREATE_FAILED }); + fakeCfn.overrideChangeSetChanges = [policyActionReplacementChange()]; + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + express: true, + forceDeployment: true, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresUnwedge' })); + await expect(deployment).rejects.toThrow(/no previous configuration to replay/); + await expect(deployment).rejects.toThrow(/Delete the stack and deploy again/); + await expect(deployment).rejects.not.toThrow(/Revert your change/); + expectNoStackMutation(); + }); + + test('a matching change set without a replacement executes normally', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: true, changes: [updateChange()] }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); +}); From e9695047f3e51823386a05a6ffe16cc3504e19a7 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Mon, 28 Sep 2026 04:55:00 +0000 Subject: [PATCH 06/14] fix(toolkit-lib): cite CloudFormation for the guard, stop asserting an undocumented ExecuteChangeSet rule --- .../lib/api/deployments/deploy-stack.ts | 34 ++++++++++++------- .../deploy-stack-express-replacement.test.ts | 6 ++-- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index 7407d6e27..ac4c5c74c 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -609,11 +609,12 @@ class FullCloudFormationDeployment { } } - // A change set carries its own rollback policy. CloudFormation persists `DeploymentConfig` when the change set is - // created and `ExecuteChangeSet` cannot override it, so when we are executing a change set that already pins the - // answer it - not this invocation's flags - decides what CloudFormation will do. Reading the current options here - // would let `--rollback` (or simply omitting `--express`) appear to enable rollback on a change set that was - // created with it disabled, which is how the replacement below would reach CloudFormation anyway. + // A change set carries its own rollback policy: CloudFormation persists `DeploymentConfig` when the change set is + // created. `ExecuteChangeSet` takes a top-level `DisableRollback` but no `DeploymentConfig`, and whether that flag + // can override a persisted EXPRESS policy is undocumented - so when we are executing a change set that already pins + // the answer we treat the persisted value, not this invocation's flags, as what CloudFormation will do. Reading the + // current options here would let `--rollback` (or simply omitting `--express`) appear to enable rollback on a change + // set that was created with it disabled, which is how the replacement below would reach CloudFormation anyway. const persistedRollbackDisabled = expressRollbackDisabled(changeSetReport.changeSet.DeploymentConfig); const requestedRollbackDisabled = this.rollbackDisabled(); @@ -627,11 +628,18 @@ class FullCloudFormationDeployment { const rollbackWillBeDisabled = persistedRollbackDisabled ?? requestedRollbackDisabled; const isExpress = this.options.express || changeSetReport.changeSet.DeploymentConfig?.Mode === 'EXPRESS'; - // CloudFormation rejects replacement-type updates while rollback is disabled. Standard mode only disables rollback - // for `--no-rollback`, but Express Mode disables it by default - which is why this condition must not be scoped to - // non-express deployments. #1745 dropped the express half of it, #1785 restructured what was left, and #1931 is the - // resulting SEV: the update is submitted, CloudFormation refuses it, and the express stack is left in UPDATE_FAILED - // with no rollback available. + // CloudFormation rejects replacement-type updates while rollback is disabled. This is documented under "Express + // mode and rollback" in + // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/stack-failure-options.html: "Disabling rollback + // isn't supported for immutable update operations. If an update requires replacing a resource and the operation + // fails, the failed state can't be preserved for retry." That last sentence is why we refuse up front instead of + // letting CloudFormation surface the error: there is no preserved failed state to retry from. + // + // Standard mode only disables rollback for `--no-rollback`, but Express Mode disables it by default - which is why + // this condition must not be scoped to non-express deployments. #1745 disabled it for express twice over (an + // unconditional early return to `executeChangeSet`, plus deletion of the `expressNoRollback` disjunct), #1785 + // restructured what was left, and #1931 is the resulting SEV: the update is submitted, CloudFormation refuses it, + // and the express stack is left in UPDATE_FAILED with no rollback available. // // Shelf life: CloudFormation has a server-side fix with a tentative ECD of 2026-11-15. Once that is confirmed in // all regions, the express half of this guard - and CDK_TOOLKIT_W5903 - can be deleted. @@ -1230,9 +1238,9 @@ function mentionsReplacementRejection(message: string): boolean { /** * Whether a persisted change set `DeploymentConfig` pins the rollback choice, and if so which way. * - * Only Express Mode records a rollback choice in `DeploymentConfig`, and `ExecuteChangeSet` cannot override it. Standard - * mode carries none: rollback there is decided at execute time by the `DisableRollback` flag, so the current - * invocation's options stay authoritative and this returns `undefined`. + * Only Express Mode records a rollback choice in `DeploymentConfig`, and `ExecuteChangeSet` has no `DeploymentConfig` + * parameter to restate it with. Standard mode carries none: rollback there is decided at execute time by the + * `DisableRollback` flag, so the current invocation's options stay authoritative and this returns `undefined`. */ function expressRollbackDisabled(config: DeploymentConfig | undefined): boolean | undefined { if (config?.Mode !== 'EXPRESS') { diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 8e5509e24..126d1cfc8 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -492,8 +492,10 @@ describe('change set path', () => { /** * The express replacement guard has been deleted once and restructured once without the suite noticing: * - * - aws/aws-cdk-cli#1745 removed `expressNoRollback` from the guard condition and relaxed the test that covered it, - * which shipped the regression in CLI 2.1133.0. + * - aws/aws-cdk-cli#1745 disabled it twice over - an unconditional early return to `executeChangeSet` for express, plus + * deletion of the `expressNoRollback` disjunct from the guard condition - and inverted the case that covered it, so + * `['express, no explicit rollback', { express: true }]` asserted `did-deploy-stack` rather than + * `replacement-requires-rollback`. That shipped the regression in CLI 2.1133.0. * - aws/aws-cdk-cli#1785 then restructured what was left into `if (!this.options.express) { ... }`. * - aws/aws-cdk-cli#1931 is the resulting SEV: `cdk deploy --express` submits a replacement while CloudFormation has * rollback disabled, CloudFormation rejects it, and the stack is stranded in UPDATE_FAILED. From 365361975ece20ed3273f2b29fe748e9bf2f4549 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 01:18:34 +0000 Subject: [PATCH 07/14] fix(cli): forward --express to the execute-change-set path `--express` decides what a missing `--rollback` means: Express Mode disables rollback by default, standard mode enables it. The `execute-change-set` delegation in `CdkToolkit.deploy` enumerates its options explicitly and dropped `express`, so toolkit-lib read plain `--express` as "rollback enabled" and the change-set policy guard refused Express change sets this same CLI had just created with rollback disabled: cdk deploy --express --method=prepare-change-set # persists DisableRollback: true cdk deploy --express --method=execute-change-set # ChangeSetRollbackPolicyMismatch Forward `express` alongside `rollback` so the policy is derived identically on both paths. The guard stays unscoped to `--express`: a persisted EXPRESS policy still governs regardless of this invocation's flags, so omitting `--express` on execute is still refused. Also derive the effective policy for the replacement guard from what will actually be sent. Only Express records a rollback choice on the change set; anything else is governed by the `DisableRollback` on `ExecuteChangeSet`, which is only sent for an explicit `--no-rollback`. Without this, forwarding `express` would turn a standard `prepare` followed by `execute --express` into a false refusal. Replace the "undocumented" note on whether execute-time `DisableRollback` can override a persisted EXPRESS policy with the verified behaviour: it is a consistency assertion, not an override. A matching value is accepted; a conflicting one fails synchronously with `ValidationError: DisableRollback specified on ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` --- .../lib/api/deployments/deploy-stack.ts | 23 +++- .../deploy-stack-express-replacement.test.ts | 103 ++++++++++++++++-- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 5 + packages/aws-cdk/test/cli/cdk-toolkit.test.ts | 32 ++++++ 4 files changed, 150 insertions(+), 13 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index ac4c5c74c..b22f2ab46 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -610,11 +610,18 @@ class FullCloudFormationDeployment { } // A change set carries its own rollback policy: CloudFormation persists `DeploymentConfig` when the change set is - // created. `ExecuteChangeSet` takes a top-level `DisableRollback` but no `DeploymentConfig`, and whether that flag - // can override a persisted EXPRESS policy is undocumented - so when we are executing a change set that already pins - // the answer we treat the persisted value, not this invocation's flags, as what CloudFormation will do. Reading the - // current options here would let `--rollback` (or simply omitting `--express`) appear to enable rollback on a change - // set that was created with it disabled, which is how the replacement below would reach CloudFormation anyway. + // created, and `ExecuteChangeSet` cannot change it. Its input takes a top-level `DisableRollback`, but that is a + // consistency assertion rather than an override: a value matching the change set is accepted, and a conflicting one + // is rejected synchronously with + // + // ValidationError: DisableRollback specified on ExecuteChangeSet conflicts with the value DisableRollback the + // ChangeSet was created with. + // + // (verified against CloudFormation in us-east-1 - see the empirical notes on #1969). So when we are executing a + // change set that already pins the answer, the persisted value - not this invocation's flags - is what + // CloudFormation will do. Reading the current options here would let `--rollback` (or simply omitting `--express`) + // appear to enable rollback on a change set that was created with it disabled, which is how the replacement below + // would reach CloudFormation anyway. const persistedRollbackDisabled = expressRollbackDisabled(changeSetReport.changeSet.DeploymentConfig); const requestedRollbackDisabled = this.rollbackDisabled(); @@ -625,7 +632,11 @@ class FullCloudFormationDeployment { ); } - const rollbackWillBeDisabled = persistedRollbackDisabled ?? requestedRollbackDisabled; + // What CloudFormation will actually do. Only Express records a rollback choice on the change set; anything else is + // governed by the `DisableRollback` we are about to send, which `commonExecuteOptions()` only sets for an explicit + // `--no-rollback`. `--express` arriving at execute time therefore cannot disable rollback on a non-Express change + // set, and must not make the replacement guard below believe it did. + const rollbackWillBeDisabled = persistedRollbackDisabled ?? this.options.rollback === false; const isExpress = this.options.express || changeSetReport.changeSet.DeploymentConfig?.Mode === 'EXPRESS'; // CloudFormation rejects replacement-type updates while rollback is disabled. This is documented under "Express diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 126d1cfc8..3a7aedf52 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -1,5 +1,5 @@ import type { CloudFormationStackArtifact } from '@aws-cdk/cloud-assembly-api'; -import type { Change, CreateChangeSetCommandInput, Stack } from '@aws-sdk/client-cloudformation'; +import type { Change, CreateChangeSetCommandInput, DeploymentConfig, Stack } from '@aws-sdk/client-cloudformation'; import { CreateChangeSetCommand, DescribeChangeSetCommand, @@ -669,12 +669,14 @@ describe('direct path', () => { /** * A change set carries the rollback policy it was created with. CloudFormation persists `DeploymentConfig` on - * `CreateChangeSet`, returns it from `DescribeChangeSet`, and `ExecuteChangeSet` cannot override it (its input has no - * `DeploymentConfig` field). So when create and execute happen in separate invocations - `--method=change-set - * --no-execute` then `--method=execute-change-set`, or the toolkit's own retry - the second invocation's flags do not - * decide what CloudFormation does. Deriving rollback safety from the current options there would let the SEV in #1931 - * back in: the guard would believe rollback is enabled and execute a rollback-disabled change set containing a - * replacement. + * `CreateChangeSet`, returns it from `DescribeChangeSet`, and `ExecuteChangeSet` cannot override it: its input has no + * `DeploymentConfig` field, and its top-level `DisableRollback` is a consistency assertion rather than an override - a + * matching value is accepted, a conflicting one fails synchronously with `ValidationError: DisableRollback specified on + * ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` So when create and execute + * happen in separate invocations - `--method=change-set --no-execute` then `--method=execute-change-set`, or the + * toolkit's own retry - the second invocation's flags do not decide what CloudFormation does. Deriving rollback safety + * from the current options there would let the SEV in #1931 back in: the guard would believe rollback is enabled and + * execute a rollback-disabled change set containing a replacement. */ describe('executing a change set created by an earlier invocation', () => { function givenExpressChangeSetExists(opts: { rollbackDisabled: boolean; changes: Change[] }) { @@ -793,6 +795,93 @@ describe('executing a change set created by an earlier invocation', () => { ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'does not support while rollback is disabled' }); }); + function givenChangeSetExists(opts: { deploymentConfig?: DeploymentConfig; changes: Change[] }) { + fakeCfn.createChangeSetSync({ + StackName: 'withouterrors', + ChangeSetName: 'prepared', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + Changes: opts.changes, + DeploymentConfig: opts.deploymentConfig, + }); + } + + /** + * The full flag matrix for the policy comparison: `--express` decides what a missing `--rollback` means, so it has to + * be read here exactly as it is when the change set is created. aws/aws-cdk-cli#1969 shipped with the CLI dropping + * `express` on the `execute-change-set` delegation, which made plain `--express` mean "rollback enabled" and refused + * Express change sets the same CLI had just created (`persisted disabled` x `express` x `rollback: undefined` below). + */ + describe.each([ + // persisted rollback DISABLED (Express default) + [true, { express: true, rollback: true }, 'mismatch'], + [true, { express: true }, 'match'], + [true, { express: true, rollback: false }, 'match'], + [true, { rollback: true }, 'mismatch'], + [true, {}, 'mismatch'], + [true, { rollback: false }, 'match'], + // persisted rollback ENABLED + [false, { express: true, rollback: true }, 'match'], + [false, { express: true }, 'mismatch'], + [false, { express: true, rollback: false }, 'mismatch'], + [false, { rollback: true }, 'match'], + [false, {}, 'match'], + [false, { rollback: false }, 'mismatch'], + ] as Array<[boolean, Partial, 'match' | 'mismatch']>)( + 'persisted rollbackDisabled=%s executed with %j', + (persistedRollbackDisabled, flags, expected) => { + test(`is a ${expected}`, async () => { + // GIVEN - a non-replacing change, so a matching policy is free to execute and only the comparison is under test + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: persistedRollbackDisabled, changes: [updateChange()] }); + + if (expected === 'mismatch') { + failOnAnyStackMutation(); + } + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + ...flags, + }); + + // THEN + if (expected === 'mismatch') { + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + expectNoStackMutation(); + } else { + await expect(deployment).resolves.toEqual(expect.objectContaining({ type: 'did-deploy-stack' })); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + } + }); + }, + ); + + /** + * Only Express records a rollback choice on the change set. A STANDARD change set is governed by the `DisableRollback` + * flag sent at execute time, and that flag is only sent for an explicit `--no-rollback` - so `--express` arriving at + * execute time cannot disable rollback on it, and must not make the replacement guard think it did. Without this, + * plumbing `express` through to the execute path turns `prepare` (standard) + `execute --express` into a false refusal. + */ + test('--express does not imply rollback-disabled for a change set that is not Express', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenChangeSetExists({ deploymentConfig: { Mode: 'STANDARD' }, changes: [policyActionReplacementChange()] }); + + // WHEN - rollback is not disabled server-side here, so the replacement is deployable + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + // `isRollbackable` also covers CREATE_FAILED, where there is no previously deployed configuration to replay. Telling // such a user to "revert your change and redeploy the last configuration that deployed successfully" would be // impossible advice, so that state gets its own guidance. diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 7fc83daad..1f7f1e492 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -490,6 +490,11 @@ export class CdkToolkit { roleArn: options.roleArn, forceDeployment: options.force, rollback: options.rollback, + // Must travel with `rollback`: together they decide the rollback policy this invocation is asking for, and + // Express Mode flips what a missing `rollback` means (disabled, rather than standard mode's enabled). Omitting + // it made toolkit-lib read plain `--express` as "rollback enabled" and refuse Express change sets this same CLI + // had just created with rollback disabled. + express: options.express, reuseAssets: options.reuseAssets, concurrency: options.concurrency, traceLogs: options.traceLogs, diff --git a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts index 349893c6c..f176bee27 100644 --- a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts @@ -866,6 +866,38 @@ describe('deploy', () => { expect(mockCfnDeployments.deployStack).not.toHaveBeenCalled(); expect(mockCfnDeployments.prepareStack).not.toHaveBeenCalled(); }); + + // `--express` decides what a missing `--rollback` means: Express Mode disables rollback by default, standard mode + // enables it. Dropping the flag here made toolkit-lib read plain `--express` as "rollback enabled", so the + // change-set policy guard refused Express change sets that this same CLI had just created. + test.each([ + ['plain --express', { express: true }, { express: true, rollback: undefined }], + ['--express --rollback', { express: true, rollback: true }, { express: true, rollback: true }], + ['--express --no-rollback', { express: true, rollback: false }, { express: true, rollback: false }], + ['no --express', { rollback: false }, { express: undefined, rollback: false }], + ])('forwards the rollback policy flags to toolkit-lib: %s', async (_name, flags, expected) => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + const toolkitDeploySpy = jest.spyOn(Toolkit.prototype, 'deploy').mockResolvedValue(undefined as any); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: selectWithUpstream('Test-Stack-A-Display-Name'), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'MyCS' }, + ...flags, + }); + + // THEN + expect(toolkitDeploySpy).toHaveBeenCalledWith(cloudExecutable, expect.objectContaining(expected)); + }); }); test('fails when no valid stack names are given', async () => { From 0223dd0e4dfd18f13a07cf64856819aedbec5b9c Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 03:21:02 +0000 Subject: [PATCH 08/14] test(toolkit-lib): pin the ordering of the replacement recovery guidance `cdk deploy --express --rollback` cannot update a stack that is already failed - CloudFormation answers "This stack is currently in a non-terminal [UPDATE_FAILED] state" - so the guidance has to lead with the state the stack is in, then the steps that return it to a terminal state, and only then suggest deploying with rollback. The existing tests asserted only that those strings were present, so a reordering that led with `--rollback` would have shipped green. Compare offsets instead, at both sites that render the `replay` variant. The assertion deliberately anchors on the suggestion phrasing rather than on `indexOf('--rollback')`: the first occurrence of that flag is the clause saying it cannot update a failed stack, which legitimately precedes the unwedge steps, so a naive "unwedge before any --rollback mention" check would fail on correct output. Also pin the `recreate` variant, whose wrongness is hardest to spot by hand: a stack that never completed a deployment has no configuration to replay, so that guidance must not contain replay instructions at all. --- .../deploy-stack-express-replacement.test.ts | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 3a7aedf52..bb3c1c94a 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -251,6 +251,46 @@ function expectNoStackMutation() { expect(mockCloudFormationClient).not.toHaveReceivedCommand(UpdateStackCommand); } +/** + * Assert the `replay` guidance is ordered the way a wedged user needs to read it: the state the stack is actually in, + * then the steps that return it to a terminal state, and only then the suggestion to deploy with rollback enabled. + * + * Ordering is the whole point of this message. `cdk deploy --express --rollback` cannot update a stack that is already + * failed - CloudFormation answers "This stack is currently in a non-terminal [UPDATE_FAILED] state" - so guidance that + * led with that command would be advice the user cannot act on. Containment assertions alone would not catch a + * reordering, which is why these compare offsets. + * + * Deliberately anchored on the suggestion phrasing and NOT on `indexOf('--rollback')`: the FIRST occurrence of that flag + * is the clause saying it cannot update a failed stack, and that one legitimately comes before the unwedge steps. A + * naive "unwedge before any --rollback mention" assertion would fail on the correct message. + */ +function expectUnwedgeBeforeRollbackSuggestion(message: string) { + const state = message.indexOf('in a failed state'); + const step1 = message.indexOf('Revert your change'); + const step2 = message.indexOf('cdk deploy --express --method=direct'); + const suggestion = message.indexOf('Re-apply your change and deploy it with'); + + expect(state).toBeGreaterThanOrEqual(0); + expect(step1).toBeGreaterThan(state); + expect(step2).toBeGreaterThan(step1); + expect(suggestion).toBeGreaterThan(step2); +} + +/** + * Assert the `recreate` guidance never offers replay instructions. + * + * A stack that never completed a deployment has no previous configuration to go back to, so telling its owner to + * "revert your change and redeploy the last configuration that deployed successfully" is impossible advice. This is the + * variant whose wrongness is hardest to spot by hand, so it is pinned positively and negatively. + */ +function expectRecreateGuidance(message: string) { + expect(message).toMatch(/no previous configuration to replay/); + expect(message).toMatch(/Delete the stack and deploy again/); + expect(message).not.toMatch(/Revert your change/); + expect(message).not.toMatch(/Re-apply your change and deploy it with/); + expect(message).not.toMatch(/--method=direct/); +} + describe('change set path, replacement reported as policy action with Replacement=True', () => { const replacementChange = policyActionReplacementChange; test('express with rollback disabled is gated before ExecuteChangeSet', async () => { @@ -378,6 +418,7 @@ describe('change set path, replacement reported as policy action with Replacemen await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresUnwedge' })); await expect(deployment).rejects.toThrow(/Revert your change/); await expect(deployment).rejects.toThrow(/cdk deploy --express --method=direct/); + expectUnwedgeBeforeRollbackSuggestion(await deployment.then(() => '', (e) => e.message)); expectNoStackMutation(); // ... and it is reported once, by the error, not also as a warning @@ -585,6 +626,7 @@ describe('direct path', () => { ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'Revert your change' }); ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'cdk deploy --express --method=direct' }); ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'cdk deploy --express --rollback' }); + expectUnwedgeBeforeRollbackSuggestion(ioHost.messagesWithCode(W5903)[0].message); expect(ioHost.messagesWithCode(W5903)[0].data).toEqual(expect.objectContaining({ stackName: 'withouterrors', detectedBy: 'service-error', @@ -903,6 +945,7 @@ describe('executing a change set created by an earlier invocation', () => { await expect(deployment).rejects.toThrow(/no previous configuration to replay/); await expect(deployment).rejects.toThrow(/Delete the stack and deploy again/); await expect(deployment).rejects.not.toThrow(/Revert your change/); + expectRecreateGuidance(await deployment.then(() => '', (e) => e.message)); expectNoStackMutation(); }); From dfe7ebf071d8edaf394f70bb78fc09f31f6b3667 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 03:55:52 +0000 Subject: [PATCH 09/14] docs(toolkit-lib): correct three comments about the express replacement guard The confirm-and-retry prompt is not the only recovery path: it is what a healthy stack gets, because the guard returns `replacement-requires-rollback` there. An already-failed stack throws `ReplacementRequiresUnwedge` instead, since the retry cannot succeed from that state. Scope the fake's rollback note to replacements. Rollback being disabled does not by itself strand a stack - an ordinary express deployment with no replacement completes normally. It is a replacement submitted while rollback is disabled that CloudFormation rejects during execution. Say `ExecuteChangeSet` "cannot change" the persisted `DeploymentConfig` rather than "cannot override" it: a `DisableRollback` matching the change set is accepted, and only a conflicting one is rejected. --- .../test/_helpers/fake-aws/fake-cloudformation.ts | 6 +++--- packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts index 5eb767c26..834871d83 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts @@ -517,7 +517,7 @@ export class FakeCloudFormation { Description: cs.description, CreationTime: cs.creationTime, // The real API returns the DeploymentConfig that was persisted by CreateChangeSet. It matters because - // ExecuteChangeSet cannot override it, so code executing an existing change set has to read it from here. + // ExecuteChangeSet cannot change it, so code executing an existing change set has to read it from here. ...(cs.deploymentConfig ? { DeploymentConfig: cs.deploymentConfig } : undefined), NextToken: nextToken, $metadata: {}, @@ -1167,8 +1167,8 @@ export class FakeCloudFormation { * Whether CloudFormation would have rollback disabled for this operation. * * Standard deployments say so with `DisableRollback` on the call. Express Mode has rollback disabled server-side by - * default, and re-enables it by sending `DeploymentConfig.DisableRollback: false` - so express operations strand the - * stack in `*_FAILED` unless they explicitly opt back into rollback. + * default, and re-enables it by sending `DeploymentConfig.DisableRollback: false`. A replacement submitted while + * rollback is disabled is rejected by CloudFormation during execution. */ private rollbackIsDisabled(input: { DisableRollback?: boolean; DeploymentConfig?: DeploymentConfig }): boolean { if (input.DisableRollback) { diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts index fec70d984..2a5bb692c 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts @@ -621,8 +621,9 @@ IAM Statement Changes successfulDeployment(); }); - // The confirm-and-retry-with-rollback prompt is the entire user-visible recovery path for - // aws/aws-cdk-cli#1931, so pin both what the user is told and what we do when they agree. + // From a healthy stack the guard returns `replacement-requires-rollback`, which the deploy action turns into + // this prompt and a retry with rollback enabled. From an already-failed stack it throws instead, since the + // retry cannot succeed there. test('replacement-requires-rollback under --express explains that rollback is disabled, and retries with it enabled', async () => { // GIVEN mockDeployStack.mockImplementation(async (params) => { From a099e8523bfef83c62450edd75492851d5778aca Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 04:51:31 +0000 Subject: [PATCH 10/14] fix(toolkit-lib): gate nested-stack replacements, stop refusing safe change set executions Address review on the express replacement guard: - Gate replacements found in nested stacks' change sets, which previously bypassed the guard entirely because the root change set is replacement-free. - Stop refusing a persisted/requested rollback policy mismatch when there is no replacement; CloudFormation accepts those, so warn and proceed instead. - Make a replacement under a frozen rollback-disabled policy terminal, since retrying re-executes the same change set and cannot succeed. - Read the persisted DeploymentConfig before paused-state routing, so an express change set is not sent down the standard rollback-first path. - Emit W5903 before throwing, so 'cdk deploy --watch' still shows the guidance. - Omit DisableRollback on ExecuteChangeSet when the change set pins the policy; sending it for an explicit --no-rollback conflicted with a change set created with --rollback and made CloudFormation reject the call. - Fail closed when DeploymentConfig is absent on an express invocation. - Use the express-aware confirmation wording on both CLI deploy paths. - Document the three distinct rollback predicates, the shared detection hook, the unit-of-removal contract, and that the 2026-11-15 CloudFormation fix is second-hand and unconfirmed. --- .../lib/api/deployments/deploy-stack.ts | 293 ++++++++++++++---- .../lib/api/io/private/messages.ts | 5 + .../toolkit-lib/lib/payloads/deploy.ts | 10 + .../_helpers/fake-aws/fake-cloudformation.ts | 21 ++ .../deploy-stack-express-replacement.test.ts | 253 +++++++++++---- .../test/api/deployments/deploy-stack.test.ts | 10 + packages/aws-cdk/lib/cli/cdk-toolkit.ts | 7 +- 7 files changed, 481 insertions(+), 118 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index b22f2ab46..9d0c82877 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -5,6 +5,7 @@ import { diffTemplate } from '@aws-cdk/cloudformation-diff'; import type { CreateChangeSetCommandInput, CreateStackCommandInput, + DescribeChangeSetCommandOutput, ExecuteChangeSetCommandInput, UpdateStackCommandInput, Tag, @@ -524,7 +525,7 @@ class FullCloudFormationDeployment { } // If there are replacements in the changeset, check the rollback flag and stack status - return this.checkAndExecuteChangeSet(changeSetReport); + return this.checkAndExecuteChangeSet(changeSetReport, { preExistingChangeSet: false }); } private async executeExistingChangeSet(deploymentMethod: ExecuteChangeSetDeployment): Promise { @@ -540,7 +541,7 @@ class FullCloudFormationDeployment { changeSetNameOrArn: deploymentMethod.changeSetName, }).describeForExecution({ diagnoser: this.diagnoser }); - return this.checkAndExecuteChangeSet(changeSetReport); + return this.checkAndExecuteChangeSet(changeSetReport, { preExistingChangeSet: true }); } /** @@ -566,13 +567,22 @@ class FullCloudFormationDeployment { } /** - * Whether CloudFormation will have rollback disabled for this deployment. + * Whether THIS invocation's flags ask for rollback to be disabled. * * Express Mode disables rollback unless it is explicitly requested; standard mode only disables it when - * `--no-rollback` was passed. Both the replacement guard and `deployConfig()` derive from this, so they cannot - * disagree about whether rollback ends up disabled server-side. + * `--no-rollback` was passed. * - * Note this is deliberately NOT the predicate used by `commonExecuteOptions()`. That one decides whether to send + * This is one of three rollback predicates in this file, and they answer different questions - do not treat them as + * interchangeable: + * + * - `rollbackDisabled()` (this one): what the request asks for. Used by `deployConfig()` when creating a change set, + * and as the comparison input when checking a persisted policy against the request. + * - `rollbackWillBeDisabled()`: what CloudFormation will actually do when an existing change set executes. For an + * EXPRESS change set the persisted `DeploymentConfig` decides and this predicate does not apply. + * - `this.options.rollback ?? true` in `checkAndExecuteChangeSet`: standard-mode paused-state routing only, which + * keys off the raw flag default rather than the express-aware answer. + * + * Note this is also deliberately NOT the predicate used by `commonExecuteOptions()`. That one decides whether to send * `DisableRollback: true` on the API call, which express deployments must never do - they express it through * `DeploymentConfig` instead. */ @@ -591,16 +601,119 @@ class FullCloudFormationDeployment { }; } + /** + * Every replacement this change set will perform, including those that only appear in a nested stack. + * + * `createChangeSet()` always passes `IncludeNestedStacks: true` for non-import deployments, and CloudFormation reports + * nested changes in a SEPARATE change set per nested stack, linked from the root only by the nested stack resource's + * `ChangeSetId`. Reading the root's `Changes` alone therefore sees no replacement when the replaced resource lives + * inside a nested stack: the deployment is submitted, CloudFormation refuses it mid-execution, and the stack is left + * in `UPDATE_FAILED` - #1931 again. Verified against `FakeCloudFormation`: before this traversal the guard returned + * `did-deploy-stack` and called `ExecuteChangeSet` for exactly that shape. + * + * A child that cannot be described is skipped with a debug line rather than failing the deployment. This guard is + * advisory - `routeReplacementRejectedWithRollbackDisabled` still catches the rejection afterwards - so an + * inaccessible child must not turn a working deployment into an error. + */ + private async findAllReplacements(changeSet: DescribeChangeSetCommandOutput): Promise { + const visited = new Set(); + + const collect = async (current: DescribeChangeSetCommandOutput, depth: number): Promise => { + const replacements = findReplacements(current); + if (depth >= MAX_NESTED_CHANGE_SET_DEPTH) { + await this.ioHelper.defaults.debug(format( + 'Stopped looking for nested replacements at depth %d; deeper nested stacks were not inspected.', + depth, + )); + return replacements; + } + + for (const change of current.Changes ?? []) { + const nested = change.ResourceChange; + if (nested?.ResourceType !== 'AWS::CloudFormation::Stack' || !nested.ChangeSetId) { + continue; + } + + // Guards against a child that links back to an ancestor, which would otherwise recurse forever. + if (visited.has(nested.ChangeSetId)) { + continue; + } + visited.add(nested.ChangeSetId); + + try { + const child = await new ChangeSetDescriber({ + cfn: this.cfn, + ioHelper: this.ioHelper, + stackNameOrArn: nested.PhysicalResourceId ?? nested.LogicalResourceId ?? this.stackName, + changeSetNameOrArn: nested.ChangeSetId, + }).waitForSettled(); + + replacements.push(...await collect(child, depth + 1)); + } catch (e: any) { + await this.ioHelper.defaults.debug(format( + 'Could not describe nested change set %s for %s, so it was not inspected for replacements: %s', + nested.ChangeSetId, + nested.LogicalResourceId, + formatErrorMessage(e), + )); + } + } + + return replacements; + }; + + return collect(changeSet, 0); + } + + /** + * Whether rollback will be disabled when this change set executes. + * + * An EXPRESS change set records the answer and `ExecuteChangeSet` cannot change it, so the persisted value decides. A + * STANDARD change set records no rollback choice: there it is decided at execute time by the `DisableRollback` we are + * about to send, which `commonExecuteOptions()` only sets for an explicit `--no-rollback`. + * + * `DeploymentConfig` missing entirely is the ambiguous case - it could mean "standard change set, nothing recorded" or + * "express change set, field not returned" - and we cannot tell those apart. Every response observed so far includes + * it. If that ever changes, an express invocation assumes the express default (rollback disabled) so the guard still + * fires: a needless gate costs one clear error the user can act on, a missed gate costs a wedged stack. + */ + private rollbackWillBeDisabled(changeSet: DescribeChangeSetCommandOutput, persistedRollbackDisabled: boolean | undefined): boolean { + if (persistedRollbackDisabled !== undefined) { + return persistedRollbackDisabled; + } + if (changeSet.DeploymentConfig === undefined && this.options.express) { + return this.rollbackDisabled(); + } + return this.options.rollback === false; + } + /** * Check rollback/replacement constraints and execute the change set if all checks pass. */ - private async checkAndExecuteChangeSet(changeSetReport: ChangeSetReport): Promise { - const replacements = findReplacements(changeSetReport); + private async checkAndExecuteChangeSet( + changeSetReport: ChangeSetReport, + opts: { preExistingChangeSet: boolean }, + ): Promise { + const changeSet = changeSetReport.changeSet; + + // The persisted `DeploymentConfig` is read BEFORE any routing because for an existing change set it is + // authoritative: CloudFormation fixed both the mode and the rollback choice when the change set was created, and + // `ExecuteChangeSet` cannot change either. Routing on this invocation's flags first inverts two cases - an express + // change set executed without `--express` would be sent down the standard rollback-first path, which recommends + // `RollbackStack` (unsupported for express-failed stacks); and a standard change set executed with `--express` + // would skip paused-state handling and reach `ExecuteChangeSet`, which CloudFormation rejects synchronously. The + // request flags are only used to validate consistency with what was persisted. + const persistedMode = changeSet.DeploymentConfig?.Mode; + const isExpress = persistedMode !== undefined ? persistedMode === 'EXPRESS' : (this.options.express ?? false); + const persistedRollbackDisabled = expressRollbackDisabled(changeSet.DeploymentConfig); + const requestedRollbackDisabled = this.rollbackDisabled(); + + const replacements = await this.findAllReplacements(changeSet); const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; - // For express mode deployments, don't check paused and failed, since express mode stacks cannot use rollback API - if (!this.options.express) { + // Express stacks cannot use the rollback API, so standard rollback-first routing does not apply to them. + if (!isExpress) { if (isPausedFailState && replacements.length > 0) { return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name }; } @@ -609,35 +722,7 @@ class FullCloudFormationDeployment { } } - // A change set carries its own rollback policy: CloudFormation persists `DeploymentConfig` when the change set is - // created, and `ExecuteChangeSet` cannot change it. Its input takes a top-level `DisableRollback`, but that is a - // consistency assertion rather than an override: a value matching the change set is accepted, and a conflicting one - // is rejected synchronously with - // - // ValidationError: DisableRollback specified on ExecuteChangeSet conflicts with the value DisableRollback the - // ChangeSet was created with. - // - // (verified against CloudFormation in us-east-1 - see the empirical notes on #1969). So when we are executing a - // change set that already pins the answer, the persisted value - not this invocation's flags - is what - // CloudFormation will do. Reading the current options here would let `--rollback` (or simply omitting `--express`) - // appear to enable rollback on a change set that was created with it disabled, which is how the replacement below - // would reach CloudFormation anyway. - const persistedRollbackDisabled = expressRollbackDisabled(changeSetReport.changeSet.DeploymentConfig); - const requestedRollbackDisabled = this.rollbackDisabled(); - - if (persistedRollbackDisabled !== undefined && persistedRollbackDisabled !== requestedRollbackDisabled) { - throw new ToolkitError( - 'ChangeSetRollbackPolicyMismatch', - changeSetPolicyMismatchMessage(changeSetReport.changeSet.ChangeSetName, persistedRollbackDisabled), - ); - } - - // What CloudFormation will actually do. Only Express records a rollback choice on the change set; anything else is - // governed by the `DisableRollback` we are about to send, which `commonExecuteOptions()` only sets for an explicit - // `--no-rollback`. `--express` arriving at execute time therefore cannot disable rollback on a non-Express change - // set, and must not make the replacement guard below believe it did. - const rollbackWillBeDisabled = persistedRollbackDisabled ?? this.options.rollback === false; - const isExpress = this.options.express || changeSetReport.changeSet.DeploymentConfig?.Mode === 'EXPRESS'; + const rollbackWillBeDisabled = this.rollbackWillBeDisabled(changeSet, persistedRollbackDisabled); // CloudFormation rejects replacement-type updates while rollback is disabled. This is documented under "Express // mode and rollback" in @@ -649,11 +734,25 @@ class FullCloudFormationDeployment { // Standard mode only disables rollback for `--no-rollback`, but Express Mode disables it by default - which is why // this condition must not be scoped to non-express deployments. #1745 disabled it for express twice over (an // unconditional early return to `executeChangeSet`, plus deletion of the `expressNoRollback` disjunct), #1785 - // restructured what was left, and #1931 is the resulting SEV: the update is submitted, CloudFormation refuses it, - // and the express stack is left in UPDATE_FAILED with no rollback available. + // restructured what was left, and #1931 is the resulting SEV. + // + // REMOVAL CONTRACT. This guard exists only because CloudFormation refuses replacements while rollback is disabled. + // If that restriction is lifted, the pieces below are one unit and must be removed TOGETHER - leaving a subset + // behind gives users a gate that fires with no reason to, or guidance pointing at a recovery that is no longer + // needed: + // 1. this replacement branch, including the `ReplacementRequiresRecreateChangeSet` and + // `ReplacementRequiresUnwedge` throws and the `replacement-requires-rollback` result; + // 2. the persisted-vs-requested policy mismatch report further down; + // 3. `routeReplacementRejectedWithRollbackDisabled` and `CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON`, the + // after-the-fact net in `monitorDeployment`; + // 4. the public payload surface: `ReplacementRequiresRollback`, `ReplacedResource`, and + // `ReplacementRequiresRollbackStackResult` in `payloads/deploy.ts`; + // 5. `CDK_TOOLKIT_W5903` in `api/io/private/messages.ts`. + // Items 4 and 5 are public API, so removal is a breaking change and needs its own deprecation cycle. // - // Shelf life: CloudFormation has a server-side fix with a tentative ECD of 2026-11-15. Once that is confirmed in - // all regions, the express half of this guard - and CDK_TOOLKIT_W5903 - can be deleted. + // Do NOT treat this as scheduled work. A server-side fix around 2026-11-15 was reported to us second-hand and is + // NOT confirmed with CloudFormation; nothing here should be removed before the restriction is observed to be gone + // in all regions. if (replacements.length > 0 && rollbackWillBeDisabled) { if (isExpress) { const guidance = replacementRoutingMessage({ @@ -662,32 +761,63 @@ class FullCloudFormationDeployment { status: this.cloudFormationStack.stackStatus.name, }); - // A stack that is already in a failed state cannot be updated with rollback enabled either - CloudFormation - // answers "This stack is currently in a non-terminal [UPDATE_FAILED] state". Returning - // `replacement-requires-rollback` here would make the toolkit offer exactly that deployment, and because the - // confirmation defaults to yes, a non-interactive caller would run it and fail a second time. Report once. - if (isPausedFailState) { - throw new ToolkitError('ReplacementRequiresUnwedge', guidance); - } - + // Emitted BEFORE the throws below: `cdk deploy --watch` catches and discards the thrown error + // (`cdk-toolkit.ts` watch loop), so guidance carried only by the exception would never reach the user. await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg(guidance, { stackName: this.stackName, - changeSetId: changeSetReport.changeSet.ChangeSetId, + changeSetId: changeSet.ChangeSetId, replacements, detectedBy: 'change-set', })); + + // An existing change set's rollback policy is frozen. Returning `replacement-requires-rollback` would make the + // toolkit offer a retry with rollback enabled, and that retry re-executes THIS change set - which fails with + // `ChangeSetRollbackPolicyMismatch` because the persisted policy cannot be changed. Offering a recovery that + // is guaranteed to fail is the defect we already removed for already-failed stacks; the change set has to be + // recreated instead. + if (opts.preExistingChangeSet && persistedRollbackDisabled === true) { + throw new ToolkitError( + 'ReplacementRequiresRecreateChangeSet', + changeSetRecreateForReplacementMessage(changeSet.ChangeSetName), + ); + } + + // A stack already in a failed state cannot be updated with rollback enabled either - CloudFormation answers + // "This stack is currently in a non-terminal [UPDATE_FAILED] state" - so the retry cannot succeed here. + if (isPausedFailState) { + throw new ToolkitError('ReplacementRequiresUnwedge', guidance); + } } return { type: 'replacement-requires-rollback' }; } - const changeSet = changeSetReport.changeSet; + // Only reached when there is no replacement to gate. A rollback policy that disagrees with this invocation's flags + // still means the flags are not being honoured, but without a replacement CloudFormation accepts the execution, so + // refusing here would break deployments unrelated to #1931 - executing an express change set without `--express`, + // or with `--rollback`. Report it and continue. + if (persistedRollbackDisabled !== undefined && persistedRollbackDisabled !== requestedRollbackDisabled) { + await this.ioHelper.defaults.warn( + changeSetPolicyMismatchMessage(changeSet.ChangeSetName, persistedRollbackDisabled), + ); + } + await this.ioHelper.defaults.debug(format('Initiating execution of changeset %s on stack %s', changeSet.ChangeSetId, this.stackName)); + // `DisableRollback` on ExecuteChangeSet is a consistency assertion, not an override: CloudFormation rejects a value + // conflicting with the change set's persisted policy with `ValidationError: DisableRollback specified on + // ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` Once the change set + // pins the policy, sending the flag can only restate it or fail the call outright, so omit it there and let the + // persisted value stand - the ignored request is already reported above. Only a change set that records no policy + // (standard mode) is still governed by this flag. + const { DisableRollback, ...sharedExecuteOptions } = this.commonExecuteOptions(); + const rollbackFlagStillDecides = persistedRollbackDisabled === undefined; + await this.cfn.executeChangeSet({ StackName: changeSet.StackId ?? this.stackName, ChangeSetName: changeSet.ChangeSetId!, ClientRequestToken: `exec${this.uuid}`, - ...this.commonExecuteOptions(), + ...sharedExecuteOptions, + ...(rollbackFlagStillDecides && DisableRollback !== undefined ? { DisableRollback } : undefined), }); await this.ioHelper.defaults.debug( @@ -886,8 +1016,13 @@ class FullCloudFormationDeployment { /** * Tell the user how to perform a replacement when CloudFormation rejected one because rollback was disabled. * - * The `--method=direct` path has no change set to inspect, so it cannot be gated up front the way the change set - * path is; a replacement there is only discovered from the failure CloudFormation reports. + * This runs from `monitorDeployment`, which BOTH the change set path and `--method=direct` go through, so it is not a + * direct-path-only hook. It is the after-the-fact net for every replacement the up-front change set gate could not + * see: + * + * - `--method=direct` has no change set to inspect at all, so nothing there can be gated up front. + * - A change set can report `Replacement: "Conditional"`, which the up-front gate does not treat as a replacement + * (see #1971). If CloudFormation then resolves it to an actual replacement, this is the only thing that fires. * * `--express --method=direct` is deliberately NOT refused up front, and that gap should not be "fixed": replaying the * previous configuration that way is the only exit from a stack already stranded in UPDATE_FAILED, so refusing the @@ -908,9 +1043,10 @@ class FullCloudFormationDeployment { const matched = rejected.length > 0 || mentionsReplacementRejection(error?.message ?? ''); if (!matched) { - // CloudFormation owns the wording we match on and has a change landing around 2026-11-15. If it is reworded, - // this is the branch that will start being taken - log what we did see so that shows up in a debug log instead - // of arriving as a second SEV. + // CloudFormation owns the wording we match on, so it can be reworded at any time. (A rewording around 2026-11-15 + // was reported to us second-hand; we have NOT confirmed it with CloudFormation, so treat the date as a rumour and + // the fragility as permanent.) If it is reworded, this is the branch that will start being taken - log what we did + // see so that shows up in a debug log instead of arriving as a second SEV. const reported = errors.allErrorMessages.filter((m) => m.trim() !== ''); await this.ioHelper.defaults.debug(format( 'Deployment failed with rollback disabled but no reported error mentioned %j, so no replacement guidance was emitted. Reported reasons: %s', @@ -1210,8 +1346,8 @@ function arrayEquals(a: any[], b: any[]): boolean { * deployment. A `Conditional` change that does turn out to replace is caught after the fact by * `routeReplacementRejectedWithRollbackDisabled`. */ -function findReplacements(report: ChangeSetReport): ReplacedResource[] { - return (report.changeSet.Changes ?? []).flatMap((c) => { +function findReplacements(changeSet: DescribeChangeSetCommandOutput): ReplacedResource[] { + return (changeSet.Changes ?? []).flatMap((c) => { const change = c.ResourceChange; const policyAction = change?.PolicyAction; const replacesResource = policyAction === 'ReplaceAndDelete' @@ -1236,9 +1372,11 @@ function findReplacements(report: ChangeSetReport): ReplacedResource[] { * * CloudFormation surfaces this as a resource status reason with no structured error code attached (`extractErrorCode` * finds no `HandlerErrorCode:`/`Error Code:` prefix in it), so matching this text is the only trigger available. That - * makes it fragile: CloudFormation owns the string and has a change landing around 2026-11-15. Replace this match with - * a structured discriminator if CloudFormation ever exposes one. A miss is logged at debug level and only costs the - * extra guidance - the underlying CloudFormation error is reported either way. + * makes it fragile: CloudFormation owns the string and can reword it at any time. (A rewording around 2026-11-15 was + * reported to us second-hand and is NOT confirmed with CloudFormation - do not treat that date as a deadline or as + * permission to assume the match is stable until then.) Replace this match with a structured discriminator if + * CloudFormation ever exposes one. A miss is logged at debug level and only costs the extra guidance - the underlying + * CloudFormation error is reported either way. */ export const CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON = 'Replacement type updates not supported on stack with disable-rollback'; @@ -1280,6 +1418,33 @@ function changeSetPolicyMismatchMessage(changeSetName: string | undefined, persi type ReplacementRecovery = 'none' | 'replay' | 'recreate' | 'resolve-state'; +/** + * How deep to follow nested change sets when looking for replacements. + * + * CloudFormation's own nested stack limit is 5 levels, so this only stops pathological or cyclic shapes; the + * `visited` set handles cycles, and this bounds the number of `DescribeChangeSet` calls a deployment can make. + */ +const MAX_NESTED_CHANGE_SET_DEPTH = 10; + +/** + * Explain that a replacement cannot be deployed by executing this change set, whatever flags are passed. + * + * Separate from `changeSetPolicyMismatchMessage`: that one is about a flag being ignored, this one is about a + * deployment that cannot be made to work without creating a new change set. + */ +function changeSetRecreateForReplacementMessage(changeSetName: string | undefined): string { + const named = changeSetName ? ` ${chalk.blue(changeSetName)}` : ''; + const recreateWith = chalk.blue('cdk deploy --express --rollback'); + + return [ + `Change set${named} was created with rollback disabled and replaces a resource, which CloudFormation does not`, + 'support. A change set fixes its rollback policy when it is created and executing it cannot change that, so there', + 'is no way to execute this change set successfully.', + '', + `Create a new change set with rollback enabled instead: ${recreateWith}`, + ].join('\n'); +} + /** * Explain how to deploy a replacement when rollback is disabled. * diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts index e7a41179d..382b8f1d9 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts @@ -334,6 +334,11 @@ export const IO = { code: 'CDK_TOOLKIT_W5902', description: 'Express Mode deployment completed with resources still stabilizing', }), + /** + * Tied to a CloudFormation restriction (replacements are refused while rollback is disabled) that is expected to be + * TEMPORARY. If the restriction is lifted this message is removed together with the `ReplacementRequiresRollback` and + * `ReplacedResource` payload types - see the REMOVAL CONTRACT comment in `api/deployments/deploy-stack.ts`. + */ CDK_TOOLKIT_W5903: make.warn({ code: 'CDK_TOOLKIT_W5903', description: 'Deployment includes a replacement that CloudFormation does not support while rollback is disabled', diff --git a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts index 8298dfff4..fd11ba13b 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts @@ -78,6 +78,11 @@ export interface PublishAssetEvent { /** * A resource that CloudFormation reported it would replace + * + * This type exists only to describe a CloudFormation restriction: replacements are refused while rollback is disabled. + * That restriction is expected to be TEMPORARY, so treat this type as provisional - if CloudFormation lifts it, this + * type, `ReplacementRequiresRollback` and `CDK_TOOLKIT_W5903` are removed together (see the REMOVAL CONTRACT comment in + * `api/deployments/deploy-stack.ts`). Do not build anything on it that cannot tolerate deprecation. */ export interface ReplacedResource { /** @@ -105,6 +110,11 @@ export interface ReplacedResource { /** * A deployment includes a replacement that CloudFormation will not perform while rollback is disabled + * + * This type exists only to describe a CloudFormation restriction that is expected to be TEMPORARY, so treat it as + * provisional - if CloudFormation lifts the restriction, this type, `ReplacedResource` and `CDK_TOOLKIT_W5903` are + * removed together (see the REMOVAL CONTRACT comment in `api/deployments/deploy-stack.ts`). Do not build anything on it + * that cannot tolerate deprecation. */ export interface ReplacementRequiresRollback { /** diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts index 834871d83..9a4ec38d8 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts @@ -531,6 +531,27 @@ export class FakeCloudFormation { cfnError('InvalidChangeSetStatus', `ChangeSet [${cs.name}] is in ${cs.executionStatus} state and cannot be executed`); } + // `DisableRollback` on ExecuteChangeSet is a consistency assertion against the policy the change set was created + // with, not an override: a matching value is accepted, and a conflicting one is rejected synchronously, before + // anything is submitted and with both the stack and the change set left untouched. Verified against CloudFormation + // in us-east-1. + // + // Scoped to EXPRESS because only EXPRESS persists a rollback choice on the change set. A STANDARD change set + // records none, so there the execute-time flag decides and cannot conflict with anything. + const persistedRollbackDisabled = cs.deploymentConfig?.Mode === 'EXPRESS' + ? cs.deploymentConfig.DisableRollback !== false + : undefined; + if ( + input.DisableRollback !== undefined && + persistedRollbackDisabled !== undefined && + input.DisableRollback !== persistedRollbackDisabled + ) { + cfnError( + 'ValidationError', + 'DisableRollback specified on ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.', + ); + } + // Remove the executed change set from the stack's list. Real CloudFormation // also deletes all other change sets, but we skip that to avoid interfering // with concurrent operations on the same stack in tests. diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index bb3c1c94a..28bf7445b 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -11,6 +11,7 @@ import type { DeployStackOptions as DeployStackApiOptions } from '../../../lib/a import { CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON, deployStack } from '../../../lib/api/deployments/deploy-stack'; import { CloudFormationStackDiagnoser } from '../../../lib/api/diagnosing/stack-diagnoser'; import { NoBootstrapStackEnvironmentResources } from '../../../lib/api/environment'; +import { IO } from '../../../lib/api/io/private'; import { StackArtifactSourceTracer } from '../../../lib/api/source-tracing/private/stack-source-tracing'; import { testStack } from '../../_helpers/assembly'; import { FakeCloudFormation } from '../../_helpers/fake-aws/fake-cloudformation'; @@ -24,7 +25,9 @@ import { } from '../../_helpers/mock-sdk'; import { TestIoHost } from '../../_helpers/test-io-host'; -const W5903 = 'CDK_TOOLKIT_W5903'; +// Taken from the production message rather than restated, so renaming the code cannot leave the negative assertions +// below (`messagesWithCode(W5903)` returning nothing) vacuously true. +const W5903 = IO.CDK_TOOLKIT_W5903.code; let ioHost = new TestIoHost('debug', true); let ioHelper = ioHost.asHelper('deploy'); @@ -421,8 +424,9 @@ describe('change set path, replacement reported as policy action with Replacemen expectUnwedgeBeforeRollbackSuggestion(await deployment.then(() => '', (e) => e.message)); expectNoStackMutation(); - // ... and it is reported once, by the error, not also as a warning - expect(ioHost.messagesWithCode(W5903)).toEqual([]); + // ... and the guidance is ALSO emitted as W5903, because `cdk deploy --watch` swallows the thrown error and would + // otherwise show the user nothing actionable. + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'Revert your change' }); }); }); @@ -739,14 +743,15 @@ describe('executing a change set created by an earlier invocation', () => { forceDeployment: true, }; - test('a rollback-disabled change set is not executed just because this invocation passes --rollback', async () => { + // `--rollback` cannot be honoured on an existing change set, but without a replacement CloudFormation accepts the + // execution, so refusing would break a deployment that works. Report that the flag is being ignored and continue. + test('a rollback-disabled change set with no replacement is executed, with the ignored flag reported', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: true, changes: [updateChange()] }); - failOnAnyStackMutation(); // WHEN - const deployment = testDeployStack({ + const result = await testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, express: true, @@ -754,9 +759,9 @@ describe('executing a change set created by an earlier invocation', () => { }); // THEN - await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); - await expect(deployment).rejects.toThrow(/created with rollback disabled/); - expectNoStackMutation(); + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + ioHost.expectMessage({ level: 'warn', containing: 'created with rollback disabled' }); }); // The SEV path: the change set contains a replacement and was created with rollback disabled. Executing it would put @@ -775,8 +780,9 @@ describe('executing a change set created by an earlier invocation', () => { rollback: true, }); - // THEN - await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + // THEN - terminal, because the retry would re-execute this same immutable change set + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); + await expect(deployment).rejects.toThrow(/Create a new change set with rollback enabled/); expectNoStackMutation(); expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_COMPLETE); }); @@ -793,46 +799,46 @@ describe('executing a change set created by an earlier invocation', () => { ...executePrepared, }); - // THEN - await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); + // THEN - the persisted mode is authoritative, so this is still gated as an express replacement + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); expectNoStackMutation(); }); - test('the opposite mismatch is refused too: rollback-enabled change set executed as rollback-disabled', async () => { + test('the opposite mismatch is reported too: rollback-enabled change set executed as rollback-disabled', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: false, changes: [updateChange()] }); - failOnAnyStackMutation(); // WHEN - plain `--express` requests rollback disabled, but the change set was created with it enabled - const deployment = testDeployStack({ + const result = await testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, express: true, }); - // THEN - await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); - await expect(deployment).rejects.toThrow(/created with rollback enabled/); - expectNoStackMutation(); + // THEN - rollback stays ENABLED (the safer direction) and the ignored flag is reported + expect(result.type).toEqual('did-deploy-stack'); + ioHost.expectMessage({ level: 'warn', containing: 'created with rollback enabled' }); }); - // The matching case still has to be gated on the replacement itself, using the persisted policy. - test('a matching rollback-disabled change set with a replacement is gated, not executed', async () => { + // Even with the flags matching, an existing rollback-disabled change set containing a replacement cannot be made to + // work: the retry the generic result would trigger re-executes this same change set. So it is terminal, and the + // guidance says to recreate rather than to retry. + test('a matching rollback-disabled change set with a replacement is terminal, not offered a retry', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: true, changes: [policyActionReplacementChange()] }); failOnAnyStackMutation(); // WHEN - const result = await testDeployStack({ + const deployment = testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, express: true, }); // THEN - expect(result.type).toEqual('replacement-requires-rollback'); + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); expectNoStackMutation(); ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'does not support while rollback is disabled' }); }); @@ -848,54 +854,195 @@ describe('executing a change set created by an earlier invocation', () => { }); } + /** + * Seed a nested-stack shape: a child stack carrying its own change set, referenced from the root change set through + * the nested stack resource's `ChangeSetId`. This is how CloudFormation reports nested changes when the root change + * set is created with `IncludeNestedStacks: true`, which `createChangeSet()` always does for non-import deployments. + * + * The root entry deliberately reports no replacement of its own - `AWS::CloudFormation::Stack` is merely modified - + * so a guard that only reads the root's `Changes` sees nothing to gate on. + */ + function givenNestedChangeSetExists(opts: { rollbackDisabled: boolean; childChanges: Change[] }) { + const childStackName = 'withouterrors-NestedChild-ABC123'; + const deploymentConfig: DeploymentConfig = opts.rollbackDisabled + ? { Mode: 'EXPRESS' } + : { Mode: 'EXPRESS', DisableRollback: false }; + + fakeCfn.createStackSync({ StackName: childStackName, StackStatus: StackStatus.UPDATE_COMPLETE }); + const child = fakeCfn.createChangeSetSync({ + StackName: childStackName, + ChangeSetName: 'prepared-nested-child', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + Changes: opts.childChanges, + DeploymentConfig: deploymentConfig, + }); + + givenChangeSetExists({ + deploymentConfig, + changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'NestedChild', + PhysicalResourceId: childStackName, + ResourceType: 'AWS::CloudFormation::Stack', + Replacement: 'False', + ChangeSetId: child.Id, + }, + }], + }); + + return { childStackName, childChangeSetId: child.Id }; + } + + /** + * A replacement that only exists in a nested stack must be gated exactly like one in the root. + * + * Confirmed as a real bypass before it was fixed: with this exact shape the guard returned `did-deploy-stack` and + * called `ExecuteChangeSet` once, emitting no `W5903` - i.e. #1931 straight through. + */ + test('a replacement inside a nested stack is gated, not executed', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenNestedChangeSetExists({ rollbackDisabled: true, childChanges: [policyActionReplacementChange()] }); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); + expectNoStackMutation(); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'does not support while rollback is disabled' }); + }); + + // The traversal must not gate a nested stack whose child changes nothing of consequence. + test('a nested stack with no replacement executes normally', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenNestedChangeSetExists({ rollbackDisabled: true, childChanges: [updateChange()] }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(ioHost.messagesWithCode(W5903)).toEqual([]); + }); + /** * The full flag matrix for the policy comparison: `--express` decides what a missing `--rollback` means, so it has to * be read here exactly as it is when the change set is created. aws/aws-cdk-cli#1969 shipped with the CLI dropping * `express` on the `execute-change-set` delegation, which made plain `--express` mean "rollback enabled" and refused * Express change sets the same CLI had just created (`persisted disabled` x `express` x `rollback: undefined` below). */ - describe.each([ + const POLICY_MATRIX = [ // persisted rollback DISABLED (Express default) - [true, { express: true, rollback: true }, 'mismatch'], - [true, { express: true }, 'match'], - [true, { express: true, rollback: false }, 'match'], - [true, { rollback: true }, 'mismatch'], - [true, {}, 'mismatch'], - [true, { rollback: false }, 'match'], + [true, { express: true, rollback: true }, 'reported'], + [true, { express: true }, 'silent'], + [true, { express: true, rollback: false }, 'silent'], + [true, { rollback: true }, 'reported'], + [true, {}, 'reported'], + [true, { rollback: false }, 'silent'], // persisted rollback ENABLED - [false, { express: true, rollback: true }, 'match'], - [false, { express: true }, 'mismatch'], - [false, { express: true, rollback: false }, 'mismatch'], - [false, { rollback: true }, 'match'], - [false, {}, 'match'], - [false, { rollback: false }, 'mismatch'], - ] as Array<[boolean, Partial, 'match' | 'mismatch']>)( + [false, { express: true, rollback: true }, 'silent'], + [false, { express: true }, 'reported'], + [false, { express: true, rollback: false }, 'reported'], + [false, { rollback: true }, 'silent'], + [false, {}, 'silent'], + [false, { rollback: false }, 'reported'], + ] as Array<[boolean, Partial, 'silent' | 'reported']>; + + // Deleting a row would silently shrink this matrix, so its size is pinned. + test('the policy matrix covers every express/rollback/persisted combination', () => { + expect(POLICY_MATRIX).toHaveLength(12); + }); + + /** + * `DisableRollback` on `ExecuteChangeSet` is a consistency assertion, not an override, so a value conflicting with the + * change set's persisted policy fails the call outright with `ValidationError: DisableRollback specified on + * ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` + * + * `commonExecuteOptions()` sends `DisableRollback: true` for an explicit `--no-rollback`, so executing a change set + * created with rollback ENABLED under `--no-rollback` would send exactly that conflicting value. The flag cannot move + * an already-pinned policy, so it must not be sent at all. + */ + test('no DisableRollback is sent when the change set already pins the policy', async () => { + // GIVEN - persisted policy says rollback ENABLED, the request asks for it DISABLED + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenExpressChangeSetExists({ rollbackDisabled: false, changes: [updateChange()] }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + rollback: false, + }); + + // THEN - executed rather than failing with ValidationError, and the flag was withheld + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + const sent = mockCloudFormationClient.commandCalls(ExecuteChangeSetCommand)[0].args[0].input; + expect(sent).not.toHaveProperty('DisableRollback'); + }); + + /** + * The mirror of the above: a standard change set records no policy, so there the execute-time flag is the only thing + * that decides and must still be sent. + */ + test('DisableRollback is still sent when the change set records no policy', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenChangeSetExists({ deploymentConfig: { Mode: 'STANDARD' }, changes: [updateChange()] }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + rollback: false, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + const sent = mockCloudFormationClient.commandCalls(ExecuteChangeSetCommand)[0].args[0].input; + expect(sent.DisableRollback).toEqual(true); + }); + + describe.each(POLICY_MATRIX)( 'persisted rollbackDisabled=%s executed with %j', (persistedRollbackDisabled, flags, expected) => { - test(`is a ${expected}`, async () => { - // GIVEN - a non-replacing change, so a matching policy is free to execute and only the comparison is under test + test(`is ${expected}`, async () => { + // GIVEN - a non-replacing change, so nothing is gated and only the policy comparison is under test givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: persistedRollbackDisabled, changes: [updateChange()] }); - if (expected === 'mismatch') { - failOnAnyStackMutation(); - } - // WHEN - const deployment = testDeployStack({ + const result = await testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, ...flags, }); - // THEN - if (expected === 'mismatch') { - await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ChangeSetRollbackPolicyMismatch' })); - expectNoStackMutation(); - } else { - await expect(deployment).resolves.toEqual(expect.objectContaining({ type: 'did-deploy-stack' })); - expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); - } + // THEN - without a replacement CloudFormation accepts the execution either way, so it always runs. A policy + // that disagrees with the flags means the flags are not being honoured, which is reported but not refused. + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + + const warnings = ioHost.notifySpy.mock.calls + .map((c) => c[0]) + .filter((m: any) => m.level === 'warn' && /was created with rollback/.test(m.message)); + expect(warnings).toHaveLength(expected === 'reported' ? 1 : 0); }); }, ); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts index 2f376b40d..44c52c90d 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts @@ -1613,6 +1613,9 @@ test.each([ // See aws/aws-cdk-cli#1931. The `express, no explicit rollback` expectation below was // changed from 'replacement-requires-rollback' to 'did-deploy-stack' by #1745, which is // why the regression shipped unnoticed; #1785 then restructured the guard it removed. +// +// The result type alone is the surface #1745 edited, so it is not pinned on its own here: each row also asserts whether +// anything was actually submitted. Flipping an expectation now requires flipping a claim about CloudFormation calls too. test.each([ // --express alone (rollback disabled server-side): a replacement must not be submitted ['express, no explicit rollback', { express: true } as Partial, 'replacement-requires-rollback'], @@ -1637,6 +1640,13 @@ test.each([ // THEN expect(result.type).toEqual(expectedType); + + if (expectedType === 'replacement-requires-rollback') { + expect(mockCloudFormationClient).not.toHaveReceivedCommand(ExecuteChangeSetCommand); + expect(mockCloudFormationClient).not.toHaveReceivedCommand(UpdateStackCommand); + } else { + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + } }, ); diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index 761be0842..bdbc0f6fc 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -2570,7 +2570,12 @@ class WorkGraphDeploymentActions implements WorkGraphActions { } case 'replacement-requires-rollback': { - const motivation = 'Change includes a replacement which cannot be deployed with "--no-rollback"'; + // Express Mode disables rollback by DEFAULT, so naming `--no-rollback` here would blame a flag the user + // never passed. This path and `toolkit.ts` must stay worded the same: both are reachable from `cdk deploy` + // depending on `--method`. + const motivation = this.options.express + ? 'Change includes a replacement, which CloudFormation does not support while rollback is disabled (the default for Express Mode)' + : 'Change includes a replacement which cannot be deployed with "--no-rollback"'; if (this.options.force) { await this.ioHost.asIoHelper().defaults.warn(`${motivation}. Proceeding with deployment with rollback enabled (--force).`); From 2200e3d9283719d73a124c566fc8269ac3481030 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 04:54:47 +0000 Subject: [PATCH 11/14] test(toolkit-lib): catch a rollback suggestion prepended to the unwedge steps Mutation testing found a gap: prepending an actionable 'Deploy it with cdk deploy --express --rollback' line ahead of the whole explanation passed every existing assertion, because the unwedge steps still followed 'in a failed state' and the closing suggestion still came last. The helper deliberately avoided indexOf('--rollback') to allow the legitimate 'cannot update' clause, and that carve-out was the blind spot. Pin it explicitly: the first mention of the rollback command must be the 'cannot update' clause. The mutation now fails 2 tests instead of 0. --- .../deployments/deploy-stack-express-replacement.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 28bf7445b..96faa32b8 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -264,16 +264,23 @@ function expectNoStackMutation() { * reordering, which is why these compare offsets. * * Deliberately anchored on the suggestion phrasing and NOT on `indexOf('--rollback')`: the FIRST occurrence of that flag - * is the clause saying it cannot update a failed stack, and that one legitimately comes before the unwedge steps. A + * is the clause saying it cannot update a failed state, and that one legitimately comes before the unwedge steps. A * naive "unwedge before any --rollback mention" assertion would fail on the correct message. + * + * That carve-out left a blind spot, so it is pinned explicitly: the first mention of the rollback command must be the + * "cannot update" clause. Without this, prepending an actionable `Deploy it with cdk deploy --express --rollback` line + * ahead of the whole explanation is invisible to every other assertion here - the steps still follow "in a failed + * state", and the closing suggestion still comes last. */ function expectUnwedgeBeforeRollbackSuggestion(message: string) { const state = message.indexOf('in a failed state'); const step1 = message.indexOf('Revert your change'); const step2 = message.indexOf('cdk deploy --express --method=direct'); const suggestion = message.indexOf('Re-apply your change and deploy it with'); + const firstRollbackMention = message.indexOf('cdk deploy --express --rollback'); expect(state).toBeGreaterThanOrEqual(0); + expect(firstRollbackMention).toBeGreaterThan(state); expect(step1).toBeGreaterThan(state); expect(step2).toBeGreaterThan(step1); expect(suggestion).toBeGreaterThan(step2); From fc0d3b2126787bf79cdfc8fb7f6694c9c79f374e Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 05:12:35 +0000 Subject: [PATCH 12/14] chore(toolkit-lib): strip the explanatory comments from the express replacement guard Leave the code to carry itself. Removed the rationale prose from deploy-stack.ts (the rollbackDisabled/rollbackWillBeDisabled/findAllReplacements docstrings, the persisted-DeploymentConfig routing note, the CloudFormation citation and regression history, the removal-contract and ECD notes, the ExecuteChangeSet consistency-assertion note, the Conditional exclusion rationale and the prose-match fragility note), from the CLI's execute-change-set delegation and prompt wording, from the FakeCloudFormation helpers, and from the tests. Kept only what is required: the one-line public-API docstrings on the payload types and their members, and the description/interface fields the message registry generator reads. Regenerating docs/message-registry.md produces no diff, so no generated output changed. GIVEN/WHEN/THEN markers are kept as repo convention. --- .../lib/api/deployments/deploy-stack.ts | 182 ---------------- .../lib/api/io/private/messages.ts | 5 - .../toolkit-lib/lib/payloads/deploy.ts | 10 - .../_helpers/fake-aws/fake-cloudformation.ts | 22 -- .../toolkit-lib/test/_helpers/test-io-host.ts | 3 - .../toolkit-lib/test/actions/deploy.test.ts | 4 - .../deploy-stack-express-replacement.test.ts | 203 ++---------------- .../test/api/deployments/deploy-stack.test.ts | 21 -- packages/aws-cdk/lib/cli/cdk-toolkit.ts | 7 - packages/aws-cdk/test/cli/cdk-toolkit.test.ts | 3 - 10 files changed, 22 insertions(+), 438 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index 9d0c82877..4bcf2a06a 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -544,14 +544,6 @@ class FullCloudFormationDeployment { return this.checkAndExecuteChangeSet(changeSetReport, { preExistingChangeSet: true }); } - /** - * Which recovery advice applies to the stack's current state. - * - * Only `UPDATE_FAILED` is known to be recoverable by replaying the previously deployed configuration - that is the - * case verified against CloudFormation. `isRollbackable` is broader (it also covers `CREATE_FAILED` and - * `UPDATE_ROLLBACK_FAILED`), and a stack that never deployed successfully has no configuration to replay, so those - * must not be given replay instructions. - */ private replacementRecovery(): ReplacementRecovery { if (!this.cloudFormationStack.stackStatus.isRollbackable) { return 'none'; @@ -566,26 +558,6 @@ class FullCloudFormationDeployment { } } - /** - * Whether THIS invocation's flags ask for rollback to be disabled. - * - * Express Mode disables rollback unless it is explicitly requested; standard mode only disables it when - * `--no-rollback` was passed. - * - * This is one of three rollback predicates in this file, and they answer different questions - do not treat them as - * interchangeable: - * - * - `rollbackDisabled()` (this one): what the request asks for. Used by `deployConfig()` when creating a change set, - * and as the comparison input when checking a persisted policy against the request. - * - `rollbackWillBeDisabled()`: what CloudFormation will actually do when an existing change set executes. For an - * EXPRESS change set the persisted `DeploymentConfig` decides and this predicate does not apply. - * - `this.options.rollback ?? true` in `checkAndExecuteChangeSet`: standard-mode paused-state routing only, which - * keys off the raw flag default rather than the express-aware answer. - * - * Note this is also deliberately NOT the predicate used by `commonExecuteOptions()`. That one decides whether to send - * `DisableRollback: true` on the API call, which express deployments must never do - they express it through - * `DeploymentConfig` instead. - */ private rollbackDisabled(): boolean { return this.options.express ? this.options.rollback !== true : this.options.rollback === false; } @@ -601,20 +573,6 @@ class FullCloudFormationDeployment { }; } - /** - * Every replacement this change set will perform, including those that only appear in a nested stack. - * - * `createChangeSet()` always passes `IncludeNestedStacks: true` for non-import deployments, and CloudFormation reports - * nested changes in a SEPARATE change set per nested stack, linked from the root only by the nested stack resource's - * `ChangeSetId`. Reading the root's `Changes` alone therefore sees no replacement when the replaced resource lives - * inside a nested stack: the deployment is submitted, CloudFormation refuses it mid-execution, and the stack is left - * in `UPDATE_FAILED` - #1931 again. Verified against `FakeCloudFormation`: before this traversal the guard returned - * `did-deploy-stack` and called `ExecuteChangeSet` for exactly that shape. - * - * A child that cannot be described is skipped with a debug line rather than failing the deployment. This guard is - * advisory - `routeReplacementRejectedWithRollbackDisabled` still catches the rejection afterwards - so an - * inaccessible child must not turn a working deployment into an error. - */ private async findAllReplacements(changeSet: DescribeChangeSetCommandOutput): Promise { const visited = new Set(); @@ -634,7 +592,6 @@ class FullCloudFormationDeployment { continue; } - // Guards against a child that links back to an ancestor, which would otherwise recurse forever. if (visited.has(nested.ChangeSetId)) { continue; } @@ -665,18 +622,6 @@ class FullCloudFormationDeployment { return collect(changeSet, 0); } - /** - * Whether rollback will be disabled when this change set executes. - * - * An EXPRESS change set records the answer and `ExecuteChangeSet` cannot change it, so the persisted value decides. A - * STANDARD change set records no rollback choice: there it is decided at execute time by the `DisableRollback` we are - * about to send, which `commonExecuteOptions()` only sets for an explicit `--no-rollback`. - * - * `DeploymentConfig` missing entirely is the ambiguous case - it could mean "standard change set, nothing recorded" or - * "express change set, field not returned" - and we cannot tell those apart. Every response observed so far includes - * it. If that ever changes, an express invocation assumes the express default (rollback disabled) so the guard still - * fires: a needless gate costs one clear error the user can act on, a missed gate costs a wedged stack. - */ private rollbackWillBeDisabled(changeSet: DescribeChangeSetCommandOutput, persistedRollbackDisabled: boolean | undefined): boolean { if (persistedRollbackDisabled !== undefined) { return persistedRollbackDisabled; @@ -687,22 +632,12 @@ class FullCloudFormationDeployment { return this.options.rollback === false; } - /** - * Check rollback/replacement constraints and execute the change set if all checks pass. - */ private async checkAndExecuteChangeSet( changeSetReport: ChangeSetReport, opts: { preExistingChangeSet: boolean }, ): Promise { const changeSet = changeSetReport.changeSet; - // The persisted `DeploymentConfig` is read BEFORE any routing because for an existing change set it is - // authoritative: CloudFormation fixed both the mode and the rollback choice when the change set was created, and - // `ExecuteChangeSet` cannot change either. Routing on this invocation's flags first inverts two cases - an express - // change set executed without `--express` would be sent down the standard rollback-first path, which recommends - // `RollbackStack` (unsupported for express-failed stacks); and a standard change set executed with `--express` - // would skip paused-state handling and reach `ExecuteChangeSet`, which CloudFormation rejects synchronously. The - // request flags are only used to validate consistency with what was persisted. const persistedMode = changeSet.DeploymentConfig?.Mode; const isExpress = persistedMode !== undefined ? persistedMode === 'EXPRESS' : (this.options.express ?? false); const persistedRollbackDisabled = expressRollbackDisabled(changeSet.DeploymentConfig); @@ -712,7 +647,6 @@ class FullCloudFormationDeployment { const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; - // Express stacks cannot use the rollback API, so standard rollback-first routing does not apply to them. if (!isExpress) { if (isPausedFailState && replacements.length > 0) { return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name }; @@ -724,35 +658,6 @@ class FullCloudFormationDeployment { const rollbackWillBeDisabled = this.rollbackWillBeDisabled(changeSet, persistedRollbackDisabled); - // CloudFormation rejects replacement-type updates while rollback is disabled. This is documented under "Express - // mode and rollback" in - // https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/stack-failure-options.html: "Disabling rollback - // isn't supported for immutable update operations. If an update requires replacing a resource and the operation - // fails, the failed state can't be preserved for retry." That last sentence is why we refuse up front instead of - // letting CloudFormation surface the error: there is no preserved failed state to retry from. - // - // Standard mode only disables rollback for `--no-rollback`, but Express Mode disables it by default - which is why - // this condition must not be scoped to non-express deployments. #1745 disabled it for express twice over (an - // unconditional early return to `executeChangeSet`, plus deletion of the `expressNoRollback` disjunct), #1785 - // restructured what was left, and #1931 is the resulting SEV. - // - // REMOVAL CONTRACT. This guard exists only because CloudFormation refuses replacements while rollback is disabled. - // If that restriction is lifted, the pieces below are one unit and must be removed TOGETHER - leaving a subset - // behind gives users a gate that fires with no reason to, or guidance pointing at a recovery that is no longer - // needed: - // 1. this replacement branch, including the `ReplacementRequiresRecreateChangeSet` and - // `ReplacementRequiresUnwedge` throws and the `replacement-requires-rollback` result; - // 2. the persisted-vs-requested policy mismatch report further down; - // 3. `routeReplacementRejectedWithRollbackDisabled` and `CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON`, the - // after-the-fact net in `monitorDeployment`; - // 4. the public payload surface: `ReplacementRequiresRollback`, `ReplacedResource`, and - // `ReplacementRequiresRollbackStackResult` in `payloads/deploy.ts`; - // 5. `CDK_TOOLKIT_W5903` in `api/io/private/messages.ts`. - // Items 4 and 5 are public API, so removal is a breaking change and needs its own deprecation cycle. - // - // Do NOT treat this as scheduled work. A server-side fix around 2026-11-15 was reported to us second-hand and is - // NOT confirmed with CloudFormation; nothing here should be removed before the restriction is observed to be gone - // in all regions. if (replacements.length > 0 && rollbackWillBeDisabled) { if (isExpress) { const guidance = replacementRoutingMessage({ @@ -761,8 +666,6 @@ class FullCloudFormationDeployment { status: this.cloudFormationStack.stackStatus.name, }); - // Emitted BEFORE the throws below: `cdk deploy --watch` catches and discards the thrown error - // (`cdk-toolkit.ts` watch loop), so guidance carried only by the exception would never reach the user. await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg(guidance, { stackName: this.stackName, changeSetId: changeSet.ChangeSetId, @@ -770,11 +673,6 @@ class FullCloudFormationDeployment { detectedBy: 'change-set', })); - // An existing change set's rollback policy is frozen. Returning `replacement-requires-rollback` would make the - // toolkit offer a retry with rollback enabled, and that retry re-executes THIS change set - which fails with - // `ChangeSetRollbackPolicyMismatch` because the persisted policy cannot be changed. Offering a recovery that - // is guaranteed to fail is the defect we already removed for already-failed stacks; the change set has to be - // recreated instead. if (opts.preExistingChangeSet && persistedRollbackDisabled === true) { throw new ToolkitError( 'ReplacementRequiresRecreateChangeSet', @@ -782,8 +680,6 @@ class FullCloudFormationDeployment { ); } - // A stack already in a failed state cannot be updated with rollback enabled either - CloudFormation answers - // "This stack is currently in a non-terminal [UPDATE_FAILED] state" - so the retry cannot succeed here. if (isPausedFailState) { throw new ToolkitError('ReplacementRequiresUnwedge', guidance); } @@ -791,10 +687,6 @@ class FullCloudFormationDeployment { return { type: 'replacement-requires-rollback' }; } - // Only reached when there is no replacement to gate. A rollback policy that disagrees with this invocation's flags - // still means the flags are not being honoured, but without a replacement CloudFormation accepts the execution, so - // refusing here would break deployments unrelated to #1931 - executing an express change set without `--express`, - // or with `--rollback`. Report it and continue. if (persistedRollbackDisabled !== undefined && persistedRollbackDisabled !== requestedRollbackDisabled) { await this.ioHelper.defaults.warn( changeSetPolicyMismatchMessage(changeSet.ChangeSetName, persistedRollbackDisabled), @@ -803,12 +695,6 @@ class FullCloudFormationDeployment { await this.ioHelper.defaults.debug(format('Initiating execution of changeset %s on stack %s', changeSet.ChangeSetId, this.stackName)); - // `DisableRollback` on ExecuteChangeSet is a consistency assertion, not an override: CloudFormation rejects a value - // conflicting with the change set's persisted policy with `ValidationError: DisableRollback specified on - // ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` Once the change set - // pins the policy, sending the flag can only restate it or fail the call outright, so omit it there and let the - // persisted value stand - the ignored request is already reported above. Only a change set that records no policy - // (standard mode) is still governed by this flag. const { DisableRollback, ...sharedExecuteOptions } = this.commonExecuteOptions(); const rollbackFlagStillDecides = persistedRollbackDisabled === undefined; @@ -1013,28 +899,7 @@ class FullCloudFormationDeployment { }; } - /** - * Tell the user how to perform a replacement when CloudFormation rejected one because rollback was disabled. - * - * This runs from `monitorDeployment`, which BOTH the change set path and `--method=direct` go through, so it is not a - * direct-path-only hook. It is the after-the-fact net for every replacement the up-front change set gate could not - * see: - * - * - `--method=direct` has no change set to inspect at all, so nothing there can be gated up front. - * - A change set can report `Replacement: "Conditional"`, which the up-front gate does not treat as a replacement - * (see #1971). If CloudFormation then resolves it to an actual replacement, this is the only thing that fires. - * - * `--express --method=direct` is deliberately NOT refused up front, and that gap should not be "fixed": replaying the - * previous configuration that way is the only exit from a stack already stranded in UPDATE_FAILED, so refusing the - * combination would strand users permanently. - * - * The original error is left to propagate untouched, so a genuinely failing replacement still reports its real - * underlying service error. - */ private async routeReplacementRejectedWithRollbackDisabled(error: any, errors: ResourceErrors): Promise { - // Express only: the guidance below names Express Mode flags, and switching a standard-mode deployment to Express - // Mode would be actively harmful (the mode is sticky and gives up `cdk rollback`). A standard `--no-rollback` - // deployment that hits this recovers with a plain `cdk deploy`, which the README already documents. if (!this.options.express || !this.rollbackDisabled()) { return; } @@ -1043,10 +908,6 @@ class FullCloudFormationDeployment { const matched = rejected.length > 0 || mentionsReplacementRejection(error?.message ?? ''); if (!matched) { - // CloudFormation owns the wording we match on, so it can be reworded at any time. (A rewording around 2026-11-15 - // was reported to us second-hand; we have NOT confirmed it with CloudFormation, so treat the date as a rumour and - // the fragility as permanent.) If it is reworded, this is the branch that will start being taken - log what we did - // see so that shows up in a debug log instead of arriving as a second SEV. const reported = errors.allErrorMessages.filter((m) => m.trim() !== ''); await this.ioHelper.defaults.debug(format( 'Deployment failed with rollback disabled but no reported error mentioned %j, so no replacement guidance was emitted. Reported reasons: %s', @@ -1059,8 +920,6 @@ class FullCloudFormationDeployment { await this.ioHelper.notify(IO.CDK_TOOLKIT_W5903.msg( replacementRoutingMessage({ rejected: true, - // The failure just happened, so the cached stack status predates it. An update had a previous configuration to - // replay; a failed create never did. recovery: this.update ? 'replay' : 'recreate', }), { @@ -1123,9 +982,6 @@ class FullCloudFormationDeployment { * deployed everywhere yet. */ private commonExecuteOptions(): Partial> { - // Deliberately not `rollbackDisabled()`, which is also true for plain `--express`. This flag tracks the explicit - // `--no-rollback` request only, so that plain `--express` does not start sending a `DisableRollback` it never - // asked for. `--express --no-rollback` sends both this flag and `DeploymentConfig`, which is intended. const shouldSendDisableRollbackFlag = this.options.rollback === false; return { @@ -1340,11 +1196,6 @@ function arrayEquals(a: any[], b: any[]): boolean { /** * Find the resource changes in a change set that CloudFormation would perform by replacement - * - * `Replacement: 'Conditional'` is deliberately excluded: `CDKMetadata` reports it on essentially every CDK deployment - * (its `Analytics` property is `RequiresRecreation: 'Conditionally'`), so gating on it would gate almost every express - * deployment. A `Conditional` change that does turn out to replace is caught after the fact by - * `routeReplacementRejectedWithRollbackDisabled`. */ function findReplacements(changeSet: DescribeChangeSetCommandOutput): ReplacedResource[] { return (changeSet.Changes ?? []).flatMap((c) => { @@ -1369,14 +1220,6 @@ function findReplacements(changeSet: DescribeChangeSetCommandOutput): ReplacedRe /** * The reason CloudFormation reports when it refuses a replacement because rollback is disabled. - * - * CloudFormation surfaces this as a resource status reason with no structured error code attached (`extractErrorCode` - * finds no `HandlerErrorCode:`/`Error Code:` prefix in it), so matching this text is the only trigger available. That - * makes it fragile: CloudFormation owns the string and can reword it at any time. (A rewording around 2026-11-15 was - * reported to us second-hand and is NOT confirmed with CloudFormation - do not treat that date as a deadline or as - * permission to assume the match is stable until then.) Replace this match with a structured discriminator if - * CloudFormation ever exposes one. A miss is logged at debug level and only costs the extra guidance - the underlying - * CloudFormation error is reported either way. */ export const CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON = 'Replacement type updates not supported on stack with disable-rollback'; @@ -1386,10 +1229,6 @@ function mentionsReplacementRejection(message: string): boolean { /** * Whether a persisted change set `DeploymentConfig` pins the rollback choice, and if so which way. - * - * Only Express Mode records a rollback choice in `DeploymentConfig`, and `ExecuteChangeSet` has no `DeploymentConfig` - * parameter to restate it with. Standard mode carries none: rollback there is decided at execute time by the - * `DisableRollback` flag, so the current invocation's options stay authoritative and this returns `undefined`. */ function expressRollbackDisabled(config: DeploymentConfig | undefined): boolean | undefined { if (config?.Mode !== 'EXPRESS') { @@ -1418,19 +1257,10 @@ function changeSetPolicyMismatchMessage(changeSetName: string | undefined, persi type ReplacementRecovery = 'none' | 'replay' | 'recreate' | 'resolve-state'; -/** - * How deep to follow nested change sets when looking for replacements. - * - * CloudFormation's own nested stack limit is 5 levels, so this only stops pathological or cyclic shapes; the - * `visited` set handles cycles, and this bounds the number of `DescribeChangeSet` calls a deployment can make. - */ const MAX_NESTED_CHANGE_SET_DEPTH = 10; /** * Explain that a replacement cannot be deployed by executing this change set, whatever flags are passed. - * - * Separate from `changeSetPolicyMismatchMessage`: that one is about a flag being ignored, this one is about a - * deployment that cannot be made to work without creating a new change set. */ function changeSetRecreateForReplacementMessage(changeSetName: string | undefined): string { const named = changeSetName ? ` ${chalk.blue(changeSetName)}` : ''; @@ -1447,15 +1277,6 @@ function changeSetRecreateForReplacementMessage(changeSetName: string | undefine /** * Explain how to deploy a replacement when rollback is disabled. - * - * Replacements are supported in Express Mode; they are not supported while rollback is disabled, which Express Mode - * does by default. So this routes the user to what actually works instead of just refusing. - * - * When we stopped before submitting anything and the stack is healthy, this stays deliberately short and does NOT tell - * the user to run anything: the caller is about to offer to retry with rollback enabled (which `--force` accepts - * automatically), so an instruction to run the deployment by hand would be contradicted by what happens next. - * - * TODO: point at a CloudFormation User Guide anchor for Express Mode rollback behaviour once one exists. */ function replacementRoutingMessage(opts: { rejected: boolean; recovery: ReplacementRecovery; status?: string }): string { const withRollback = chalk.blue('cdk deploy --express --rollback'); @@ -1471,9 +1292,6 @@ function replacementRoutingMessage(opts: { rejected: boolean; recovery: Replacem 'Express Mode disables rollback unless you ask for it with --rollback; replacements themselves are supported.', ]; - // Deploying with rollback enabled cannot update a stack that is already in a failed state - CloudFormation answers - // "This stack is currently in a non-terminal [UPDATE_FAILED] state" (verified against CloudFormation). What to do - // instead depends on whether a previously deployed configuration exists to go back to. switch (opts.recovery) { case 'none': return headline.join('\n'); diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts index 382b8f1d9..e7a41179d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts @@ -334,11 +334,6 @@ export const IO = { code: 'CDK_TOOLKIT_W5902', description: 'Express Mode deployment completed with resources still stabilizing', }), - /** - * Tied to a CloudFormation restriction (replacements are refused while rollback is disabled) that is expected to be - * TEMPORARY. If the restriction is lifted this message is removed together with the `ReplacementRequiresRollback` and - * `ReplacedResource` payload types - see the REMOVAL CONTRACT comment in `api/deployments/deploy-stack.ts`. - */ CDK_TOOLKIT_W5903: make.warn({ code: 'CDK_TOOLKIT_W5903', description: 'Deployment includes a replacement that CloudFormation does not support while rollback is disabled', diff --git a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts index fd11ba13b..8298dfff4 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/payloads/deploy.ts @@ -78,11 +78,6 @@ export interface PublishAssetEvent { /** * A resource that CloudFormation reported it would replace - * - * This type exists only to describe a CloudFormation restriction: replacements are refused while rollback is disabled. - * That restriction is expected to be TEMPORARY, so treat this type as provisional - if CloudFormation lifts it, this - * type, `ReplacementRequiresRollback` and `CDK_TOOLKIT_W5903` are removed together (see the REMOVAL CONTRACT comment in - * `api/deployments/deploy-stack.ts`). Do not build anything on it that cannot tolerate deprecation. */ export interface ReplacedResource { /** @@ -110,11 +105,6 @@ export interface ReplacedResource { /** * A deployment includes a replacement that CloudFormation will not perform while rollback is disabled - * - * This type exists only to describe a CloudFormation restriction that is expected to be TEMPORARY, so treat it as - * provisional - if CloudFormation lifts the restriction, this type, `ReplacedResource` and `CDK_TOOLKIT_W5903` are - * removed together (see the REMOVAL CONTRACT comment in `api/deployments/deploy-stack.ts`). Do not build anything on it - * that cannot tolerate deprecation. */ export interface ReplacementRequiresRollback { /** diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts index 9a4ec38d8..be22e5484 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/fake-aws/fake-cloudformation.ts @@ -516,8 +516,6 @@ export class FakeCloudFormation { Capabilities: cs.capabilities as any, Description: cs.description, CreationTime: cs.creationTime, - // The real API returns the DeploymentConfig that was persisted by CreateChangeSet. It matters because - // ExecuteChangeSet cannot change it, so code executing an existing change set has to read it from here. ...(cs.deploymentConfig ? { DeploymentConfig: cs.deploymentConfig } : undefined), NextToken: nextToken, $metadata: {}, @@ -531,13 +529,6 @@ export class FakeCloudFormation { cfnError('InvalidChangeSetStatus', `ChangeSet [${cs.name}] is in ${cs.executionStatus} state and cannot be executed`); } - // `DisableRollback` on ExecuteChangeSet is a consistency assertion against the policy the change set was created - // with, not an override: a matching value is accepted, and a conflicting one is rejected synchronously, before - // anything is submitted and with both the stack and the change set left untouched. Verified against CloudFormation - // in us-east-1. - // - // Scoped to EXPRESS because only EXPRESS persists a rollback choice on the change set. A STANDARD change set - // records none, so there the execute-time flag decides and cannot conflict with anything. const persistedRollbackDisabled = cs.deploymentConfig?.Mode === 'EXPRESS' ? cs.deploymentConfig.DisableRollback !== false : undefined; @@ -1184,13 +1175,6 @@ export class FakeCloudFormation { }); } - /** - * Whether CloudFormation would have rollback disabled for this operation. - * - * Standard deployments say so with `DisableRollback` on the call. Express Mode has rollback disabled server-side by - * default, and re-enables it by sending `DeploymentConfig.DisableRollback: false`. A replacement submitted while - * rollback is disabled is rejected by CloudFormation during execution. - */ private rollbackIsDisabled(input: { DisableRollback?: boolean; DeploymentConfig?: DeploymentConfig }): boolean { if (input.DisableRollback) { return true; @@ -1216,12 +1200,6 @@ export class FakeCloudFormation { }); } - /** - * Emit resource-level failure events for every failing resource that declares a `FailReason`. - * - * Real CloudFormation reports why an operation failed on a resource event, not on the stack event. Resources that - * only set `Fail: true` keep the old behaviour of producing stack-level events only. - */ private addFailedUpdateResourceEvents(stack: InMemoryStack, template: Record, operationId?: string, verb: 'UPDATE' | 'CREATE' = 'UPDATE') { for (const [logicalId, res] of Object.entries(templateResources(template))) { const r = res as any; diff --git a/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts b/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts index a09c76c59..cc49a9723 100644 --- a/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts +++ b/packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts @@ -77,9 +77,6 @@ export class TestIoHost implements IIoHost { })); } - /** - * Return all messages emitted with a given code - */ public messagesWithCode(code: IoMessageCode): Array> { return this.messages.filter((m) => m.code === code); } diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts index 2a5bb692c..1ce555aee 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts @@ -621,9 +621,6 @@ IAM Statement Changes successfulDeployment(); }); - // From a healthy stack the guard returns `replacement-requires-rollback`, which the deploy action turns into - // this prompt and a retry with rollback enabled. From an already-failed stack it throws instead, since the - // retry cannot succeed there. test('replacement-requires-rollback under --express explains that rollback is disabled, and retries with it enabled', async () => { // GIVEN mockDeployStack.mockImplementation(async (params) => { @@ -652,7 +649,6 @@ IAM Statement Changes }), })); - // ... and we retried the deployment ourselves with rollback enabled expect(mockDeployStack).toHaveBeenCalledWith(expect.objectContaining({ express: true, rollback: true })); successfulDeployment(); }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 96faa32b8..7601968fd 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -25,8 +25,6 @@ import { } from '../../_helpers/mock-sdk'; import { TestIoHost } from '../../_helpers/test-io-host'; -// Taken from the production message rather than restated, so renaming the code cannot leave the negative assertions -// below (`messagesWithCode(W5903)` returning nothing) vacuously true. const W5903 = IO.CDK_TOOLKIT_W5903.code; let ioHost = new TestIoHost('debug', true); @@ -64,9 +62,6 @@ function targetTemplate() { }; } -/** - * A template whose update fails the way CloudFormation fails a replacement on a rollback-disabled stack - */ function templateRejectingReplacement() { return { Resources: { @@ -75,7 +70,6 @@ function templateRejectingReplacement() { Properties: { Bar: 'Bar', Fail: true, - // CloudFormation appends a period; we match on a substring, so keep the fixture faithful to the service. FailReason: `${CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON}.`, }, }, @@ -153,12 +147,6 @@ function givenStackExists(overrides: Partial & { StackName?: string } = { fakeCfn.accessStack(stackName).template = startTemplate(); } -/** - * The shape CloudFormation actually emitted for the change in aws/aws-cdk-cli#1931. - * - * A replacement of a resource with a default deletion policy carries BOTH a `ReplaceAndDelete` policy action and - * `Replacement: 'True'` - verified against a real `DescribeChangeSet` response for the reproduction stack. - */ function policyActionReplacementChange(logicalId = 'TaskDef54694570'): Change { return { Type: 'Resource', @@ -192,10 +180,6 @@ function updateChange(logicalId = 'Queue4A7E3555'): Change { }; } -/** - * `CDKMetadata` reports `Replacement: 'Conditional'` on essentially every real CDK deployment (its `Analytics` - * property is `RequiresRecreation: 'Conditionally'`), so `Conditional` must never be treated as a replacement. - */ function conditionalChange(logicalId = 'CDKMetadata'): Change { return { Type: 'Resource', @@ -216,11 +200,6 @@ function conditionalChange(logicalId = 'CDKMetadata'): Change { }; } -/** - * A replacement CloudFormation reports only via `Replacement: 'True'`, with no policy action attached. - * - * Not gated up front today - see the negative test below and #1971. - */ function replacementOnlyChange(logicalId = 'TaskDef54694570'): Change { return { Type: 'Resource', @@ -234,12 +213,6 @@ function replacementOnlyChange(logicalId = 'TaskDef54694570'): Change { }; } -/** - * Make any attempt to actually mutate the stack a hard test failure. - * - * The point of the guard is that we stop *before* submitting the update, so a test that only asserted on the returned - * result type would still pass if the deployment happened anyway. - */ function failOnAnyStackMutation() { mockCloudFormationClient.on(ExecuteChangeSetCommand).callsFake(() => { throw new Error('ExecuteChangeSet must not be called when the replacement guard trips'); @@ -254,24 +227,6 @@ function expectNoStackMutation() { expect(mockCloudFormationClient).not.toHaveReceivedCommand(UpdateStackCommand); } -/** - * Assert the `replay` guidance is ordered the way a wedged user needs to read it: the state the stack is actually in, - * then the steps that return it to a terminal state, and only then the suggestion to deploy with rollback enabled. - * - * Ordering is the whole point of this message. `cdk deploy --express --rollback` cannot update a stack that is already - * failed - CloudFormation answers "This stack is currently in a non-terminal [UPDATE_FAILED] state" - so guidance that - * led with that command would be advice the user cannot act on. Containment assertions alone would not catch a - * reordering, which is why these compare offsets. - * - * Deliberately anchored on the suggestion phrasing and NOT on `indexOf('--rollback')`: the FIRST occurrence of that flag - * is the clause saying it cannot update a failed state, and that one legitimately comes before the unwedge steps. A - * naive "unwedge before any --rollback mention" assertion would fail on the correct message. - * - * That carve-out left a blind spot, so it is pinned explicitly: the first mention of the rollback command must be the - * "cannot update" clause. Without this, prepending an actionable `Deploy it with cdk deploy --express --rollback` line - * ahead of the whole explanation is invisible to every other assertion here - the steps still follow "in a failed - * state", and the closing suggestion still comes last. - */ function expectUnwedgeBeforeRollbackSuggestion(message: string) { const state = message.indexOf('in a failed state'); const step1 = message.indexOf('Revert your change'); @@ -286,13 +241,6 @@ function expectUnwedgeBeforeRollbackSuggestion(message: string) { expect(suggestion).toBeGreaterThan(step2); } -/** - * Assert the `recreate` guidance never offers replay instructions. - * - * A stack that never completed a deployment has no previous configuration to go back to, so telling its owner to - * "revert your change and redeploy the last configuration that deployed successfully" is impossible advice. This is the - * variant whose wrongness is hardest to spot by hand, so it is pinned positively and negatively. - */ function expectRecreateGuidance(message: string) { expect(message).toMatch(/no previous configuration to replay/); expect(message).toMatch(/Delete the stack and deploy again/); @@ -327,12 +275,9 @@ describe('change set path, replacement reported as policy action with Replacemen }); ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'unless you ask for it with --rollback' }); - // From a healthy stack the caller is about to offer to retry with rollback enabled, so we must NOT tell the user - // to run a deployment themselves, and the recovery runbook does not apply yet. expect(ioHost.messagesWithCode(W5903)[0].message).not.toContain('To recover'); expect(ioHost.messagesWithCode(W5903)[0].message).not.toContain('cdk deploy --express --rollback'); - // Nothing was submitted, so we know this from the change set rather than from a failure expect(ioHost.messagesWithCode(W5903)[0].data).toEqual(expect.objectContaining({ stackName: 'withouterrors', detectedBy: 'change-set', @@ -356,7 +301,7 @@ describe('change set path, replacement reported as policy action with Replacemen forceDeployment: true, }); - // THEN - this is the route users are sent to, so pin both halves of it + // THEN expect(result.type).toEqual('did-deploy-stack'); expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); expect(mockCloudFormationClient).toHaveReceivedCommandWith(CreateChangeSetCommand, { @@ -406,10 +351,6 @@ describe('change set path, replacement reported as policy action with Replacemen expectNoStackMutation(); }); - // Verified against CloudFormation: a stack already in a failed state cannot be updated with rollback enabled either - - // it answers "This stack is currently in a non-terminal [UPDATE_FAILED] state". Returning - // `replacement-requires-rollback` would make the toolkit offer that deployment, and since the confirmation defaults - // to yes, a non-interactive caller would run it and fail again. So this must be a terminal error, not a prompt. test('express from an already-failed stack fails terminally instead of offering a doomed retry', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); @@ -423,7 +364,7 @@ describe('change set path, replacement reported as policy action with Replacemen forceDeployment: true, }); - // THEN - thrown, so there is no result the toolkit could turn into a retry prompt + // THEN await expect(deployment).rejects.toThrow(/in a failed state/); await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresUnwedge' })); await expect(deployment).rejects.toThrow(/Revert your change/); @@ -431,8 +372,6 @@ describe('change set path, replacement reported as policy action with Replacemen expectUnwedgeBeforeRollbackSuggestion(await deployment.then(() => '', (e) => e.message)); expectNoStackMutation(); - // ... and the guidance is ALSO emitted as W5903, because `cdk deploy --watch` swallows the thrown error and would - // otherwise show the user nothing actionable. ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'Revert your change' }); }); }); @@ -460,9 +399,6 @@ describe('change set path', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); - // Pins today's behaviour for #1971: detection keys on `PolicyAction`, so a replacement CloudFormation reports only - // via `Replacement: 'True'` is NOT gated up front. It is still caught after the fact on the failure path. If #1971 is - // fixed by widening detection, this test should flip to expecting the gate. test('a replacement reported without a policy action is not gated up front (#1971)', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -482,7 +418,7 @@ describe('change set path', () => { }); test('a replacement on the second page of DescribeChangeSet results is still gated', async () => { - // GIVEN - one change per page, with the replacement on page two + // GIVEN fakeCfn.reset({ pageSize: 1 }); givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); fakeCfn.overrideChangeSetChanges = [updateChange('First'), policyActionReplacementChange('Second')]; @@ -517,15 +453,12 @@ describe('change set path', () => { expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); }); - // This is why the routing hook lives in `monitorDeployment` rather than in `directDeployment`: a change set can - // contain a replacement the pre-flight check cannot see (CloudFormation reports `Conditional`), so the change set - // path needs the after-the-fact guidance too. test('a replacement the change set did not declare is still routed after CloudFormation rejects it', async () => { - // GIVEN - the change set only reports a Conditional change, so the gate does not fire... + // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); fakeCfn.overrideChangeSetChanges = [conditionalChange()]; - // WHEN - ...but executing it fails the way CloudFormation fails a rejected replacement + // WHEN await expect(testDeployStack({ ...standardDeployStackArguments(FAKE_STACK_REJECTING_REPLACEMENT), express: true, @@ -541,21 +474,6 @@ describe('change set path', () => { }); }); -/** - * The express replacement guard has been deleted once and restructured once without the suite noticing: - * - * - aws/aws-cdk-cli#1745 disabled it twice over - an unconditional early return to `executeChangeSet` for express, plus - * deletion of the `expressNoRollback` disjunct from the guard condition - and inverted the case that covered it, so - * `['express, no explicit rollback', { express: true }]` asserted `did-deploy-stack` rather than - * `replacement-requires-rollback`. That shipped the regression in CLI 2.1133.0. - * - aws/aws-cdk-cli#1785 then restructured what was left into `if (!this.options.express) { ... }`. - * - aws/aws-cdk-cli#1931 is the resulting SEV: `cdk deploy --express` submits a replacement while CloudFormation has - * rollback disabled, CloudFormation rejects it, and the stack is stranded in UPDATE_FAILED. - * - * These cases exist to fail loudly if the guard is removed again, or if its scope is changed so that express - * deployments stop consulting it. They deliberately assert the *negative* (nothing was submitted to CloudFormation) - * rather than that a message was printed. - */ describe('REGRESSION aws/aws-cdk-cli#1931: the express replacement guard must not be removed or rescoped', () => { test.each([ ['rollback not specified (express disables rollback by default)', undefined], @@ -574,7 +492,7 @@ describe('REGRESSION aws/aws-cdk-cli#1931: the express replacement guard must no forceDeployment: true, }); - // THEN - the result type `cdk-toolkit` turns into a confirm-and-retry-with-rollback prompt + // THEN expect(result).toEqual({ type: 'replacement-requires-rollback' }); expectNoStackMutation(); expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_COMPLETE); @@ -599,9 +517,6 @@ describe('REGRESSION aws/aws-cdk-cli#1931: the express replacement guard must no }); describe('direct path', () => { - // Standard mode also runs with rollback disabled under `--no-rollback`, and CloudFormation rejects replacements the - // same way - but the guidance names Express Mode flags, and Express Mode is sticky and gives up `cdk rollback`. A - // wedged standard-mode user recovers with a plain `cdk deploy`, so they must NOT be pushed towards `--express`. test('a non-express --no-rollback rejection is not routed towards Express Mode', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -614,7 +529,7 @@ describe('direct path', () => { forceDeployment: true, })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); - // THEN - the real CloudFormation error, and no Express Mode guidance + // THEN expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); @@ -630,7 +545,7 @@ describe('direct path', () => { forceDeployment: true, })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); - // THEN - this is the SEV: rollback is disabled server-side, so the stack cannot roll itself back + // THEN expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_FAILED); ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'CloudFormation refused a replacement' }); @@ -658,20 +573,13 @@ describe('direct path', () => { forceDeployment: true, })).rejects.toThrow(CFN_REPLACEMENT_WITH_ROLLBACK_DISABLED_REASON); - // THEN - a genuinely failing replacement under `--express --rollback` recovers on its own + // THEN expect(fakeCfn.accessStack('withouterrors').status).toEqual(StackStatus.UPDATE_ROLLBACK_COMPLETE); expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); - // `cdk deploy --express --method=direct` with the previous configuration is the documented way to unwedge a stack - // that is already stranded in UPDATE_FAILED, so it must stay a plain no-op and must never be refused up front. - // - // NOTE: the stranded-but-replayable state is hand-seeded here. The fake assigns the failed change set's template to - // the stack and does not restore the previous template the way a real resource rollback does, so this pins that we - // handle such a state correctly - not that a simulated failed replacement naturally arrives at it. That a real - // failed express replacement does leave the stack replayable was established by deploys against CloudFormation. test('a no-op replay against a hand-seeded stranded stack is not blocked', async () => { - // GIVEN - the stranded stack already has the template we are about to deploy + // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_FAILED }); fakeCfn.accessStack('withouterrors').template = targetTemplate(); @@ -688,10 +596,8 @@ describe('direct path', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); - // CloudFormation owns the prose we match on and has a change landing around 2026-11-15. When it stops matching we - // want a breadcrumb in the debug log rather than silence. test('a failure that does not mention the rejection logs what it did see', async () => { - // GIVEN - a failing update whose reason is not the replacement rejection + // GIVEN const otherFailure = testStack({ stackName: 'withouterrors', template: { @@ -720,17 +626,6 @@ describe('direct path', () => { }); }); -/** - * A change set carries the rollback policy it was created with. CloudFormation persists `DeploymentConfig` on - * `CreateChangeSet`, returns it from `DescribeChangeSet`, and `ExecuteChangeSet` cannot override it: its input has no - * `DeploymentConfig` field, and its top-level `DisableRollback` is a consistency assertion rather than an override - a - * matching value is accepted, a conflicting one fails synchronously with `ValidationError: DisableRollback specified on - * ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` So when create and execute - * happen in separate invocations - `--method=change-set --no-execute` then `--method=execute-change-set`, or the - * toolkit's own retry - the second invocation's flags do not decide what CloudFormation does. Deriving rollback safety - * from the current options there would let the SEV in #1931 back in: the guard would believe rollback is enabled and - * execute a rollback-disabled change set containing a replacement. - */ describe('executing a change set created by an earlier invocation', () => { function givenExpressChangeSetExists(opts: { rollbackDisabled: boolean; changes: Change[] }) { fakeCfn.createChangeSetSync({ @@ -750,8 +645,6 @@ describe('executing a change set created by an earlier invocation', () => { forceDeployment: true, }; - // `--rollback` cannot be honoured on an existing change set, but without a replacement CloudFormation accepts the - // execution, so refusing would break a deployment that works. Report that the flag is being ignored and continue. test('a rollback-disabled change set with no replacement is executed, with the ignored flag reported', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -771,15 +664,13 @@ describe('executing a change set created by an earlier invocation', () => { ioHost.expectMessage({ level: 'warn', containing: 'created with rollback disabled' }); }); - // The SEV path: the change set contains a replacement and was created with rollback disabled. Executing it would put - // the prohibited combination in front of CloudFormation and strand the stack. test('a rollback-disabled change set containing a replacement is never executed', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: true, changes: [policyActionReplacementChange()] }); failOnAnyStackMutation(); - // WHEN - this is what the toolkit's retry does: same change set, rollback flipped on + // WHEN const deployment = testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, @@ -787,7 +678,7 @@ describe('executing a change set created by an earlier invocation', () => { rollback: true, }); - // THEN - terminal, because the retry would re-execute this same immutable change set + // THEN await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); await expect(deployment).rejects.toThrow(/Create a new change set with rollback enabled/); expectNoStackMutation(); @@ -800,13 +691,13 @@ describe('executing a change set created by an earlier invocation', () => { givenExpressChangeSetExists({ rollbackDisabled: true, changes: [policyActionReplacementChange()] }); failOnAnyStackMutation(); - // WHEN - the invocation looks like standard mode, but the change set is still Express + rollback disabled + // WHEN const deployment = testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, }); - // THEN - the persisted mode is authoritative, so this is still gated as an express replacement + // THEN await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); expectNoStackMutation(); }); @@ -816,21 +707,18 @@ describe('executing a change set created by an earlier invocation', () => { givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: false, changes: [updateChange()] }); - // WHEN - plain `--express` requests rollback disabled, but the change set was created with it enabled + // WHEN const result = await testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, express: true, }); - // THEN - rollback stays ENABLED (the safer direction) and the ignored flag is reported + // THEN expect(result.type).toEqual('did-deploy-stack'); ioHost.expectMessage({ level: 'warn', containing: 'created with rollback enabled' }); }); - // Even with the flags matching, an existing rollback-disabled change set containing a replacement cannot be made to - // work: the retry the generic result would trigger re-executes this same change set. So it is terminal, and the - // guidance says to recreate rather than to retry. test('a matching rollback-disabled change set with a replacement is terminal, not offered a retry', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -861,14 +749,6 @@ describe('executing a change set created by an earlier invocation', () => { }); } - /** - * Seed a nested-stack shape: a child stack carrying its own change set, referenced from the root change set through - * the nested stack resource's `ChangeSetId`. This is how CloudFormation reports nested changes when the root change - * set is created with `IncludeNestedStacks: true`, which `createChangeSet()` always does for non-import deployments. - * - * The root entry deliberately reports no replacement of its own - `AWS::CloudFormation::Stack` is merely modified - - * so a guard that only reads the root's `Changes` sees nothing to gate on. - */ function givenNestedChangeSetExists(opts: { rollbackDisabled: boolean; childChanges: Change[] }) { const childStackName = 'withouterrors-NestedChild-ABC123'; const deploymentConfig: DeploymentConfig = opts.rollbackDisabled @@ -903,12 +783,6 @@ describe('executing a change set created by an earlier invocation', () => { return { childStackName, childChangeSetId: child.Id }; } - /** - * A replacement that only exists in a nested stack must be gated exactly like one in the root. - * - * Confirmed as a real bypass before it was fixed: with this exact shape the guard returned `did-deploy-stack` and - * called `ExecuteChangeSet` once, emitting no `W5903` - i.e. #1931 straight through. - */ test('a replacement inside a nested stack is gated, not executed', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -928,7 +802,6 @@ describe('executing a change set created by an earlier invocation', () => { ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'does not support while rollback is disabled' }); }); - // The traversal must not gate a nested stack whose child changes nothing of consequence. test('a nested stack with no replacement executes normally', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -947,21 +820,13 @@ describe('executing a change set created by an earlier invocation', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); - /** - * The full flag matrix for the policy comparison: `--express` decides what a missing `--rollback` means, so it has to - * be read here exactly as it is when the change set is created. aws/aws-cdk-cli#1969 shipped with the CLI dropping - * `express` on the `execute-change-set` delegation, which made plain `--express` mean "rollback enabled" and refused - * Express change sets the same CLI had just created (`persisted disabled` x `express` x `rollback: undefined` below). - */ const POLICY_MATRIX = [ - // persisted rollback DISABLED (Express default) [true, { express: true, rollback: true }, 'reported'], [true, { express: true }, 'silent'], [true, { express: true, rollback: false }, 'silent'], [true, { rollback: true }, 'reported'], [true, {}, 'reported'], [true, { rollback: false }, 'silent'], - // persisted rollback ENABLED [false, { express: true, rollback: true }, 'silent'], [false, { express: true }, 'reported'], [false, { express: true, rollback: false }, 'reported'], @@ -970,22 +835,12 @@ describe('executing a change set created by an earlier invocation', () => { [false, { rollback: false }, 'reported'], ] as Array<[boolean, Partial, 'silent' | 'reported']>; - // Deleting a row would silently shrink this matrix, so its size is pinned. test('the policy matrix covers every express/rollback/persisted combination', () => { expect(POLICY_MATRIX).toHaveLength(12); }); - /** - * `DisableRollback` on `ExecuteChangeSet` is a consistency assertion, not an override, so a value conflicting with the - * change set's persisted policy fails the call outright with `ValidationError: DisableRollback specified on - * ExecuteChangeSet conflicts with the value DisableRollback the ChangeSet was created with.` - * - * `commonExecuteOptions()` sends `DisableRollback: true` for an explicit `--no-rollback`, so executing a change set - * created with rollback ENABLED under `--no-rollback` would send exactly that conflicting value. The flag cannot move - * an already-pinned policy, so it must not be sent at all. - */ test('no DisableRollback is sent when the change set already pins the policy', async () => { - // GIVEN - persisted policy says rollback ENABLED, the request asks for it DISABLED + // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: false, changes: [updateChange()] }); @@ -997,17 +852,13 @@ describe('executing a change set created by an earlier invocation', () => { rollback: false, }); - // THEN - executed rather than failing with ValidationError, and the flag was withheld + // THEN expect(result.type).toEqual('did-deploy-stack'); expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); const sent = mockCloudFormationClient.commandCalls(ExecuteChangeSetCommand)[0].args[0].input; expect(sent).not.toHaveProperty('DisableRollback'); }); - /** - * The mirror of the above: a standard change set records no policy, so there the execute-time flag is the only thing - * that decides and must still be sent. - */ test('DisableRollback is still sent when the change set records no policy', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); @@ -1030,7 +881,7 @@ describe('executing a change set created by an earlier invocation', () => { 'persisted rollbackDisabled=%s executed with %j', (persistedRollbackDisabled, flags, expected) => { test(`is ${expected}`, async () => { - // GIVEN - a non-replacing change, so nothing is gated and only the policy comparison is under test + // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenExpressChangeSetExists({ rollbackDisabled: persistedRollbackDisabled, changes: [updateChange()] }); @@ -1041,8 +892,7 @@ describe('executing a change set created by an earlier invocation', () => { ...flags, }); - // THEN - without a replacement CloudFormation accepts the execution either way, so it always runs. A policy - // that disagrees with the flags means the flags are not being honoured, which is reported but not refused. + // THEN expect(result.type).toEqual('did-deploy-stack'); expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); @@ -1054,18 +904,12 @@ describe('executing a change set created by an earlier invocation', () => { }, ); - /** - * Only Express records a rollback choice on the change set. A STANDARD change set is governed by the `DisableRollback` - * flag sent at execute time, and that flag is only sent for an explicit `--no-rollback` - so `--express` arriving at - * execute time cannot disable rollback on it, and must not make the replacement guard think it did. Without this, - * plumbing `express` through to the execute path turns `prepare` (standard) + `execute --express` into a false refusal. - */ test('--express does not imply rollback-disabled for a change set that is not Express', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); givenChangeSetExists({ deploymentConfig: { Mode: 'STANDARD' }, changes: [policyActionReplacementChange()] }); - // WHEN - rollback is not disabled server-side here, so the replacement is deployable + // WHEN const result = await testDeployStack({ ...standardDeployStackArguments(), ...executePrepared, @@ -1078,9 +922,6 @@ describe('executing a change set created by an earlier invocation', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); - // `isRollbackable` also covers CREATE_FAILED, where there is no previously deployed configuration to replay. Telling - // such a user to "revert your change and redeploy the last configuration that deployed successfully" would be - // impossible advice, so that state gets its own guidance. test('a failed initial create is not told to replay a configuration that never existed', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.CREATE_FAILED }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts index 44c52c90d..4a1560085 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts @@ -1603,21 +1603,7 @@ test.each([ // CloudFormation's RollbackStack API is not supported for stacks last deployed // with express mode, so `cdk deploy --express` (without an explicit `--rollback`) -// must never route through the rollback path. -// -// It must, however, still refuse to submit a replacement while rollback is disabled: -// CloudFormation rejects those and leaves the stack in UPDATE_FAILED, from which an -// express stack cannot be rolled back. `--express --rollback` re-enables rollback -// server-side (DisableRollback: false) and is the route users are sent to. -// -// See aws/aws-cdk-cli#1931. The `express, no explicit rollback` expectation below was -// changed from 'replacement-requires-rollback' to 'did-deploy-stack' by #1745, which is -// why the regression shipped unnoticed; #1785 then restructured the guard it removed. -// -// The result type alone is the surface #1745 edited, so it is not pinned on its own here: each row also asserts whether -// anything was actually submitted. Flipping an expectation now requires flipping a claim about CloudFormation calls too. test.each([ - // --express alone (rollback disabled server-side): a replacement must not be submitted ['express, no explicit rollback', { express: true } as Partial, 'replacement-requires-rollback'], // --express --rollback: rollback is explicitly enabled, so the replacement deploys directly ['express with rollback=true', { express: true, rollback: true } as Partial, 'did-deploy-stack'], @@ -1652,13 +1638,6 @@ test.each([ // A stack last deployed with express mode that is in a paused fail state // (UPDATE_FAILED) cannot be recovered via the RollbackStack API. `cdk deploy -// --express` must therefore never return 'failpaused-need-rollback-first'; it -// fix-forwards via createChangeSet/UpdateStack instead. -// -// The replacement case is deliberately absent here: it does not return a result at -// all, it throws, because deploying with rollback enabled cannot update an -// already-failed stack either. It is covered by -// deploy-stack-express-replacement.test.ts. See aws/aws-cdk-cli#1931. test.each([ ['express, no explicit rollback, no-replacement', { express: true } as Partial, 'no-replacement', 'did-deploy-stack'], ['express with rollback=true, no-replacement', { express: true, rollback: true } as Partial, 'no-replacement', 'did-deploy-stack'], diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index bdbc0f6fc..946f76937 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -491,10 +491,6 @@ export class CdkToolkit { roleArn: options.roleArn, forceDeployment: options.force, rollback: options.rollback, - // Must travel with `rollback`: together they decide the rollback policy this invocation is asking for, and - // Express Mode flips what a missing `rollback` means (disabled, rather than standard mode's enabled). Omitting - // it made toolkit-lib read plain `--express` as "rollback enabled" and refuse Express change sets this same CLI - // had just created with rollback disabled. express: options.express, reuseAssets: options.reuseAssets, concurrency: options.concurrency, @@ -2570,9 +2566,6 @@ class WorkGraphDeploymentActions implements WorkGraphActions { } case 'replacement-requires-rollback': { - // Express Mode disables rollback by DEFAULT, so naming `--no-rollback` here would blame a flag the user - // never passed. This path and `toolkit.ts` must stay worded the same: both are reachable from `cdk deploy` - // depending on `--method`. const motivation = this.options.express ? 'Change includes a replacement, which CloudFormation does not support while rollback is disabled (the default for Express Mode)' : 'Change includes a replacement which cannot be deployed with "--no-rollback"'; diff --git a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts index f176bee27..b9ae13e69 100644 --- a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts @@ -867,9 +867,6 @@ describe('deploy', () => { expect(mockCfnDeployments.prepareStack).not.toHaveBeenCalled(); }); - // `--express` decides what a missing `--rollback` means: Express Mode disables rollback by default, standard mode - // enables it. Dropping the flag here made toolkit-lib read plain `--express` as "rollback enabled", so the - // change-set policy guard refused Express change sets that this same CLI had just created. test.each([ ['plain --express', { express: true }, { express: true, rollback: undefined }], ['--express --rollback', { express: true, rollback: true }, { express: true, rollback: true }], From 2ca347e386ac74aefbd4621503575f279f75b714 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 05:48:17 +0000 Subject: [PATCH 13/14] fix(toolkit-lib): fail closed when the nested change set hierarchy cannot be fully inspected An uninspectable child was treated as proof that no replacement exists, which is the same failure shape as the original bug: the guard disappeared exactly when it could not see. Three paths ended in 'no replacement found, proceed' - a missing child ChangeSetId, a DescribeChangeSet failure (permissions or throttling), and a child change set in a non-CREATE_COMPLETE status whose absent Changes read as empty. The depth cap did the same at the boundary. findAllReplacements now returns a ReplacementScan that separates 'traversal was complete' from 'no replacements found'. When rollback will be disabled and any part of the hierarchy could not be read, the deployment is refused with NestedChangeSetInspectionIncomplete naming what could not be inspected. Nothing changes for rollback-enabled deployments, where a replacement is allowed anyway. Action: 'Remove' is exempt, verified against CloudFormation in us-east-1: a removed nested stack appears in the root Changes with ChangeSetId absent, so failing closed on it would block every express deploy that deletes a nested stack. Modify and Add both carry a ChangeSetId, and an unchanged nested stack does not appear in Changes at all. Child change sets are CREATE_COMPLETE but ExecutionStatus UNAVAILABLE ('Only executable from the root change set'), so the status gate reads Status, not ExecutionStatus. --- .../lib/api/deployments/deploy-stack.ts | 89 +++++-- .../deploy-stack-express-replacement.test.ts | 247 ++++++++++++++++++ 2 files changed, 315 insertions(+), 21 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index 4bcf2a06a..f634e79f7 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -573,53 +573,68 @@ class FullCloudFormationDeployment { }; } - private async findAllReplacements(changeSet: DescribeChangeSetCommandOutput): Promise { + private async findAllReplacements(changeSet: DescribeChangeSetCommandOutput): Promise { const visited = new Set(); + const uninspected: string[] = []; const collect = async (current: DescribeChangeSetCommandOutput, depth: number): Promise => { const replacements = findReplacements(current); - if (depth >= MAX_NESTED_CHANGE_SET_DEPTH) { - await this.ioHelper.defaults.debug(format( - 'Stopped looking for nested replacements at depth %d; deeper nested stacks were not inspected.', + const nestedStackChanges = (current.Changes ?? []) + .map((change) => change.ResourceChange) + .filter((nested) => nested?.ResourceType === 'AWS::CloudFormation::Stack') + .filter((nested) => nested!.Action !== 'Remove'); + + if (nestedStackChanges.length > 0 && depth >= MAX_NESTED_CHANGE_SET_DEPTH) { + uninspected.push(format( + 'nested stacks below depth %d (%s) were not inspected', depth, + nestedStackChanges.map((nested) => nested!.LogicalResourceId ?? '').join(', '), )); return replacements; } - for (const change of current.Changes ?? []) { - const nested = change.ResourceChange; - if (nested?.ResourceType !== 'AWS::CloudFormation::Stack' || !nested.ChangeSetId) { + for (const nested of nestedStackChanges) { + const logicalId = nested!.LogicalResourceId ?? ''; + + if (!nested!.ChangeSetId) { + uninspected.push(format('nested stack %s reported no change set to inspect', logicalId)); continue; } - if (visited.has(nested.ChangeSetId)) { + if (visited.has(nested!.ChangeSetId)) { continue; } - visited.add(nested.ChangeSetId); + visited.add(nested!.ChangeSetId); + let child: DescribeChangeSetCommandOutput; try { - const child = await new ChangeSetDescriber({ + child = await new ChangeSetDescriber({ cfn: this.cfn, ioHelper: this.ioHelper, - stackNameOrArn: nested.PhysicalResourceId ?? nested.LogicalResourceId ?? this.stackName, - changeSetNameOrArn: nested.ChangeSetId, + stackNameOrArn: nested!.PhysicalResourceId ?? logicalId, + changeSetNameOrArn: nested!.ChangeSetId, }).waitForSettled(); - - replacements.push(...await collect(child, depth + 1)); } catch (e: any) { - await this.ioHelper.defaults.debug(format( - 'Could not describe nested change set %s for %s, so it was not inspected for replacements: %s', - nested.ChangeSetId, - nested.LogicalResourceId, - formatErrorMessage(e), + uninspected.push(format('nested stack %s could not be described (%s)', logicalId, formatErrorMessage(e))); + continue; + } + + if (child.Status !== 'CREATE_COMPLETE') { + uninspected.push(format( + 'nested stack %s has change set status %s, so its changes could not be read', + logicalId, + child.Status ?? '', )); + continue; } + + replacements.push(...await collect(child, depth + 1)); } return replacements; }; - return collect(changeSet, 0); + return { replacements: await collect(changeSet, 0), uninspected }; } private rollbackWillBeDisabled(changeSet: DescribeChangeSetCommandOutput, persistedRollbackDisabled: boolean | undefined): boolean { @@ -643,7 +658,8 @@ class FullCloudFormationDeployment { const persistedRollbackDisabled = expressRollbackDisabled(changeSet.DeploymentConfig); const requestedRollbackDisabled = this.rollbackDisabled(); - const replacements = await this.findAllReplacements(changeSet); + const scan = await this.findAllReplacements(changeSet); + const replacements = scan.replacements; const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; @@ -687,6 +703,13 @@ class FullCloudFormationDeployment { return { type: 'replacement-requires-rollback' }; } + if (rollbackWillBeDisabled && scan.uninspected.length > 0) { + throw new ToolkitError( + 'NestedChangeSetInspectionIncomplete', + nestedInspectionIncompleteMessage(scan.uninspected), + ); + } + if (persistedRollbackDisabled !== undefined && persistedRollbackDisabled !== requestedRollbackDisabled) { await this.ioHelper.defaults.warn( changeSetPolicyMismatchMessage(changeSet.ChangeSetName, persistedRollbackDisabled), @@ -1257,6 +1280,30 @@ function changeSetPolicyMismatchMessage(changeSetName: string | undefined, persi type ReplacementRecovery = 'none' | 'replay' | 'recreate' | 'resolve-state'; +/** + * The outcome of scanning a change set hierarchy for replacements. + * + * `uninspected` is non-empty when part of the hierarchy could not be read, in which case an empty `replacements` + * does NOT mean there are none. + */ +interface ReplacementScan { + readonly replacements: ReplacedResource[]; + readonly uninspected: string[]; +} + +function nestedInspectionIncompleteMessage(uninspected: string[]): string { + const withRollback = chalk.blue('cdk deploy --express --rollback'); + + return [ + 'Rollback is disabled for this deployment, so a replacement would be rejected mid-execution and leave the stack', + 'in UPDATE_FAILED. Part of the nested stack hierarchy could not be inspected, so it cannot be confirmed that this', + 'deployment contains no replacement:', + ...uninspected.map((reason) => ` - ${reason}`), + '', + `Deploy with rollback enabled instead, which allows replacements: ${withRollback}`, + ].join('\n'); +} + const MAX_NESTED_CHANGE_SET_DEPTH = 10; /** diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 7601968fd..53a28c3a3 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -820,6 +820,253 @@ describe('executing a change set created by an earlier invocation', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); + function givenRootWithNestedStackChange(nested: Record) { + givenChangeSetExists({ + deploymentConfig: { Mode: 'EXPRESS' }, + changes: [{ + Type: 'Resource', + ResourceChange: { + LogicalResourceId: 'NestedChild', + ResourceType: 'AWS::CloudFormation::Stack', + ...nested, + }, + }], + }); + } + + function givenNestedChain(levels: number, deepestChanges: Change[]) { + const deploymentConfig: DeploymentConfig = { Mode: 'EXPRESS' }; + let child: { id: string; stackName: string } | undefined; + + for (let i = levels; i >= 1; i--) { + const stackName = `withouterrors-Nested${i}`; + fakeCfn.createStackSync({ StackName: stackName, StackStatus: StackStatus.UPDATE_COMPLETE }); + const changes: Change[] = child + ? [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: `Nested${i + 1}`, + PhysicalResourceId: child.stackName, + ResourceType: 'AWS::CloudFormation::Stack', + Replacement: 'False', + ChangeSetId: child.id, + }, + }] + : deepestChanges; + + const cs = fakeCfn.createChangeSetSync({ + StackName: stackName, + ChangeSetName: `prepared-nested-${i}`, + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + Changes: changes, + DeploymentConfig: deploymentConfig, + }); + child = { id: cs.Id!, stackName }; + } + + givenRootWithNestedStackChange({ + Action: 'Modify', + LogicalResourceId: 'Nested1', + PhysicalResourceId: child!.stackName, + Replacement: 'False', + ChangeSetId: child!.id, + }); + } + + async function expectBlockedAsIncomplete(deployment: Promise) { + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'NestedChangeSetInspectionIncomplete' })); + expectNoStackMutation(); + } + + test('a nested stack change carrying no child change set blocks instead of assuming no replacement', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenRootWithNestedStackChange({ Action: 'Modify', PhysicalResourceId: 'some-child', Replacement: 'False' }); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expectBlockedAsIncomplete(deployment); + }); + + test('a REMOVED nested stack legitimately has no child change set and does not block', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenRootWithNestedStackChange({ Action: 'Remove', PhysicalResourceId: 'some-child' }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + }); + + test('a malformed or not-found child change set blocks instead of assuming no replacement', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenRootWithNestedStackChange({ + Action: 'Modify', + PhysicalResourceId: 'some-child', + Replacement: 'False', + ChangeSetId: 'malformed-or-not-found', + }); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expectBlockedAsIncomplete(deployment); + }); + + test('a child DescribeChangeSet failure blocks instead of assuming no replacement', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + const { childChangeSetId } = givenNestedChangeSetExists({ + rollbackDisabled: true, + childChanges: [policyActionReplacementChange()], + }); + mockCloudFormationClient + .on(DescribeChangeSetCommand, { ChangeSetName: childChangeSetId }) + .rejects(new Error('Rate exceeded')); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expectBlockedAsIncomplete(deployment); + }); + + test('a child change set in CREATE_FAILED blocks instead of reading its absent changes as empty', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + const childStackName = 'withouterrors-NestedChild-FAILED'; + fakeCfn.createStackSync({ StackName: childStackName, StackStatus: StackStatus.UPDATE_COMPLETE }); + const child = fakeCfn.createChangeSetSync({ + StackName: childStackName, + ChangeSetName: 'prepared-nested-failed', + Status: 'CREATE_FAILED', + StatusReason: 'Insufficient permissions to describe the nested template', + ExecutionStatus: 'UNAVAILABLE', + DeploymentConfig: { Mode: 'EXPRESS' }, + }); + givenRootWithNestedStackChange({ + Action: 'Modify', + PhysicalResourceId: childStackName, + Replacement: 'False', + ChangeSetId: child.Id, + }); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expectBlockedAsIncomplete(deployment); + }); + + test('a hierarchy deeper than the traversal cap blocks instead of skipping the unread levels', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + givenNestedChain(11, [policyActionReplacementChange()]); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expectBlockedAsIncomplete(deployment); + }); + + test('a cycle terminates and still reports the replacement it found', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + const childStackName = 'withouterrors-NestedCycle'; + fakeCfn.createStackSync({ StackName: childStackName, StackStatus: StackStatus.UPDATE_COMPLETE }); + const root = fakeCfn.createChangeSetSync({ + StackName: 'withouterrors', + ChangeSetName: 'prepared', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + DeploymentConfig: { Mode: 'EXPRESS' }, + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'NestedCycle', + PhysicalResourceId: childStackName, + ResourceType: 'AWS::CloudFormation::Stack', + Replacement: 'False', + ChangeSetId: 'cycle-child', + }, + }], + }); + fakeCfn.createChangeSetSync({ + StackName: childStackName, + ChangeSetName: 'cycle-child', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + DeploymentConfig: { Mode: 'EXPRESS' }, + Changes: [ + policyActionReplacementChange(), + { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'BackToRoot', + PhysicalResourceId: 'withouterrors', + ResourceType: 'AWS::CloudFormation::Stack', + Replacement: 'False', + ChangeSetId: root.Id, + }, + }, + ], + }); + failOnAnyStackMutation(); + + // WHEN + const deployment = testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'ReplacementRequiresRecreateChangeSet' })); + expectNoStackMutation(); + ioHost.expectMessage({ level: 'warn', code: W5903, containing: 'does not support while rollback is disabled' }); + }); + const POLICY_MATRIX = [ [true, { express: true, rollback: true }, 'reported'], [true, { express: true }, 'silent'], From aa9a81fb917684a0f05c2fc27ebcb4d09da3dbc4 Mon Sep 17 00:00:00 2001 From: sanjrkmr Date: Tue, 29 Sep 2026 06:46:30 +0000 Subject: [PATCH 14/14] test(toolkit-lib): pin the rollback-enabled exemption and the cycle guard Four test-side gaps in the nested fail-closed work: - Nothing pinned the rollback-ENABLED exemption. All five fail-closed tests drove the rollback-disabled side, so collapsing the gate to 'if (uninspected) throw' would keep CI green while every express deploy with rollback on and an unreadable child started refusing a legal update. Added a test.each over all four uninspected reasons with rollback enabled, asserting did-deploy-stack and that ExecuteChangeSet was called. - The happy-path fixtures seeded ExecutionStatus AVAILABLE on child change sets. Real children are CREATE_COMPLETE + UNAVAILABLE ('Only executable from the root change set'), so a gate additionally requiring AVAILABLE - which would break every nested deploy - passed. Child fixtures now seed UNAVAILABLE. - The cycle test did not pin the visited set: the depth cap terminates and the replacement is found on the first visit, so it passes without visited. Retitled it to what it checks, and added a cycle with NO replacement, which only visited can stop before the depth cap. - expectBlockedAsIncomplete only asserted the error name, so all four uninspected reasons could collapse into one message. Each test now pins a distinguishing substring of its own reason. --- .../deploy-stack-express-replacement.test.ts | 164 ++++++++++++++---- 1 file changed, 134 insertions(+), 30 deletions(-) diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts index 53a28c3a3..54c4508aa 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack-express-replacement.test.ts @@ -760,7 +760,7 @@ describe('executing a change set created by an earlier invocation', () => { StackName: childStackName, ChangeSetName: 'prepared-nested-child', Status: 'CREATE_COMPLETE', - ExecutionStatus: 'AVAILABLE', + ExecutionStatus: 'UNAVAILABLE', Changes: opts.childChanges, DeploymentConfig: deploymentConfig, }); @@ -820,9 +820,11 @@ describe('executing a change set created by an earlier invocation', () => { expect(ioHost.messagesWithCode(W5903)).toEqual([]); }); - function givenRootWithNestedStackChange(nested: Record) { + function givenRootWithNestedStackChange(nested: Record, opts: { rollbackDisabled?: boolean } = {}) { givenChangeSetExists({ - deploymentConfig: { Mode: 'EXPRESS' }, + deploymentConfig: opts.rollbackDisabled === false + ? { Mode: 'EXPRESS', DisableRollback: false } + : { Mode: 'EXPRESS' }, changes: [{ Type: 'Resource', ResourceChange: { @@ -834,7 +836,7 @@ describe('executing a change set created by an earlier invocation', () => { }); } - function givenNestedChain(levels: number, deepestChanges: Change[]) { + function givenNestedChain(levels: number, deepestChanges: Change[], opts: { rollbackDisabled?: boolean } = {}) { const deploymentConfig: DeploymentConfig = { Mode: 'EXPRESS' }; let child: { id: string; stackName: string } | undefined; @@ -859,7 +861,7 @@ describe('executing a change set created by an earlier invocation', () => { StackName: stackName, ChangeSetName: `prepared-nested-${i}`, Status: 'CREATE_COMPLETE', - ExecutionStatus: 'AVAILABLE', + ExecutionStatus: 'UNAVAILABLE', Changes: changes, DeploymentConfig: deploymentConfig, }); @@ -872,11 +874,33 @@ describe('executing a change set created by an earlier invocation', () => { PhysicalResourceId: child!.stackName, Replacement: 'False', ChangeSetId: child!.id, + }, opts); + } + + function givenFailedChildChangeSet(opts: { rollbackDisabled?: boolean } = {}) { + const childStackName = 'withouterrors-NestedChild-FAILED'; + fakeCfn.createStackSync({ StackName: childStackName, StackStatus: StackStatus.UPDATE_COMPLETE }); + const child = fakeCfn.createChangeSetSync({ + StackName: childStackName, + ChangeSetName: 'prepared-nested-failed', + Status: 'CREATE_FAILED', + StatusReason: 'Insufficient permissions to describe the nested template', + ExecutionStatus: 'UNAVAILABLE', + DeploymentConfig: { Mode: 'EXPRESS' }, }); + givenRootWithNestedStackChange({ + Action: 'Modify', + PhysicalResourceId: childStackName, + Replacement: 'False', + ChangeSetId: child.Id, + }, opts); } - async function expectBlockedAsIncomplete(deployment: Promise) { - await expect(deployment).rejects.toThrow(expect.objectContaining({ name: 'NestedChangeSetInspectionIncomplete' })); + async function expectBlockedAsIncomplete(deployment: Promise, containing: string) { + await expect(deployment).rejects.toThrow(expect.objectContaining({ + name: 'NestedChangeSetInspectionIncomplete', + message: expect.stringContaining(containing), + })); expectNoStackMutation(); } @@ -894,7 +918,7 @@ describe('executing a change set created by an earlier invocation', () => { }); // THEN - await expectBlockedAsIncomplete(deployment); + await expectBlockedAsIncomplete(deployment, 'reported no change set to inspect'); }); test('a REMOVED nested stack legitimately has no child change set and does not block', async () => { @@ -933,7 +957,7 @@ describe('executing a change set created by an earlier invocation', () => { }); // THEN - await expectBlockedAsIncomplete(deployment); + await expectBlockedAsIncomplete(deployment, 'could not be described'); }); test('a child DescribeChangeSet failure blocks instead of assuming no replacement', async () => { @@ -956,28 +980,13 @@ describe('executing a change set created by an earlier invocation', () => { }); // THEN - await expectBlockedAsIncomplete(deployment); + await expectBlockedAsIncomplete(deployment, 'Rate exceeded'); }); test('a child change set in CREATE_FAILED blocks instead of reading its absent changes as empty', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); - const childStackName = 'withouterrors-NestedChild-FAILED'; - fakeCfn.createStackSync({ StackName: childStackName, StackStatus: StackStatus.UPDATE_COMPLETE }); - const child = fakeCfn.createChangeSetSync({ - StackName: childStackName, - ChangeSetName: 'prepared-nested-failed', - Status: 'CREATE_FAILED', - StatusReason: 'Insufficient permissions to describe the nested template', - ExecutionStatus: 'UNAVAILABLE', - DeploymentConfig: { Mode: 'EXPRESS' }, - }); - givenRootWithNestedStackChange({ - Action: 'Modify', - PhysicalResourceId: childStackName, - Replacement: 'False', - ChangeSetId: child.Id, - }); + givenFailedChildChangeSet(); failOnAnyStackMutation(); // WHEN @@ -988,7 +997,7 @@ describe('executing a change set created by an earlier invocation', () => { }); // THEN - await expectBlockedAsIncomplete(deployment); + await expectBlockedAsIncomplete(deployment, 'has change set status CREATE_FAILED'); }); test('a hierarchy deeper than the traversal cap blocks instead of skipping the unread levels', async () => { @@ -1005,10 +1014,105 @@ describe('executing a change set created by an earlier invocation', () => { }); // THEN - await expectBlockedAsIncomplete(deployment); + await expectBlockedAsIncomplete(deployment, 'were not inspected'); + }); + + test.each([ + ['a missing child change set', () => givenRootWithNestedStackChange( + { Action: 'Modify', PhysicalResourceId: 'some-child', Replacement: 'False' }, + { rollbackDisabled: false }, + )], + ['a child that cannot be described', () => { + const { childChangeSetId } = givenNestedChangeSetExists({ + rollbackDisabled: false, + childChanges: [updateChange()], + }); + mockCloudFormationClient + .on(DescribeChangeSetCommand, { ChangeSetName: childChangeSetId }) + .rejects(new Error('Rate exceeded')); + }], + ['a child change set that is not CREATE_COMPLETE', () => givenFailedChildChangeSet({ rollbackDisabled: false })], + ['a hierarchy deeper than the traversal cap', () => givenNestedChain( + 11, + [policyActionReplacementChange()], + { rollbackDisabled: false }, + )], + ])('rollback enabled deploys normally despite %s', async (_name, setup) => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + setup(); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + rollback: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + }); + + test('a cycle with no replacement is cut short by the visited set rather than walked to the depth cap', async () => { + // GIVEN + givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); + const childStackName = 'withouterrors-NestedCycleClean'; + fakeCfn.createStackSync({ StackName: childStackName, StackStatus: StackStatus.UPDATE_COMPLETE }); + const root = fakeCfn.createChangeSetSync({ + StackName: 'withouterrors', + ChangeSetName: 'prepared', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + DeploymentConfig: { Mode: 'EXPRESS' }, + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'NestedCycleClean', + PhysicalResourceId: childStackName, + ResourceType: 'AWS::CloudFormation::Stack', + Replacement: 'False', + ChangeSetId: 'cycle-child-clean', + }, + }], + }); + fakeCfn.createChangeSetSync({ + StackName: childStackName, + ChangeSetName: 'cycle-child-clean', + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'UNAVAILABLE', + DeploymentConfig: { Mode: 'EXPRESS' }, + Changes: [ + updateChange(), + { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'BackToRoot', + PhysicalResourceId: 'withouterrors', + ResourceType: 'AWS::CloudFormation::Stack', + Replacement: 'False', + ChangeSetId: root.Id, + }, + }, + ], + }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + ...executePrepared, + express: true, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); }); - test('a cycle terminates and still reports the replacement it found', async () => { + test('a replacement is still reported when it is reached through a cycle', async () => { // GIVEN givenStackExists({ StackStatus: StackStatus.UPDATE_COMPLETE }); const childStackName = 'withouterrors-NestedCycle'; @@ -1035,7 +1139,7 @@ describe('executing a change set created by an earlier invocation', () => { StackName: childStackName, ChangeSetName: 'cycle-child', Status: 'CREATE_COMPLETE', - ExecutionStatus: 'AVAILABLE', + ExecutionStatus: 'UNAVAILABLE', DeploymentConfig: { Mode: 'EXPRESS' }, Changes: [ policyActionReplacementChange(),