fixup(errs): address review on the HTTP classifier

- Stop declining context.DeadlineExceeded. Only context.Canceled is left to the
  generic classifier now. A deadline that elapses mid-request means the remote
  end did not answer in time, so it belongs to the dependency; declining it also
  stranded the node, because generic matches only Canceled and nothing else
  claimed it unless platform/errs/mysql happened to be wired. A processor test
  with just generic + http pins that the classifier stands on its own.
- Bound StatusError.Error to 1 KiB of body. The rendered string reaches the
  queue's dead-letter record through Reject(ctx, err.Error()) and last_error is
  finite, so a large error page could fail that write and leave the message stuck
  instead of dead-lettered. Body is still kept whole.
- Convert the GitHub Actions client's two status paths as well. Stovepipe runs
  both build runners, so leaving one untyped meant a 502 was retried or
  dead-lettered depending only on which runner the queue used.
- Assert with errors.As that both clients return *StatusError and carry the code.
  The old tests only checked that an error came back, so they passed before the
  typed error existed and would keep passing if it were reverted.
- Document that a retried create can duplicate a build, on both clients' create
  paths. Note that codes below 100, including 0, take the non-retryable
  fall-through deliberately, and cover 0 in the table.

Author: mnoah1

platform/errs/http/http.go

@@ -79,38 +79,26 @@ func (classifier) Classify(err error) errs.Verdict {
 	}
 
 	if ue, ok := err.(*url.Error); ok {
-		// A cancelled or expired context means we gave up, not that the remote
-		// end misbehaved: the deadline belongs to this process. Report Unknown
-		// so the walk continues into the cause, where the generic classifier
-		// picks up context.Canceled as plain retryable infra. That keeps
-		// shutdown noise out of the dependency-error metrics for this backend.
-		// Reading the Err field is not a chain walk, and declining here leaves
-		// the framework's walk free to reach that node.
-		//
-		// This covers the shape http.Client.Do produces for context failures. A
-		// context error buried under further transport wrapping falls through to
-		// the retryable dependency verdict below, which retries the same way and
-		// differs only in the dependency tag.
-		if ue.Err == context.Canceled || ue.Err == context.DeadlineExceeded {
+		// A cancelled context is ours, not theirs — process shutdown, or a parent
+		// operation that went away — so decline it and let the generic classifier
+		// claim context.Canceled as plain retryable infra, keeping shutdowns out
+		// of this backend's dependency metrics. An expired deadline is theirs:
+		// the remote end did not answer in time, so it takes the verdict below.
+		// Declining that one would strand it, since generic matches only Canceled.
+		if ue.Err == context.Canceled {
 			return errs.Unknown
 		}
-		// Anything else at this layer is a failure to complete an exchange with
-		// the remote end — reset connection, refused dial, DNS, TLS, client
-		// timeout. None of those say the request was invalid, so another attempt
-		// is worth making.
+		// Everything else at this layer is a failed exchange with the remote end,
+		// and none of those shapes says the request was invalid.
 		return errs.InfraDependencyRetryable
 	}
 
 	return errs.Unknown
 }
 
