fix(toolkit-lib): stale DescribeStacks read fails fresh stack creates, masked as NoStack - #1803
fix(toolkit-lib): stale DescribeStacks read fails fresh stack creates, masked as NoStack#1803svozza wants to merge 4 commits into
Conversation
…yment `DescribeStacks` is eventually consistent, so a poll issued after `ExecuteChangeSet` can still report the pre-execution `REVIEW_IN_PROGRESS` status. `stabilizeStack` treated that as a stable state and returned it, after which `waitForStackDeploy` rejected the deployment with `StackDeployFailed` even though CloudFormation went on to complete the create successfully. A stack cannot transition from an in-progress state back to `REVIEW_IN_PROGRESS`, so that status is a stale read whenever it is reported for a stack we know execution has been issued for. Keep polling for the real status in that case, identifying the stack by id: polling by name can otherwise observe a different stack that a concurrent operation created under the same name, whose review status is genuine. `monitorDeployment` passes the executing stack id in, so a stale first read is recognised without having to observe the operation in progress first. Reads are tolerated in bounded number so that a stack genuinely left in `REVIEW_IN_PROGRESS` still terminates the wait. `waitFor` has no timeout, and nothing moves an unexecuted ChangeSet on its own, so the pre-execution behaviour is still reached for the abandoned ChangeSet case it was written for. Relates to aws#1802
… to deploy `monitorDeployment` passed `finalState.wrapped` to the diagnoser from inside its catch block. `finalState` is still the pre-deploy lookup at that point, which holds no stack when the deployment was creating one from scratch, so the getter threw `NoStack`. Because that happened while evaluating an argument, it replaced the deployment error being reported. Every failed deployment of a new stack was affected, not just the ones caused by a stale stabilization read: a resource failure that rolled the stack back reported `NoStack` instead of naming the resource. Describe the stack that was actually deployed instead. The pre-deploy lookup is the wrong input even when it does hold a stack, because it describes a state the deployment has since left, and the diagnoser reads the status off it. Diagnosing is best-effort, so a failed lookup now leaves the original deployment error to propagate rather than replacing it with an `ErrorDiagnosisFailed` that says less. Relates to aws#1802
| ioHelper: IoHelper, | ||
| stackName: string, | ||
| stabilizationPollingInterval?: number, | ||
| executingStackId?: string, |
There was a problem hiding this comment.
Flagging a design choice for maintainer input.
This adds a fifth positional parameter, so waitForStackDeploy now reads (cfn, ioHelper, stackName, stabilizationPollingInterval, executingStackId), and stabilizeStack below takes the same shape. Two optional undefined-able tail parameters of different types are easy to transpose at a call site.
An options object would read better and scale if more parameters get added. I kept positional parameters to keep the diff minimal, and because each function has a single caller today — but I'm happy to switch in this PR if you'd prefer. Neither function is exported from lib/index.ts, so there's no API Extractor impact either way.
Happy to go whichever way you'd rather.
There was a problem hiding this comment.
See other comment, it's not needed because stabilizeStack already takes a stackNameOrArn. We can make the same change to waitForStackDeploy. The main todo then is to make sure that we use the display name for logs and errors.
There was a problem hiding this comment.
Will do — stackNameOrArn on waitForStackDeploy too. Noting the display-name point: the two DeploymentError messages interpolate the raw stackName, so they'd leak a full ARN; will run them through stackNameFromArn() as stabilizeStack already does. Follow-up PR.
There was a problem hiding this comment.
Answered by mrgrain'''s review — stabilizeStack already takes a stackNameOrArn, and a stack ARN is the stack id, so the extra parameter isn'''t needed at all. It'''s gone, along with the id-comparison logic it existed for.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1803 +/- ##
==========================================
- Coverage 90.32% 90.29% -0.04%
==========================================
Files 80 80
Lines 12124 12124
Branches 1716 1714 -2
==========================================
- Hits 10951 10947 -4
- Misses 1139 1143 +4
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ioHelper: IoHelper, | ||
| stackNameOrArn: string, | ||
| pollingInterval?: number, | ||
| executingStackId?: string, |
There was a problem hiding this comment.
This is not needed, stackNameOrArn can already take a Stack ARN as input and Stack Id is just the Stack ARN.
There was a problem hiding this comment.
Agreed, and verified — a stack ARN is the stack id, and CloudFormationStack.lookup passes it straight to DescribeStacks as StackName, so passing the ARN gives identity-pinning for free. A replacement stack created under the same name simply isn't found by the old ARN. Dropping executingStackId and the id comparison entirely. Moving to the follow-up PR with the rest of the stale-read fix.
| } | ||
| const status = stack.stackStatus; | ||
| if (status.isInProgress) { | ||
| inProgressStackId = stack.stackId; |
There was a problem hiding this comment.
This seems good as a general idea, but will need a code change:
- If the originally provided
stackNameOrArnwas NOT an ARN, then we "upgrade" the value to the ARN from the first check. - If it already was an ARN, we keep using it.
There was a problem hiding this comment.
Makes sense — that covers the callers passing a bare name (toolkit-info.ts:57, deployments.ts:622), so they get identity-pinning after their first successful read. Follow-up PR.
| // a stack genuinely left in review - which nothing will move on its own - still terminates the wait. | ||
| if (stack.stackId === inProgressStackId && staleReviewReads < STALE_REVIEW_READ_TOLERANCE) { | ||
| staleReviewReads++; | ||
| await ioHelper.defaults.debug(format('Stack %s reported REVIEW_IN_PROGRESS after being in progress; treating as a stale read (%s)', stackDisplayName, status)); |
There was a problem hiding this comment.
Can you phrase this a bit more concrete on what the next action is (which I guess is we try again a few more times)
There was a problem hiding this comment.
Will do. Follow-up PR.
| ioHelper: IoHelper, | ||
| stackName: string, | ||
| stabilizationPollingInterval?: number, | ||
| executingStackId?: string, |
There was a problem hiding this comment.
See other comment, it's not needed because stabilizeStack already takes a stackNameOrArn. We can make the same change to waitForStackDeploy. The main todo then is to make sure that we use the display name for logs and errors.
| * How many consecutive REVIEW_IN_PROGRESS reads to attribute to DescribeStacks eventual consistency | ||
| * before accepting the status as genuine. | ||
| */ | ||
| const STALE_REVIEW_READ_TOLERANCE = 5; |
There was a problem hiding this comment.
q: what is the upper limit for wall clock time impact here? How does this compare to CFNs published eventual consistency guarantees?
| let finalState = this.cloudFormationStack; | ||
| try { | ||
| const successStack = await waitForStackDeploy(this.cfn, this.ioHelper, this.stackName, this.options.stackEventPollingInterval); | ||
| const successStack = await waitForStackDeploy(this.cfn, this.ioHelper, this.stackName, this.options.stackEventPollingInterval, stackArn); |
There was a problem hiding this comment.
Probably just this...
| const successStack = await waitForStackDeploy(this.cfn, this.ioHelper, this.stackName, this.options.stackEventPollingInterval, stackArn); | |
| const successStack = await waitForStackDeploy(this.cfn, this.ioHelper, stackArn, this.options.stackEventPollingInterval); |
There was a problem hiding this comment.
Taking this suggestion. Depends on the two above. Follow-up PR.
| // `finalState` is still the pre-deploy lookup, which holds no stack at all when we were creating one from | ||
| // scratch, and describes a state the deployment has since left when it does. Describe the stack we actually | ||
| // deployed instead. Diagnosing is best-effort: if it cannot tell us anything, report the deployment error. | ||
| const deployedState = await CloudFormationStack.lookup(this.cfn, stackArn).catch(() => undefined); |
There was a problem hiding this comment.
but this can also fail... what then?
There was a problem hiding this comment.
Addressed in #1845. If the describe fails, diagnoseDeploymentFailure returns without throwing, so the original DeploymentError propagates — diagnosis can only ever replace it with something that says more. There's a test that fails the describe with a throttling error and asserts the real ROLLBACK_COMPLETE error survives.
The failure is also logged at debug now, matching stack-diagnoser.ts:163 — previously a throttled or access-denied diagnosis was invisible even under -v.
| if (deployedState?.exists) { | ||
| const diagnosis = await this.diagnoser.diagnoseFromErrorCollection(monitor.errors, deployedState.wrapped, true, { | ||
| rollbackEnabled: this.options.rollback !== false, | ||
| }); | ||
| diagnosis.throwOnError(); | ||
| } |
There was a problem hiding this comment.
this whole code block seems very convoluted to me and the new if is making it even worse. can you take a step back and look at what this is aiming to do? What errors can be throw and when and how do we react to it?
There was a problem hiding this comment.
Fair — took a step back on this in #1845.
The invariant the block was groping at: diagnosis is an enrichment of the deployment error, so it may only replace that error with something that says more. Enumerated what can be thrown:
| Path | Old | New |
|---|---|---|
fresh create, no stack in finalState |
wrapped threw NoStack, replacing the real error |
describes the deployed stack instead |
| describe fails while diagnosing | unreachable — threw before getting there | returns, original error propagates |
| diagnosis finds no cause | original rethrown (correct) | unchanged |
That's now one helper holding to the invariant, and the catch block is three lines.
Two things fell out of it. finalState's initialiser turned out to be provably dead once the catch stopped reading it, so the mutable local that meant two different things at two points in the method no longer starts out holding a pre-deploy value — which is what made the bug representable. And with the helper describing the stack directly rather than via CloudFormationStack.lookup(...).wrapped, the wrapped getter had no callers left anywhere in the repo, so it's gone.
|
Splitting this per @mrgrain's suggestion, error-masking fix first. #1845 — Stale I've replied on each thread above pointing to where it's handled. Will close this once #1845 is in, unless you'd rather I retarget it for the second half. Thanks for the review, the ARN point in particular made that fix materially smaller. |
… to deploy (aws#1845) Relates to aws#1802 First of two PRs, split per review feedback on aws#1803. This is the error-masking half; the stale `DescribeStacks` read follows separately. ## The bug `monitorDeployment` passed `finalState.wrapped` to the diagnoser from inside its catch block. `finalState` is still the *pre-deploy* lookup at that point, which holds no stack when the deployment was creating one from scratch — so the `wrapped` getter threw `NoStack`. Because that happened while evaluating an **argument**, it replaced the deployment error being reported: ``` NoStack: CloudFormationStack object does not hold a stack ``` Every failed deployment of a new stack was affected, not only the ones from the stale-read bug in aws#1802. A plain resource failure that rolled the stack back reported `NoStack` instead of naming the resource, and the diagnoser's CloudTrail enrichment was discarded. ## What changed Diagnosis is an *enrichment* of the deployment error — it can only ever replace that error with something that says more. The old code broke that in two ways, and the guard for a third was missing. It now lives in one helper that holds to the invariant: ```ts } catch (e: any) { // 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)) { await this.diagnoseDeploymentFailure(stackArn, monitor.errors); } throw e; } ``` - **Describe the stack that was actually deployed.** The pre-deploy description is absent when creating and stale when updating, and the diagnoser reads the status off it. - **A failed describe returns without throwing**, leaving the original error to propagate rather than replacing it with something less useful. This answers the *"but this can also fail... what then?"* comment. - **The lookup failure is logged at debug**, matching `stack-diagnoser.ts:163` — a throttled or access-denied diagnosis was previously invisible even under `-v`. Two consequences worth calling out, both of which shrink the surface rather than grow it: - `finalState`'s initialiser is now provably dead — the try block either assigns or throws, and the catch always rethrows — so the mutable local that meant two different things at two points in the method no longer starts life holding a pre-deploy value. That's what made this bug representable in the first place. - With the helper describing the stack directly rather than through `CloudFormationStack.lookup(...).wrapped`, the `wrapped` getter has **no callers left anywhere in the repo**, so it's removed. The class whose throwing getter caused this no longer has one. ## Testing New `deploy-stack-error-surfacing.test.ts`, 6 cases: a failing create via change-set and direct, each with rollback on and off; a failing update of an existing stack (guards against a fix that only works when the stack is missing); and a describe that fails mid-diagnosis, asserting the original `ROLLBACK_COMPLETE` error survives and the swallowed failure is still visible at debug level. Written before the fix — 5 of the 6 failed at `stack-helpers.ts:75` via the catch block, and the existing-stack case passed both before and after. Each subsequent cleanup was verified load-bearing by reverting it and confirming a specific test failed. Full `toolkit-lib` suite passes (1905 tests). No integ test: no new resource types or cross-service interactions. ## Follow-ups, deliberately not here - `waitForStackDeploy` already holds the settled `CloudFormationStack` from `stabilizeStack` and discards it when throwing, so this helper re-describes the same stack. Worth fixing — the CFN client is configured with 7 retries capped at 15s, so on a throttled account that redundant read can add up to ~60s per failed stack, on precisely the path where throttling is the likely cause. It needs a `cfn-api.ts` signature change, so it's out of scope for this PR. - Moving diagnosis into `CloudFormationStackDiagnoser` (so callers never hold a raw `Stack`) needs `Diagnosis.throwOnError()` split into `throwOnError`/`throwOnProblem` first. Otherwise a diagnoser-owned lookup failure surfaces as `ErrorDiagnosisFailed` and masks the real error — this same bug class through a different door. ### Checklist - [ ] This change contains a major version upgrade for a dependency and I confirm all breaking changes are addressed - Release notes for the new version: --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
|
Both halves are now filed, so closing this in favour of them:
Two things worth flagging from your review, @mrgrain: The ARN point was right and made the fix materially smaller — On the wall-clock question: rather than keep guessing at the retry constant, I ran the case against CloudFormation directly — a CREATE ChangeSet whose only resource fails immediately, executed, polling Thanks for the review — it caught two unbounded-polling bugs and a false-success on |
Pull request was closed
…yment (aws#1848) Fixes aws#1802 Second of two PRs, split per review feedback on aws#1803. aws#1845 fixed the error masking; this is the stale `DescribeStacks` read that caused the failure in the first place. ## The bug `stabilizeStack` polls `DescribeStacks` until the stack is no longer `*_IN_PROGRESS`, and treated `REVIEW_IN_PROGRESS` as a stable, terminal status. Because that read is eventually consistent, a poll issued after `ExecuteChangeSet` can still report the pre-execution status — so `waitForStackDeploy` rejected the deployment with `StackDeployFailed` while CloudFormation went on to reach `CREATE_COMPLETE`. `REVIEW_IN_PROGRESS` is ambiguous, and `stabilizeStack` cannot tell its two meanings apart from what it observes: - a `CREATE` ChangeSet was created and never executed — nothing will move the stack, so waiting hangs forever; - a ChangeSet *was* executed and this read is stale — treating it as terminal fails a running deployment. The caller knows which. `waitForStackDeploy` is reached immediately after `ExecuteChangeSet`/`CreateStack`; the delete, rollback and bootstrap-lookup callers have executed nothing. So that fact is now passed down rather than guessed at: ```ts const stack = await stabilizeStack(cfn, ioHelper, stackNameOrArn, { pollingInterval: stabilizationPollingInterval, changeSetExecuted: true, }); ``` ## What `REVIEW_IN_PROGRESS` actually does The first version of this bounded every stale read by a retry budget, which was a guess. To replace the guess I ran the case directly against CloudFormation — a `CREATE` ChangeSet whose only resource fails immediately (an SNS topic with an invalid name), executed, polling `DescribeStacks` at 0.5s: ``` 13:57:35 REVIEW_IN_PROGRESS User Initiated 13:58:00 CREATE_IN_PROGRESS User Initiated 13:58:02 Bad CREATE_IN_PROGRESS 13:58:03 Bad CREATE_FAILED Invalid parameter: Topic Name 13:58:03 ROLLBACK_IN_PROGRESS The following resource(s) failed to create: [Bad] 14:00:27 ROLLBACK_COMPLETE ``` `REVIEW_IN_PROGRESS` appears exactly once, as the stack's initial state, and is never re-entered — including on the path where execution fails before touching a single resource, which was the case I had assumed could return to it. That gives a rule rather than a guess: - **once the stack has been seen with an operation in progress**, a `REVIEW_IN_PROGRESS` read is *provably* stale, so keep polling with no budget at all; - **before that**, the two meanings are genuinely indistinguishable, so allow a bounded number of re-reads (`STALE_REVIEW_READ_ATTEMPTS`) and then believe the status, because nothing moves an unexecuted ChangeSet on its own. The distinction matters: a budget spent across the whole wait would exhaust itself on a long deployment with several scattered stale reads and reinstate the original bug. There is a test for that case. ## Also here **Polling is pinned to the stack's ARN** after the first successful read. Polling by name can otherwise observe a *different* stack that a concurrent operation created under the same name, and mistake it for the one being deployed. `monitorDeployment` and `rollbackStack` now pass the ARN they already have, per the review suggestion on aws#1803. **A stack deleted mid-wait is reported as gone.** This falls out of the ARN pinning: `DescribeStacks` keeps answering for a `DELETE_COMPLETE` stack when asked by ARN, where a by-name read stops finding it. Without this, `cdk rollback` returned `success: true` for a stack that had been deleted out from under it — `stabilizeStack` handed back the deleted stack as a stable status, skipping the `StackDisappeared` error, and a clean delete leaves no monitor errors to fail on. Caught by review and fixed with a test that reproduced the false success first. **Deployment errors name the stack** rather than echoing the caller's argument, which would now put a full ARN in front of the user. ## Testing `cfn-api-stabilization-polling-interval.test.ts` is renamed to `cfn-api-stabilization.test.ts` and the new cases join it, rather than adding a second file with a near-identical harness and a confusingly similar name. 12 tests, covering: a stale read mid-wait; more stale reads than the leading budget across a long deployment; a leading review status that never resolves; a stack deleted mid-wait; name-to-ARN narrowing; and the delete/rollback/bootstrap callers keeping today's behaviour. Every new test was confirmed to fail against the unfixed code and pass after; each guard was re-checked by reverting it and watching the specific test fail. Full `toolkit-lib` suite passes (1915 tests). `deploy-stack-error-surfacing.test.ts` needed one narrowing: its throttle mock keyed on "any ARN", which was a fine proxy when only the diagnosis read used one. Now that stabilization does too, it throttles reads after the deployment settles instead. ## Not here `deployments.ts` reads `cloudFormationStack.stackId` before checking `.exists`, and that getter throws when the stack is absent — a pre-existing latent crash on the rollback path, unrelated to this change. Happy to fix separately. ### Checklist - [ ] This change contains a major version upgrade for a dependency and I confirm all breaking changes are addressed - Release notes for the new version: --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
3 participants
Fixes #1802
Two independent bugs that combine to fail fresh stack creates under CloudFormation read pressure with
NoStack: CloudFormationStack object does not hold a stack. One commit each.1. A stale
DescribeStacksread failed a running deploymentDescribeStacksis eventually consistent, so a poll issued afterExecuteChangeSetcan still report the pre-executionREVIEW_IN_PROGRESSstatus.StackStatus.isInProgressexcludes review, so that read fell into theisReviewInProgresscarve-out instabilizeStack, which returned the stack as stable;waitForStackDeploythen rejected the deployment withStackDeployFailedwhile CloudFormation went on to reachCREATE_COMPLETE.A stack cannot transition from an in-progress state back to
REVIEW_IN_PROGRESS, so that status is a stale read whenever it is reported for a stack whose execution we know has been issued.stabilizeStacknow keeps polling in that case.Two details worth calling out:
stack A: UPDATE_IN_PROGRESS→ deleted →stack Bsame name:REVIEW_IN_PROGRESS). That review status is genuine, and treating it as stale would wait forever, sincewaitForhas no timeout. Comparing stack ids keeps the two cases apart.monitorDeploymentpasses the executing stack id in. Nothing guarantees the firstDescribeStacksafter execution observes the new status, so recognising a stale read cannot depend on having seen the operation in progress first.Stale reads are tolerated in bounded number (
STALE_REVIEW_READ_TOLERANCE), so a stack genuinely left inREVIEW_IN_PROGRESSstill terminates the wait and reaches the pre-execution behaviour the carve-out was written for — nothing moves an unexecuted ChangeSet on its own.2.
NoStackmasked the real error on any failed createmonitorDeploymentpassedfinalState.wrappedto the diagnoser from inside its catch block.finalStateis still the pre-deploy lookup there, which holds no stack when creating one from scratch, so the getter threwNoStack— and because that happened while evaluating an argument, it replaced the deployment error being reported.This affected every failed deployment of a new stack, not only those caused by bug 1: a resource failure that rolled the stack back reported
NoStackinstead of naming the resource.It now describes the stack that was actually deployed. The pre-deploy lookup is the wrong input even when it does hold a stack, since it describes a state the deployment has since left and the diagnoser reads the status off it. Diagnosing is best-effort, so a failed lookup leaves the original deployment error to propagate rather than replacing it with an
ErrorDiagnosisFailedthat says less.Testing
Unit tests only; no new AWS resource types or cross-service interactions, so no integ test.
cfn-api-stabilization.test.ts(new) — stale read mid-wait, stale read on the first poll, a different stack id treated as genuine, persistent review terminating rather than hanging, and the abandoned-ChangeSet escape hatch still failing fast.deploy-stack-error-surfacing.test.ts(new) — failing create via change-set and direct, with and without rollback, plus a failing update of an existing stack to guard against a fix that only works when the stack is missing.Every new test was confirmed to fail against the unfixed code and pass after. Full
toolkit-libsuite passes (1896 tests).deploy-stack-polling-interval.test.tsneeded one update: it assertswaitForStackDeploy's exact argument list, which the new parameter changes.How this was found
The e2e CI for Powertools for AWS Lambda (TypeScript) — ~40 parallel jobs deploying small stacks into one account/region — failed ~15% of matrix cells per run. CloudTrail across three failing stacks showed every
DescribeStacksreturning 200 with noerrorCode(a genuinely absent stack returnsValidationError) under heavyThrottlingExceptionon CFN reads, consistent with a stale replica read.Both fixes were validated there as a load-time monkey-patch before being written properly here —
patchWaitForStackDeployretried stabilization on the spurious error (bug 1), andpatchWrappedGettermadewrappednon-throwing (bug 2).Results: the matrix went 40/40 green against 6/40 failing on baseline, and in a later run 13 genuine fresh-create failures all surfaced their real
DeploymentErrors through the path that previously producedNoStack.The patch is a workaround rather than a model for this PR — it retries at the
waitForStackDeployboundary rather than fixing the stale-read classification insidestabilizeStack, and makingwrappedreturn{}hides a real invariant instead of not violating it. What it does establish is that the two behaviours being changed here are the ones responsible for the failures.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license