From 9ce59f66e15ec50739334ad2d35e60884c9afcbb Mon Sep 17 00:00:00 2001 From: Tmwakalasya Date: Mon, 19 Oct 2026 13:14:38 -0400 Subject: [PATCH] fix(changeprovider): preserve Phabricator HTTP status errors --- .../changeprovider/phabricator/BUILD.bazel | 4 ++ .../changeprovider/phabricator/conduit.go | 4 +- .../phabricator/conduit_test.go | 15 ++++++- .../phabricator/provider_test.go | 42 ++++++++++++++++++- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel index bad67893b..84ff2f62c 100644 --- a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel +++ b/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", diff --git a/submitqueue/extension/changeprovider/phabricator/conduit.go b/submitqueue/extension/changeprovider/phabricator/conduit.go index 38c9aef40..6b10cf1a7 100644 --- a/submitqueue/extension/changeprovider/phabricator/conduit.go +++ b/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 diff --git a/submitqueue/extension/changeprovider/phabricator/conduit_test.go b/submitqueue/extension/changeprovider/phabricator/conduit_test.go index f61740a3f..bfcdae044 100644 --- a/submitqueue/extension/changeprovider/phabricator/conduit_test.go +++ b/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() diff --git a/submitqueue/extension/changeprovider/phabricator/provider_test.go b/submitqueue/extension/changeprovider/phabricator/provider_test.go index 1a594b3af..eebafe207 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider_test.go +++ b/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{