fix: make checkAndWaitForRateLimit context-cancellable#43848
Conversation
Replace the 4 bare time.Sleep calls in checkAndWaitForRateLimit with a new sleepWithContext helper that selects on both the timer and ctx.Done(). Add ctx context.Context as the first parameter and update the call site in logs_orchestrator.go to pass activeCtx so that Ctrl-C or a deadline wakes the rate-limit wait immediately. Also propagate context errors from checkAndWaitForRateLimit in the orchestrator loop so the top-of-loop cancellation check handles the graceful shutdown path. Closes #43812 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ndling Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Summary: DRAFT. Makes
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43848 does not have the 'implementation' label and has only 68 new lines of code in business logic directories (≤100 threshold). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This pull request makes the log-download rate-limit waiting logic context-cancellable so Ctrl-C (or deadlines) can interrupt long waits instead of blocking on time.Sleep.
Changes:
- Added
sleepWithContext(ctx, d)and updatedcheckAndWaitForRateLimitto accept acontext.Contextand use context-aware sleeping. - Updated the logs orchestrator to pass
activeCtxand treat context cancellation/timeouts during rate-limit waits as a normal loop continuation toward graceful shutdown. - Added unit tests covering
sleepWithContextcancellation, deadline expiry, and normal timer completion.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_rate_limit.go | Introduces context-aware sleeping and threads ctx through rate-limit wait logic. |
| pkg/cli/logs_orchestrator.go | Passes activeCtx into rate-limit waiting and handles ctx-cancellation/timeout outcomes gracefully. |
| pkg/cli/logs_rate_limit_test.go | Adds unit tests validating sleepWithContext behavior under cancellation/deadlines and normal completion. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| func sleepWithContext(ctx context.Context, d time.Duration) error { | ||
| timer := time.NewTimer(d) | ||
| defer timer.Stop() | ||
| select { |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (6 tests)
Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
The implementation is clean and correct.
sleepWithContextcorrectly usestime.NewTimer+selectonctx.Done()withdefer timer.Stop()to prevent timer leaks.- All four
time.Sleepcall sites are replaced, so no sleep can block Ctrl-C. - The
continueon context cancellation (orchestrator line 374) is safe: the top-of-loopselect { case <-activeCtx.Done() }check immediately catches the cancelled context and exits gracefully, anditerationis not incremented, preserving loop invariants. - Unit tests cover the three relevant cases: already-cancelled context, deadline expiry, and normal timer fire.
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 37.9 AIC · ⌖ 5.97 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Non-blocking observation
The elapsed < time.Second timing threshold in TestSleepWithContextCancellation (and similarly in TestSleepWithContextDeadlineExceeded) is too loose to verify that cancellation is actually prompt — a 1-second bound only catches catastrophic regressions. Tightening to ~100ms would give the tests real signal. See inline comment for the suggested change.
Everything else is clean and correct: the sleepWithContext helper is idiomatic, all four former time.Sleep callsites are covered, the defer timer.Stop() is placed correctly, and the orchestrator continue path correctly routes through the existing top-of-loop activeCtx.Done() check.
🔎 Code quality review by PR Code Quality Reviewer · 55.7 AIC · ⌖ 6.16 AIC · ⊞ 5.5K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_rate_limit_test.go:129
Test timing threshold is too loose to verify promptness: elapsed < time.Second would pass even if cancellation took 900ms, giving no signal about actual responsiveness.
<details>
<summary>💡 Suggested fix</summary>
Tighten both the Cancellation and DeadlineExceeded tests to assert promptness:
// In TestSleepWithContextCancellation
assert.Less(t, elapsed, 100*time.Millisecond, "should return near-instantly on already-cancelled context")
// In TestSleepWithContextDeadlineExcee…
</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — overall a clean, well-motivated fix. Leaving as COMMENT rather than APPROVE due to a few actionable items.
📋 Key Themes & Highlights
Key Themes
- Missing regression test for the root bug:
sleepWithContextis well-tested in isolation, but there's no test provingcheckAndWaitForRateLimitas a whole unblocks on cancellation. The bug (blocking 60 min through Ctrl-C) can't be regression-tested by the unit tests alone becausefetchRateLimitis not injectable. - Silent error discard in the fetch-failure path: when
fetchRateLimitfails and the context is also cancelled, the original fetch error is dropped; the caller only seescontext.Canceled. continuecomment clarity: thecontinuein the orchestrator is correct but the reason (preserveCanceledvsDeadlineExceededbranching above) deserves a tighter comment to prevent a well-meaning simplification.- Potential flakiness in
TestSleepWithContextTimerFires: 1 ms is at the edge of OS timer resolution on loaded CI machines.
Positive Highlights
- ✅
sleepWithContextis a deep module: tiny interface (3 lines), handles all three cases (cancel, deadline, normal), documented clearly. - ✅ All four
time.Sleepcall sites updated consistently — no partial fix. - ✅ Three unit tests cover the three observable paths of the helper.
- ✅ PR description is precise and accurate: root cause, fix, and tests all described correctly.
- ✅
defer timer.Stop()is present — no timer leak on cancellation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 77.6 AIC · ⌖ 5.6 AIC · ⊞ 6.7K
Comment /matt to run again
| if errors.Is(rlErr, context.Canceled) || errors.Is(rlErr, context.DeadlineExceeded) { | ||
| // Context was cancelled or timed out during the rate-limit wait; | ||
| // let the top-of-loop check handle the graceful exit path. | ||
| continue |
There was a problem hiding this comment.
[/diagnosing-bugs] The continue here is correct — it routes through the top-of-loop activeCtx.Done() check, which preserves the Canceled vs DeadlineExceeded distinction. However, a future maintainer might replace it with break outerLoop, which would silently suppress the "Operation cancelled" message and treat Ctrl-C as a successful run. The comment should make this trade-off explicit.
💡 Suggested comment improvement
// Context was cancelled or timed out during the rate-limit wait.
// Use continue (not break outerLoop) so the top-of-loop activeCtx.Done()
// handler can distinguish context.Canceled (user Ctrl-C → return error) from
// context.DeadlineExceeded (our own timeout → set timeoutReached, break cleanly).
continue@copilot please address this.
| // Best-effort: fall back to static cooldown so the caller can continue. | ||
| logsRateLimitLog.Printf("Could not fetch rate limit, using static cooldown: %v", err) | ||
| time.Sleep(APICallCooldown) | ||
| if sleepErr := sleepWithContext(ctx, APICallCooldown); sleepErr != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] When fetchRateLimit fails, the function sleeps the cooldown and then returns the original fetchRateLimit error. If sleepWithContext also returns an error (context cancelled), the original rate-limit fetch error is silently swallowed — the caller only sees context.Canceled, losing the root cause.
💡 Suggested fix
Consider wrapping both errors:
if sleepErr := sleepWithContext(ctx, APICallCooldown); sleepErr != nil {
return fmt.Errorf("%w (during cooldown after rate-limit fetch failure: %v)", sleepErr, err)
}
return errOr at minimum document that err is dropped when the context is cancelled so future readers don't wonder.
@copilot please address this.
| // timer fires before context cancellation. | ||
| func TestSleepWithContextTimerFires(t *testing.T) { | ||
| ctx := context.Background() | ||
| err := sleepWithContext(ctx, time.Millisecond) |
There was a problem hiding this comment.
[/tdd] TestSleepWithContextTimerFires uses a 1 ms sleep — this can be flaky on a heavily loaded CI runner where timer resolution is coarser. The other two tests correctly avoid real waits by pre-cancelling; this test exercises the normal path, but a 1 ms timer fire is right on the edge of OS scheduler jitter.
💡 Suggestion
Use a slightly longer duration (e.g. 5*time.Millisecond) and set an upper-bound assertion to prevent future regressions from the test accidentally blocking:
start := time.Now()
err := sleepWithContext(ctx, 5*time.Millisecond)
elapsed := time.Since(start)
require.NoError(t, err)
assert.Less(t, elapsed, time.Second, "timer should have fired, not blocked")Adding the upper-bound check also makes the test self-documenting — it proves the function returned because the timer fired, not because something else unblocked it.
@copilot please address this.
| cancel() // cancel immediately | ||
|
|
||
| start := time.Now() | ||
| err := sleepWithContext(ctx, 10*time.Second) |
There was a problem hiding this comment.
[/tdd] Missing a test for the integration between checkAndWaitForRateLimit and cancellation. The three new sleepWithContext tests are solid unit tests, but there's no test verifying that checkAndWaitForRateLimit itself propagates a cancelled context correctly when the rate-limit fetch returns data indicating a long wait.
💡 Suggested test sketch
func TestCheckAndWaitForRateLimitContextCancelled(t *testing.T) {
// Patch fetchRateLimit to return a future reset time requiring a long wait
// (using a test double or by stubbing the HTTP call), then cancel the context.
// Verify that checkAndWaitForRateLimit returns context.Canceled promptly.
}This is the regression test /diagnosing-bugs recommends: prove the root bug (blocking for up to 60 min despite Ctrl-C) cannot reappear without the test catching it.
@copilot please address this.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in commit Reran local checks on the updated branch:
Current PR workflows for this head SHA are in |
|
@copilot please run the
|
Ran the
Hand-off: a maintainer needs to re-trigger CI (close/reopen PR, |
|
🎉 This pull request is included in a new release. Release: |
checkAndWaitForRateLimitused baretime.Sleepcalls that could block for up to ~60 minutes waiting for a GitHub API rate-limit reset, completely ignoring the context already held byDownloadWorkflowLogs. Ctrl-C had no effect during this wait.Changes
pkg/cli/logs_rate_limit.go— Addctx context.Contextas first parameter tocheckAndWaitForRateLimit. Extract asleepWithContexthelper that replaces all fourtime.Sleepcalls:pkg/cli/logs_orchestrator.go— PassactiveCtxat the call site; treat returnedcontext.Canceled/context.DeadlineExceedederrors as acontinueso the existing top-of-loopactiveCtx.Done()check handles graceful shutdown.pkg/cli/logs_rate_limit_test.go— Add unit tests forsleepWithContextcovering: already-cancelled context, deadline expiry, and normal timer fire.