Publishing a campaign with failed jobs fails

Created by: eseliger

• Sourcegraph version: master
• Platform information: macOS 10.15.4, Chrome 81

Steps to reproduce:

1. Have a draft campaign (publishing needs to fail to repro, try no privilege token or so)
2. Click publish on one single changeset
3. It should fail, check the error messages in the campaign status
4. Click publish on the campaign
5. See error

Expected behavior:

It works

Actual behavior:

A pg unique constraint error is returned

This dirty patch fixes it, but I am not sure what the actual behavior should be here. I think it would be expected that already failed are not retried when publishing, because just the will to publish not should not change the reason it failed initially. When someone is convinced it is fixed, he would click "Retry".

Potential fix

diff --git a/enterprise/internal/campaigns/service.go b/enterprise/internal/campaigns/service.go
index 8316436eac..2680d444eb 100644
--- a/enterprise/internal/campaigns/service.go
+++ b/enterprise/internal/campaigns/service.go
@@ -154,24 +154,27 @@ func (s *Service) createChangesetJobsWithStore(ctx context.Context, store *Store
 		return errors.New("cannot create changesets for campaign with no patch set")
 	}
 
-	jobs, _, err := store.ListPatches(ctx, ListPatchesOpts{
+	patches, _, err := store.ListPatches(ctx, ListPatchesOpts{
 		PatchSetID:                c.PatchSetID,
 		Limit:                     -1,
 		OnlyWithDiff:              true,
 		OnlyUnpublishedInCampaign: c.ID,
+		// Don't attempt to create changeset jobs for patches that failed before.
+		// A user would have to retry that job specifically.
+		OmitFailedInCampaign: c.ID,
 	})
 	if err != nil {
 		return err
 	}
 
-	if len(jobs) == 0 {
+	if len(patches) == 0 {
 		return ErrNoPatches
 	}
 
-	for _, job := range jobs {
+	for _, patch := range patches {
 		changesetJob := &campaigns.ChangesetJob{
 			CampaignID: c.ID,
-			PatchID:    job.ID,
+			PatchID:    patch.ID,
 		}
 		err = store.CreateChangesetJob(ctx, changesetJob)
 		if err != nil {
diff --git a/enterprise/internal/campaigns/store.go b/enterprise/internal/campaigns/store.go
index edeee63793..3da82418a8 100644
--- a/enterprise/internal/campaigns/store.go
+++ b/enterprise/internal/campaigns/store.go
@@ -2103,6 +2103,12 @@ type ListPatchesOpts struct {
 	// If this is set only the Patches where diff_stat_added OR
 	// diff_stat_changed OR diff_stat_deleted are NULL.
 	OnlyWithoutDiffStats bool
+
+	// If this is set to a Campaign ID only the Patches are returned that
+	// are _not_ associated with a failed ChangesetJob (meaning
+	// that creating a Changeset on the codehost was attempted but failed)
+	// for the given Campaign.
+	OmitFailedInCampaign int64
 }
 
 // ListPatches lists Patches with the given filters.
@@ -2173,6 +2179,10 @@ func listPatchesQuery(opts *ListPatchesOpts) *sqlf.Query {
 		preds = append(preds, onlyUnpublishedInCampaignQuery(opts.OnlyUnpublishedInCampaign))
 	}
 
+	if opts.OmitFailedInCampaign != 0 {
+		preds = append(preds, omitFailedInCampaignQuery(opts.OmitFailedInCampaign))
+	}
+
 	if opts.OnlyWithoutDiffStats {
 		preds = append(preds, sqlf.Sprintf("(diff_stat_added IS NULL OR diff_stat_deleted IS NULL OR diff_stat_changed IS NULL)"))
 	}
@@ -2200,6 +2210,23 @@ func onlyUnpublishedInCampaignQuery(campaignID int64) *sqlf.Query {
 	return sqlf.Sprintf(onlyUnpublishedInCampaignQueryFmtstr, campaignID)
 }
 
+var omitFailedInCampaignQueryFmtstr = `
+NOT EXISTS (
+  SELECT 1
+  FROM changeset_jobs
+  WHERE
+    patch_id = patches.id
+  AND
+    campaign_id = %s
+  AND
+    changeset_id IS NULL
+)
+`
+
+func omitFailedInCampaignQuery(campaignID int64) *sqlf.Query {
+	return sqlf.Sprintf(omitFailedInCampaignQueryFmtstr, campaignID)
+}
+
 // CreateChangesetJob creates the given ChangesetJob.
 func (s *Store) CreateChangesetJob(ctx context.Context, c *campaigns.ChangesetJob) error {
 	q, err := s.createChangesetJobQuery(c)