Skip to content

fix(toolkit-lib): throttled final event poll fails a successful deploy - #1941

Open
polothy wants to merge 1 commit into
aws:mainfrom
polothy:fix/final-stack-event-poll-guard
Open

fix(toolkit-lib): throttled final event poll fails a successful deploy#1941
polothy wants to merge 1 commit into
aws:mainfrom
polothy:fix/final-stack-event-poll-guard

Conversation

@polothy

@polothy polothy commented Sep 3, 2026

Copy link
Copy Markdown

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 three monitor.stop() call sites go through — FullCloudFormationDeployment.monitorDeployment, destroyStack (both in deploy-stack.ts) and Deployments.rollbackStack. Guarding at the call sites instead would have missed rollback.

Three changes in packages/@aws-cdk/toolkit-lib:

  1. finalPollToEnd() — the final read no longer propagates. A failed readNewEvents() is reported as CDK_TOOLKIT_W5500 and stop() returns normally. This read only completes the event log; the operation's outcome came from DescribeStacks via waitForStackDeploy long before.

  2. finalPollToEnd() — the awaited in-flight readPromise is caught. This path was equally unguarded and is a separate failure mode: a poll that is still in flight when stop() is called, and rejects. It is swallowed rather than reported, because tick() awaits that same promise inside its own try/catch and has already emitted CDK_TOOLKIT_E5500 for it. Without this catch, the rejection skips the final read entirely — the very flush stop() exists to perform.

  3. tick()readPromise is cleared in a finally. It was cleared only on the success path, so after any failed poll the field kept an already-rejected promise, and the next stop() 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 by tick()" an invariant rather than an assumption.

Both (1) and (2) are load-bearing; see the mutation testing below.

Error path

                     doPoll() ── DescribeStackEvents ── SDK retries exhausted (Throttling)
                        │
                        ▼
                    poll() → readNewEvents()
                        │
        ┌───────────────┴────────────────┐
        │                                │
   from tick()                    from stop() → finalPollToEnd()
        │                                │
        │                    ┌───────────┴────────────┐
        │                    │                        │
        │            await readPromise         await readNewEvents()
        │            (in-flight poll)             (the final read)
        │                    │                        │
   ┌────▼─────┐         ┌────▼─────┐            ┌─────▼──────┐
   │ caught   │         │ caught   │            │  caught    │
   │ E5500    │         │ ignored  │            │  W5500     │
   │ (before  │         │ (tick    │            │  (new)     │
   │  & after)│         │  reports)│            │            │
   └──────────┘         └──────────┘            └────────────┘

Before this PR, only the left branch was caught. Either branch under stop() threw, and because both call sites invoke stop() from a finally, the throw replaced the value the try was about to return:

finalPollToEnd() throws
  → monitor.stop() throws
  → finally { await monitor.stop() }      ← discards the successful return value
  → monitorDeployment() rejects
  → deployStack() rejects  →  Deploy action rejects  →  exec() rejects
  → cli() .catch()  →  process.exitCode = 1

Before / after for cdk users

Scenario: a stack reaches UPDATE_COMPLETE, and the account is throttling DescribeStackEvents (10 req/s, non-adjustable) hard enough that a call exhausts its retries.

Before

 ✅  my-stack-a
 ❌  my-stack-b failed: Throttling: Rate exceeded
  • The stack is UPDATE_COMPLETE in CloudFormation but reported as failed.
  • cdk deploy exits 1.
  • Remaining stacks in a --all run are abandoned.
  • toolkit-lib callers get a rejected deployStack() for a deployment that succeeded.

After

 ✅  my-stack-a
 ✅  my-stack-b

plus one warning on the affected stack:

[Warning at my-stack-b] Error occurred during final stack event poll, event log may be incomplete: Throttling: Rate exceeded
    at ...
  • Exit code 0; the rest of the --all run continues.
  • Programmatic callers get { type: 'did-deploy-stack', ... }.
  • The only loss is presentational: trailing stack events for that stack may be missing from the printed log, which is what the warning says.

Unchanged: the periodic polls. tick() already swallowed throttles and re-polls 2s later, so missed events catch up there. Also unchanged: any failure from DescribeStacks/waitForStackDeploy, which is how the outcome is actually determined, still fails the deployment.

Why warn and a new message code

The first draft reused CDK_TOOLKIT_E5500, matching what tick() emits for the identical failure on the identical API call. Changed to a new CDK_TOOLKIT_W5500 (warn, ErrorPayload) because:

  • error is the one level CliIoHost.selectStreamFromLevel does not redirect to stdout under --ci, so a green deploy wrote to stderr — noise for exactly the CI pipelines reporting this issue.
  • Reporting error on 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".
  • Sibling best-effort failures in the same directory already warn — hook-result-details.ts does so twice for the same "optional detail fetch failed, carry on" shape.

tick()'s existing E5500 is deliberately left alone: it fires while the outcome is still unknown, so error remains defensible there. docs/message-registry.md was regenerated with npx projen registry. No API report change — IO lives under lib/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 DescribeStackEvents per stack — ConfiguredRetryStrategy(7, cappedExponentialBackoff(1000, 15_000)), so up to roughly a minute — inside a finally.

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 — new stack monitor, failures while reading events block: a throttled final poll is reported as W5500 while stop() resolves and still emits I5503; a poll that fails while stop() is waiting on it does not fail stop() and does not skip the final read.
  • test/api/deployments/deploy-stack-event-poll-failures.test.ts (new) — end-to-end: describeStackEvents throttling throughout, deployStack still returns did-deploy-stack for both change-set and direct, and destroyStack still returns its stack ARN.
  • test/api/deployments/cloudformation-deployments.test.tsrollbackStack under the same throttling still returns { success: true }, pinning the third call site.

Verified by mutation, since these tests must fail for the right reason:

Mutation Result
Revert the guard entirely 5 tests fail
Keep the swallow, drop the W5500 notify 5 tests fail
Remove only the readPromise catch 1 test fails (the in-flight case)

test/api/deployments + test/api/stack-events + the deploy/destroy/rollback action suites pass (324 tests). Full toolkit-lib suite: 1941 pass, with 4 suites failing for pre-existing environment reasons unrelated to this change (bootstrap* need post-compile to copy bootstrap-template.yaml; sdk-provider needs NODE_OPTIONS=--experimental-vm-modules). eslint clean.

Follow-ups, not in this PR

Both come from #1777 and are separate concerns:

  • cappedExponentialBackoff is deterministic, so ConfiguredRetryStrategy replaces 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.
  • stackEventPollingInterval exists only in toolkit-lib; there is no cdk deploy flag 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

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(toolkit-lib): throttled final stack-event poll in monitor.stop() fails an already-successful deployment

1 participant