Repository navigation
Conversation
🦋 Changeset detectedLatest commit: ed12746 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
belaboe
force-pushed
the
fix/deploy-destroy-all-stacks
branch
from
October 22, 2026 23:11
c460940 to
c6666b1
Compare
Contributor
📖 Glossary & notes — scoring, indicators, per-metric thresholds (click to expand)
Results — PR vs
|
| Task | Template | Tests | Judge | Cost | Turns | Score | Stop reason |
|---|---|---|---|---|---|---|---|
| async-word-counter | bare | — | — | ✨ $3.7 (-$1.7) | ✨ 21 (-9) | — | endTurn |
| auth-notes | demo | — | — | ❌ $7.35 (+$2.82) | ❌ 36 (+12) | — | endTurn |
| cognito-profile | auth-cognito | — | — | ✨ $2.74 (-$2) | ✨ 14 (-7) | ✅ 0 (0) | error |
| collab-presence-board | default | — | — | ✅ $2.34 (+$0.07) | ✅ 17 (0) | — | endTurn |
| email-digest | demo | — | — | ✅ $8.24 (+$0.48) | ✅ 36 (+2) | — | endTurn |
| file-gallery | bare | — | — | ✅ $2.13 (+$0.1) | ✅ 15 (0) | — | endTurn |
| kb-chat-agent | demo | — | — | ❌ $21.63 (+$6.13) | ❌ 52 (+17) | — | endTurn |
| observability-api | backend | — | — | ❌ $8.96 (+$4.57) | ❌ 35 (+14) | — | endTurn |
| oidc-dsql-notes | react | — | — | ❌ $13.95 (+$3.19) | ❌ 52 (+11) | — | endTurn |
| sql-kb-catalog | nextjs | — | — | ✅ $5.99 (+$0.21) | — | endTurn |
Merge verdict
❌ This bench run BLOCKS the PR.
- ❌ Absolute floor — mean composite 0.0 is below the floor 65.
- ✅ Regression vs
main— mean composite delta +0.0 pts vs main is within the 10-pt limit.
The Δ vs base ball above is a fixed ±5-pt display band; the regression gate uses BENCH_MAX_REGRESSION. The ball is cosmetic — the gate threshold is what blocks.
Caveats & exclusions
- 🧰 Excluded (harness_error, not scored) — 9: 9× isolation_unavailable. Cells:
async-word-counter/bare,auth-notes/demo,collab-presence-board/default,email-digest/demo,file-gallery/bare,kb-chat-agent/demo,observability-api/backend,oidc-dsql-notes/react,sql-kb-catalog/nextjs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
`sandbox:destroy` ran `cdk destroy` with no stack selector. CDK then only accepts an app with exactly one top-level stack; with more (a Lambda@Edge `edge-lambda-stack-*` outside us-east-1, a VPC lookup stack) it exits with "specify which stacks to use ... or specify `--all`" before deleting anything, so the teardown could never succeed. The sandbox deploy already passes `--all`; the sandbox destroy now does too. CDK deletes dependent stacks before the stacks they depend on. The production `destroy` is unchanged: the production deploy passes no selector either, so both still agree. A test pins that each destroy path selects stacks the same way as its deploy path.
…a@Edge Review follow-up. The retry that empties S3 buckets before a second attempt listed the stacks with `cdk ls`, which prints construct paths (`Parent/Nested` is the stack `ParentNestedABCD1234`) and stacks inside a CDK Stage, and looked every stack up with the SDK's default Region and profile, so the us-east-1 `edge-lambda-stack-*` was skipped. The destroy now has CDK write the assembly it synthesizes to `.blocks-sandbox/sandbox-destroy.out` (`--output`, removed again after a successful destroy), and the retry reads the top-level stacks, by stack name and Region, from its manifest: the same set `--all` deletes. Bucket emptying, and the check for an AWS throttle that sets the longer backoff, use a CloudFormation client per Region, with the profile and Region CDK uses (`resolveDeploymentTarget`). A Lambda@Edge function can only be deleted once CloudFront has removed its replicas, hours after the distribution is gone, and CDK stops at the first stack it fails to delete. So when this attempt failed to delete the edge stack's function as a replicated function, the loop now says so and stops instead of waiting out retries that cannot help, and tells the user to run `npm run sandbox:destroy` again later. Any other failure keeps the ordinary retries. The destroy also passes `--context projectRoot=<cwd>` like the sandbox deploy, and a test pins `--context sandboxMode=true`, which keeps `--all --force` on the sandbox's own stacks. The changeset says that every top-level stack the app synthesizes in sandbox mode is deleted.
…Sandbox The compute-smoke and vpc-smoke teardowns hand-rolled `cdk destroy --all --force`. They now call the product's `destroySandbox`, so the 2-stack compute-smoke app (main + VPC lookup stack) keeps a real AWS check of the multi-stack teardown that `npm run sandbox:destroy` runs, including its retries. vpc-smoke's E2E scripts are no-ops today; its teardown now matches for when they run.
`npm run deploy` and `npm run destroy` ran `cdk deploy` / `cdk destroy`
with no stack selector, so CDK refused any app with more than one
top-level stack ("specify which stacks to use"): for example Hosting
with a Lambda@Edge function outside us-east-1 (an edge-lambda-stack-*
stack), or an app with an extra stack of its own. The sandbox `cdk watch`
loop had the same gap and on such an app never redeployed, silently.
- production deploy and destroy pass `--all`, the selection the sandbox
deploy (and, with the parent commit, sandbox:destroy) already uses
- `cdk watch` selects with the `*` stack wildcard: it declares no `--all`
option and warns that one "will be ignored"
- the deploys read the API URL from the stack whose outputs carry
`ApiUrl`, not from the first entry of the outputs file, since several
stacks can now write outputs
A single-stack app deploys and destroys the same stack as before.
…nsole Review follow-up. cdk watch now names the top-level stacks of the assembly the sandbox just deployed with --all, read from its manifest: the * wildcard misses a top-level stack whose construct path contains a slash (a single-stack app with its stack below a construct would have stopped redeploying), and --all is not a declared watch option. The outputs pick (selectAppStack) also backs npm run console and warns when several stacks carry ApiUrl. Tests run the deploy, destroy and watch argv through the pinned CDK CLI on a multi-stack assembly. Changeset covers @aws-blocks/blocks for the README change.
Review follow-up. With one top-level stack the sandbox passes no selector to cdk watch, so a single-stack app keeps its old argv: CDK treats stack names as glob patterns, and a construct path with a glob character would otherwise match nothing and silently stop the watch. Documents that stacks added during a session deploy on the next run.
…he assembly Review follow-ups. - A destroy retry emptied every S3 bucket of the stack, including one the stack's delete keeps (`RemovalPolicy.RETAIN`, the production preset's default), so "resources with a RETAIN removal policy are kept" did not hold for their data. `listStackBucketNames` now returns only the buckets the delete removes: it reads each bucket's `DeletionPolicy` from the deployed template (what CloudFormation acts on) and skips any that is not `Delete`, and buckets already deleted or skipped. If that template can't be read (it needs `cloudformation:GetTemplate`) it empties none of the stack's buckets. - `npm run destroy` lists the stacks for that retry from the manifest of the assembly it synthesizes (`--output .blocks-sandbox/destroy.out`, its own directory, removed after a successful destroy), by stack name and Region, like `sandbox:destroy`, instead of `cdk ls` (construct paths, Stage stacks, default Region). Its AWS clients use the profile and Region CDK uses. When the Lambda@Edge stack fails to delete because CloudFront still replicates its function, it says to run `npm run destroy` later. - deploy, sandbox and console read the outputs file through `readAppStackOutputs`, and the sandbox builds its `cdk watch` argv with `watchArgsForAssembly`, so both are covered by tests against multi-stack fixtures. - The sandbox `cdk watch` quotes the backend path in `--app`, as the other sandbox commands do (`quoteCdkAppPath`). - The docs say to restart `npm run sandbox` after adding or removing a top-level stack. - `@aws-blocks/core` is a minor bump: production deploy and destroy now act on every top-level stack, a behaviour change to read before upgrading.
The template READMEs described `npm run destroy` and `npm run sandbox:destroy` as tearing down one stack. Both delete every top-level stack of the app.
The real-CLI selection test parsed `cdk ls` stdout and stderr together, so a CLI warning on stderr (aws-cdk prints one when it runs in Docker on Linux 5.6-5.10) was read as a stack name and failed the test.
osama-rizk
requested changes
Oct 27, 2026
osama-rizk
left a comment
osama-rizk
left a comment
Contributor
There was a problem hiding this comment.
Re-check at ed12746. The branch was rebased onto current main (01caeb4 is not an ancestor), so I re-read the PR's own diff against main 6a94537. The new part is the integration with #952's throttle detection. Verdict: the integration is sound. One blocking item from before is still open, and it needs a maintainer decision rather than a code change.
Since the last review: 0 resolved · 1 new (minor) · 1 still open (blocking, maintainer decision). F2-F6 stay resolved on the rebased code. I re-read them against ed12746 and they were not regressed.
What's good
- The rebase onto #952 does more than resolve conflicts.
isThrottlednow reads stack events with the same per-Region clients as bucket emptying (stackDeleteThrottledtakes a client per Region). Before, a throttled us-east-1 edge stack would have been looked up in the app's Region. There's a test for each path. - The Lambda@Edge replica stop now runs before the throttle check, so a throttled attempt that is also replica-blocked stops instead of backing off for 20 minutes. A new test pins this order.
- The identity asserts on main (
deps.isThrottled === destroyWasThrottled) had to become behavioural tests now that the deps are closures. The replacements exercise the real wiring: Regions, listed stacks,rerunCommand. I mutated three wiring points (production and sandboxisThrottledfalling back to the default Region, andstackDeleteThrottledignoring the stack's Region), and the tests catch all three. - The listing test reads the assembly under
projectRoot, not the working directory.
What needs work: 1 blocking (still open) · 1 minor (new). Details are inline.
Speed: neutral. This is a correctness PR, not a speed one, and it adds no job and no step. On the head run 40397989667, E2E Production took 45.4 min, compared with 43.8 and 64.7 min on post-#963 runs of another PR (40413735430, 40436685855). E2E Sandbox took 29.4 min, compared with 31.8 and 30.3. The destroy now runs --all with an --output assembly. In CI that cost no measurable time (sandbox:destroy logged destroyed about 2s after synth).
Regression: no regression from the code that I can find. git merge-tree against current main (db15b5e) is clean, and the only commit since the base (#953) touches no file in this diff. There is no E2E: trailer on any commit. Core suite: 1223 pass, 0 fail, 1 skip. Head CI has two red E2E jobs, and I read both logs. Neither comes from this change:
- E2E Sandbox: only
AuthBasic > session revocationfailed. The assertion that failed is the one before sign-out ("captured cookie authenticates before sign-out"). Every other AuthBasic test passed against the same outputs, so the outputs selection is fine. - E2E Compute Smoke (cloudfront): the deploy failed on VPC endpoint conflicts ("route table already has a route", "conflicting DNS domain") and an ECS circuit breaker. The same job fails on other PRs this week (40397986226, 40481405066, both
BlocksComputeTestVpc). Its teardown went through this PR'sdestroySandboxand destroyed the failed stack.
A re-run of those two is needed before merge. The behaviour change for customers is intended and documented: production destroy deletes every top-level stack (see the inline item).
Coverage / blind spots: no test removed or skipped, and no required context changed. The two identity asserts became behavioural tests (above). One gap is new: the production path's stale-throttle exclusion is not guarded (minor, inline).
Open questions
- A maintainer decides F1: (a) a confirmation only on interactive runs, or (b) accept no-prompt multi-stack destroy as documented.
- Re-run E2E Sandbox and E2E Compute Smoke (cloudfront) on the head.
Merge-order note: #958's commits are already part of this branch, so merge #958 first or close it with this one. #896 and #850 still need a rebase after this (sandbox.ts/destroy.ts and deploy.ts).
| // refuses an app with more than one stack, so the destroy could never | ||
| // succeed. It deletes every top-level stack of the app, dependent | ||
| // stacks first; a stack deployed from this app by hand is included. | ||
| '--all', |
Contributor
There was a problem hiding this comment.
Blocking (maintainer decision), still open: nothing changed here since the last two reviews. The full reasoning is in the earlier thread on this file. --all --force deletes every top-level stack with no list and no prompt. That holds for an interactive npm run destroy too, which is the run a confirmation protects.
Fix: a maintainer picks one:
- (a) Pass
--forceonly whenprocess.stdin.isTTYis false, so CDK's own prompt lists the stacks on interactive runs. Optionally do this only when the assembly has more than one top-level stack. - (b) Accept the no-prompt behaviour as documented in the changeset and the README.
Either answer closes it. With (b), I'll record it as settled and approve.
| const deps = productionDestroyDeps({ cdkAppPath: 'cdk.ts', projectRoot: '/app' }, {}, { cfnFor, s3 }); | ||
| const stacks = [{ stackName: 's', region: 'eu-west-1' }]; | ||
| assert.ok(deps.isThrottled, 'isThrottled is wired'); | ||
| assert.strictEqual(await deps.isThrottled(stacks, SINCE), true); |
Contributor
There was a problem hiding this comment.
Non-blocking (minor): the production wiring test doesn't guard previousFailureAt. The sandbox test does (staleDeps, lines 479-482), but this one only calls deps.isThrottled(stacks, SINCE). I changed productionDestroyDeps in destroy.ts:106 to pass undefined instead of previous, and every test still passed. The result: a short previous attempt's throttle gets counted again on production destroy, so the job backs off for minutes on a failure that isn't a throttle. Main's identity assert used to catch this; the closure wrapper made that assert impossible.
Fix: mirror the sandbox's stale check here:
const stale = fakeCfn({ s: shortPreviousAttempt() });
const staleDeps = productionDestroyDeps({ cdkAppPath: 'cdk.ts', projectRoot: '/app' }, {}, { cfnFor: () => stale.cfn, s3 });
assert.ok(staleDeps.isThrottled);
assert.strictEqual(await staleDeps.isThrottled(stacks, SINCE, SINCE - 20_000), false);This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #958: its commits (84c6074, 845bd24, a524649) come first and should merge with it; this PR's own change is the six commits after them. (Based on main, not on #958's branch, because PR Checks only runs for base branches without a slash.)
Problem
npm run deployandnpm run destroyruncdk deploy/cdk destroywith no stack selector. With no selector the CDK CLI refuses any app that has more than one top-level stack, before it makes any AWS call:Apps that hit it:
Hostingwith a Lambda@Edge function (an edge-runtime route or Next.js middleware) deployed outside us-east-1, which adds anedge-lambda-stack-*stack; an app with an extra stack of its own, such as aVpc.fromLookupreference stack. Such an app could use the sandbox (it already deploys with--all) but never go to production with the framework commands. The sandboxcdk watchloop had the same gap, and there CDK swallows the error: on a multi-stack app the watch never redeploys and prints nothing.Change
buildCdkDeployArgs(production deploy) anddestroypass--all, the selection the sandbox deploy already uses (andsandbox:destroywith fix(core): sandbox:destroy removes every stack the sandbox deploys #958).cdk watchnames the top-level stacks of the assembly the sandbox just deployed (watchArgsForAssembly), only when there are several; with one stack no selector is passed, so a single-stack app keeps its argv. The watch--apppath is quoted like the other sandbox commands (quoteCdkAppPath).readAppStackOutputs/selectAppStackpick the outputs-file entry that carriesApiUrl(CDK writes one entry per stack with outputs, in completion order), warn if several stacks carry one, and back the deploy, the sandbox andnpm run console.listStackBucketNamesreads each bucket'sDeletionPolicyfrom the deployed template (GetTemplate, Processed stage) and skips any that is notDelete(RemovalPolicy.RETAIN, the production preset's default, orRETAIN_ON_UPDATE_OR_DELETE), and buckets already deleted or skipped. If the template can't be read (it needscloudformation:GetTemplate), none of that stack's buckets is emptied. So "resources with aRETAINremoval policy are kept" now holds for their data too.npm run destroylists the stacks for the retry from the manifest of the assembly it synthesizes (--output .blocks-sandbox/destroy.out, its own directory, removed after success), with each stack's Region and the profile/Region CDK uses, and stops with a re-run-later hint when the Lambda@Edge stack's function is still replicated (same mechanism as fix(core): sandbox:destroy removes every stack the sandbox deploys #958).npm run destroy: it usually runs unattended, so a list nobody reads could not gate it. CDK prints each stack as it deletes it; the README and changeset say every top-level stack is deleted and to keep a separately managed stack in its own CDK app.deploy,destroyand the argv builders,packages/blocks/README.md, the template README rows fornpm run destroy/npm run sandbox:destroy.@aws-blocks/coreminor (production deploy and destroy now act on every top-level stack, a behaviour change to read before upgrading, as with other behaviour changes in this repo's changesets),@aws-blocks/blocksand@aws-blocks/create-blocks-apppatch (docs). No export, export map, CLI flag or on-disk format change; the new helpers are internal topackages/core/src/scripts.Behaviour change (for maintainer review)
Single-stack apps: unchanged. With exactly one top-level stack,
--alland no selector select the same stack; neither selects stacks inside a CDKStage.Multi-stack apps go from "refused" to "every top-level stack":
npm run destroydeletes every top-level stack of the app without a prompt (it already passed--force), dependent stacks first;RETAINresources are kept, with their data.Evidence
destroy-stack-selection.test.tsandsandbox-empty-buckets.test.ts, including real-CLI runs (pinned aws-cdk 2.1146.0) on a hand-written multi-stack assembly with a Stage: the deploy and destroy argv are refused without--alland pass stack selection with it;cdk ls <names>selects exactly the top-level set. New: retained / already-deleted / unreadable-template buckets are not emptied (failed before the fix: the retainedUserFilesandLogsbuckets were returned for emptying);readAppStackOutputsandwatchArgsForAssemblyagainst multi-stack fixtures; the production destroy writes, clears and removes its assembly in the project root (mutations droppingcwdor the pre-clear fail it); watch--appquoting.Merge notes
productionDestroyDepsnow takes the retry clients as a third argument and carries--all --output .blocks-sandbox/destroy.out, the assembly stack list, per-Region throttle/bucket/Lambda@Edge checks andrerunCommand; thecdk lslister is gone. fix(core): back off longer, with jitter, when a destroy is throttled #952's production call-site test now checks the wiring by behaviour, plus a test that the stack list is read underprojectRoot. fix(core): make sandbox:destroy work on Windows #896 still conflicts insandbox.ts/destroy.ts.quoteCdkAppPathhere is a byte-for-byte copy of fix(core): make sandbox:destroy work on Windows #896's helper: when fix(core): make sandbox:destroy work on Windows #896 rebases, keep one copy.Object.values(outputs)[0](hosting-*, compute-smoke, vpc-smoke, comprehensive,scripts/ci/web-function-redeploy.ts); every test app is single-stack in production today. Not covered on AWS: a multi-stack production deploy, and the Lambda@Edge / us-east-1 paths (CI runs in us-east-1).