fix: avoid hitting Github limit on commit status updates (#688)

#685 reports the Github limit of 1000 commit status updates being
routinely hit.

This PR does too things to avoid this limit being hit:
* ensures the same status update is not sent more than once, thanks to
the use of a cache.
* removes the `running` abstract VCS status, for which Github has no
equivalent, and is not actually used anywhere else in OTF

internal/daemon/daemon.go

@@ -480,6 +480,7 @@ func (d *Daemon) Start(ctx context.Context, started chan struct{}) error {
 				Workspaces:      d.Workspaces,
 				Runs:            d.Runs,
 				Configs:         d.Configs,
+				Cache:           make(map[string]vcs.Status),
 			},
 		},
 		{

internal/github/client.go

@@ -2,14 +2,13 @@ package github
 
 import (
 	"context"
-	"sort"
-
 	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
 	"os"
 	"path"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -190,7 +189,7 @@ func (g *Client) ListRepositories(ctx context.Context, opts vcs.ListRepositories
 		// Apps.ListRepos endpoint does not support ordering on the server-side,
 		// so instead we request *all* repos, page-by-page, and then sort
 		// client-side.
-		var page = 1
+		page := 1
 		for {
 			result, resp, err := g.client.Apps.ListRepos(ctx, &github.ListOptions{
 				PerPage: opts.PageSize,
@@ -448,7 +447,7 @@ func (g *Client) SetStatus(ctx context.Context, opts vcs.SetStatusOptions) error
 
 	var status string
 	switch opts.Status {
-	case vcs.PendingStatus, vcs.RunningStatus:
+	case vcs.PendingStatus:
 		status = "pending"
 	case vcs.SuccessStatus:
 		status = "success"

internal/integration/connect_repo_e2e_test.go

@@ -59,11 +59,8 @@ func TestConnectRepoE2E(t *testing.T) {
 		err = expect.Locator(page.Locator(`//div[@class='widget']//img[@id='run-trigger-github']`)).ToBeVisible()
 		require.NoError(t, err)
 
-		// github should receive three pending status updates followed by a final
-		// update with details of planned resources
-		require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState())
-		require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState())
-		require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState())
+		// GitHub should receive one pending status update followed by a final
+		// update with details of planned resources.
 		require.Equal(t, "pending", daemon.GetStatus(t, ctx).GetState())
 		got := daemon.GetStatus(t, ctx)
 		require.Equal(t, "success", got.GetState())

internal/run/reporter.go

@@ -27,6 +27,16 @@ type (
 		Workspaces reporterWorkspaceClient
 		VCS        reporterVCSClient
 		Runs       reporterRunClient
+
+		// Cache most recently set status for each incomplete run to ensure the
+		// same status is not set more than once on an upstream VCS provider.
+		// This is important to avoid hitting rate limits on VCS providers, e.g.
+		// GitHub has a limit of 1000 status updates on a commit:
+		//
+		// https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status
+		//
+		// key is the run ID.
+		Cache map[string]vcs.Status
 	}
 
 	reporterWorkspaceClient interface {
@@ -58,7 +68,10 @@ func (r *Reporter) Start(ctx context.Context) error {
 			continue
 		}
 		if err := r.handleRun(ctx, event.Payload); err != nil {
-			return err
+			// any error is treated as non-fatal because reporting on runs is
+			// considered "best-effort" rather than an integral operation
+			r.Error(err, "reporting run vcs status", "run_id", event.Payload.ID)
+			return nil
 		}
 	}
 	return pubsub.ErrSubscriptionTerminated
@@ -99,10 +112,8 @@ func (r *Reporter) handleRun(ctx context.Context, run *Run) error {
 		description string
 	)
 	switch run.Status {
-	case RunPending, RunPlanQueued, RunApplyQueued:
+	case RunPending, RunPlanQueued, RunApplyQueued, RunPlanning, RunApplying, RunPlanned, RunConfirmed:
 		status = vcs.PendingStatus
-	case RunPlanning, RunApplying, RunPlanned, RunConfirmed:
-		status = vcs.RunningStatus
 	case RunPlannedAndFinished:
 		status = vcs.SuccessStatus
 		if run.Plan.ResourceReport != nil {
@@ -119,12 +130,37 @@ func (r *Reporter) handleRun(ctx context.Context, run *Run) error {
 	default:
 		return fmt.Errorf("unknown run status: %s", run.Status)
 	}
-	return client.SetStatus(ctx, vcs.SetStatusOptions{
+
+	// Check status cache. If there is a hit for the same run and status then
+	// skip setting the status again.
+	if lastStatus, ok := r.Cache[run.ID]; ok && lastStatus == status {
+		r.V(8).Info("skipped setting duplicate run status on vcs",
+			"run_id", run.ID,
+			"run_status", run.Status,
+			"vcs_status", status,
+		)
+		return nil
+	}
+
+	err = client.SetStatus(ctx, vcs.SetStatusOptions{
 		Workspace:   ws.Name,
 		Ref:         cv.IngressAttributes.CommitSHA,
 		Repo:        cv.IngressAttributes.Repo,
 		Status:      status,
 		Description: description,
 		TargetURL:   r.URL(paths.Run(run.ID)),
 	})
+	if err != nil {
+		return err
+	}
+
+	// Update status cache. If the run is complete then remove the run from the
+	// cache because no further status updates are expected.
+	if run.Done() {
+		delete(r.Cache, run.ID)
+	} else {
+		r.Cache[run.ID] = status
+	}
+
+	return nil
 }

internal/run/reporter_test.go

@@ -20,10 +20,12 @@ func TestReporter_HandleRun(t *testing.T) {
 		run  *Run
 		ws   *workspace.Workspace
 		cv   *configversion.ConfigurationVersion
-		want vcs.SetStatusOptions
+		// expect the given status options to be set. If nil then expect no
+		// status options to be set.
+		want *vcs.SetStatusOptions
 	}{
 		{
-			name: "pending run",
+			name: "set pending status",
 			run:  &Run{ID: "run-123", Status: RunPending},
 			ws: &workspace.Workspace{
 				Name:       "dev",
@@ -35,7 +37,7 @@ func TestReporter_HandleRun(t *testing.T) {
 					Repo:      "leg100/otf",
 				},
 			},
-			want: vcs.SetStatusOptions{
+			want: &vcs.SetStatusOptions{
 				Workspace: "dev",
 				Ref:       "abc123",
 				Repo:      "leg100/otf",
@@ -49,36 +51,87 @@ func TestReporter_HandleRun(t *testing.T) {
 			cv: &configversion.ConfigurationVersion{
 				IngressAttributes: nil,
 			},
-			want: vcs.SetStatusOptions{},
+			want: nil,
 		},
 		{
 			name: "skip UI-triggered run",
 			run:  &Run{ID: "run-123", Source: SourceUI},
-			want: vcs.SetStatusOptions{},
+			want: nil,
 		},
 		{
 			name: "skip API-triggered run",
 			run:  &Run{ID: "run-123", Source: SourceAPI},
-			want: vcs.SetStatusOptions{},
+			want: nil,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			var got vcs.SetStatusOptions
+			got := make(chan vcs.SetStatusOptions, 1)
 			reporter := &Reporter{
 				Workspaces:      &fakeReporterWorkspaceService{ws: tt.ws},
 				Configs:         &fakeReporterConfigurationVersionService{cv: tt.cv},
-				VCS:             &fakeReporterVCSProviderService{got: &got},
+				VCS:             &fakeReporterVCSProviderService{got: got},
 				HostnameService: internal.NewHostnameService("otf-host.org"),
+				Cache:           make(map[string]vcs.Status),
 			}
 			err := reporter.handleRun(ctx, tt.run)
 			require.NoError(t, err)
 
-			assert.Equal(t, tt.want, got)
+			if tt.want == nil {
+				assert.Equal(t, 0, len(got))
+			} else {
+				assert.Equal(t, *tt.want, <-got)
+			}
 		})
 	}
 }
 
+// TestReporter_DontSetStatusTwice tests that the same status is not set more
+// than once for a given run.
+func TestReporter_DontSetStatusTwice(t *testing.T) {
+	ctx := context.Background()
+
+	run := &Run{ID: "run-123", Status: RunPending}
+	ws := &workspace.Workspace{
+		Name:       "dev",
+		Connection: &workspace.Connection{},
+	}
+	cv := &configversion.ConfigurationVersion{
+		IngressAttributes: &configversion.IngressAttributes{
+			CommitSHA: "abc123",
+			Repo:      "leg100/otf",
+		},
+	}
+
+	got := make(chan vcs.SetStatusOptions, 1)
+	reporter := &Reporter{
+		Workspaces:      &fakeReporterWorkspaceService{ws: ws},
+		Configs:         &fakeReporterConfigurationVersionService{cv: cv},
+		VCS:             &fakeReporterVCSProviderService{got: got},
+		HostnameService: internal.NewHostnameService("otf-host.org"),
+		Cache:           make(map[string]vcs.Status),
+	}
+
+	// handle run the first time and expect status to be set
+	err := reporter.handleRun(ctx, run)
+	require.NoError(t, err)
+
+	want := vcs.SetStatusOptions{
+		Workspace: "dev",
+		Ref:       "abc123",
+		Repo:      "leg100/otf",
+		Status:    vcs.PendingStatus,
+		TargetURL: "https://otf-host.org/app/runs/run-123",
+	}
+	assert.Equal(t, want, <-got)
+
+	// handle run the second time with the same status and expect status to
+	// *not* be set
+	err = reporter.handleRun(ctx, run)
+	require.NoError(t, err)
+	assert.Equal(t, 0, len(got))
+}
+
 type fakeReporterConfigurationVersionService struct {
 	configversion.Service
 
@@ -100,7 +153,7 @@ func (f *fakeReporterWorkspaceService) Get(context.Context, string) (*workspace.
 }
 
 type fakeReporterVCSProviderService struct {
-	got *vcs.SetStatusOptions
+	got chan vcs.SetStatusOptions
 }
 
 func (f *fakeReporterVCSProviderService) GetVCSClient(context.Context, string) (vcs.Client, error) {
@@ -110,10 +163,10 @@ func (f *fakeReporterVCSProviderService) GetVCSClient(context.Context, string) (
 type fakeReporterCloudClient struct {
 	vcs.Client
 
-	got *vcs.SetStatusOptions
+	got chan vcs.SetStatusOptions
 }
 
 func (f *fakeReporterCloudClient) SetStatus(ctx context.Context, opts vcs.SetStatusOptions) error {
-	*f.got = opts
+	f.got <- opts
 	return nil
 }

internal/vcs/status.go

@@ -4,7 +4,6 @@ type Status string
 
 const (
 	PendingStatus Status = "pending"
-	RunningStatus Status = "running"
 	SuccessStatus Status = "success"
 	ErrorStatus   Status = "error"
 	FailureStatus Status = "failure"