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
Contributor

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>
@mrgrain
mrgrain enabled auto-merge September 4, 2026 23:06
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>
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

2 participants