fix(toolkit-lib): throttled final event poll fails a successful deploy - #1941
Open
polothy wants to merge 1 commit into
Open
fix(toolkit-lib): throttled final event poll fails a successful deploy#1941polothy wants to merge 1 commit into
polothy wants to merge 1 commit into
Conversation
The final `DescribeStackEvents` read in `StackActivityMonitor.stop()` is presentational, but its failures propagated out of the `finally` blocks that call `stop()`, replacing the result of an operation that had already succeeded. Guard `finalPollToEnd()` so a failed read is reported as `CDK_TOOLKIT_W5500` instead of propagating. This covers the deploy, destroy and rollback monitors from one place. fixes aws#1777 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
polothy
requested a deployment
to
integ-approval
September 3, 2026 21:50 — with
GitHub Actions
Waiting
polothy
marked this pull request as ready for review
September 3, 2026 21:55
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.
Note
This PR was created by AI (Claude Code, Anthropic's agentic CLI), including the code, tests and this description, working from the analysis in #1777. A human directed the work and reviewed the result, but the reasoning and wording below are the model's. Please review it on its merits rather than assuming human verification of every claim.
fixes #1777
That issue has the full diagnosis, the throttling arithmetic and the reproduction; this description covers only what was implemented.
The fix
StackActivityMonitor.finalPollToEnd()is now guarded, which is the one place all threemonitor.stop()call sites go through —FullCloudFormationDeployment.monitorDeployment,destroyStack(both indeploy-stack.ts) andDeployments.rollbackStack. Guarding at the call sites instead would have missed rollback.Three changes in
packages/@aws-cdk/toolkit-lib:finalPollToEnd()— the final read no longer propagates. A failedreadNewEvents()is reported asCDK_TOOLKIT_W5500andstop()returns normally. This read only completes the event log; the operation's outcome came fromDescribeStacksviawaitForStackDeploylong before.finalPollToEnd()— the awaited in-flightreadPromiseis caught. This path was equally unguarded and is a separate failure mode: a poll that is still in flight whenstop()is called, and rejects. It is swallowed rather than reported, becausetick()awaits that same promise inside its owntry/catchand has already emittedCDK_TOOLKIT_E5500for it. Without this catch, the rejection skips the final read entirely — the very flushstop()exists to perform.tick()—readPromiseis cleared in afinally. It was cleared only on the success path, so after any failed poll the field kept an already-rejected promise, and the nextstop()re-raised it before doing anything. Now the field only ever holds a read that is genuinely in flight, which makes (2)'s "already reported bytick()" an invariant rather than an assumption.Both (1) and (2) are load-bearing; see the mutation testing below.
Error path
Before this PR, only the left branch was caught. Either branch under
stop()threw, and because both call sites invokestop()from afinally, the throw replaced the value thetrywas about to return:Before / after for
cdkusersScenario: a stack reaches
UPDATE_COMPLETE, and the account is throttlingDescribeStackEvents(10 req/s, non-adjustable) hard enough that a call exhausts its retries.Before
UPDATE_COMPLETEin CloudFormation but reported as failed.cdk deployexits1.--allrun are abandoned.toolkit-libcallers get a rejecteddeployStack()for a deployment that succeeded.After
plus one warning on the affected stack:
0; the rest of the--allrun continues.{ type: 'did-deploy-stack', ... }.Unchanged: the periodic polls.
tick()already swallowed throttles and re-polls 2s later, so missed events catch up there. Also unchanged: any failure fromDescribeStacks/waitForStackDeploy, which is how the outcome is actually determined, still fails the deployment.Why
warnand a new message codeThe first draft reused
CDK_TOOLKIT_E5500, matching whattick()emits for the identical failure on the identical API call. Changed to a newCDK_TOOLKIT_W5500(warn,ErrorPayload) because:erroris the one levelCliIoHost.selectStreamFromLeveldoes not redirect to stdout under--ci, so a green deploy wrote to stderr — noise for exactly the CI pipelines reporting this issue.erroron an operation that succeeded misstates the outcome. (toolkit-lib): throttled final stack-event poll in monitor.stop() fails an already-successful deployment #1777 asked for "at most warns".hook-result-details.tsdoes so twice for the same "optional detail fetch failed, carry on" shape.tick()'s existingE5500is deliberately left alone: it fires while the outcome is still unknown, soerrorremains defensible there.docs/message-registry.mdwas regenerated withnpx projen registry. No API report change —IOlives underlib/api/io/private.Trade-off worth reviewing
When the in-flight read fails, the final read now still runs, where previously the throw short-circuited it. Under sustained throttling that is one additional fully-retried
DescribeStackEventsper stack —ConfiguredRetryStrategy(7, cappedExponentialBackoff(1000, 15_000)), so up to roughly a minute — inside afinally.Kept deliberately: that read is the last chance to print the resource-level failure reasons for a stack that genuinely failed, so skipping it would lose diagnostics exactly when they matter. Cost is bounded per stack and concurrent across stacks, so it is tail latency rather than an additive delay, and the alternative it replaces is a wrongly failed deployment.
Tests
test/api/stack-events/stack-activity-monitor.test.ts— newstack monitor, failures while reading eventsblock: a throttled final poll is reported asW5500whilestop()resolves and still emitsI5503; a poll that fails whilestop()is waiting on it does not failstop()and does not skip the final read.test/api/deployments/deploy-stack-event-poll-failures.test.ts(new) — end-to-end:describeStackEventsthrottling throughout,deployStackstill returnsdid-deploy-stackfor bothchange-setanddirect, anddestroyStackstill returns its stack ARN.test/api/deployments/cloudformation-deployments.test.ts—rollbackStackunder the same throttling still returns{ success: true }, pinning the third call site.Verified by mutation, since these tests must fail for the right reason:
W5500notifyreadPromisecatchtest/api/deployments+test/api/stack-events+ the deploy/destroy/rollback action suites pass (324 tests). Fulltoolkit-libsuite: 1941 pass, with 4 suites failing for pre-existing environment reasons unrelated to this change (bootstrap*needpost-compileto copybootstrap-template.yaml;sdk-providerneedsNODE_OPTIONS=--experimental-vm-modules).eslintclean.Follow-ups, not in this PR
Both come from #1777 and are separate concerns:
cappedExponentialBackoffis deterministic, soConfiguredRetryStrategyreplaces smithy's jittered default and throttled pollers re-collide at the same instants. Adding jitter would reduce how often the retry budget is exhausted in the first place.stackEventPollingIntervalexists only intoolkit-lib; there is nocdk deployflag for it, so CLI users cannot lower their request rate.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license