-// classifyStatusCode maps an HTTP status code to a Verdict.
-//
-// The split is whether the code describes the state of the server or a verdict
-// on the request. Server-state codes change on their own, so the same request
-// can succeed later. Request-verdict codes will not: replaying the request
-// reproduces the answer, and the message should dead-letter for a human or a
-// reconciler instead of retrying until it runs out of attempts.
+// classifyStatusCode maps an HTTP status code to a Verdict. The split is whether
+// the code describes the state of the server, which can change on its own, or a
+// verdict on the request, which replaying only reproduces.
 func classifyStatusCode(code int) errs.Verdict {
 	switch code {
 	case nethttp.StatusRequestTimeout, // 408 — the server stopped waiting; sending it again is reasonable.
@@ -130,7 +118,8 @@ func classifyStatusCode(code int) errs.Verdict {
 	}
 
 	// 4xx other than the two above, 3xx the client was not configured to follow,
-	// and anything else a caller chose to reject: a verdict on this request. Not
-	// retryable.
+	// and anything else a caller chose to reject — including a code that was
+	// never a response, such as 0: a verdict on the request, or on a malformed
+	// call. Neither changes on a second attempt.
 	return errs.InfraDependency
 }

platform/errs/http/http_test.go

@@ -54,6 +54,9 @@ func TestClassifier_StatusCodes(t *testing.T) {
 		{"not found", nethttp.StatusNotFound, errs.InfraDependency},
 		{"unprocessable entity", nethttp.StatusUnprocessableEntity, errs.InfraDependency},
 		{"unfollowed redirect", nethttp.StatusFound, errs.InfraDependency},
+
+		// Never a response: a caller built this from something else.
+		{"zero code", 0, errs.InfraDependency},
 	}
 
 	for _, tt := range tests {
@@ -87,9 +90,10 @@ func TestClassifier_TransportFailures(t *testing.T) {
 			want: errs.Unknown,
 		},
 		{
+			// Theirs, not ours: the remote end did not answer in time.
 			name: "context deadline exceeded",
 			err:  &url.Error{Op: "Get", URL: "http://api.example", Err: context.DeadlineExceeded},
-			want: errs.Unknown,
+			want: errs.InfraDependencyRetryable,
 		},
 	}
 
@@ -155,6 +159,13 @@ func TestClassifier_AppliedViaProcessor(t *testing.T) {
 		assert.False(t, errs.IsDependencyError(out))
 	})
 
+	t.Run("expired deadline is retryable without mysqlerrs claiming it", func(t *testing.T) {
+		err := fmt.Errorf("send: %w", &url.Error{Op: "Get", URL: "http://api.example", Err: context.DeadlineExceeded})
+		out := processor.Process(err)
+		assert.True(t, errs.IsRetryable(out))
+		assert.True(t, errs.IsDependencyError(out), "should be attributed to the HTTP dependency")
+	})
+
 	t.Run("a controller verdict wins over the classifier", func(t *testing.T) {
 		// Pass 1 of the processor short-circuits on the existing framework wrap,
 		// so a 502 a controller decided was fatal stays fatal.
@@ -164,3 +175,28 @@ func TestClassifier_AppliedViaProcessor(t *testing.T) {
 		assert.False(t, errs.IsRetryable(out))
 	})
 }
+
+// TestClassifier_WithoutMySQLClassifier covers a service with no MySQL
+// dependency: no verdict here may rely on mysqlerrs' net.Error rule.
+func TestClassifier_WithoutMySQLClassifier(t *testing.T) {
+	processor := errs.NewClassifierProcessor(genericerrs.Classifier, Classifier)
+
+	tests := []struct {
+		name           string
+		cause          error
+		wantDependency bool
+	}{
+		{name: "connection reset", cause: errors.New("connection reset by peer"), wantDependency: true},
+		{name: "expired deadline", cause: context.DeadlineExceeded, wantDependency: true},
+		{name: "our cancellation", cause: context.Canceled, wantDependency: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := fmt.Errorf("send: %w", &url.Error{Op: "Get", URL: "http://api.example", Err: tt.cause})
+			out := processor.Process(err)
+			assert.True(t, errs.IsRetryable(out), "must not depend on mysqlerrs being wired")
+			assert.Equal(t, tt.wantDependency, errs.IsDependencyError(out))
+		})
+	}
+}

platform/extension/buildrunner/buildkite/client.go

@@ -66,6 +66,11 @@ type BuildResponse struct {
 }
 
 // CreateBuild creates a new Buildkite build.
