fix(core): restore FinishJob retries on transport errors

core/client.go

@@ -229,11 +229,24 @@ func (c *Client) FinishJob(ctx context.Context, job *api.Job, finishedAt time.Ti
 	).DoWithContext(ctx, func(retrier *roko.Retrier) error {
 		response, err := c.APIClient.FinishJob(ctx, job, ignoreAgentInDispatches)
 		if err != nil {
-			// Non-retryable responses (e.g. 422, or 401 when job tokens are being used) mean the job has been cancelled or
-			// otherwise already finished. We should stop trying and go find more work to do.
-			if !api.BreakOnNonRetryable(retrier, response, err) {
-				c.Logger.Warnf("%s (%s)", err, retrier)
+			// If the API returns with a 422, that means that we successfully tried to
+			// finish the job, but Buildkite rejected the finish for some reason. This
+			// can sometimes mean that Buildkite has cancelled the job before we get a
+			// chance to send the final API call (maybe this agent took too long to kill
+			// the process).
+			// The API may also return a 401 when job tokens are enabled.
+			// In either case, we don't want to keep trying to finish the job forever so
+			// we'll just bail out and go find some more work to do.
+			//
+			// Unlike other API calls, we intentionally do not use
+			// api.BreakOnNonRetryable here: finish is critical, so unknown transport
+			// errors (e.g. "http2: client connection lost") must keep retrying.
+			if response != nil && (response.StatusCode == 422 || response.StatusCode == 401) {
+				c.Logger.Warnf("Buildkite rejected the call to finish the job (%s)", err)
+				retrier.Break()
+				return err
 			}
+			c.Logger.Warnf("%s (%s)", err, retrier)
 		}
 
 		return err

core/client_finish_job_test.go

@@ -0,0 +1,78 @@
+package core
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/buildkite/agent/v3/api"
+	"github.com/buildkite/agent/v3/logger"
+)
+
+type finishJobAPIClient struct {
+	APIClient // panics if unexpected methods are called
+
+	calls    atomic.Int32
+	failOnce error
+	failWith *api.Response
+}
+
+func (f *finishJobAPIClient) FinishJob(context.Context, *api.Job, *bool) (*api.Response, error) {
+	n := f.calls.Add(1)
+	if n == 1 && f.failOnce != nil {
+		return f.failWith, f.failOnce
+	}
+	return &api.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil
+}
+
+func TestFinishJobRetriesHTTP2ClientConnectionLost(t *testing.T) {
+	t.Parallel()
+
+	fake := &finishJobAPIClient{
+		failOnce: errors.New("http2: client connection lost"),
+	}
+	sleeps := 0
+	client := &Client{
+		APIClient: fake,
+		Logger:    logger.Discard,
+		RetrySleepFunc: func(time.Duration) {
+			sleeps++
+		},
+	}
+
+	err := client.FinishJob(t.Context(), &api.Job{ID: "job-1"}, time.Now(), ProcessExit{Status: 0}, 0, nil)
+	if err != nil {
+		t.Fatalf("FinishJob() error = %v, want nil", err)
+	}
+	if got := fake.calls.Load(); got != 2 {
+		t.Fatalf("FinishJob API calls = %d, want 2 (retry after transport error)", got)
+	}
+	if sleeps != 1 {
+		t.Fatalf("retry sleeps = %d, want 1", sleeps)
+	}
+}
+
+func TestFinishJobDoesNotRetryOn401(t *testing.T) {
+	t.Parallel()
+
+	fake := &finishJobAPIClient{
+		failOnce: errors.New("unauthorized"),
+		failWith: &api.Response{Response: &http.Response{StatusCode: http.StatusUnauthorized}},
+	}
+	client := &Client{
+		APIClient:      fake,
+		Logger:         logger.Discard,
+		RetrySleepFunc: func(time.Duration) {},
+	}
+
+	err := client.FinishJob(t.Context(), &api.Job{ID: "job-1"}, time.Now(), ProcessExit{Status: 0}, 0, nil)
+	if err == nil {
+		t.Fatal("FinishJob() error = nil, want unauthorized")
+	}
+	if got := fake.calls.Load(); got != 1 {
+		t.Fatalf("FinishJob API calls = %d, want 1 (no retry on 401)", got)
+	}
+}