From 58b6bdb0f2bfe7bee674c13b7989000ef929063a Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Wed, 4 Feb 2026 12:53:35 -0500 Subject: [PATCH] Use requeue polling instead of check_run events for CI completion When CI is pending, the reconciler now returns RequeueAfterSeconds: 60 to poll for CI completion every minute, instead of relying on check_run events which are difficult to route through the workqueue. - Remove check-run-workqueue from trigger.tf - Remove processCheckRunEvent and related parsing code - Add requeue when CIFixPending is true - Update agent.md to remove check_run event documentation --- driftlessaf/agent.md | 108 --------------------- driftlessaf/cmd/pr-reconciler/main.go | 105 ++------------------ driftlessaf/cmd/pr-reconciler/main_test.go | 62 ------------ driftlessaf/trigger.tf | 40 -------- 4 files changed, 8 insertions(+), 307 deletions(-) diff --git a/driftlessaf/agent.md b/driftlessaf/agent.md index 71b5b5e..b32279a 100644 --- a/driftlessaf/agent.md +++ b/driftlessaf/agent.md @@ -1184,114 +1184,6 @@ func runCIFixer(ctx context.Context, gh *github.Client, agent claudeexecutor.Int --- -## Event Handling for CI Completion - -### The check_run Event Problem - -The CI fixer needs to be notified when CI checks complete so it can re-evaluate failing PRs. However, there's a complication with how GitHub events are processed: - -**The issue:** The `github-events` Terraform module from `terraform-infra-common` does NOT set the `pullrequesturl` CloudEvents extension for `check_run` events. The relevant code in the trampoline is commented out: - -```go -// case "check_run": -// if len(info.CheckRun.PullRequests) > 0 { -// prNumber = info.CheckRun.PullRequests[0].Number -// } -``` - -This means `check_run` events don't match the primary workqueue filter (which uses `extension_key = "pullrequesturl"`), so they never trigger reconciliation. - -### Solution: Separate check_run Workqueue - -We deploy a second workqueue specifically for `check_run` events: - -```hcl -# In trigger.tf -module "check-run-workqueue" { - source = "chainguard-dev/common/infra//modules/cloudevents-workqueue" - - # Filter for completed check_run events - filters = [{ - attributes = { - type = "dev.chainguard.github.check_run" - action = "completed" - } - }] - - # Use repo URL as key (check_run doesn't have pullrequesturl) - extension_key = "repo" - - workqueue = { - name = module.pr-reconciler.receiver.name - } -} -``` - -### How the Reconciler Handles check_run Events - -When the reconciler receives a repo-keyed event (not a PR URL), it: - -1. **Detects the key format**: Distinguishes `https://github.com/owner/repo` from `https://github.com/owner/repo/pull/123` - -2. **Lists relevant PRs**: Queries for open PRs in that repo with the `ci-autofix` label - -3. **Re-enqueues them**: Returns the PR URLs in `ProcessResponse.Requeue` so they get reconciled - -```go -func (r *PRReconciler) processCheckRunEvent(ctx context.Context, owner, repo string) (*workqueue.ProcessResponse, error) { - // List open PRs with ci-autofix label - prs, _, err := gh.PullRequests.List(ctx, owner, repo, &github.PullRequestListOptions{ - State: "open", - }) - - // Filter to PRs with the label and re-queue them - var requeue []string - for _, pr := range prs { - if hasLabel(pr.Labels, r.cfg.CIFixerLabel) { - requeue = append(requeue, pr.GetHTMLURL()) - } - } - - return &workqueue.ProcessResponse{Requeue: requeue}, nil -} -``` - -### Event Flow Diagram - -``` -┌───────────────┐ ┌──────────────────┐ ┌─────────────────┐ -│ CI Workflow │────▶│ check_run event │────▶│ check-run-queue │ -│ completes │ │ (repo-keyed) │ │ │ -└───────────────┘ └──────────────────┘ └────────┬────────┘ - │ - ▼ - ┌─────────────────┐ - │ Reconciler │ - │ (processCheck │ - │ RunEvent) │ - └────────┬────────┘ - │ - Lists PRs with ci-autofix label - │ - ▼ -┌───────────────┐ ┌──────────────────┐ ┌─────────────────┐ -│ Reconciler │◀────│ PR URLs │◀────│ Requeue PRs │ -│ (PR process) │ │ re-enqueued │ │ │ -└───────────────┘ └──────────────────┘ └─────────────────┘ -``` - -### Why Not Patch terraform-infra-common? - -While uncommenting the `check_run` case in `terraform-infra-common` would be cleaner, this approach: - -1. **Doesn't require upstream changes** - Works with the module as-is -2. **Is explicit** - The separate workqueue makes the event handling obvious -3. **Is flexible** - We can add custom filtering (e.g., only certain check names) - -If the upstream module is eventually updated to set `pullrequesturl` for `check_run` events, this separate workqueue can be removed. - ---- - ## Quality Gates Before each phase transition, ensure: diff --git a/driftlessaf/cmd/pr-reconciler/main.go b/driftlessaf/cmd/pr-reconciler/main.go index 4b928e1..d65cce7 100644 --- a/driftlessaf/cmd/pr-reconciler/main.go +++ b/driftlessaf/cmd/pr-reconciler/main.go @@ -289,12 +289,6 @@ func (r *PRReconciler) getTransportForRepo(ctx context.Context, owner, repo stri // prURLRegex matches GitHub PR URLs like https://github.com/owner/repo/pull/123 var prURLRegex = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)$`) -// repoURLRegex matches GitHub repo URLs like https://github.com/owner/repo -var repoURLRegex = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)$`) - -// repoSubjectRegex matches repo subject format like owner/repo (from CloudEvents subject) -var repoSubjectRegex = regexp.MustCompile(`^([^/]+)/([^/]+)$`) - // ciState represents the evaluated state of CI checks type ciState int @@ -360,36 +354,13 @@ func parsePRURL(url string) (owner, repo string, number int, err error) { return matches[1], matches[2], number, nil } -// parseRepoKey parses a repo identifier which can be either: -// - Full URL: https://github.com/owner/repo -// - Subject format: owner/repo (from CloudEvents subject attribute) -func parseRepoKey(key string) (owner, repo string, err error) { - // Try full URL format first - matches := repoURLRegex.FindStringSubmatch(key) - if matches != nil { - return matches[1], matches[2], nil - } - // Try subject format (owner/repo) - matches = repoSubjectRegex.FindStringSubmatch(key) - if matches != nil { - return matches[1], matches[2], nil - } - return "", "", fmt.Errorf("invalid repo key: %s", key) -} - func (r *PRReconciler) Process(ctx context.Context, req *workqueue.ProcessRequest) (*workqueue.ProcessResponse, error) { log := clog.FromContext(ctx).With("key", req.Key) owner, repo, number, err := parsePRURL(req.Key) if err != nil { - // Key is not a PR URL - might be a repo URL from check_run events. - // Try to parse as repo URL and handle check_run completion. - owner, repo, parseErr := parseRepoKey(req.Key) - if parseErr != nil { - log.Warnf("skipping unknown key format: %v", err) - return &workqueue.ProcessResponse{}, nil - } - return r.processCheckRunEvent(ctx, owner, repo) + log.Warnf("skipping unknown key format: %v", err) + return &workqueue.ProcessResponse{}, nil } log = log.With("owner", owner, "repo", repo, "number", number) @@ -525,6 +496,12 @@ func (r *PRReconciler) Process(ctx context.Context, req *workqueue.ProcessReques log.Infof("reconciliation complete: labels=%v ci_fix=%v", details.LabelsApplied, details.CIFixSuccess) + // If CI is pending, requeue to check again in 60 seconds + if details.CIFixPending { + log.Infof("CI pending, requeueing to check again in 60 seconds") + return &workqueue.ProcessResponse{RequeueAfterSeconds: 60}, nil + } + return &workqueue.ProcessResponse{}, nil } @@ -794,69 +771,3 @@ func hasLabel(labels []*github.Label, name string) bool { func (r *PRReconciler) GetKeyState(ctx context.Context, req *workqueue.GetKeyStateRequest) (*workqueue.KeyState, error) { return &workqueue.KeyState{}, nil } - -// processCheckRunEvent handles check_run completion events. -// When a CI check completes, we need to re-evaluate any open PRs that have the -// ci-autofix label and might need fixing. Since check_run events don't include -// a PR URL directly (the github-events module doesn't set pullrequesturl for them), -// we receive them keyed by repo URL and look up relevant PRs ourselves. -func (r *PRReconciler) processCheckRunEvent(ctx context.Context, owner, repo string) (*workqueue.ProcessResponse, error) { - log := clog.FromContext(ctx).With("owner", owner, "repo", repo) - log.Infof("processing check_run event for repo") - - // If CI fixer is not enabled, nothing to do - if r.ciFixerAgent == nil { - log.Debugf("CI fixer not enabled, skipping check_run event") - return &workqueue.ProcessResponse{}, nil - } - - gh, err := r.getClientForRepo(ctx, owner, repo) - if err != nil { - return nil, fmt.Errorf("getting client: %w", err) - } - - // List open PRs with the ci-autofix label - prs, _, err := gh.PullRequests.List(ctx, owner, repo, &github.PullRequestListOptions{ - State: "open", - ListOptions: github.ListOptions{ - PerPage: 100, - }, - }) - if err != nil { - return nil, fmt.Errorf("listing PRs: %w", err) - } - - // Filter to PRs with ci-autofix label - var relevantPRs []*github.PullRequest - for _, pr := range prs { - if hasLabel(pr.Labels, r.cfg.CIFixerLabel) { - relevantPRs = append(relevantPRs, pr) - } - } - - if len(relevantPRs) == 0 { - log.Infof("no open PRs with %s label", r.cfg.CIFixerLabel) - return &workqueue.ProcessResponse{}, nil - } - - log.Infof("found %d PRs with %s label to check", len(relevantPRs), r.cfg.CIFixerLabel) - - // Process each relevant PR directly - // We can't re-enqueue to the workqueue, so we process them inline. - // This is fine because check_run events are relatively infrequent. - for _, pr := range relevantPRs { - prURL := pr.GetHTMLURL() - log.Infof("triggering reconciliation for PR %s", prURL) - - // Call Process with a synthetic request for each PR - _, err := r.Process(ctx, &workqueue.ProcessRequest{ - Key: prURL, - }) - if err != nil { - log.Warnf("failed to process PR %s: %v", prURL, err) - // Continue processing other PRs - } - } - - return &workqueue.ProcessResponse{}, nil -} diff --git a/driftlessaf/cmd/pr-reconciler/main_test.go b/driftlessaf/cmd/pr-reconciler/main_test.go index 41aa2a0..38ba00a 100644 --- a/driftlessaf/cmd/pr-reconciler/main_test.go +++ b/driftlessaf/cmd/pr-reconciler/main_test.go @@ -313,65 +313,3 @@ func TestParsePRURL(t *testing.T) { }) } } - -func TestParseRepoKey(t *testing.T) { - for _, tt := range []struct { - desc string - key string - wantOwner string - wantRepo string - wantErr bool - }{{ - desc: "valid repo URL", - key: "https://github.com/owner/repo", - wantOwner: "owner", - wantRepo: "repo", - }, { - desc: "valid repo URL with dashes", - key: "https://github.com/my-org/my-repo", - wantOwner: "my-org", - wantRepo: "my-repo", - }, { - desc: "subject format owner/repo", - key: "owner/repo", - wantOwner: "owner", - wantRepo: "repo", - }, { - desc: "subject format with dashes", - key: "my-org/my-repo", - wantOwner: "my-org", - wantRepo: "my-repo", - }, { - desc: "PR URL is not a repo key", - key: "https://github.com/owner/repo/pull/123", - wantErr: true, - }, { - desc: "empty string", - key: "", - wantErr: true, - }, { - desc: "just owner no repo", - key: "owner", - wantErr: true, - }} { - t.Run(tt.desc, func(t *testing.T) { - owner, repo, err := parseRepoKey(tt.key) - if tt.wantErr { - if err == nil { - t.Errorf("expected error, got owner=%q repo=%q", owner, repo) - } - return - } - if err != nil { - t.Errorf("unexpected error: %v", err) - return - } - if owner != tt.wantOwner { - t.Errorf("owner: got %q, want %q", owner, tt.wantOwner) - } - if repo != tt.wantRepo { - t.Errorf("repo: got %q, want %q", repo, tt.wantRepo) - } - }) - } -} diff --git a/driftlessaf/trigger.tf b/driftlessaf/trigger.tf index b4a57eb..5b88749 100644 --- a/driftlessaf/trigger.tf +++ b/driftlessaf/trigger.tf @@ -20,43 +20,3 @@ module "pr-workqueue" { notification_channels = [] deletion_protection = var.deletion_protection } - -# Separate workqueue for check_run events (CI completion notifications). -# -# Why this is needed: -# The github-events module does NOT set the pullrequesturl extension for check_run -# events (the code is commented out in terraform-infra-common). This means check_run -# events don't match the pr-workqueue filter above. -# -# For the CI fixer to work, we need to be notified when CI checks complete so we can -# re-evaluate whether to attempt a fix. This workqueue listens for check_run events -# directly and the reconciler extracts the PR URL from the event payload. -# -# See: https://github.com/chainguard-dev/terraform-infra-common/blob/main/modules/github-events/internal/trampoline/server.go -module "check-run-workqueue" { - source = "chainguard-dev/common/infra//modules/cloudevents-workqueue" - - project_id = var.project_id - name = "check-run-events" - regions = module.networking.regional-networks - broker = module.cloudevent-broker.broker - - # Filter for check_run events with "completed" action - filters = [{ - "type" = "dev.chainguard.github.check_run" - "action" = "completed" - }] - - # Use subject (repo full name like "owner/repo") as the key. - # The trampoline sets event.SetSubject(repoFullName) which becomes ce-subject attribute. - # Note: "repo" extension doesn't exist - the trampoline only sets subject, not a repo extension. - extension_key = "subject" - - workqueue = { - name = module.pr-reconciler.receiver.name - } - - team = var.team - notification_channels = [] - deletion_protection = var.deletion_protection -}