From 561592e0b928d86378c2e5cd3d0746e1717614e0 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 27 Aug 2026 14:34:23 +0000 Subject: [PATCH 1/3] feat(errs): classify YARPC status errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Retry transient YARPC failures instead of dead-lettering them as unknown errors. - Preserve dependency attribution while distinguishing caller cancellation. Changes: - Classify typed YARPC statuses by retryability and origin. - Register the classifier with Stovepipe and document the mapping. --- Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace --- platform/errs/README.md | 8 +- platform/errs/yarpc/BUILD.bazel | 23 +++++ platform/errs/yarpc/yarpc.go | 76 +++++++++++++++++ platform/errs/yarpc/yarpc_test.go | 123 +++++++++++++++++++++++++++ service/stovepipe/server/BUILD.bazel | 1 + service/stovepipe/server/main.go | 2 + 6 files changed, 231 insertions(+), 2 deletions(-) create mode 100644 platform/errs/yarpc/BUILD.bazel create mode 100644 platform/errs/yarpc/yarpc.go create mode 100644 platform/errs/yarpc/yarpc_test.go diff --git a/platform/errs/README.md b/platform/errs/README.md index ab314b25..df66ee93 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -89,7 +89,7 @@ One operational consequence worth knowing before relying on any of this: **retry ## Adding a Backend-Specific Classifier -Backend classifiers live alongside the extension they classify, under `platform/errs//`. The canonical examples are `platform/errs/mysql` (MySQL driver errors), `platform/errs/http` (rejected status codes and transport failures from clients built on `platform/http`), and `platform/errs/generic` (transport-agnostic concerns such as `context.Canceled`). +Backend classifiers live alongside the extension they classify, under `platform/errs//`. The canonical examples are `platform/errs/mysql` (MySQL driver errors), `platform/errs/http` (rejected status codes and transport failures from clients built on `platform/http`), `platform/errs/yarpc` (YARPC status codes), and `platform/errs/generic` (transport-agnostic concerns such as `context.Canceled`). A classifier: @@ -124,12 +124,14 @@ import ( genericerrs "github.com/uber/submitqueue/platform/errs/generic" httperrs "github.com/uber/submitqueue/platform/errs/http" mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" + yarpcerrs "github.com/uber/submitqueue/platform/errs/yarpc" ) c := consumer.New(logger, scope, registry, errs.NewClassifierProcessor( genericerrs.Classifier, httperrs.Classifier, + yarpcerrs.Classifier, mysqlerrs.Classifier, ), ) @@ -137,7 +139,9 @@ c := consumer.New(logger, scope, registry, `httperrs` precedes `mysqlerrs` for a reason worth knowing before reordering the list: the MySQL classifier treats any `net.Error` as retryable infra, and the `*url.Error` an HTTP client returns satisfies `net.Error`. Whichever runs first claims that node, so with the order reversed an HTTP transport failure is classified as a MySQL one — retryable either way, but no longer attributed to the dependency it came from. This is the cross-extension ambiguity `NewClassifierProcessor` documents as deferred; registration order is the workaround. -Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go` and `platform/errs/generic/generic_test.go`. +The YARPC classifier reads the typed status code rather than matching its rendered message. Cancellation is retryable caller-side infrastructure; transient or ambiguous server codes (`Unknown`, `DeadlineExceeded`, `ResourceExhausted`, `Aborted`, `Internal`, and `Unavailable`) are retryable dependency failures; request verdicts and permanent server failures are non-retryable dependency failures. A deadline may expire after a mutating RPC succeeded, so this classification relies on the repository-wide requirement that queue-driven operations are idempotent. + +Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go`, `platform/errs/yarpc/yarpc_test.go`, and `platform/errs/generic/generic_test.go`. ## Overriding Classification from a Controller diff --git a/platform/errs/yarpc/BUILD.bazel b/platform/errs/yarpc/BUILD.bazel new file mode 100644 index 00000000..592d891b --- /dev/null +++ b/platform/errs/yarpc/BUILD.bazel @@ -0,0 +1,23 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["yarpc.go"], + importpath = "github.com/uber/submitqueue/platform/errs/yarpc", + visibility = ["//visibility:public"], + deps = [ + "//platform/errs:go_default_library", + "@org_uber_go_yarpc//yarpcerrors:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["yarpc_test.go"], + embed = [":go_default_library"], + deps = [ + "//platform/errs:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@org_uber_go_yarpc//yarpcerrors:go_default_library", + ], +) diff --git a/platform/errs/yarpc/yarpc.go b/platform/errs/yarpc/yarpc.go new file mode 100644 index 00000000..402ed4ce --- /dev/null +++ b/platform/errs/yarpc/yarpc.go @@ -0,0 +1,76 @@ +// Copyright (c) 2026 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 yarpc classifies YARPC status errors by code. +package yarpc + +import ( + "github.com/uber/submitqueue/platform/errs" + "go.uber.org/yarpc/yarpcerrors" +) + +// Classifier is the canonical YARPC error classifier. +var Classifier errs.Classifier = classifier{} + +type classifier struct{} + +// yarpcError is YARPC's single-node status contract for transport-specific +// errors. The classifier processor still owns traversal to reach this node. +type yarpcError interface { + YARPCError() *yarpcerrors.Status +} + +func (classifier) Classify(err error) errs.Verdict { + var status *yarpcerrors.Status + switch e := err.(type) { + case *yarpcerrors.Status: + status = e + case yarpcError: + status = e.YARPCError() + default: + return errs.Unknown + } + if status == nil { + return errs.Unknown + } + + switch status.Code() { + case yarpcerrors.CodeCancelled: + // Cancellation belongs to the caller's operation rather than to the + // downstream service, matching generic's context.Canceled verdict. + return errs.InfraRetryable + + case yarpcerrors.CodeUnknown, + yarpcerrors.CodeDeadlineExceeded, + yarpcerrors.CodeResourceExhausted, + yarpcerrors.CodeAborted, + yarpcerrors.CodeInternal, + yarpcerrors.CodeUnavailable: + return errs.InfraDependencyRetryable + + case yarpcerrors.CodeInvalidArgument, + yarpcerrors.CodeNotFound, + yarpcerrors.CodeAlreadyExists, + yarpcerrors.CodePermissionDenied, + yarpcerrors.CodeFailedPrecondition, + yarpcerrors.CodeOutOfRange, + yarpcerrors.CodeUnimplemented, + yarpcerrors.CodeDataLoss, + yarpcerrors.CodeUnauthenticated: + return errs.InfraDependency + + default: + return errs.Unknown + } +} diff --git a/platform/errs/yarpc/yarpc_test.go b/platform/errs/yarpc/yarpc_test.go new file mode 100644 index 00000000..9336e4da --- /dev/null +++ b/platform/errs/yarpc/yarpc_test.go @@ -0,0 +1,123 @@ +// Copyright (c) 2026 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 yarpc + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/uber/submitqueue/platform/errs" + "go.uber.org/yarpc/yarpcerrors" +) + +type customYARPCError struct { + status *yarpcerrors.Status +} + +func (e customYARPCError) Error() string { + return e.status.Error() +} + +func (e customYARPCError) YARPCError() *yarpcerrors.Status { + return e.status +} + +func TestClassifier_StatusCodes(t *testing.T) { + tests := []struct { + name string + code yarpcerrors.Code + want errs.Verdict + }{ + {name: "cancelled", code: yarpcerrors.CodeCancelled, want: errs.InfraRetryable}, + {name: "unknown", code: yarpcerrors.CodeUnknown, want: errs.InfraDependencyRetryable}, + {name: "deadline exceeded", code: yarpcerrors.CodeDeadlineExceeded, want: errs.InfraDependencyRetryable}, + {name: "resource exhausted", code: yarpcerrors.CodeResourceExhausted, want: errs.InfraDependencyRetryable}, + {name: "aborted", code: yarpcerrors.CodeAborted, want: errs.InfraDependencyRetryable}, + {name: "internal", code: yarpcerrors.CodeInternal, want: errs.InfraDependencyRetryable}, + {name: "unavailable", code: yarpcerrors.CodeUnavailable, want: errs.InfraDependencyRetryable}, + {name: "invalid argument", code: yarpcerrors.CodeInvalidArgument, want: errs.InfraDependency}, + {name: "not found", code: yarpcerrors.CodeNotFound, want: errs.InfraDependency}, + {name: "already exists", code: yarpcerrors.CodeAlreadyExists, want: errs.InfraDependency}, + {name: "permission denied", code: yarpcerrors.CodePermissionDenied, want: errs.InfraDependency}, + {name: "failed precondition", code: yarpcerrors.CodeFailedPrecondition, want: errs.InfraDependency}, + {name: "out of range", code: yarpcerrors.CodeOutOfRange, want: errs.InfraDependency}, + {name: "unimplemented", code: yarpcerrors.CodeUnimplemented, want: errs.InfraDependency}, + {name: "data loss", code: yarpcerrors.CodeDataLoss, want: errs.InfraDependency}, + {name: "unauthenticated", code: yarpcerrors.CodeUnauthenticated, want: errs.InfraDependency}, + {name: "ok", code: yarpcerrors.CodeOK, want: errs.Unknown}, + {name: "unrecognized code", code: yarpcerrors.Code(99), want: errs.Unknown}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, Classifier.Classify(yarpcerrors.Newf(tt.code, "rpc failed"))) + }) + } +} + +func TestClassifier_Unknown(t *testing.T) { + tests := []struct { + name string + err error + }{ + {name: "wrapped status", err: fmt.Errorf("call failed: %w", yarpcerrors.DeadlineExceededErrorf("late"))}, + {name: "plain error", err: errors.New("anything")}, + {name: "nil", err: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, errs.Unknown, Classifier.Classify(tt.err)) + }) + } +} + +func TestClassifier_TransportSpecificYARPCError(t *testing.T) { + err := customYARPCError{status: yarpcerrors.Newf(yarpcerrors.CodeUnavailable, "down")} + assert.Equal(t, errs.InfraDependencyRetryable, Classifier.Classify(err)) +} + +func TestClassifier_AppliedViaProcessor(t *testing.T) { + processor := errs.NewClassifierProcessor(Classifier) + + t.Run("wrapped deadline is a retryable dependency error", func(t *testing.T) { + err := fmt.Errorf("set ref: %w", yarpcerrors.DeadlineExceededErrorf("context deadline exceeded")) + out := processor.Process(err) + assert.True(t, errs.IsRetryable(out)) + assert.True(t, errs.IsDependencyError(out)) + }) + + t.Run("wrapped invalid argument is a non-retryable dependency error", func(t *testing.T) { + err := fmt.Errorf("set ref: %w", yarpcerrors.InvalidArgumentErrorf("bad ref")) + out := processor.Process(err) + assert.False(t, errs.IsRetryable(out)) + assert.True(t, errs.IsDependencyError(out)) + }) + + t.Run("cancelled is retryable without dependency attribution", func(t *testing.T) { + out := processor.Process(yarpcerrors.CancelledErrorf("caller cancelled")) + assert.True(t, errs.IsRetryable(out)) + assert.False(t, errs.IsDependencyError(out)) + }) + + t.Run("a controller verdict wins over the classifier", func(t *testing.T) { + err := errs.NewDependencyError(yarpcerrors.UnavailableErrorf("down")) + out := processor.Process(err) + assert.Same(t, err, out) + assert.False(t, errs.IsRetryable(out)) + }) +} diff --git a/service/stovepipe/server/BUILD.bazel b/service/stovepipe/server/BUILD.bazel index 63d9ec5a..db1e9640 100644 --- a/service/stovepipe/server/BUILD.bazel +++ b/service/stovepipe/server/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//platform/errs/generic:go_default_library", "//platform/errs/http:go_default_library", "//platform/errs/mysql:go_default_library", + "//platform/errs/yarpc:go_default_library", "//platform/extension/consumergate/noop:go_default_library", "//platform/extension/counter:go_default_library", "//platform/extension/hook:go_default_library", diff --git a/service/stovepipe/server/main.go b/service/stovepipe/server/main.go index 06c93290..ba33fabf 100644 --- a/service/stovepipe/server/main.go +++ b/service/stovepipe/server/main.go @@ -35,6 +35,7 @@ import ( genericerrs "github.com/uber/submitqueue/platform/errs/generic" httperrs "github.com/uber/submitqueue/platform/errs/http" mysqlerrs "github.com/uber/submitqueue/platform/errs/mysql" + yarpcerrs "github.com/uber/submitqueue/platform/errs/yarpc" consumergatenoop "github.com/uber/submitqueue/platform/extension/consumergate/noop" "github.com/uber/submitqueue/platform/extension/counter" hookext "github.com/uber/submitqueue/platform/extension/hook" @@ -284,6 +285,7 @@ func run() error { errs.NewClassifierProcessor( genericerrs.Classifier, httperrs.Classifier, + yarpcerrs.Classifier, mysqlerrs.Classifier, ), consumergatenoop.New(), From 3a3de98439d5d900d6e45c93ce3ae32276034709 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 27 Aug 2026 15:01:24 +0000 Subject: [PATCH 2/3] docs(errs): clarify classifier registration --- platform/errs/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/platform/errs/README.md b/platform/errs/README.md index df66ee93..e6b579e1 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -137,11 +137,13 @@ c := consumer.New(logger, scope, registry, ) ``` +Classifiers are not installed globally. A host that wants YARPC statuses classified adds `yarpcerrs.Classifier` to the `ErrorProcessor` at the boundary that consumes those errors, as above. This wiring covers outbound YARPC failures returned into that processor; inbound RPC handlers do not pass through it automatically and need their own transport middleware or mapper if they require the same classification. + `httperrs` precedes `mysqlerrs` for a reason worth knowing before reordering the list: the MySQL classifier treats any `net.Error` as retryable infra, and the `*url.Error` an HTTP client returns satisfies `net.Error`. Whichever runs first claims that node, so with the order reversed an HTTP transport failure is classified as a MySQL one — retryable either way, but no longer attributed to the dependency it came from. This is the cross-extension ambiguity `NewClassifierProcessor` documents as deferred; registration order is the workaround. The YARPC classifier reads the typed status code rather than matching its rendered message. Cancellation is retryable caller-side infrastructure; transient or ambiguous server codes (`Unknown`, `DeadlineExceeded`, `ResourceExhausted`, `Aborted`, `Internal`, and `Unavailable`) are retryable dependency failures; request verdicts and permanent server failures are non-retryable dependency failures. A deadline may expire after a mutating RPC succeeded, so this classification relies on the repository-wide requirement that queue-driven operations are idempotent. -Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go`, `platform/errs/yarpc/yarpc_test.go`, and `platform/errs/generic/generic_test.go`. +Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go` and `platform/errs/generic/generic_test.go`. ## Overriding Classification from a Controller From 71a964c29a5e24933dc8447a3781e1fcd618807b Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Thu, 27 Aug 2026 15:07:11 +0000 Subject: [PATCH 3/3] docs(errs): reference YARPC classifier tests --- platform/errs/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platform/errs/README.md b/platform/errs/README.md index e6b579e1..b86c3b10 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -143,7 +143,7 @@ Classifiers are not installed globally. A host that wants YARPC statuses classif The YARPC classifier reads the typed status code rather than matching its rendered message. Cancellation is retryable caller-side infrastructure; transient or ambiguous server codes (`Unknown`, `DeadlineExceeded`, `ResourceExhausted`, `Aborted`, `Internal`, and `Unavailable`) are retryable dependency failures; request verdicts and permanent server failures are non-retryable dependency failures. A deadline may expire after a mutating RPC succeeded, so this classification relies on the repository-wide requirement that queue-driven operations are idempotent. -Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go` and `platform/errs/generic/generic_test.go`. +Tests follow the same shape: assert per-node behaviour against `Classifier.Classify(node)` directly, and assert end-to-end behaviour by running `errs.NewClassifierProcessor(Classifier).Process(err)` and checking the helpers (`IsRetryable`, `IsUserError`, …) on the result. See `platform/errs/mysql/mysql_test.go`, `platform/errs/yarpc/yarpc_test.go`, and `platform/errs/generic/generic_test.go`. ## Overriding Classification from a Controller