Skip to content

Refactor audit analysis fanout to errgroup.WithContext and propagate cancellation#46973

Merged
pelikhan merged 3 commits into
mainfrom
copilot/migrate-collectauditanalysisresults-to-errgroup
Jul 21, 2026
Merged

Refactor audit analysis fanout to errgroup.WithContext and propagate cancellation#46973
pelikhan merged 3 commits into
mainfrom
copilot/migrate-collectauditanalysisresults-to-errgroup

Conversation

Copilot AI commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

collectAuditAnalysisResults fanned out ~14 goroutines via sync.WaitGroup, which hid cancellation/error signaling from the caller. This change moves the fanout to errgroup.WithContext so cancellation is observable while preserving existing soft-failure behavior for per-analysis I/O.

  • Core concurrency migration

    • Replaced sync.WaitGroup with g, gctx := errgroup.WithContext(ctx) in collectAuditAnalysisResults.
    • Updated the function signature to return (auditAnalysisResults, error) and propagated the error to AuditWorkflowRun.
  • Context propagation across analysis launchers

    • Threaded gctx through all launch* helpers and runAuditAnalysis.
    • Converted goroutine sites to g.Go(func() error { ... }).
  • Failure semantics preserved

    • Kept non-fatal analysis failures as log-and-continue (return nil) to match prior behavior.
    • Allowed hard cancellation to surface through g.Wait().
  • Type/import cleanup

    • Added golang.org/x/sync/errgroup import in audit.go.
    • Removed sync import from audit.go.
  • Focused regression coverage

    • Added tests for:
      • cancellation surfacing as context.Canceled from collectAuditAnalysisResults
      • soft failures in runAuditAnalysis remaining non-fatal to the group
g, gctx := errgroup.WithContext(ctx)

launchCoreAuditAnalyses(g, gctx, &results, run, runOutputDir, verbose)
if hasFirewallArtifact {
	launchFirewallAuditAnalyses(g, gctx, &results, runOutputDir, verbose)
}
launchSupplementalAuditAnalyses(g, gctx, &results, runOutputDir, verbose)

if err := g.Wait(); err != nil {
	return results, err
}

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Migrate collectAuditAnalysisResults to use errgroup.WithContext Refactor audit analysis fanout to errgroup.WithContext and propagate cancellation Jul 21, 2026
Copilot AI requested a review from pelikhan July 21, 2026 05:14
@pelikhan
pelikhan marked this pull request as ready for review July 21, 2026 05:14
Copilot AI review requested due to automatic review settings July 21, 2026 05:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors audit analysis fanout to use errgroup and propagate context cancellation.

Changes:

  • Replaces sync.WaitGroup with errgroup.WithContext.
  • Threads context through audit launchers and returns cancellation errors.
  • Adds cancellation and soft-failure tests.
Show a summary per file
File Description
pkg/cli/audit.go Migrates concurrent audit collection to errgroup.
pkg/cli/audit_concurrency_test.go Adds focused concurrency regression tests.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

pkg/cli/audit.go:740

  • The context stops at this launcher: fetchJobDetailsWithCounts invokes workflow.RunGHCombined, which uses context.Background() (pkg/workflow/github_cli.go:213-225). Canceling an audit therefore cannot terminate a blocked gh api subprocess, and g.Wait() waits for it to exit. Add a context-aware helper path using RunGHCombinedContext, pass gctx, and treat its cancellation error as hard rather than as the existing soft I/O failure.
func launchJobDetailsAnalysis(g *errgroup.Group, gctx context.Context, results *auditAnalysisResults, runID int64, verbose bool) {
	g.Go(func() error {
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread pkg/cli/audit.go
Comment on lines +675 to +678
if err := g.Wait(); err != nil {
return results, err
}
launchSupplementalAuditAnalyses(&wg, &results, runOutputDir, verbose)
wg.Wait()
return results
return results, nil
@pelikhan

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts on this branch.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts on this branch.

Resolved in de1b85b7c2 by merging origin/main into this branch and fixing the merge conflicts (including pkg/cli/audit.go).

@github-actions

Copy link
Copy Markdown
Contributor

Excellent work on this concurrency refactor! 🎯 The migration from sync.WaitGroup to errgroup.WithContext in collectAuditAnalysisResults is clean and well-executed.

What stands out:

  • ✅ Context cancellation now properly surfaces to callers (fixes [Code Quality] Migrate collectAuditAnalysisResults from sync.WaitGroup to errgroup.WithContext #46942)
  • ✅ Soft-failure behavior preserved (analysis I/O errors log and continue, not fatal)
  • ✅ Tests cover both scenarios: cancellation handling and non-fatal failures
  • ✅ Follows existing patterns in the codebase (mcp_inspect_inspector.go)
  • ✅ Import cleanup: removed sync, added errgroup
  • ✅ All launch* helpers threaded with gctx for proper propagation

This PR looks ready for review by the team. The refactor strengthens error handling observability without changing functional behavior for the existing audit flow.

Generated by ✅ Contribution Check · 68.1 AIC · ⌖ 9.02 AIC · ⊞ 6.2K ·

@pelikhan
pelikhan merged commit bdd171e into main Jul 21, 2026
23 checks passed
@pelikhan
pelikhan deleted the copilot/migrate-collectauditanalysisresults-to-errgroup branch July 21, 2026 05:56
@github-actions

Copy link
Copy Markdown
Contributor

🎉 This pull request is included in a new release.

Release: v0.82.15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code Quality] Migrate collectAuditAnalysisResults from sync.WaitGroup to errgroup.WithContext

3 participants