Skip to content

Commit 062742c

Browse files
authored
fix(changeprovider): preserve Phabricator HTTP status errors (#887)
## Why? The Phabricator provider reports rejected Conduit HTTP responses as plain error strings. The HTTP classifier cannot recover their status codes, so rate limits (429) and temporary outages (503) take the non-retryable path. ## What? Wrap the existing `platform/http.StatusError` at the Conduit response boundary, preserving the status and response body through the provider's error chain. The existing classifier can then distinguish retryable responses from permanent failures such as 400 and 401. Body-read failures retain their existing wrapped read error. Add provider-level regression cases for 429, 503, 400, and 401, plus an assertion that body-read errors remain discoverable with `errors.Is`. ## Test Plan The four new status-error cases failed against the original implementation because `*http.StatusError` was absent from the returned error chain, then passed with the fix. Passed locally: ```sh ./tool/bazel test //submitqueue/extension/changeprovider/phabricator:go_default_test //platform/http:go_default_test //platform/errs/http:go_default_test --test_output=errors ./tool/bazel run @rules_go//go -- vet -mod=readonly ./submitqueue/extension/changeprovider/phabricator make fmt make lint check-tidy check-gazelle ``` Full integration and end-to-end suites were not run; validation covers the provider and HTTP error-classification boundaries. ## Issue No linked issue.
1 parent b5bd5eb commit 062742c

4 files changed

Lines changed: 62 additions & 3 deletions

File tree

‎submitqueue/extension/changeprovider/phabricator/BUILD.bazel‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ go_library(
1111
visibility = ["//visibility:public"],
1212
deps = [
1313
"//platform/base/change/phabricator:go_default_library",
14+
"//platform/http:go_default_library",
1415
"//platform/metrics:go_default_library",
1516
"//submitqueue/entity:go_default_library",
1617
"//submitqueue/extension/changeprovider:go_default_library",
@@ -30,6 +31,9 @@ go_test(
3031
deps = [
3132
"//platform/base/change:go_default_library",
3233
"//platform/base/change/phabricator:go_default_library",
34+
"//platform/errs:go_default_library",
35+
"//platform/errs/http:go_default_library",
36+
"//platform/http:go_default_library",
3337
"//submitqueue/entity:go_default_library",
3438
"//submitqueue/extension/changeprovider:go_default_library",
3539
"@com_github_stretchr_testify//assert:go_default_library",

‎submitqueue/extension/changeprovider/phabricator/conduit.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"net/http"
99
"net/url"
1010
"strconv"
11+
12+
phttp "github.com/uber/submitqueue/platform/http"
1113
)
1214

1315
// 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
6769
if err != nil {
6870
return nil, fmt.Errorf("Conduit API returned status %d and failed to read body: %w", resp.StatusCode, err)
6971
}
70-
return nil, fmt.Errorf("Conduit API returned status %d: %s", resp.StatusCode, string(body))
72+
return nil, fmt.Errorf("Conduit API request failed: %w", phttp.NewStatusError(resp.StatusCode, body))
7173
}
7274

7375
var phabResponse conduitResponse

‎submitqueue/extension/changeprovider/phabricator/conduit_test.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http/httptest"
99
"strings"
1010
"testing"
11+
"testing/iotest"
1112

1213
"github.com/stretchr/testify/assert"
1314
"github.com/stretchr/testify/require"
@@ -96,7 +97,7 @@ func TestParseConduitResponse(t *testing.T) {
9697
name: "HTTP error",
9798
resp: newPlainHTTPResponse("server error", http.StatusInternalServerError),
9899
diffIDs: []int{100},
99-
wantErr: "Conduit API returned status 500",
100+
wantErr: "unexpected status 500",
100101
},
101102
{
102103
name: "HTTP error with unreadable body",
@@ -149,6 +150,18 @@ func TestParseConduitResponse(t *testing.T) {
149150
}
150151
}
151152

153+
func TestParseConduitResponse_BodyReadError(t *testing.T) {
154+
resp := &http.Response{
155+
StatusCode: http.StatusBadGateway,
156+
Body: io.NopCloser(iotest.ErrReader(io.ErrUnexpectedEOF)),
157+
}
158+
t.Cleanup(func() { _ = resp.Body.Close() })
159+
160+
_, err := parseConduitResponse(resp, []int{100})
161+
162+
require.ErrorIs(t, err, io.ErrUnexpectedEOF)
163+
}
164+
152165
// newConduitHTTPResponse builds an http.Response with a successful Conduit envelope.
153166
func newConduitHTTPResponse(t *testing.T, result any, statusCode int) *http.Response {
154167
t.Helper()

‎submitqueue/extension/changeprovider/phabricator/provider_test.go‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ import (
1414
"go.uber.org/zap/zaptest"
1515

1616
"github.com/uber/submitqueue/platform/base/change"
17+
"github.com/uber/submitqueue/platform/errs"
18+
httperrs "github.com/uber/submitqueue/platform/errs/http"
19+
phttp "github.com/uber/submitqueue/platform/http"
1720
"github.com/uber/submitqueue/submitqueue/entity"
1821
"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
1922
)
@@ -65,7 +68,7 @@ func TestProvider_Get(t *testing.T) {
6568
w.WriteHeader(http.StatusInternalServerError)
6669
},
6770
uris: []string{"phab://phab.example.com/D200/100"},
68-
wantErr: "Conduit API returned status 500",
71+
wantErr: "unexpected status 500",
6972
},
7073
}
7174

@@ -140,6 +143,43 @@ func TestProvider_Get_ConnectionError(t *testing.T) {
140143
assert.Contains(t, err.Error(), "failed to fetch diffs")
141144
}
142145

146+
func TestProvider_Get_HTTPStatusError(t *testing.T) {
147+
processor := errs.NewClassifierProcessor(httperrs.Classifier)
148+
for _, tt := range []struct {
149+
name string
150+
status int
151+
retryable bool
152+
}{
153+
{name: "service unavailable", status: http.StatusServiceUnavailable, retryable: true},
154+
{name: "rate limited", status: http.StatusTooManyRequests, retryable: true},
155+
{name: "bad request", status: http.StatusBadRequest},
156+
{name: "unauthorized", status: http.StatusUnauthorized},
157+
} {
158+
t.Run(tt.name, func(t *testing.T) {
159+
const body = "upstream failure"
160+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
161+
w.WriteHeader(tt.status)
162+
_, _ = w.Write([]byte(body))
163+
}))
164+
t.Cleanup(server.Close)
165+
166+
client := &http.Client{Transport: &testTransport{baseURL: server.URL}}
167+
p := newTestProvider(t, client)
168+
_, err := p.Get(t.Context(), entity.Request{Change: change.Change{
169+
URIs: []string{"phab://phab.example.com/D200/100"},
170+
}})
171+
var statusErr *phttp.StatusError
172+
require.ErrorAs(t, err, &statusErr)
173+
assert.Equal(t, tt.status, statusErr.StatusCode)
174+
assert.Equal(t, body, statusErr.Body)
175+
176+
classified := processor.Process(err)
177+
assert.Equal(t, tt.retryable, errs.IsRetryable(classified))
178+
assert.True(t, errs.IsDependencyError(classified))
179+
})
180+
}
181+
}
182+
143183
func TestProvider_Get_FileDetails(t *testing.T) {
144184
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
145185
result := map[string]*diffResult{

0 commit comments

Comments
 (0)