From 64d711bde65322eb862e983f0390966619cf5623 Mon Sep 17 00:00:00 2001 From: James Price Date: Wed, 9 Sep 2026 12:02:55 +0100 Subject: [PATCH] fix(cloudformation): report ChangeSetNotFound for an unknown change set `cdk deploy` and `cdk bootstrap` hung forever against a stack that already exists. Before creating its change set the CDK CLI deletes any leftover one with the same name, then polls `DescribeChangeSet` until the call reports `DELETE_COMPLETE`/`DELETE_FAILED` or raises `ChangeSetNotFoundException` (`waitForGone`). On a lookup miss `DescribeChangeSet` fabricated `{"Status": "CREATE_COMPLETE", "ExecutionStatus": "AVAILABLE"}`, so the change set was never gone and the poll never terminated: one `DeleteChangeSet` followed by `DescribeChangeSet` every five seconds, with `CreateChangeSet` never reached. `ChangeSetNotFoundException` is declared on `DescribeChangeSet`, `DescribeChangeSetHooks` and `ExecuteChangeSet`, with awsQueryError code `ChangeSetNotFound` and HTTP 404 (aws-models/cloudformation.json), so returning it costs no conformance. `DeleteChangeSet` does not declare it and still succeeds for a change set that was never created, matching AWS. All three reads had the same fabricated success with the lookup copy-pasted, `ExecuteChangeSet` explicitly so ("pass-through success rather than hard-fail"). They now share one `find_change_set` and map a miss to the error, which is also what CONTRIBUTING's no-stub-responses rule asks for. The unit coverage was asserting the stub: `change_sets` built a fresh service per call, so `DescribeChangeSet("cs")` only passed because the miss was faked. It now runs the lifecycle against one service. --- crates/fakecloud-cloudformation/src/extras.rs | 172 ++++++++++-------- .../cloudformation_execute_change_set.rs | 83 +++++++++ 2 files changed, 175 insertions(+), 80 deletions(-) diff --git a/crates/fakecloud-cloudformation/src/extras.rs b/crates/fakecloud-cloudformation/src/extras.rs index c214e8cb4..66c13788a 100644 --- a/crates/fakecloud-cloudformation/src/extras.rs +++ b/crates/fakecloud-cloudformation/src/extras.rs @@ -199,6 +199,17 @@ fn record_hook_results( } } +/// `ChangeSetNotFoundException` — declared on `DescribeChangeSet`, +/// `DescribeChangeSetHooks` and `ExecuteChangeSet`, awsQueryError code +/// `ChangeSetNotFound`, HTTP 404 (aws-models/cloudformation.json). +fn change_set_not_found(cs: &str) -> AwsServiceError { + AwsServiceError::aws_error( + StatusCode::NOT_FOUND, + "ChangeSetNotFound", + format!("ChangeSet [{cs}] does not exist"), + ) +} + fn missing(name: &str) -> AwsServiceError { AwsServiceError::aws_error( StatusCode::BAD_REQUEST, @@ -635,6 +646,32 @@ impl CloudFormationService { Some(exists) } + /// Look a change set up by name or ARN, optionally constrained to a stack. + /// + /// Every caller previously inlined this and then fabricated a + /// `CREATE_COMPLETE` stub on a miss. That made `cdk deploy`/`cdk bootstrap` + /// hang against an existing stack: the CLI deletes its change set and polls + /// `DescribeChangeSet` until it 404s (`waitForGone`), so a stubbed success + /// never terminates. Callers now map `None` to `change_set_not_found`. + fn find_change_set(&self, account_id: &str, cs: &str, stack: Option<&str>) -> Option { + let accounts = self.state.read(); + accounts + .get(account_id) + .and_then(|s| s.extras.get("change_sets")) + .and_then(|m| { + m.values() + .find(|v| { + let id_match = + v["Id"].as_str() == Some(cs) || v["ChangeSetName"].as_str() == Some(cs); + let stack_match = stack.is_none_or(|sf| { + v["StackName"].as_str() == Some(sf) || v["StackId"].as_str() == Some(sf) + }); + id_match && stack_match + }) + .cloned() + }) + } + pub(crate) fn handle_extra_action( &self, req: &AwsRequest, @@ -1016,20 +1053,9 @@ impl CloudFormationService { .ok_or_else(|| missing("ChangeSetName"))? .clone(); let stack_filter = params.get("StackName").cloned(); - let accounts = self.state.read(); - let entry = accounts.get(&aid) - .and_then(|s| s.extras.get("change_sets")) - .and_then(|m| m.values().find(|v| { - let id_match = v["Id"].as_str() == Some(&cs) - || v["ChangeSetName"].as_str() == Some(&cs); - let stack_match = stack_filter.as_deref().is_none_or(|sf| { - v["StackName"].as_str() == Some(sf) - || v["StackId"].as_str() == Some(sf) - }); - id_match && stack_match - })) - .cloned() - .unwrap_or_else(|| json!({"ChangeSetName": cs.clone(), "Status": "CREATE_COMPLETE", "ExecutionStatus": "AVAILABLE"})); + let entry = self + .find_change_set(&aid, &cs, stack_filter.as_deref()) + .ok_or_else(|| change_set_not_found(&cs))?; let changes_xml = entry["Changes"] .as_array() .map(|arr| { @@ -1094,41 +1120,16 @@ impl CloudFormationService { // Read the hooks snapshotted onto the change set at // CreateChangeSet time instead of always returning empty // (bug-audit 2026-06-13, 1.8). - let entry = { - let accounts = self.state.read(); - accounts - .get(&aid) - .and_then(|s| s.extras.get("change_sets")) - .and_then(|m| { - m.values() - .find(|v| { - let id_match = v["Id"].as_str() == Some(&cs) - || v["ChangeSetName"].as_str() == Some(&cs); - let stack_match = stack_filter.as_deref().is_none_or(|sf| { - v["StackName"].as_str() == Some(sf) - || v["StackId"].as_str() == Some(sf) - }); - id_match && stack_match - }) - .cloned() - }) - }; - let (cs_id, cs_name, stack_id, stack_name, hooks) = match &entry { - Some(e) => ( - e["Id"].as_str().unwrap_or("").to_string(), - e["ChangeSetName"].as_str().unwrap_or("").to_string(), - e["StackId"].as_str().unwrap_or("").to_string(), - e["StackName"].as_str().unwrap_or("").to_string(), - e["Hooks"].as_array().cloned().unwrap_or_default(), - ), - None => ( - cs.clone(), - cs.clone(), - String::new(), - String::new(), - Vec::new(), - ), - }; + let entry = self + .find_change_set(&aid, &cs, stack_filter.as_deref()) + .ok_or_else(|| change_set_not_found(&cs))?; + let (cs_id, cs_name, stack_id, stack_name, hooks) = ( + entry["Id"].as_str().unwrap_or("").to_string(), + entry["ChangeSetName"].as_str().unwrap_or("").to_string(), + entry["StackId"].as_str().unwrap_or("").to_string(), + entry["StackName"].as_str().unwrap_or("").to_string(), + entry["Hooks"].as_array().cloned().unwrap_or_default(), + ); let logical_filter = params.get("LogicalResourceId").cloned(); let hooks_xml = if hooks.is_empty() { " ".to_string() @@ -1181,31 +1182,9 @@ impl CloudFormationService { .ok_or_else(|| missing("ChangeSetName"))?; let stack_filter = params.get("StackName").cloned(); - let entry = { - let accounts = self.state.read(); - accounts - .get(&aid) - .and_then(|s| s.extras.get("change_sets")) - .and_then(|m| { - m.values() - .find(|v| { - let id_match = v["Id"].as_str() == Some(&cs) - || v["ChangeSetName"].as_str() == Some(&cs); - let stack_match = stack_filter.as_deref().is_none_or(|sf| { - v["StackName"].as_str() == Some(sf) - || v["StackId"].as_str() == Some(sf) - }); - id_match && stack_match - }) - .cloned() - }) - }; - let Some(entry) = entry else { - // Unknown change set: pass-through success rather than - // hard-fail to preserve route-coverage semantics for - // callers that don't first call CreateChangeSet. - return Ok(xml_response("ExecuteChangeSet", String::new(), &rid)); - }; + let entry = self + .find_change_set(&aid, &cs, stack_filter.as_deref()) + .ok_or_else(|| change_set_not_found(&cs))?; if entry["ExecutionStatus"].as_str() != Some("AVAILABLE") { return Err(AwsServiceError::aws_error( @@ -2628,6 +2607,7 @@ pub(crate) mod tests { use fakecloud_core::multi_account::MultiAccountState; use fakecloud_core::service::{AwsRequest, AwsResponse, AwsServiceError}; use http::Method; + use http::StatusCode; use parking_lot::RwLock; use std::collections::HashMap; use std::sync::Arc; @@ -3436,15 +3416,47 @@ pub(crate) mod tests { #[test] fn change_sets() { - ok( + // One service for the whole lifecycle: the reads only succeed because + // the change set really is there, not because a miss is stubbed out. + let svc = svc(); + ok_on( + &svc, "CreateChangeSet", &[("StackName", "s"), ("ChangeSetName", "cs")], ); - ok("DescribeChangeSet", &[("ChangeSetName", "cs")]); - ok("DescribeChangeSetHooks", &[("ChangeSetName", "cs")]); - ok("ListChangeSets", &[("StackName", "s")]); - ok("ExecuteChangeSet", &[("ChangeSetName", "cs")]); - ok("DeleteChangeSet", &[("ChangeSetName", "cs")]); + ok_on(&svc, "DescribeChangeSet", &[("ChangeSetName", "cs")]); + ok_on(&svc, "DescribeChangeSetHooks", &[("ChangeSetName", "cs")]); + ok_on(&svc, "ListChangeSets", &[("StackName", "s")]); + ok_on(&svc, "ExecuteChangeSet", &[("ChangeSetName", "cs")]); + // DeleteChangeSet declares no not-found error: AWS succeeds either way. + ok_on(&svc, "DeleteChangeSet", &[("ChangeSetName", "cs")]); + ok_on( + &svc, + "DeleteChangeSet", + &[("ChangeSetName", "never-existed")], + ); + } + + /// The reads must 404 once the change set is gone. CDK's `waitForGone` + /// polls `DescribeChangeSet` until it does, so a stubbed success hangs + /// `cdk deploy` and `cdk bootstrap` against an existing stack forever. + #[test] + fn change_set_reads_report_not_found() { + let svc = svc(); + for action in [ + "DescribeChangeSet", + "DescribeChangeSetHooks", + "ExecuteChangeSet", + ] { + let Err(err) = svc.handle_extra_action(&req(action, &[("ChangeSetName", "cs")])) else { + panic!("{action} on an unknown change set must fail"); + }; + let AwsServiceError::AwsError { status, code, .. } = &err else { + panic!("{action}: unexpected error {err:?}"); + }; + assert_eq!(*status, StatusCode::NOT_FOUND, "{action}"); + assert_eq!(code, "ChangeSetNotFound", "{action}"); + } } fn body_str(resp: &fakecloud_core::service::AwsResponse) -> String { diff --git a/crates/fakecloud-e2e/tests/cloudformation_execute_change_set.rs b/crates/fakecloud-e2e/tests/cloudformation_execute_change_set.rs index b888a40ec..f9d73d59a 100644 --- a/crates/fakecloud-e2e/tests/cloudformation_execute_change_set.rs +++ b/crates/fakecloud-e2e/tests/cloudformation_execute_change_set.rs @@ -605,3 +605,86 @@ async fn create_type_change_set_rejected_for_existing_stack() { "CREATE change set against an existing stack must be rejected, got {err:?}" ); } + +/// An unknown change set must report `ChangeSetNotFound`, not a fabricated +/// `CREATE_COMPLETE`. The CDK CLI deletes its change set and then polls +/// `DescribeChangeSet` until the call 404s (`waitForGone`), so a stubbed +/// success makes `cdk deploy`/`cdk bootstrap` against an existing stack hang +/// forever. +#[tokio::test] +async fn describe_change_set_reports_unknown_change_set_as_not_found() { + let server = TestServer::start().await; + let cf = server.cloudformation_client().await; + + let template = r#"{ + "Resources": { + "QueueGone": { + "Type": "AWS::SQS::Queue", + "Properties": {"QueueName": "cs-gone-queue"} + } + } + }"#; + + cf.create_stack() + .stack_name("cs-gone-stack") + .template_body(template) + .send() + .await + .unwrap(); + + // Never created. + let err = cf + .describe_change_set() + .stack_name("cs-gone-stack") + .change_set_name("never-created") + .send() + .await + .expect_err("DescribeChangeSet on an unknown change set must fail"); + assert!(err.into_service_error().is_change_set_not_found_exception()); + + // Created, then deleted: this is the exact sequence the CDK CLI runs. + cf.create_change_set() + .stack_name("cs-gone-stack") + .change_set_name("cs-transient") + .change_set_type(ChangeSetType::Update) + .template_body(template) + .send() + .await + .unwrap(); + + // DeleteChangeSet declares no not-found error: AWS succeeds either way. + cf.delete_change_set() + .stack_name("cs-gone-stack") + .change_set_name("cs-transient") + .send() + .await + .unwrap(); + + let err = cf + .describe_change_set() + .stack_name("cs-gone-stack") + .change_set_name("cs-transient") + .send() + .await + .expect_err("DescribeChangeSet after DeleteChangeSet must fail"); + assert!(err.into_service_error().is_change_set_not_found_exception()); + + // Same for the hooks view and for execution. + let err = cf + .describe_change_set_hooks() + .stack_name("cs-gone-stack") + .change_set_name("cs-transient") + .send() + .await + .expect_err("DescribeChangeSetHooks on an unknown change set must fail"); + assert!(err.into_service_error().is_change_set_not_found_exception()); + + let err = cf + .execute_change_set() + .stack_name("cs-gone-stack") + .change_set_name("cs-transient") + .send() + .await + .expect_err("ExecuteChangeSet on an unknown change set must fail"); + assert!(err.into_service_error().is_change_set_not_found_exception()); +}