Fix stale pull request merge revisions

docs/reference.md

@@ -163,8 +163,10 @@ The webhook endpoint accepts only signed GitHub `POST` deliveries up to 10 MiB
 and requires exactly one configured project for the installation. Supported
 deliveries return HTTP 202 with `{"accepted":true,"queued":true}`. Unsupported
 event names, conflicted pull requests, and pull requests from fork repositories
-return HTTP 202 with `{"accepted":true,"queued":false}`. Pull request deliveries
-whose merge ref is still being prepared are queued and resolved asynchronously.
+return HTTP 202 with `{"accepted":true,"queued":false}`. Open pull request
+workflows use GitHub's test merge revision for the webhook head. Deliveries wait
+up to two minutes for that revision; unavailable or superseded revisions
+produce a `Failed` delivery.
 
 Queued deliveries are processed asynchronously. Invalid or unsupported workflow
 definitions fail the whole delivery before any `WorkflowRun` resources are

internal/github/client.go

@@ -45,6 +45,13 @@ type Content struct {
 	Type string `json:"type"`
 }
 
+type repositoryCommit struct {
+	SHA     string `json:"sha"`
+	Parents []struct {
+		SHA string `json:"sha"`
+	} `json:"parents"`
+}
+
 // APIError describes a non-success response from the GitHub API.
 type APIError struct {
 	StatusCode int
@@ -169,23 +176,45 @@ func (c *InstallationClient) GetFile(ctx context.Context, owner, repository, fil
 // ResolveRevision resolves a branch, tag, or commit expression to a full commit
 // SHA.
 func (c *InstallationClient) ResolveRevision(ctx context.Context, owner, repository, revision string) (string, error) {
+	commit, err := c.resolveRevision(ctx, owner, repository, revision)
+	if err != nil {
+		return "", err
+	}
+	return commit.SHA, nil
+}
+
+func (c *InstallationClient) resolveRevision(ctx context.Context, owner, repository, revision string) (repositoryCommit, error) {
 	identity := fmt.Sprintf("resolve repository revision %q from %s/%s", revision, owner, repository)
 	requestPath := "repos/" + owner + "/" + repository + "/commits"
-	commits := []struct {
-		SHA string `json:"sha"`
-	}{}
+	commits := []repositoryCommit{}
 	if err := c.client.doJSONWithQuery(ctx, http.MethodGet, requestPath, url.Values{"sha": []string{revision}, "per_page": []string{"1"}}, c.token, &commits); err != nil {
-		return "", fmt.Errorf("%s: %w", identity, err)
+		return repositoryCommit{}, fmt.Errorf("%s: %w", identity, err)
 	}
 	if len(commits) == 0 {
-		return "", fmt.Errorf("%s: GitHub returned no commits", identity)
+		return repositoryCommit{}, fmt.Errorf("%s: GitHub returned no commits", identity)
 	}
-	sha := commits[0].SHA
+	commit := commits[0]
+	sha := commit.SHA
 	decoded, err := hex.DecodeString(sha)
 	if err != nil || len(decoded) != gitSHA1Bytes || sha != strings.ToLower(sha) {
-		return "", fmt.Errorf("%s: GitHub returned invalid commit SHA %q", identity, sha)
+		return repositoryCommit{}, fmt.Errorf("%s: GitHub returned invalid commit SHA %q", identity, sha)
+	}
+	return commit, nil
+}
+
+// ResolvePullRequestRevision resolves a pull request merge ref and reports
+// whether its merge commit includes the expected head commit.
+func (c *InstallationClient) ResolvePullRequestRevision(ctx context.Context, owner, repository, revision, headSHA string) (string, bool, error) {
+	commit, err := c.resolveRevision(ctx, owner, repository, revision)
+	if err != nil {
+		return "", false, err
+	}
+	for _, parent := range commit.Parents {
+		if parent.SHA == headSHA {
+			return commit.SHA, true, nil
+		}
 	}
-	return sha, nil
+	return commit.SHA, false, nil
 }
 
 func (c *Client) doJSON(ctx context.Context, method, requestPath, token string, destination any) error {

internal/github/client_test.go

@@ -126,6 +126,43 @@ func TestResolveRevisionErrorIncludesRepositoryIdentity(t *testing.T) {
 	}
 }
 
+func TestResolvePullRequestRevisionRequiresExpectedHeadParent(t *testing.T) {
+	mergeSHA := strings.Repeat("c", 40)
+	expectedHeadSHA := strings.Repeat("b", 40)
+	for _, tt := range []struct {
+		name      string
+		parentSHA string
+		wantReady bool
+	}{
+		{name: "current", parentSHA: expectedHeadSHA, wantReady: true},
+		{name: "stale", parentSHA: strings.Repeat("a", 40)},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+				if request.URL.Path != "/repos/acme/example/commits" || request.URL.Query().Get("sha") != "refs/pull/9/merge" {
+					http.NotFound(writer, request)
+					return
+				}
+				fmt.Fprintf(writer, `[{"sha":%q,"parents":[{"sha":%q}]}]`, mergeSHA, tt.parentSHA)
+			}))
+			defer server.Close()
+			client, err := NewClient(server.URL, server.Client())
+			if err != nil {
+				t.Fatal(err)
+			}
+			installation := &InstallationClient{client: client, token: "token"}
+
+			resolved, ready, err := installation.ResolvePullRequestRevision(context.Background(), "acme", "example", "refs/pull/9/merge", expectedHeadSHA)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if resolved != mergeSHA || ready != tt.wantReady {
+				t.Errorf("resolved = %q, ready = %v", resolved, ready)
+			}
+		})
+	}
+}
+
 func TestAPIErrorPreservesStatusAndMessage(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) {
 		writer.WriteHeader(http.StatusNotFound)

internal/webhook/delivery.go

@@ -33,6 +33,7 @@ var digestEncoding = base32.StdEncoding.WithPadding(base32.NoPadding)
 const (
 	deliveryLabel          = "actions.kelos.dev/webhook-delivery"
 	deliveryDataKey        = "delivery.json"
+	deliveryRevisionKey    = "resolvedRevision"
 	deliveryStateKey       = "state"
 	deliveryMessageKey     = "message"
 	deliveryRunCountKey    = "workflowRuns"
@@ -42,6 +43,7 @@ const (
 	maxDeliveryBytes       = 900_000
 	maxWorkflowFiles       = 100
 	maxWorkflowJobs        = 1000
+	mergeRefWaitTimeout    = 2 * time.Minute
 	deliveryRetention      = 24 * time.Hour
 )
 
@@ -164,6 +166,34 @@ func missingWorkflowDirectory(err error) bool {
 	return errors.As(err, &apiError) && apiError.StatusCode == http.StatusNotFound && apiError.Message == "Not Found"
 }
 
+func missingPullRequestMergeRef(err error) bool {
+	apiError := &githubclient.APIError{}
+	return errors.As(err, &apiError) && apiError.StatusCode == http.StatusNotFound
+}
+
+func mergeRefRetryInterval(age time.Duration) time.Duration {
+	switch {
+	case age >= 30*time.Second:
+		return 15 * time.Second
+	case age >= 10*time.Second:
+		return 5 * time.Second
+	default:
+		return 2 * time.Second
+	}
+}
+
+func resolveDeliveryRevision(ctx context.Context, installation *githubclient.InstallationClient, owner, repository string, event normalizedEvent) (string, bool, error) {
+	if event.HeadSHA == "" {
+		revision, err := installation.ResolveRevision(ctx, owner, repository, event.ResolveRef)
+		return revision, err == nil, err
+	}
+	revision, ready, err := installation.ResolvePullRequestRevision(ctx, owner, repository, event.ResolveRef, event.HeadSHA)
+	if err != nil && missingPullRequestMergeRef(err) {
+		return "", false, nil
+	}
+	return revision, ready, err
+}
+
 func (r *DeliveryReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
 	object := &corev1.ConfigMap{}
 	if err := r.Get(ctx, request.NamespacedName, object); err != nil {
@@ -179,6 +209,12 @@ func (r *DeliveryReconciler) Reconcile(ctx context.Context, request ctrl.Request
 	if err := json.Unmarshal([]byte(object.Data[deliveryDataKey]), &delivery); err != nil {
 		return ctrl.Result{}, r.finish(ctx, object, deliveryStateFailed, 0, fmt.Sprintf("decode delivery: %v", err))
 	}
+	if revision := object.Data[deliveryRevisionKey]; revision != "" {
+		if !validGitSHA(revision) {
+			return ctrl.Result{}, r.finish(ctx, object, deliveryStateFailed, 0, "delivery contains an invalid resolved revision")
+		}
+		delivery.Event.SHA = revision
+	}
 	reader := r.APIReader
 	project := &actionsv1alpha1.Project{}
 	if err := reader.Get(ctx, client.ObjectKey{Namespace: object.Namespace, Name: delivery.ProjectName}, project); err != nil {
@@ -199,11 +235,25 @@ func (r *DeliveryReconciler) Reconcile(ctx context.Context, request ctrl.Request
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	if delivery.Event.ResolveRef != "" {
-		delivery.Event.SHA, err = installation.ResolveRevision(ctx, delivery.Payload.Repository.Owner.Login, delivery.Payload.Repository.Name, delivery.Event.ResolveRef)
+	if delivery.Event.ResolveRef != "" && delivery.Event.SHA == "" {
+		revision, ready, err := resolveDeliveryRevision(ctx, installation, delivery.Payload.Repository.Owner.Login, delivery.Payload.Repository.Name, delivery.Event)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
+		if !ready {
+			age := r.deliveryAge(object)
+			if age >= mergeRefWaitTimeout {
+				message := fmt.Sprintf("GitHub pull request merge revision did not become ready for head %s within %s", delivery.Event.HeadSHA, mergeRefWaitTimeout)
+				return ctrl.Result{}, r.finish(ctx, object, deliveryStateFailed, 0, message)
+			}
+			retryAfter := mergeRefRetryInterval(age)
+			r.Logger.Debug("waiting for GitHub pull request merge revision", "delivery_id", delivery.DeliveryID, "head_sha", delivery.Event.HeadSHA, "retry_after", retryAfter)
+			return ctrl.Result{RequeueAfter: retryAfter}, nil
+		}
+		delivery.Event.SHA = revision
+		if err := r.persistResolvedRevision(ctx, object, delivery.Event.SHA); err != nil {
+			return ctrl.Result{}, err
+		}
 	}
 	contents, err := installation.ListDirectory(ctx, delivery.Payload.Repository.Owner.Login, delivery.Payload.Repository.Name, project.Spec.WorkflowDirectory, delivery.Event.SHA)
 	if err != nil {
@@ -322,6 +372,15 @@ func (r *DeliveryReconciler) finish(ctx context.Context, object *corev1.ConfigMa
 	return r.Patch(ctx, object, client.MergeFrom(before))
 }
 
+func (r *DeliveryReconciler) persistResolvedRevision(ctx context.Context, object *corev1.ConfigMap, revision string) error {
+	before := object.DeepCopy()
+	if object.Data == nil {
+		object.Data = map[string]string{}
+	}
+	object.Data[deliveryRevisionKey] = revision
+	return r.Patch(ctx, object, client.MergeFrom(before))
+}
+
 func (r *DeliveryReconciler) retain(ctx context.Context, object *corev1.ConfigMap) (ctrl.Result, error) {
 	finishedAt, err := time.Parse(time.RFC3339, object.Data[deliveryFinishedKey])
 	if err != nil {
@@ -344,6 +403,17 @@ func (r *DeliveryReconciler) now() time.Time {
 	return time.Now()
 }
 
+func (r *DeliveryReconciler) deliveryAge(object *corev1.ConfigMap) time.Duration {
+	if object.CreationTimestamp.IsZero() {
+		return 0
+	}
+	age := r.now().Sub(object.CreationTimestamp.Time)
+	if age < 0 {
+		return 0
+	}
+	return age
+}
+
 func (r *DeliveryReconciler) SetupWithManager(manager ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(manager).
 		For(&corev1.ConfigMap{}, builder.WithPredicates(predicate.NewPredicateFuncs(isWebhookDelivery))).