fix: enable Octokit throttle retry and SSM adaptive retry under burst#5136
Conversation
There was a problem hiding this comment.
Pull request overview
Improves burst-load resilience in the control-plane Lambdas by enabling actual retries for GitHub API rate limiting via Octokit throttling callbacks and by configuring the shared SSM utility client for longer, adaptive retries when SSM throttles.
Changes:
- Make Octokit throttling callbacks return booleans to trigger retries with capped retry counts.
- Introduce a shared SSM client configuration using
retryMode: 'adaptive'andmaxAttempts: 10, and apply it across SSM helper functions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
lambdas/functions/control-plane/src/github/auth.ts |
Enables Octokit throttling retries by returning true/false from rate-limit callbacks with retry caps. |
lambdas/libs/aws-ssm-util/src/index.ts |
Applies adaptive retry settings to SSM client usage to better absorb throttling during bursts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two resilience improvements for burst load: 1. Octokit plugin-throttling callbacks now return true (with caps) so rate-limited GitHub API requests are retried instead of immediately failing. Primary: up to 2 retries. Secondary: 1 retry. 2. SSM client switched to adaptive retry mode with maxAttempts=10, giving ~30s of retry budget for PutParameter under throttling. Standard mode (3 attempts, ~3s) is insufficient when multiple concurrent Lambdas write JIT configs during burst. Fixes github-aws-runners#5135
- Use the retryCount argument the throttling plugin already passes as its 4th parameter instead of casting options through unknown. Extract the two limit handlers so the retry caps are testable at all. - Memoise the SSMClient. Adaptive retry keeps its rate-sensing token bucket on the client instance, so constructing a fresh client per call discarded it and reduced retryMode: 'adaptive' to plain retries — the change was close to a no-op as written. - Cover both: retry-cap boundaries for the limit handlers, and the client's retry configuration and reuse.
47d4834 to
1ca6aef
Compare
|
Pushed a follow-up addressing all of these — the third one was a real bug, thanks Copilot:
Would appreciate a human pass on the retry caps themselves (2 for primary, 1 for secondary) — those are judgement calls, not derived from anything. |
|
@vegardx Build is not working |
The extracted onRateLimit/onSecondaryRateLimit handlers typed their unused octokit parameter as @octokit/rest's Octokit, which is wider than the @octokit/core Octokit the plugin's LimitHandler expects. Parameter contravariance made the handlers unassignable, failing the dist build (tsc) while vitest's transpile-only path missed it.
|
Fixed in f3f65e9: the dist build runs tsc and caught a type error the tests missed, the throttle handlers typed their octokit param as @octokit/rest's Octokit but the plugin's LimitHandler expects @octokit/core's, so I typed it to match. Build and tests pass locally. |
…#5136) ## Problem Two resilience gaps that compound under burst load: 1. **Octokit throttle plugin never retries** — `onRateLimit` and `onSecondaryRateLimit` callbacks log a warning but don't return `true`, so `@octokit/plugin-throttling` throws immediately instead of retrying after the `retryAfter` delay. 2. **SSM client uses default retry** (`standard`, 3 attempts, ~3s budget) — under burst with multiple concurrent Lambdas writing JIT configs via PutParameter, SSM's ~40 TPS limit is easily exceeded and `ThrottlingException` propagates up, failing the entire batch. ## Fix ### Octokit throttle callbacks return `true` with caps ```typescript onRateLimit: (retryAfter, options) => { logger.warn(`...retrying after ${retryAfter}s`); return options.request.retryCount < 2; // retry up to 2x on primary rate limit }, onSecondaryRateLimit: (retryAfter, options) => { logger.warn(`...retrying after ${retryAfter}s`); return options.request.retryCount < 1; // retry once on secondary/abuse limit }, ``` ### SSM adaptive retry with maxAttempts=10 ```typescript const SSM_CLIENT_CONFIG = { region: process.env.AWS_REGION, maxAttempts: 10, retryMode: 'adaptive' as const, }; ``` `adaptive` mode adds client-side rate-sensing — when the SDK sees ThrottlingException it slows further calls to match the observed budget. 10 attempts gives ~30s of retry budget, which is safe because runners take ~30-50s to boot before reading their JIT config. ## Changes - `lambdas/functions/control-plane/src/github/auth.ts` — throttle callbacks return `true` with retry caps - `lambdas/libs/aws-ssm-util/src/index.ts` — shared SSM client config with adaptive retry ## Impact | Scenario | Before | After | |---|---|---| | GitHub rate limit during scale-up | Request fails immediately | Retries 1-2x with backoff | | SSM throttle during JIT config write | ThrottlingException after ~3s | Adaptive backoff for ~30s | | Burst of 100 jobs, batch_size=10 | SSM throttle → orphaned instances | Retry absorbs throttle | Fixes #5135 Refs: #5024, #5037
Problem
Two resilience gaps that compound under burst load:
Octokit throttle plugin never retries —
onRateLimitandonSecondaryRateLimitcallbacks log a warning but don't returntrue, so@octokit/plugin-throttlingthrows immediately instead of retrying after theretryAfterdelay.SSM client uses default retry (
standard, 3 attempts, ~3s budget) — under burst with multiple concurrent Lambdas writing JIT configs via PutParameter, SSM's ~40 TPS limit is easily exceeded andThrottlingExceptionpropagates up, failing the entire batch.Fix
Octokit throttle callbacks return
truewith capsSSM adaptive retry with maxAttempts=10
adaptivemode adds client-side rate-sensing — when the SDK sees ThrottlingException it slows further calls to match the observed budget. 10 attempts gives ~30s of retry budget, which is safe because runners take ~30-50s to boot before reading their JIT config.Changes
lambdas/functions/control-plane/src/github/auth.ts— throttle callbacks returntruewith retry capslambdas/libs/aws-ssm-util/src/index.ts— shared SSM client config with adaptive retryImpact
Fixes #5135
Refs: #5024, #5037