+//
+// POST /builds is not idempotent and a rejection carries its status code like any
+// other, so a caller that retries a 502 can create a second build when the first
+// was already accepted. Callers that cannot tolerate that need their own
+// idempotency check.
 func (c *Client) CreateBuild(ctx context.Context, req CreateBuildRequest) (BuildResponse, error) {
 	body, err := json.Marshal(req)
 	if err != nil {

platform/extension/buildrunner/buildkite/client_test.go

@@ -38,6 +38,15 @@ func newTestClient(t *testing.T, handler http.Handler) *Client {
 	return NewClient(c)
 }
 
+// requireStatusError asserts err carries code as a *phttp.StatusError, the shape
+// platform/errs/http needs to classify it.
+func requireStatusError(t *testing.T, err error, code int) {
+	t.Helper()
+	var se *phttp.StatusError
+	require.ErrorAs(t, err, &se)
+	assert.Equal(t, code, se.StatusCode)
+}
+
 func buildJSON(t *testing.T, number int, state, webURL string) []byte {
 	t.Helper()
 	return buildJSONWithEnv(t, number, state, webURL, nil)
@@ -86,6 +95,7 @@ func TestCreateBuild_ErrorStatus_ReturnsError(t *testing.T) {
 
 	_, err := c.CreateBuild(context.Background(), CreateBuildRequest{})
 	require.Error(t, err)
+	requireStatusError(t, err, http.StatusInternalServerError)
 }
 
 // --- GetBuild ---
@@ -150,7 +160,9 @@ func TestCancelBuild_ErrorStatus_ReturnsError(t *testing.T) {
 		w.WriteHeader(http.StatusInternalServerError)
 	}))
 
-	require.Error(t, c.CancelBuild(context.Background(), 5))
+	err := c.CancelBuild(context.Background(), 5)
+	require.Error(t, err)
+	requireStatusError(t, err, http.StatusInternalServerError)
 }
 
 // --- EncodeBuildNumber / ParseBuildNumber ---

platform/extension/buildrunner/githubactions/client.go

@@ -106,6 +106,11 @@ type WorkflowRun struct {
 }
 
 // DispatchWorkflow dispatches the bound workflow.
