Fix 429 rate limiting in evaluation pipeline - #284
Merged
Conversation
- P0: Add retry with exponential backoff to ListModelsAsync() in ValidateCommand using the existing RetryHelper (3 retries, 2s base delay, 60s budget) - P1: Reduce scheduled-run parallelism to match infra-change settings (2/3/3) to avoid overloading the API when all 5 plugins run in parallel - P2: Assign Copilot tokens deterministically by matrix job index instead of randomly, eliminating ~96% token collision probability - P3: Add retry with backoff to AgentRunner.RunAgent() for mid-eval 429s (2 retries, 5s base delay, scenario timeout + 60s budget) Fixes #168
JanKrivanek
requested review from
ViktorHofer,
dbreshears and
timheuer
as code owners
March 8, 2026 19:04
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce Copilot API 429 rate-limit failures in the evaluation pipeline after the multi-plugin split by adding targeted retries and reducing/token-sharding concurrency pressure in scheduled runs.
Changes:
- Add retry/backoff around
ValidateCommand.Run()’sListModelsAsync()to avoid immediate job failure on transient 429s. - Reduce scheduled-run per-job concurrency in
evaluation.ymlto match the “infra change” lower-parallelism settings. - Replace random Copilot token selection with deterministic sharding via
strategy.job-indexinevaluation-run.yml. - Add a new root-cause analysis doc and wrap
AgentRunner.RunAgent()withRetryHelper(but see comments re: effectiveness).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/skill-validator/src/Services/AgentRunner.cs | Wraps RunAgent with retry helper to improve resilience to transient failures during agent runs. |
| eng/skill-validator/src/Commands/ValidateCommand.cs | Retries ListModelsAsync() to prevent transient 429s from failing validation immediately. |
| docs/fix-429-rate-limiting.md | Documents the incident analysis and layered mitigation plan. |
| .github/workflows/evaluation.yml | Lowers scheduled-run concurrency inputs to reduce rate-limit pressure. |
| .github/workflows/evaluation-run.yml | Deterministically assigns Copilot tokens per matrix job to avoid token collisions. |
Comments suppressed due to low confidence (1)
docs/fix-429-rate-limiting.md:17
- In the root-cause chain, step 2 says each job "randomly selects" a token, but this PR switches selection to deterministic sharding by
strategy.job-index. Consider clarifying that this list describes the pre-fix behavior (or update it to reflect the new behavior) so the doc doesn’t become misleading post-merge.
1. The evaluation matrix now spawns **5 parallel jobs** (one per plugin) instead of 1.
2. Each job randomly selects one of 8 Copilot tokens and immediately calls `ListModelsAsync()` to validate the model.
3. With 5 jobs starting simultaneously, multiple jobs may pick the same token and hit the Copilot API `models.list` endpoint concurrently, triggering a 429.
4. `ListModelsAsync()` in `ValidateCommand.Run()` has **zero retry logic** — any exception immediately exits with code 1.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
JanKrivanek
enabled auto-merge (squash)
March 8, 2026 19:40
- Change unused ct param to _ in ListModelsAsync retry lambda since the SDK method has no CancellationToken overload - Pass CancellationToken from RetryHelper through to RunAgentCore and link it with the per-scenario CTS so budget expiry cancels in-flight work - Re-throw 429/rate-limit errors in RunAgentCore instead of swallowing them, so RetryHelper can actually retry on transient rate limits - Doc comment (re: stale table) is moot the doc was already deleted
ViktorHofer
approved these changes
Mar 8, 2026
Contributor
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
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.
Summary
Fixes the 69% failure rate on scheduled evaluation runs caused by Copilot API 429 rate limits after the multi-plugin split in #274.
Changes (layered approach)
P0: Retry on
ListModelsAsync()(immediate crash fix)Wraps the
ListModelsAsync()call inValidateCommand.Run()withRetryHelper.ExecuteWithRetry(3 retries, 2s exponential backoff, 60s budget). This directly fixes the crash a 429 onmodels.listis transient and resolves within seconds.P1: Reduce scheduled-run parallelism
Scheduled runs now use the same reduced concurrency as infra-change runs (
2/3/3instead of5/5/5). Since scheduled runs also spawn all 5 plugin matrix jobs simultaneously, they should avoid compounding the load.P2: Deterministic token sharding
Replaces random token selection (
RANDOM % N) with deterministic assignment bystrategy.job-index. With 5 tokens and 5 matrix jobs, this guarantees each job gets a unique token eliminating the ~96% chance of token collisions.P3: Retry on
RunAgent()Wraps
AgentRunner.RunAgent()withRetryHelper.ExecuteWithRetry(2 retries, 5s backoff, scenario timeout + 60s budget) to handle mid-evaluation 429s.Validation
Analysis
See docs/fix-429-rate-limiting.md for the full root cause analysis.
Fixes #168