-
Notifications
You must be signed in to change notification settings - Fork 11
feat(errs): http error classifier #566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e89bfb9
feat(errs): classify HTTP status codes and transport failures
mnoah1 30d759f
fixup(errs): address review on the HTTP classifier
mnoah1 aeeb0f6
Avoid retrying non indempotent paths
mnoah1 468182f
Update BUILD files for the new buildrunner deps
mnoah1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| load("@rules_go//go:def.bzl", "go_library", "go_test") | ||
|
|
||
| go_library( | ||
| name = "go_default_library", | ||
| srcs = ["http.go"], | ||
| importpath = "github.com/uber/submitqueue/platform/errs/http", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "//platform/errs:go_default_library", | ||
| "//platform/http:go_default_library", | ||
| ], | ||
| ) | ||
|
|
||
| go_test( | ||
| name = "go_default_test", | ||
| srcs = ["http_test.go"], | ||
| embed = [":go_default_library"], | ||
| deps = [ | ||
| "//platform/errs:go_default_library", | ||
| "//platform/errs/generic:go_default_library", | ||
| "//platform/errs/mysql:go_default_library", | ||
| "//platform/http:go_default_library", | ||
| "@com_github_stretchr_testify//assert:go_default_library", | ||
| ], | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| // Copyright (c) 2025 Uber Technologies, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // Package http provides an errs.Classifier for failures returned by HTTP | ||
| // clients: a rejected status code (platform/http.StatusError) and a transport | ||
| // failure (*url.Error, which is what http.Client.Do returns). | ||
| // | ||
| // Wire it into any service whose extensions call an HTTP API — build runners, | ||
| // CI gateways, webhook senders. Without it every status code looks the same to | ||
| // the pipeline: unclassified, and therefore non-retryable, so a 502 from a proxy | ||
| // dead-letters the message on its first attempt rather than being retried. | ||
| // | ||
| // Order matters when wiring this alongside platform/errs/mysql. The MySQL | ||
| // classifier treats any net.Error as retryable infra, and *url.Error satisfies | ||
| // net.Error, so it will claim HTTP transport failures if it runs first. List | ||
| // this classifier before it to keep those failures attributed to the dependency | ||
| // they came from: | ||
| // | ||
| // errs.NewClassifierProcessor( | ||
| // genericerrs.Classifier, | ||
| // httperrs.Classifier, | ||
| // mysqlerrs.Classifier, | ||
| // ) | ||
| package http | ||
|
|
||
| import ( | ||
| "context" | ||
| nethttp "net/http" | ||
| "net/url" | ||
|
|
||
| "github.com/uber/submitqueue/platform/errs" | ||
| phttp "github.com/uber/submitqueue/platform/http" | ||
| ) | ||
|
|
||
| // Classifier implements errs.Classifier for HTTP client failures. It recognises: | ||
| // | ||
| // - *phttp.StatusError — dispatches on the status code. Codes that describe a | ||
| // server-side or overload condition (500, 502, 503, 504, other unassigned | ||
| // 5xx, 429, 408) are retryable dependency errors. Codes that describe a | ||
| // verdict on the request itself (4xx, 3xx, and the permanently broken 501 | ||
| // and 505) are non-retryable dependency errors. | ||
| // - *url.Error — the wrapper http.Client.Do puts around connection resets, DNS | ||
| // failures, TLS errors and timeouts. A retryable dependency error, except | ||
| // for our own context cancellation (see Classify). | ||
| // | ||
| // Everything else returns errs.Unknown so the classifier-processor walk can keep | ||
| // looking down the unwrap chain. | ||
| // | ||
| // The classifier never returns errs.User. A 400 or 403 says the request was | ||
| // rejected, not that a person did something wrong; only the controller knows | ||
| // whether the request was built from user input. Controllers express that by | ||
| // wrapping with errs.NewUserError, which short-circuits pass 1 of the | ||
| // classifier-processor before this classifier is consulted. | ||
| // | ||
| // The classifier is stateless; this package-level singleton is the canonical | ||
| // handle. Pass it as one of the variadic classifiers to | ||
| // errs.NewClassifierProcessor; the resulting processor is what gets handed to | ||
| // consumer.New. | ||
| var Classifier errs.Classifier = classifier{} | ||
|
|
||
| type classifier struct{} | ||
|
|
||
| // Classify inspects a single node. Per the errs.Classifier contract, this must | ||
| // not call errors.Is / errors.As — the classifier-processor owns the chain walk. | ||
| func (classifier) Classify(err error) errs.Verdict { | ||
| if se, ok := err.(*phttp.StatusError); ok { | ||
| return classifyStatusCode(se.StatusCode) | ||
| } | ||
|
|
||
| if ue, ok := err.(*url.Error); ok { | ||
| // 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 | ||
| } | ||
| // 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, 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. | ||
| nethttp.StatusTooManyRequests: // 429 — over a rate limit that resets with time. | ||
| return errs.InfraDependencyRetryable | ||
|
|
||
| case nethttp.StatusNotImplemented, // 501 — the route will not appear because we retried. | ||
| nethttp.StatusHTTPVersionNotSupported: // 505 — a client/server mismatch to fix in config. | ||
| return errs.InfraDependency | ||
| } | ||
|
|
||
| // Remaining 5xx: the server reported its own failure. Covers 500, 502, 503 | ||
| // and 504, the shapes a proxy or overloaded backend produces, plus any | ||
| // unassigned or vendor-specific 5xx, which follow the same convention. | ||
| if code >= nethttp.StatusInternalServerError { | ||
| return errs.InfraDependencyRetryable | ||
| } | ||
|
|
||
| // 4xx other than the two above, 3xx the client was not configured to follow, | ||
| // 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 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| // Copyright (c) 2025 Uber Technologies, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package http | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "net" | ||
| nethttp "net/http" | ||
| "net/url" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/uber/submitqueue/platform/errs" | ||
| genericerrs "github.com/uber/submitqueue/platform/errs/generic" | ||
| mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" | ||
| phttp "github.com/uber/submitqueue/platform/http" | ||
| ) | ||
|
|
||
| func TestClassifier_StatusCodes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| code int | ||
| want errs.Verdict | ||
| }{ | ||
| // Server state: changes without us doing anything differently. | ||
| {"bad gateway", nethttp.StatusBadGateway, errs.InfraDependencyRetryable}, | ||
| {"service unavailable", nethttp.StatusServiceUnavailable, errs.InfraDependencyRetryable}, | ||
| {"gateway timeout", nethttp.StatusGatewayTimeout, errs.InfraDependencyRetryable}, | ||
| {"internal server error", nethttp.StatusInternalServerError, errs.InfraDependencyRetryable}, | ||
| {"unassigned 5xx", 599, errs.InfraDependencyRetryable}, | ||
| {"request timeout", nethttp.StatusRequestTimeout, errs.InfraDependencyRetryable}, | ||
| {"too many requests", nethttp.StatusTooManyRequests, errs.InfraDependencyRetryable}, | ||
|
|
||
| // Verdicts on the request: replaying reproduces the same answer. | ||
| {"not implemented", nethttp.StatusNotImplemented, errs.InfraDependency}, | ||
| {"http version not supported", nethttp.StatusHTTPVersionNotSupported, errs.InfraDependency}, | ||
| {"bad request", nethttp.StatusBadRequest, errs.InfraDependency}, | ||
| {"unauthorized", nethttp.StatusUnauthorized, errs.InfraDependency}, | ||
| {"forbidden", nethttp.StatusForbidden, errs.InfraDependency}, | ||
| {"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 { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, tt.want, Classifier.Classify(phttp.NewStatusError(tt.code, nil))) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestClassifier_TransportFailures(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| err error | ||
| want errs.Verdict | ||
| }{ | ||
| { | ||
| name: "connection refused", | ||
| err: &url.Error{Op: "Get", URL: "http://api.example", Err: errors.New("connection refused")}, | ||
| want: errs.InfraDependencyRetryable, | ||
| }, | ||
| { | ||
| name: "dns failure", | ||
| err: &url.Error{Op: "Get", URL: "http://api.example", Err: &net.DNSError{Err: "no such host"}}, | ||
| want: errs.InfraDependencyRetryable, | ||
| }, | ||
| { | ||
| // Ours, not theirs: declining lets the walk reach context.Canceled, | ||
| // where the generic classifier calls it plain retryable infra. | ||
| name: "context cancelled", | ||
| err: &url.Error{Op: "Get", URL: "http://api.example", Err: context.Canceled}, | ||
| 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.InfraDependencyRetryable, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, tt.want, Classifier.Classify(tt.err)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestClassifier_Unknown(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| err error | ||
| }{ | ||
| // Per-node contract: a wrapped StatusError must not match here. The | ||
| // classifier-processor walk reaches the inner node and asks again there. | ||
| {"wrapped status error", fmt.Errorf("get build x: %w", phttp.NewStatusError(502, nil))}, | ||
| {"plain error", errors.New("anything")}, | ||
| {"bare context.Canceled", context.Canceled}, | ||
| {"nil", nil}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, errs.Unknown, Classifier.Classify(tt.err)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestClassifier_AppliedViaProcessor(t *testing.T) { | ||
| // The order services wire: generic first, this one before mysqlerrs. | ||
| processor := errs.NewClassifierProcessor(genericerrs.Classifier, Classifier, mysqlerrs.Classifier) | ||
|
|
||
| t.Run("wrapped 502 becomes a retryable dependency error", func(t *testing.T) { | ||
| err := fmt.Errorf("get build org/pipeline/builds/1: %w", phttp.NewStatusError(nethttp.StatusBadGateway, []byte("proxy forward failed"))) | ||
| out := processor.Process(err) | ||
| assert.True(t, errs.IsRetryable(out)) | ||
| assert.True(t, errs.IsDependencyError(out)) | ||
| }) | ||
|
|
||
| t.Run("wrapped 404 stays non-retryable", func(t *testing.T) { | ||
| err := fmt.Errorf("get build org/pipeline/builds/1: %w", phttp.NewStatusError(nethttp.StatusNotFound, nil)) | ||
| out := processor.Process(err) | ||
| assert.False(t, errs.IsRetryable(out)) | ||
| assert.True(t, errs.IsDependencyError(out)) | ||
| }) | ||
|
|
||
| t.Run("transport failure is attributed to the dependency not mysql", func(t *testing.T) { | ||
| // mysqlerrs calls any net.Error retryable infra, and *url.Error is one, | ||
| // so it would claim this node and drop the dependency attribution if it | ||
| // were listed first. | ||
| err := fmt.Errorf("send: %w", &url.Error{Op: "Get", URL: "http://api.example", Err: errors.New("connection reset by peer")}) | ||
| 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("our cancellation is retryable but not a dependency failure", func(t *testing.T) { | ||
| err := fmt.Errorf("send: %w", &url.Error{Op: "Get", URL: "http://api.example", Err: context.Canceled}) | ||
| out := processor.Process(err) | ||
| assert.True(t, errs.IsRetryable(out)) | ||
| 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. | ||
| err := errs.NewDependencyError(phttp.NewStatusError(nethttp.StatusBadGateway, nil)) | ||
| out := processor.Process(err) | ||
| assert.Same(t, err, out) | ||
| 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)) | ||
| }) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.