+//
+// Dispatching is not idempotent and a rejection carries its status code like any
+// other, so a caller that retries a 502 can start a second run when the first was
+// already accepted. Callers that cannot tolerate that need their own idempotency
+// check.
 func (c *Client) DispatchWorkflow(ctx context.Context, req DispatchWorkflowRequest) (DispatchWorkflowResponse, error) {
 	body, err := json.Marshal(req)
 	if err != nil {
@@ -146,7 +151,7 @@ func (c *Client) CancelRun(ctx context.Context, runID int64) error {
 	case http.StatusNotFound:
 		return ErrNotFound
 	default:
-		return fmt.Errorf("unexpected status %d from cancel", status)
+		return fmt.Errorf("cancel run: %w", phttp.NewStatusError(status, nil))
 	}
 }
 
@@ -185,7 +190,9 @@ func (c *Client) do(ctx context.Context, method, rawURL string, body []byte, out
 		return ErrNotFound
 	}
 	if status < 200 || status >= 300 {
-		return fmt.Errorf("API returned status %d: %s", status, respBody)
+		// Typed rather than formatted so platform/errs/http can read the code
+		// and tell a transient 502 from a permanent 400.
+		return phttp.NewStatusError(status, respBody)
 	}
 
 	if out != nil && len(respBody) > 0 {

platform/extension/buildrunner/githubactions/client_test.go

@@ -40,6 +40,15 @@ func newTestClient(t *testing.T, handler http.Handler) *Client {
 	return NewClient(c, "uber", "submitqueue", "submitqueue-ci.yml")
 }
 
+// requireStatusError asserts err carries code as a *phttp.StatusError, the shape
+// platform/errs/http needs to classify it.
+func requireStatusError(t *testing.T, err error, code int) {
+	t.Helper()
+	var se *phttp.StatusError
+	require.ErrorAs(t, err, &se)
+	assert.Equal(t, code, se.StatusCode)
+}
+
 // --- NewClient / accessors ---
 
 func TestNewClient_ExposesIdentity(t *testing.T) {
@@ -89,6 +98,7 @@ func TestDispatchWorkflow_ErrorStatus_ReturnsError(t *testing.T) {
 
 	_, err := c.DispatchWorkflow(context.Background(), DispatchWorkflowRequest{})
 	require.Error(t, err)
+	requireStatusError(t, err, http.StatusInternalServerError)
 }
 
 // --- GetRun ---
@@ -147,6 +157,16 @@ func TestCancelRun_NotFound_ReturnsError(t *testing.T) {
 	require.Error(t, c.CancelRun(context.Background(), 5))
 }
 
+func TestCancelRun_ErrorStatus_ReturnsError(t *testing.T) {
+	c := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusBadGateway)
+	}))
+
+	err := c.CancelRun(context.Background(), 5)
+	require.Error(t, err)
+	requireStatusError(t, err, http.StatusBadGateway)
+}
+
 // --- EncodeRunID / ParseRunID ---
 
 func TestEncodeParseRunID_RoundTrip(t *testing.T) {

platform/http/status.go

@@ -16,6 +16,12 @@ package http
 
 import "fmt"
 
+// _maxRenderedBodyBytes bounds how much of Body reaches Error(). The rendered
+// string lands in a consumer's dead-letter record, whose column is finite, so an
+// error page from a chatty gateway must not be able to fail that write. Body
+// itself is kept whole.
+const _maxRenderedBodyBytes = 1024
+
 // StatusError reports a response whose status code the caller rejected.
 //
 // SendRequest does not build this error itself: which codes count as success
@@ -35,7 +41,8 @@ type StatusError struct {
 	// StatusCode is the HTTP status code from the response.
 	StatusCode int
 	// Body is the response body as read from the wire, or empty when the
-	// caller had no body to attach.
+	// caller had no body to attach. Error() renders at most
+	// _maxRenderedBodyBytes of it.
 	Body string
 }
 
@@ -45,13 +52,17 @@ func NewStatusError(statusCode int, body []byte) *StatusError {
 	return &StatusError{StatusCode: statusCode, Body: string(body)}
 }
 
-// Error renders the status and, when present, the response body. Callers are
-// expected to wrap it with the operation that failed, giving messages like
-// "get build org/pipeline/builds/123: unexpected status 502: proxy forward
-// failed".
+// Error renders the status and, when present, the response body truncated to
+// _maxRenderedBodyBytes. Callers are expected to wrap it with the operation that
+// failed, giving messages like "get build org/pipeline/builds/123: unexpected
+// status 502: proxy forward failed".
 func (e *StatusError) Error() string {
 	if e.Body == "" {
 		return fmt.Sprintf("unexpected status %d", e.StatusCode)
 	}
-	return fmt.Sprintf("unexpected status %d: %s", e.StatusCode, e.Body)
+	body := e.Body
+	if len(body) > _maxRenderedBodyBytes {
+		body = body[:_maxRenderedBodyBytes] + "… (truncated)"
+	}
+	return fmt.Sprintf("unexpected status %d: %s", e.StatusCode, body)
 }

platform/http/status_test.go

@@ -15,8 +15,10 @@
 package http
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -50,6 +52,17 @@ func TestNewStatusError(t *testing.T) {
 	}
 }
 
+func TestStatusError_RenderedBodyIsBounded(t *testing.T) {
+	body := bytes.Repeat([]byte("a"), _maxRenderedBodyBytes*4)
+	err := NewStatusError(502, body)
+
+	assert.Len(t, err.Body, _maxRenderedBodyBytes*4, "Body keeps what the caller passed")
+	rendered := err.Error()
+	assert.Less(t, len(rendered), _maxRenderedBodyBytes*2, "rendering must not grow with the body")
+	assert.Contains(t, rendered, "(truncated)")
+	assert.True(t, strings.HasPrefix(rendered, "unexpected status 502: aaa"))
+}
+
 func TestStatusError_SurvivesWrapping(t *testing.T) {
 	// Callers wrap with the operation that failed. The code has to stay
 	// reachable through the chain, otherwise the classifier cannot read it.