Share API rate-limit state across multi-target logs downloads - #60531
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Rate limits reached before or during artifact processing can lose or skip continuation data.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Coordinates GitHub API rate-limit handling across concurrent multi-target log downloads.
Changes:
- Adds shared rate-limit state and cancellation.
- Generates continuations for interrupted targets.
- Updates CLI guidance and tests.
File summaries
| File | Description |
|---|---|
pkg/cli/mcp_tools_privileged.go |
Updates MCP option guidance. |
pkg/cli/logs_run_processor.go |
Applies shared checks to artifact downloads. |
pkg/cli/logs_rate_limit.go |
Implements shared terminal rate-limit state. |
pkg/cli/logs_rate_limit_test.go |
Tests stopping and state reuse. |
pkg/cli/logs_orchestrator.go |
Builds rate-limit continuations. |
pkg/cli/logs_orchestrator_unit_test.go |
Updates continuation tests. |
pkg/cli/logs_orchestrator_types.go |
Adds shared state to options. |
pkg/cli/logs_orchestrator_download.go |
Propagates cancellation through collection. |
pkg/cli/logs_multi.go |
Coordinates target cancellation and continuations. |
pkg/cli/logs_multi_test.go |
Tests queued-target cancellation. |
pkg/cli/logs_command.go |
Clarifies CLI behavior. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| return logsTargetResult{target: target, result: countLimitedLogsTargetResult(targetOpts)} | ||
| } | ||
| if shared.rateLimitState.isReached() { | ||
| return logsTargetResult{target: target, result: rateLimitedLogsTargetResult(targetOpts), err: errLogsAPIRateLimitReached} |
| maxConcurrentDownloads: opts.maxConcurrentDownloads, | ||
| storageLimit: runtime.storageLimit, | ||
| maxGitHubAPIRateLimit: opts.MaxGitHubAPIRateLimit, | ||
| rateLimitState: opts.rateLimitState, |
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
🏗️ ADR Required — draft added for PR #60531An ADR was required for this PR because it adds more than 100 new lines in business logic directories ( Evidence reviewed
Draft ADR added
Gate resultThis PR now has a draft Michael Nygard ADR capturing:
Next actionPlease review and refine the draft ADR, then keep it with the PR as the decision record for this behavior change. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
This is a Go/CLI change (concurrency + rate-limit coordination logic), not a UI change, so I applied a correctness-focused review instead of Impeccable UI modes.
Found two blocking issues in the new shared rate-limit coordination path, both confirmed by tracing the actual control flow:
pkg/cli/logs_multi.go:355— queued targets get a non-nilerrwhen the ceiling is reached (unlike the parallel count-limit branch), which can turn a "ceiling reached before any run produced" scenario into a harderrors.Joinfailure instead of the gracefulhandleEmptyProcessedRunspath used for count limits.pkg/cli/logs_orchestrator_download.go:425— a ceiling hit mid-batch is only recorded on the per-runDownloadResult.Error;logsProcessWorkflowRunBatchhas no error return, so remaining runs in the same batch are misreported as plain failed downloads instead of being attributed to the rate limit and included in the continuation.
Details and suggested fixes are in the inline comments.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 117.9 AIC · ⌖ 14 AIC · ⊞ 8.4K
| return logsTargetResult{target: target, result: countLimitedLogsTargetResult(targetOpts)} | ||
| } | ||
| if shared.rateLimitState.isReached() { | ||
| return logsTargetResult{target: target, result: rateLimitedLogsTargetResult(targetOpts), err: errLogsAPIRateLimitReached} |
There was a problem hiding this comment.
collectSingleLogsTarget sets err: errLogsAPIRateLimitReached for every queued target once the ceiling is reached, unlike the analogous count-limit path a few lines above which returns err: nil (countLimitedLogsTargetResult(targetOpts) with no err). In mergeLogsTargetResults, every non-nil err is appended to allErrors, and DownloadWorkflowLogsForTargets has:
if len(processedRuns) == 0 {
if len(allErrors) > 0 {
return errors.Join(allErrors...)
}
_, err := handleEmptyProcessedRuns(...) // graceful path with continuations
return err
}So when the ceiling is hit before any target has produced a processed run (e.g. it's reached very early, or --count is small), every target — the one that detected the ceiling and all the queued ones — contributes an error, allErrors is non-empty, and the whole multi-target download now returns a hard error via errors.Join, instead of taking the graceful handleEmptyProcessedRuns branch that the count-limit case takes (which reports "no runs" plus the built continuations, exit 0). This turns an expected/handled condition (ceiling reached) into a CLI failure and discards the otherwise-correctly-built continuation data for the caller.
Suggested fix: don't set err on the logsTargetResult for the rate-limit-reached branches (mirror the count-limit branches), or explicitly filter errLogsAPIRateLimitReached out of allErrors before the len(allErrors) > 0 hard-fail check in DownloadWorkflowLogsForTargets.
@copilot please address this.
There was a problem hiding this comment.
Addressed in b5f569b and follow-up commits: rate-limit sentinel errors are now excluded from multi-target hard failures, preserving merged continuations.
| maxConcurrentDownloads: opts.maxConcurrentDownloads, | ||
| storageLimit: runtime.storageLimit, | ||
| maxGitHubAPIRateLimit: opts.MaxGitHubAPIRateLimit, | ||
| rateLimitState: opts.rateLimitState, |
There was a problem hiding this comment.
When the shared ceiling is tripped mid-batch (inside processSingleRunDownload → waitForConfiguredRateLimit → state.check), the resulting errLogsAPIRateLimitReached is stored only on the per-run DownloadResult.Error via handleArtifactDownloadError, and downloadRunArtifactsConcurrent/appendProcessedWorkflowRuns/processWorkflowRunBatch/logsProcessWorkflowRunBatch have no error return path to surface it.
shouldSkipProcessedWorkflowRun treats any non-nil result.Error as an ordinary failed download (logs a "Failed to download artifacts for run %d" warning and skips it), so every remaining run already queued in this batch's chunk also independently calls waitForConfiguredRateLimit, gets the cheap "already reached" short-circuit from logsRateLimitState.check, and is reported/logged as a plain download failure rather than being recognized as rate-limited — even though finishLogsBatch only inspects allRunsConsumed/batchStorageLimitReached (no rate-limit flag) before letting the loop continue to the next batch. The correct "stop with rate-limit continuation" behavior only kicks in on the next iteration's waitForLogsRateLimit call in collectProcessedWorkflowRuns.
Net effect: once the ceiling is hit inside a batch, the rest of that batch's runs are silently misreported as failed/errored downloads (noisy warnings, potentially confusing skip semantics) instead of being cleanly attributed to the rate limit and included in the continuation, even though the shared state (rateLimitState.isReached()) is already available to processWorkflowRunBatchOptions.
Consider checking opts.rateLimitState.isReached() in orderedLogsRunCollector.processReadyResult (similar to the existing opts.countLimit.isReached() check) to stop accepting/downloading further runs in the current batch and thread that state back through logsProcessWorkflowRunBatch's return values so finishLogsBatch can stop the batch immediately instead of continuing to process runs that are already known to be rate-limited.
@copilot please address this.
There was a problem hiding this comment.
Addressed in b5f569b and follow-up commits: a rate limit reached during batch processing returns the sentinel before cursor advancement, preserves relevant batch state, and is covered by regression tests.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — the two unresolved existing bot review comments identify real correctness bugs in the rate-limit-ceiling continuation path, and this PR adds no regression test covering either scenario.
📋 Key Themes & Highlights
Key Themes
- Untested continuation-drop path: When the shared rate ceiling is hit before any target produces a run,
DownloadWorkflowLogsForTargets(logs_multi.go:183-188) returns early onlen(allErrors) > 0beforecontinuationsreachhandleEmptyProcessedRunsor JSON rendering — so resumable continuation data for every queued target is silently lost. No test drives this path end-to-end. - Untested mid-batch cursor advance:
fetchAndProcessLogsBatch→finishLogsBatch(logs_orchestrator_download.go:411-434) doesn't checkopts.rateLimitState.isReached()before it may advancestate.beforeDatepast runs canceled mid-batch, sincelogsProcessWorkflowRunBatchhas no error return.TestCollectLogsTargetsClearsQueueWhenRateLimitReachedonly exercisescollectLogsTargets, not a mid-batch trip, so this gap isn't caught by CI. - Both issues are consistent with the design intent stated in the PR description ("Preserve continuation parameters for interrupted targets") but the implementation doesn't fully deliver on it in these two spots.
Positive Highlights
- ✅
logsRateLimitStatecleanly centralizes the shared gate/reached/cancel state and is threaded consistently through options structs (concurrentRunDownloadParams,processWorkflowRunBatchOptions,LogsDownloadOptions). - ✅ Good regression tests already exist for the state machine itself (
TestLogsRateLimitStateStopsAndReusesReachedState,TestNilLogsRateLimitStatePreservesSingleTargetWait) and for the "don't start queued targets" behavior. - ✅ CLI/MCP help text updates for
--max-github-api-rate-limitaccurately describe the new single- vs multi-target semantics.
Requesting changes — please add regression tests (or fix the underlying gaps) for the two flagged continuation-handling paths before merge; see inline comments for specifics.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 136.6 AIC · ⌖ 15.2 AIC · ⊞ 10.4K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/logs_multi.go:172
[/tdd] Confirms the existing bot finding at line 355: when the rate ceiling is hit before any target produces a run, DownloadWorkflowLogsForTargets returns errors.Join(allErrors...) at line 185 without ever reaching handleEmptyProcessedRuns, so continuations (built by mergeLogsTargetResults) are silently dropped from the report/JSON output.
<details>
<summary>💡 Suggested test</summary>
No test in logs_multi_test.go exercises the zero-processedRuns path end-to-end through `D…
pkg/cli/logs_orchestrator_download.go:434
[/tdd] Confirms the existing bot finding at line 425: logsProcessWorkflowRunBatch returns no error, so a rate-limit ceiling tripped mid-batch (visible only via shared.rateLimitState.isReached()) isn't checked here before calling finishLogsBatch, which can advance state.beforeDate past runs that were actually canceled.
<details>
<summary>💡 Suggested test</summary>
None of TestCollectLogsTargetsClearsQueueWhenRateLimitReached (logs_multi_test.go) or the rate-limit state tests (lo…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the PR-finisher pass. Latest commit: 538866f. Local change-scoped validation passes; final automated review found no comments. |
|
🎉 This pull request is included in a new release. Release: |
Multi-target
gh aw logsdownloads independently handled GitHub API rate limits, leaving queued work blocked until reset. This change coordinates rate-limit enforcement across the command and stops outstanding work once the shared ceiling is reached.Changes
Shared rate-limit state
Prompt cancellation
Partial-result recovery
CLI guidance
--max-github-api-rate-limit.