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>
mrgrain
approved these changes
Sep 4, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 4, 2026
…ep (#1942) > [!NOTE] > **This PR was created by AI** (Claude Code, Anthropic's agentic CLI), including the code, tests and this description. 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. Related to #1777 (deliberately not using a closing keyword — that issue is about a different defect and should stay open). This is the secondary finding from its comment thread, and it stands alone: it reduces how often a request exhausts its retry budget under throttling, which is the precondition for the failure described there. It does not depend on, conflict with, or supersede #1941. ## The problem `ConfiguredRetryStrategy` does not layer a custom delay onto the SDK's backoff — it **replaces** it: ```js // @smithy/core/dist-cjs/submodules/retry/index.js:572 this.retryBackoffStrategy.computeNextBackoffDelay = (completedAttempt) => { const nextAttempt = completedAttempt + 1; return this.computeNextBackoffDelay(nextAttempt); // <-- our function }; ``` The strategy it displaces is jittered (`DefaultRetryBackoffStrategy`, line 386). `cappedExponentialBackoff` was not: ```ts return (attempt: number) => Math.min(baseMs * (2 ** attempt), capMs); ``` So every request on a toolkit client that started retrying waited exactly 2s, 4s, 8s, 15s, 15s, 15s. Requests throttled in the same instant retry in the same instants: a herd stays a herd, re-colliding at each retry point rather than dispersing, and spends its budget on collisions with itself. Stack-event pollers are phase-aligned by construction (stacks in one deploy start together, nothing de-phases them), so this is the common case at high `--concurrency`, not a corner case. ## The change One function, in `packages/@aws-cdk/toolkit-lib/lib/api/aws-auth/sdk.ts`: ```ts const delay = Math.min(baseMs * (2 ** attempt), capMs); return Math.floor(delay / 2 + Math.random() * (delay / 2)); ``` The cap is still applied before the jitter, so the documented worst case remains the cap. Jitter applies **only between retries**. The first attempt of every request is unaffected, so a call that succeeds — or fails non-retryably — first time sees no change at all. Each delay moves from a fixed value `d` to a random point in `[floor(d/2), d-1]` (equal jitter; the top is `d-1` rather than `d` because of `Math.floor`). Every possible delay is therefore **less than or equal to the delay before this change** — no request can wait longer than it does today, and the capped worst case that #1500 established is preserved exactly. Both call sites of the helper are affected. The STS strategy (`sdk.ts:677`) keeps its own inline schedule deliberately: it is a low-volume credential-validity probe that is not part of any throttling herd, and its 3-attempt budget already bounds it at 400ms. ### Base client config — `ConfiguredRetryStrategy(7, cappedExponentialBackoff(300, 15_000))` Spread into every client the `SDK` class builds (S3, Lambda, SSM, ECR, CloudWatch Logs, Route 53, Secrets Manager, SFN, …). | Retry | Was (fixed) | Now (range) | |-------|-------------|-------------| | 1 | 600ms | 300–599ms | | 2 | 1200ms | 600–1199ms | | 3 | 2400ms | 1200–2399ms | | 4 | 4800ms | 2400–4799ms | | 5 | 9600ms | 4800–9599ms | | 6 | 15000ms | 7500–14999ms | | **Total across 6 retries** | **33.6s** | **16.8s min / 25.2s mean / 33.594s max** | ### CloudFormation client — `ConfiguredRetryStrategy(7, cappedExponentialBackoff(1000, 15_000))` The path in #1777. | Retry | Was (fixed) | Now (range) | |-------|-------------|-------------| | 1 | 2000ms | 1000–1999ms | | 2 | 4000ms | 2000–3999ms | | 3 | 8000ms | 4000–7999ms | | 4 | 15000ms | 7500–14999ms | | 5 | 15000ms | 7500–14999ms | | 6 | 15000ms | 7500–14999ms | | **Total across 6 retries** | **59.0s** | **29.5s min / 44.3s mean / 58.994s max** | ### Why equal jitter rather than the SDK's full jitter Full jitter (`random * d`, what smithy's `DefaultRetryBackoffStrategy` does and what #1777 proposed) would drop the CloudFormation mean to ~29.5s and allow a near-zero wait before the first retry. Equal jitter keeps the 59s worst case and a floor of half the scheduled delay, so the window a request has to ride out a throttling burst shrinks by 25% rather than 50%, while still dispersing simultaneous retries across `d/2` — 7.5s at the cap, roughly 75 request slots against the 10 req/s `DescribeStackEvents` quota. Switching to full jitter is a one-line change if reviewers prefer to match the SDK default. ## How this compares to the SDK's own jitter Same shape, one intentional deviation — smithy's default, verbatim: ```js // DefaultRetryBackoffStrategy, retry/index.js:386 const b = Math.random(); const t_i = b * Math.min(this.x * 2 ** i, MAXIMUM_RETRY_DELAY); return Math.floor(t_i); ``` | | window | mean | near-instant retry possible | |---|---|---|---| | smithy (full jitter) | `[0, d)` | `d/2` | yes | | this PR (equal jitter) | `[floor(d/2), d)` | `0.75d` | no | Both are exponential base-2, clamped, `Math.random()`-scaled and floored; only the multiplier's range differs. Full jitter disperses better but halves the mean, which would shorten the CloudFormation budget from ~59s of wall clock to ~29.5s — the opposite of what helps when the problem is exhausting that budget. Equal jitter still spreads simultaneous retries across `d/2` (7.5s at the cap, ~75 request slots against the 10 req/s `DescribeStackEvents` quota) while keeping more of the window intact. Switching to full jitter is a one-line change if maintainers prefer to match the SDK default exactly. **One related observation, pre-existing and not addressed here:** `StandardRetryStrategy` calls `setDelayBase(THROTTLING_RETRY_DELAY_BASE)` (500ms, vs 100ms otherwise) when it classifies an error as throttling (`retry/index.js:472`). A `computeNextBackoffDelay` supplied to `ConfiguredRetryStrategy` closes over its own base and never reads `this.x`, so that adaptation has always been inert for the toolkit. It matters less than it sounds — the CloudFormation client's 1000ms base already exceeds smithy's throttling base — but reviewers weighing this PR's throttling rationale should know the SDK's own throttling adaptation is not in play. ## Tests `test/api/aws-auth/sdk-retry.test.ts` — the four existing tests keep their original expectations (`2000`, `4000`, `8000`, `15_000`, `59_000`) by mocking `Math.random()` to the top of the jitter window, so they still assert the schedule rather than jitter arithmetic. Three new tests cover the jitter itself: the floor at half the scheduled delay, a property check that 1,000 draws across attempts 1–10 all land in `[floor(d/2), d)`, and that repeated draws for one attempt are not all identical. Mutation-checked by reverting the body to the deterministic version: `never waits less than half the scheduled delay`, `stays within the jitter window for every attempt` and `disperses callers that retry at the same moment` all fail. (The schedule tests pass under that mutation by design — mocking the top of the window makes them agree with the deterministic implementation, which is what keeps them readable.) The interval in the docstring was also verified against odd `baseMs`/`capMs` combinations (`333/15001`, `1/3`, `7/7`, `1001/9999`), since this is a general helper and `Math.floor` makes the bounds inexact for odd delays. All seven `test/api/aws-auth` suites pass (82 tests). `eslint` clean. No API Extractor change — the helper is exported only via `lib/api/aws-auth/private`. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Momo Kornher <kornherm@amazon.co.uk>
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