Cancel sibling logs targets promptly when shared --count limit is reached - #60323
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. 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.
|
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. 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 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 #60323This PR triggers ADR enforcement because it adds more than 100 new lines in business-logic directories ( Evidence reviewed
OutcomeI did not find an existing ADR in the PR body or on the branch that fully covered this specific decision, so I generated a draft ADR:
Inferred decisionThe PR makes this architectural decision:
Next actionPlease review and refine the draft ADR so the decision, trade-offs, and consequences reflect maintainer intent before merging.
|
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation can skip resumable runs, surface expected-cancellation errors, and produce a scheduler-dependent test failure.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds prompt sibling cancellation when multi-target log collection reaches its shared count limit.
Changes:
- Cancels active targets when the shared budget is exhausted.
- Treats count-limit cancellation as successful termination.
- Adds isolated multi-target integration coverage.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_orchestrator_download.go |
Handles expected cancellation during collection. |
pkg/cli/logs_multi.go |
Adds shared target cancellation. |
pkg/cli/logs_multi_target_count_integration_test.go |
Tests shared limits and cancellation latency. |
Makefile |
Adds a dedicated integration-test target. |
.github/workflows/ci.yml |
Isolates the new tests in CI. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| targetsCtx := ctx | ||
| if countLimit != nil { | ||
| var cancel context.CancelFunc | ||
| targetsCtx, cancel = context.WithCancel(ctx) |
| for _, target := range targets { | ||
| wg.Go(func() { | ||
| resultChannel <- collectSingleLogsTarget(ctx, opts, target, shared) | ||
| resultChannel <- collectSingleLogsTarget(targetsCtx, opts, target, shared) |
| var cancel context.CancelFunc | ||
| targetsCtx, cancel = context.WithCancel(ctx) | ||
| defer cancel() | ||
| countLimit.cancel = cancel |
| limitReached := make(chan struct{}) | ||
| elapsed := make(chan time.Duration, 2) | ||
|
|
||
| collectWorkflowLogsForTarget = func(ctx context.Context, opts LogsDownloadOptions) (workflowLogsResult, error) { | ||
| require.NoError(t, os.MkdirAll(opts.OutputDir, 0o755)) | ||
| if opts.WorkflowName == "first" { | ||
| // Immediately consume the only slot in the shared budget, then let | ||
| // the other targets know the limit has now been hit. | ||
| require.True(t, opts.countLimit.tryAdd()) | ||
| close(limitReached) | ||
| return workflowLogsResult{}, nil | ||
| } | ||
|
|
||
| // Simulate a target that is already mid-operation (e.g. downloading an | ||
| // over-fetched batch) at the moment a sibling exhausts the shared | ||
| // budget. It should be interrupted well before simulatedInFlightDuration | ||
| // elapses instead of running to completion. | ||
| <-limitReached |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The root-cause fix (a cancel context.CancelFunc on logsCountLimit, wired to a derived targetsCtx in collectLogsTargets, plus careful reclassification of context.Canceled at every call site that could see it) is well-targeted and I verified both new integration tests pass locally (TestLogsMultiTargetCountIsSharedMaxAcrossTargets, TestLogsMultiTargetCancelsSiblingsWhenSharedCountLimitReached). No correctness issues found in the production code paths.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: the new cancellation-classification branches (
handleLogsBatchError,shouldStopLogsIteration) are only proven end-to-end via the new integration tests; a few fast unit tests alongside the existingeffectiveLogsBatchCountunit tests would pin the exact logic independent of timing. - Test hygiene:
require.*calls inside goroutines spawned viawg.Go(in the fakes forcollectWorkflowLogsForTarget) don't fail safely off the main test goroutine — worth switching toassert.*there. - Minor flakiness risk: the 1s cancellation-latency threshold in the new sibling-cancellation test could be tight under loaded CI (
-parallel=8); a wider or relative margin would reduce false failures.
Positive Highlights
- ✅ Root cause correctly identified and fixed at the source (
tryAddtriggers cancellation exactly once viasync.Once) rather than patched at symptom sites only. - ✅ Thorough, well-commented reclassification of
context.Canceledacross all affected call sites so a shared-budget exhaustion is never misreported as a real error. - ✅ Strong regression tests that reproduce the original bug (shared vs. per-target
--count) and prove the fix's latency bound, isolated into a dedicated CI job.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 150.2 AIC · ⌖ 15.2 AIC · ⊞ 10.4K
Comment /matt to run again
|
|
||
| const simulatedInFlightDuration = 5 * time.Second | ||
| const maxAcceptableCancelLatency = 1 * time.Second | ||
|
|
There was a problem hiding this comment.
[/tdd] Hard-coded latency thresholds (maxAcceptableCancelLatency = 1s) in a concurrency test risk flakiness on loaded CI runners (this job already runs with -parallel=8 alongside other suites per ci.yml).
💡 Suggestion
Consider widening the margin (e.g. 2–3s) or asserting a relative bound (elapsed well under simulatedInFlightDuration, e.g. < simulatedInFlightDuration/2) instead of a fixed absolute value, so scheduling jitter under CI load doesn't intermittently fail a correctness-proving regression test.
@copilot please address this.
|
|
||
| const availableRunsForTarget = 5 | ||
| collectWorkflowLogsForTarget = func(_ context.Context, opts LogsDownloadOptions) (workflowLogsResult, error) { | ||
| require.NotNil(t, opts.countLimit, "multi-target downloads must share a countLimit across targets") |
There was a problem hiding this comment.
[/tdd] require.NoError/require.True are called from goroutines spawned by wg.Go in collectLogsTargets (via the faked collectWorkflowLogsForTarget), not the test's own goroutine.
💡 Why this matters
testify's require calls t.FailNow() on failure, which only works correctly from the test's own goroutine — from another goroutine it can panic the process or silently fail to stop the test (see testify docs and the well-known Go testing pitfall). If any of these assertions ever fail under CI, the failure could manifest as a confusing panic/hang instead of a clean, attributable test failure.
Prefer assert.* inside these goroutines (which only marks the test failed without calling FailNow), reserving require.* for the main test goroutine after joining.
@copilot please address this.
| } | ||
|
|
||
| func handleLogsBatchError(state *logsCollectionState, ctx context.Context, err error) (bool, error) { | ||
| func handleLogsBatchError(state *logsCollectionState, fetchAllInRange bool, countLimit *logsCountLimit, err error) (bool, error) { |
There was a problem hiding this comment.
[/tdd] The new cancellation-classification branches (handleLogsBatchError, shouldStopLogsIteration, markSharedLogsCountReached treating context.Canceled as expected-success when countLimit.isReached()) are only exercised indirectly through the two new (go/redacted):build integration tests, with no direct unit tests.
💡 Suggestion
These are small, pure functions (handleLogsBatchError(state, fetchAllInRange, countLimit, err), shouldStopLogsIteration(runtime, opts)) that could be unit-tested in logs_orchestrator_unit_test.go (which already covers sibling helpers like effectiveLogsBatchCount) without spinning up the full multi-target integration harness — e.g. asserting that a context.Canceled error with an exhausted countLimit returns (true, nil) from handleLogsBatchError but (true, err) when the limit is not reached. This would pin the exact classification logic independently of timing-sensitive integration behavior and run in the fast unit suite.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
gh aw logs --count Nwith multiple workflow targets shares one run-count budget across all targets, but a target already mid-batch had no way to be interrupted once a sibling exhausted that budget — it would finish downloading its full (potentially over-fetched) chunk before ever re-checking the limit.Root cause
logsCountLimit.isReached()was only consulted at three checkpoints: before a target starts, right after it acquires a semaphore slot, and at the top of each new iteration. Nothing interrupted a target already in flight.Fix: cancel on limit reached
logsCountLimitnow carries acancel context.CancelFunc(+sync.Once), invoked the instant the atomic counter reachesmax.collectLogsTargetsderives a cancellable child context (targetsCtx) shared by all targets and wires it tocountLimit.cancel. Cancellation propagates through the existingactiveCtxplumbing (semaphore waits, per-run download tasks) with no changes needed downstream.Avoid misclassifying the cancellation as an error
Several call sites treated any
ctx.Done()/context.Canceledas a real failure or printed a user-facing "Operation cancelled" warning. These now checkcountLimit.isReached()first and treat it as an expected, silent, successful stop instead:collectSingleLogsTarget's semaphore-wait and post-waitctx.Err()checksshouldStopLogsIterationhandleLogsBatchErrorwaitForLogsRateLimiterror branch incollectProcessedWorkflowRuns, which previously discarded all already-accumulatedprocessedRunson cancellationTests
TestLogsMultiTargetCountIsSharedMaxAcrossTargets: regression test proving--countis a shared max, not per-target, across 3 concurrent targets.TestLogsMultiTargetCancelsSiblingsWhenSharedCountLimitReached: proves a sibling target blocked mid-operation is interrupted well before it would naturally finish, once another target exhausts the shared budget.Both are wired into CI as a dedicated matrix job to isolate them from the broader logs test suite.