fix(changeprovider): preserve Phabricator HTTP status errors

submitqueue/extension/changeprovider/phabricator/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//platform/base/change/phabricator:go_default_library",
+        "//platform/http:go_default_library",
         "//platform/metrics:go_default_library",
         "//submitqueue/entity:go_default_library",
         "//submitqueue/extension/changeprovider:go_default_library",
@@ -30,6 +31,9 @@ go_test(
     deps = [
         "//platform/base/change:go_default_library",
         "//platform/base/change/phabricator:go_default_library",
+        "//platform/errs:go_default_library",
+        "//platform/errs/http:go_default_library",
+        "//platform/http:go_default_library",
         "//submitqueue/entity:go_default_library",
         "//submitqueue/extension/changeprovider:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",

submitqueue/extension/changeprovider/phabricator/conduit.go

@@ -8,6 +8,8 @@ import (
 	"net/http"
 	"net/url"
 	"strconv"
+
+	phttp "github.com/uber/submitqueue/platform/http"
 )
 
 // conduitResponse stores a response from the Conduit API. Every Conduit endpoint wraps its
@@ -67,7 +69,7 @@ func parseConduitResponse(resp *http.Response, diffIDs []int) (map[int]*diffResu
 		if err != nil {
 			return nil, fmt.Errorf("Conduit API returned status %d and failed to read body: %w", resp.StatusCode, err)
 		}
-		return nil, fmt.Errorf("Conduit API returned status %d: %s", resp.StatusCode, string(body))
+		return nil, fmt.Errorf("Conduit API request failed: %w", phttp.NewStatusError(resp.StatusCode, body))
 	}
 
 	var phabResponse conduitResponse

submitqueue/extension/changeprovider/phabricator/conduit_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
+	"testing/iotest"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -96,7 +97,7 @@ func TestParseConduitResponse(t *testing.T) {
 			name:    "HTTP error",
 			resp:    newPlainHTTPResponse("server error", http.StatusInternalServerError),
 			diffIDs: []int{100},
-			wantErr: "Conduit API returned status 500",
+			wantErr: "unexpected status 500",
 		},
 		{
 			name: "HTTP error with unreadable body",
@@ -149,6 +150,18 @@ func TestParseConduitResponse(t *testing.T) {
 	}
 }
 
+func TestParseConduitResponse_BodyReadError(t *testing.T) {
+	resp := &http.Response{
+		StatusCode: http.StatusBadGateway,
+		Body:       io.NopCloser(iotest.ErrReader(io.ErrUnexpectedEOF)),
+	}
+	t.Cleanup(func() { _ = resp.Body.Close() })
+
+	_, err := parseConduitResponse(resp, []int{100})
+
+	require.ErrorIs(t, err, io.ErrUnexpectedEOF)
+}
+
 // newConduitHTTPResponse builds an http.Response with a successful Conduit envelope.
 func newConduitHTTPResponse(t *testing.T, result any, statusCode int) *http.Response {
 	t.Helper()

submitqueue/extension/changeprovider/phabricator/provider_test.go

@@ -14,6 +14,9 @@ import (
 	"go.uber.org/zap/zaptest"
 
 	"github.com/uber/submitqueue/platform/base/change"
+	"github.com/uber/submitqueue/platform/errs"
+	httperrs "github.com/uber/submitqueue/platform/errs/http"
+	phttp "github.com/uber/submitqueue/platform/http"
 	"github.com/uber/submitqueue/submitqueue/entity"
 	"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
 )
@@ -65,7 +68,7 @@ func TestProvider_Get(t *testing.T) {
 				w.WriteHeader(http.StatusInternalServerError)
 			},
 			uris:    []string{"phab://phab.example.com/D200/100"},
-			wantErr: "Conduit API returned status 500",
+			wantErr: "unexpected status 500",
 		},
 	}
 
@@ -140,6 +143,43 @@ func TestProvider_Get_ConnectionError(t *testing.T) {
 	assert.Contains(t, err.Error(), "failed to fetch diffs")
 }
 
+func TestProvider_Get_HTTPStatusError(t *testing.T) {
+	processor := errs.NewClassifierProcessor(httperrs.Classifier)
+	for _, tt := range []struct {
+		name      string
+		status    int
+		retryable bool
+	}{
+		{name: "service unavailable", status: http.StatusServiceUnavailable, retryable: true},
+		{name: "rate limited", status: http.StatusTooManyRequests, retryable: true},
+		{name: "bad request", status: http.StatusBadRequest},
+		{name: "unauthorized", status: http.StatusUnauthorized},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			const body = "upstream failure"
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				w.WriteHeader(tt.status)
+				_, _ = w.Write([]byte(body))
+			}))
+			t.Cleanup(server.Close)
+
+			client := &http.Client{Transport: &testTransport{baseURL: server.URL}}
+			p := newTestProvider(t, client)
+			_, err := p.Get(t.Context(), entity.Request{Change: change.Change{
+				URIs: []string{"phab://phab.example.com/D200/100"},
+			}})
+			var statusErr *phttp.StatusError
+			require.ErrorAs(t, err, &statusErr)
+			assert.Equal(t, tt.status, statusErr.StatusCode)
+			assert.Equal(t, body, statusErr.Body)
+
+			classified := processor.Process(err)
+			assert.Equal(t, tt.retryable, errs.IsRetryable(classified))
+			assert.True(t, errs.IsDependencyError(classified))
+		})
+	}
+}
+
 func TestProvider_Get_FileDetails(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		result := map[string]*diffResult{