From 4d9d686c3d97bf34af132204f92e6b6b8e335f0a Mon Sep 17 00:00:00 2001 From: "prath.shenoy" Date: Wed, 19 Aug 2026 19:10:46 +0000 Subject: [PATCH] fix(errs): Retry transient grouped failures --- platform/errs/BUILD.bazel | 2 + platform/errs/README.md | 35 ++- platform/errs/group.go | 57 ++++ platform/errs/group_test.go | 79 ++++++ platform/errs/processor.go | 149 +++++++++-- platform/errs/processor_test.go | 253 ++++++++++++++++++ .../extension/validator/composite/BUILD.bazel | 1 + .../validator/composite/validator.go | 14 +- 8 files changed, 553 insertions(+), 37 deletions(-) create mode 100644 platform/errs/group.go create mode 100644 platform/errs/group_test.go diff --git a/platform/errs/BUILD.bazel b/platform/errs/BUILD.bazel index c7de244e9..8e5e6d32e 100644 --- a/platform/errs/BUILD.bazel +++ b/platform/errs/BUILD.bazel @@ -5,6 +5,7 @@ go_library( srcs = [ "errs.go", "failure.go", + "group.go", "processor.go", ], importpath = "github.com/uber/submitqueue/platform/errs", @@ -17,6 +18,7 @@ go_test( srcs = [ "errs_test.go", "failure_test.go", + "group_test.go", "processor_test.go", ], embed = [":go_default_library"], diff --git a/platform/errs/README.md b/platform/errs/README.md index 391054c4c..ab314b25e 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -44,13 +44,44 @@ An `ErrorProcessor` runs the per-chain pass that turns a raw chain into a wrappe Two implementations ship in this package: - **`NewClassifierProcessor(classifiers...)`** — the standard pass for primary pipeline consumers. Walks the chain twice: - 1. **Pass 1 — framework-wrap check.** A cheap type switch looks for an existing `*userError` / `*infraError` anywhere in the chain. If found, the chain is already interpretable and the processor returns `err` unchanged. **No classifier is invoked.** + 1. **Pass 1 — framework-wrap check.** Looks for an existing `*userError` / `*infraError` on the error's single-cause spine. If found, the chain is already interpretable and the processor returns `err` unchanged. **No classifier is invoked.** 2. **Pass 2 — classifier walk.** From outermost to innermost node, each registered classifier is asked for a verdict. The first non-`Unknown` verdict wins and `err` is wrapped with the matching framework constructor. If no classifier recognises anything, `err` is returned unchanged — and behaves as non-retryable infra at the helper layer. - **`AlwaysRetryableProcessor`** — unconditionally wraps every non-nil error with `NewRetryableError`, overriding any inner framework wrap. Use it for narrowly-scoped consumers — typically DLQ reconciliation — that must redeliver on any failure because there is no further dead-letter destination. Side-effect: an inner `*infraError(dependency=true)` is masked by the outer `retryable=true` wrap, since `errors.As` matches the outermost `*infraError` first. This is acceptable for the intended DLQ use case where only `IsRetryable` drives transport behaviour; do not pair this processor with a primary pipeline consumer or genuine user errors will retry forever instead of reaching their DLQ. +### Grouped errors + +`Group(errs...)` reports several failures that happened together as one error. It drops nils and returns nil when every member is nil, so a step that fans work out to independent handlers can accumulate failures in a loop and return the result directly: + +```go +var failures []error +for _, h := range handlers { + if err := h.Handle(ctx, event); err != nil { + failures = append(failures, fmt.Errorf("%s: %w", h.Name(), err)) + } +} +return errs.Group(failures...) +``` + +Pass 2 descends into a group's members as well as into ordinary single-cause wraps; Pass 1 walks only the single-cause spine. Without that descent, `errors.Unwrap` returns nil for a group, so a walk built on it alone sees the group node and nothing beneath it, and every member goes unclassified. + +**Grouping is opt-in — only `Group` is weighed.** `Unwrap() []error` is also what `errors.Join` and `fmt.Errorf` with several `%w` produce, and there the extra causes are incidental: a cleanup failure hung off the real one, or context that happens to be an error. Weighing those would let an unrelated sibling decide retryability for the whole chain. `Group` is the one spelling that means "these failures are independent, rank them against each other", so the walk keys on that type and every other multi-cause error stays opaque — classified by its outermost recognisable node like any single-cause chain. A caller that wants its members ranked says so by returning `Group`, as `submitqueue/extension/validator/composite` does for its children. + +The two shapes combine differently, because they mean different things: + +- **Down a wrap chain**, the outermost verdict wins. A wrapper saw the error it wrapped and classified anyway, so it speaks with more knowledge than its cause. +- **Across the members of a group**, nothing shadows anything. The members are independent failures reported together, and their order is the order the caller ran them in, not a precedence. They combine by rank, so the result cannot depend on which member failed first. + +The rank puts retryable above non-retryable, because the two mistakes cost differently: a wrong "retryable" spends a bounded retry budget and then dead-letters anyway, while a wrong "non-retryable" throws away a failure that would have cleared on its own. Within a retryability tier, the verdict that implicates this service outranks the one pointing elsewhere, so a partly-local failure is not reported as a pure dependency or user problem — that ordering only moves attribution, since every non-retryable verdict produces the same transport outcome. See `verdictRank` for the table. + +A framework wrap classifies the subtree beneath it and no further. Above a group it covers the whole group and Pass 1 returns the error verbatim, so no member is consulted. *Inside* a member it is one member's account of one failure, with no standing to classify the failures beside it — so it contributes its own verdict to the rank like any other member. That is what keeps a sibling's transient failure from being discarded by a member that happened to arrive pre-classified, and it also removes an ordering artifact: two wrapped members of differing retryability used to resolve by whichever one `errors.As` reached first. + +The losing member keeps its wrap in the chain, so `IsUserError` and `IsRetryable` can both report true for the same grouped error — one from a member, one from the outer wrap. Only the outer wrap drives the retry decision, the same way it does under `AlwaysRetryableProcessor`; `IsUserError` carries that precedence as a contract note, since a caller checking it before `IsRetryable` would drop the transient member. + +One operational consequence worth knowing before relying on any of this: **retrying a group re-runs everything.** The retry redelivers to every child, including the ones that succeeded, so children must be idempotent, and a child that fails persistently with a retryable-looking error (a decommissioned service returning connection-refused, say) will spend the whole retry budget on every message. Drop such a child rather than absorbing it. + ### Choosing a processor - **Primary pipeline consumer** → `NewClassifierProcessor(...)`. Controllers' explicit `NewUserError` / `NewDependencyError` wraps must survive so user errors don't get retried, and unclassified backend errors must be inspected by the registered classifiers. @@ -131,7 +162,7 @@ if err != nil { Two practical rules fall out of the short-circuit semantics: - **Wrap with a framework constructor as soon as the controller knows the right verdict.** Any wrap added later in the chain still wins, but wrapping early keeps the intent close to the decision. -- **A wrap anywhere in the chain blocks all classifiers — including for nodes deeper than the wrap.** If you want a classifier to still get a look at the cause, do not wrap above it. (In practice this is rare: controllers wrap because they have the final answer.) +- **A wrap blocks all classifiers beneath it, including for nodes deeper than the wrap.** If you want a classifier to still get a look at the cause, do not wrap above it. (In practice this is rare: controllers wrap because they have the final answer.) It does not block the sibling members of a group, which are classified and ranked independently. ### When *not* to classify in a controller diff --git a/platform/errs/group.go b/platform/errs/group.go new file mode 100644 index 000000000..7db3af3a5 --- /dev/null +++ b/platform/errs/group.go @@ -0,0 +1,57 @@ +// 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 errs + +import "strings" + +// Group reports several failures that happened together as one error, for a +// step that must run every handler rather than stop at the first failure. +// +// nil arguments are dropped and Group returns nil when all of them are nil. +// errors.Is and errors.As reach every error passed in. +func Group(errs ...error) error { + members := make([]error, 0, len(errs)) + for _, err := range errs { + if err != nil { + members = append(members, err) + } + } + if len(members) == 0 { + return nil + } + return &groupedError{members: members} +} + +// groupedError is the error Group returns. +type groupedError struct { + // members are the failures reported together, in the order given. Never + // empty, and never contains a nil. + members []error +} + +// Error joins the member messages on one line, because these land in +// structured logs where a multi-line message becomes several records. +func (e *groupedError) Error() string { + msgs := make([]string, 0, len(e.members)) + for _, m := range e.members { + msgs = append(msgs, m.Error()) + } + return strings.Join(msgs, "; ") +} + +// Unwrap returns the grouped failures for errors.Is/As compatibility. +func (e *groupedError) Unwrap() []error { + return e.members +} diff --git a/platform/errs/group_test.go b/platform/errs/group_test.go new file mode 100644 index 000000000..6d3fa2787 --- /dev/null +++ b/platform/errs/group_test.go @@ -0,0 +1,79 @@ +// 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 errs + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGroup_NoFailures(t *testing.T) { + tests := map[string][]error{ + "no members": nil, + "single nil": {nil}, + "all nil": {nil, nil}, + } + + for name, members := range tests { + t.Run(name, func(t *testing.T) { + assert.NoError(t, Group(members...)) + }) + } +} + +func TestGroup_DropsNilMembers(t *testing.T) { + a := errors.New("a") + b := errors.New("b") + + out := Group(nil, a, nil, b, nil) + + require.Error(t, out) + assert.Equal(t, "a; b", out.Error()) +} + +// Every member must stay reachable, because a member the caller cannot find is +// also a member the classifier cannot weigh. +func TestGroup_ReachesEveryMember(t *testing.T) { + sentinel := errors.New("sentinel") + other := errors.New("other") + + out := Group(fmt.Errorf("child a: %w", sentinel), other) + + assert.True(t, errors.Is(out, sentinel)) + assert.True(t, errors.Is(out, other)) + assert.False(t, errors.Is(out, errors.New("never grouped"))) +} + +func TestGroup_ErrorsAsFindsAMemberType(t *testing.T) { + out := Group(errors.New("plain"), NewUserError(errors.New("bad input"))) + + var ue *userError + require.True(t, errors.As(out, &ue)) + assert.True(t, IsUserError(out)) +} + +// One log record per failed delivery, not one per member. +func TestGroup_MessageIsSingleLine(t *testing.T) { + out := Group(errors.New("first"), errors.New("second"), errors.New("third")) + + assert.NotContains(t, out.Error(), "\n") + for _, want := range []string{"first", "second", "third"} { + assert.Contains(t, out.Error(), want) + } +} diff --git a/platform/errs/processor.go b/platform/errs/processor.go index 6fd806f02..303f9c349 100644 --- a/platform/errs/processor.go +++ b/platform/errs/processor.go @@ -50,10 +50,9 @@ type ErrorProcessor interface { // Semantics of Process on the returned processor: // // - nil in, nil out. -// - If err's chain already carries a framework classification (*userError -// or *infraError anywhere in the chain), returns err unchanged — the chain -// is already interpretable by IsUserError / IsRetryable / -// IsDependencyError. +// - If err carries a framework classification (*userError or *infraError) on +// its single-cause spine, returns err unchanged — the chain is already +// interpretable by IsUserError / IsRetryable / IsDependencyError. // - Otherwise, walks the chain from outermost to innermost, asking each // classifier per node. The FIRST non-Unknown verdict wins; the outermost // such node determines the wrap. err is wrapped with the framework @@ -61,15 +60,21 @@ type ErrorProcessor interface { // -> NewRetryableError, etc.) and the wrapped error is returned. // - Verdict Infra means "non-retryable infra" — which is already the default // behavior for an unwrapped chain, so no wrap is added. -// - If no classifier recognises anything, err is returned unchanged. +// - If no classifier recognizes anything, err is returned unchanged. // -// Implementation: two passes over the chain. Pass 1 is a cheap type check -// looking for an existing framework wrap and short-circuits if one is found — -// no classifier is invoked. Pass 2 runs the configured classifiers per node. -// Walking the chain is cheap relative to a classifier call, so this avoids -// running classifiers whenever the chain is already classified deeper down. +// Implementation: two passes over the chain. Pass 1 looks for an existing +// framework wrap and short-circuits if one is found — no classifier is invoked. +// Pass 2 runs the configured classifiers per node. Walking the chain is cheap +// relative to a classifier call, so this avoids running classifiers whenever +// the chain is already classified deeper down. // -// Passing no classifiers is valid — the processor will still honour any +// Pass 2 also descends into the members of a Group; Pass 1 walks the +// single-cause spine only. Grouping is opt-in: a Group means "independent +// failures, weigh them against each other", and no other multi-cause shape +// (errors.Join, fmt.Errorf with several %w) claims that meaning, so those stay +// opaque. classify documents how members combine. +// +// Passing no classifiers is valid — the processor will still honor any // framework wrap already in the chain and otherwise return err unchanged. // // NOTE: this central classifier model cannot disambiguate errors of the same @@ -90,29 +95,19 @@ func (p classifierProcessor) Process(err error) error { return nil } - // Pass 1 — cheap framework-wrap check. If any node already carries a - // framework type, the chain is interpretable as-is and classifiers are - // never invoked. + // Pass 1 — framework-wrap check, along the single-cause spine only. A wrap + // found here sits above everything else in err, so it classifies the whole + // error and is returned verbatim. errors.Unwrap yields nothing for a Group, + // so a wrap inside a member never answers for its siblings; classify weighs + // it against them instead. for cur := err; cur != nil; cur = errors.Unwrap(cur) { - switch cur.(type) { - case *userError, *infraError: + if wrapVerdict(cur) != Unknown { return err } } - // Pass 2 — run classifiers per node from outermost to innermost. Stop at - // the first non-Unknown verdict. - var verdict Verdict - for cur := err; cur != nil && verdict == Unknown; cur = errors.Unwrap(cur) { - for _, c := range p.classifiers { - if v := c.Classify(cur); v != Unknown { - verdict = v - break - } - } - } - - switch verdict { + // Pass 2 — classify the chain and wrap with the verdict it reaches. + switch p.classify(err) { case User: return NewUserError(err) case InfraRetryable: @@ -127,6 +122,102 @@ func (p classifierProcessor) Process(err error) error { return err } +// classify returns the verdict for err — from a framework wrap if it carries +// one, otherwise from the configured classifiers — or Unknown when nothing +// recognizes any node in it. +// +// The walk treats the two ways an error can contain another differently, +// because they mean different things: +// +// - Down a wrap chain (Unwrap() error) the outermost verdict wins. A wrapper +// saw the error it wrapped and chose to add context on top of it, so it +// speaks with more knowledge than its cause. +// - Across the members of a Group nothing shadows anything. +// The members are independent failures that happened to be reported +// together, and their order is the order the caller ran them in, not a +// precedence. They combine by verdictRank so that the result cannot depend +// on which member failed first. +// +// The second rule is why a framework wrap does not get the short-circuit it +// gets on the spine: within a group it is one member's account of one failure, +// with no standing to classify the failures beside it. +func (p classifierProcessor) classify(err error) Verdict { + for cur := err; cur != nil; { + // A wrap is authoritative for everything it contains, so it answers for + // this subtree without consulting a classifier. + if v := wrapVerdict(cur); v != Unknown { + return v + } + + for _, c := range p.classifiers { + if v := c.Classify(cur); v != Unknown { + return v + } + } + + if group, ok := cur.(*groupedError); ok { + verdict := Unknown + for _, member := range group.Unwrap() { + if v := p.classify(member); verdictRank(v) > verdictRank(verdict) { + verdict = v + } + } + return verdict + } + + cur = errors.Unwrap(cur) + } + return Unknown +} + +func wrapVerdict(err error) Verdict { + switch e := err.(type) { + case *userError: + return User + case *infraError: + switch { + case e.retryable && e.dependency: + return InfraDependencyRetryable + case e.retryable: + return InfraRetryable + case e.dependency: + return InfraDependency + default: + return Infra + } + } + return Unknown +} + +// verdictRank orders the members of a Group; the highest rank becomes the +// group's verdict. +// +// A table, not a comparison on Verdict, whose constants are declaration-ordered: +// InfraDependency exceeds InfraRetryable by value, so ranking by value would let +// a non-retryable member discard a retryable sibling. +// +// Retryable ranks above non-retryable — a wrong "retryable" spends a bounded +// retry budget, a wrong "non-retryable" strands a failure that would have +// cleared. The non-retryable verdicts all dead-letter alike, so their order only +// sets attribution, most actionable first: ours to fix, then the requester's, +// then a dependency nobody here can act on. +func verdictRank(v Verdict) int { + switch v { + case InfraRetryable: + return 5 + case InfraDependencyRetryable: + return 4 + case Infra: + return 3 + case User: + return 2 + case InfraDependency: + return 1 + default: // Unknown + return 0 + } +} + // AlwaysRetryableProcessor classifies every non-nil error as InfraRetryable by // wrapping it with NewRetryableError. The wrap is unconditional: an inner // *userError or non-retryable *infraError is overridden because errors.As diff --git a/platform/errs/processor_test.go b/platform/errs/processor_test.go index d81d5ed75..d8377ea4f 100644 --- a/platform/errs/processor_test.go +++ b/platform/errs/processor_test.go @@ -30,6 +30,21 @@ type stubClassifier struct{ verdict Verdict } func (s stubClassifier) Classify(error) Verdict { return s.verdict } +// verdictByError recognizes only the exact nodes it was built with, the way a +// real backend classifier recognizes only its own driver's errors. Everything +// else classifies Unknown. +type verdictByError map[error]Verdict + +func (m verdictByError) Classify(err error) Verdict { return m[err] } + +// singleWrap is a classifiable node with exactly one cause. fmt.Errorf cannot +// stand in for it: its wrapper node is opaque to a classifier, and two %w verbs +// produce a multi-cause node rather than a chain. +type singleWrap struct{ cause error } + +func (w singleWrap) Error() string { return "wrapped: " + w.cause.Error() } +func (w singleWrap) Unwrap() error { return w.cause } + func TestNewClassifierProcessor_NilIn(t *testing.T) { p := NewClassifierProcessor() assert.NoError(t, p.Process(nil)) @@ -69,6 +84,244 @@ func TestNewClassifierProcessor_NoClassifiersReturnsUnchanged(t *testing.T) { assert.False(t, IsUserError(out)) } +// TestNewClassifierProcessor_GroupedMembers covers the reason Group exists: a +// caller that fans work out to several children reports their failures as one +// error, and errors.Unwrap cannot see into one, so classifiers were never +// offered any member. +func TestNewClassifierProcessor_GroupedMembers(t *testing.T) { + transient := errors.New("deadlock") + permanent := errors.New("schema mismatch") + badInput := errors.New("malformed payload") + upstreamBlip := errors.New("upstream 503") + upstreamGone := errors.New("upstream decommissioned") + + p := NewClassifierProcessor(verdictByError{ + transient: InfraRetryable, + permanent: Infra, + badInput: User, + upstreamBlip: InfraDependencyRetryable, + upstreamGone: InfraDependency, + }) + + tests := []struct { + name string + err error + wantRetryable bool + wantUser bool + wantDependency bool + }{ + { + // A group of one is still a group, so a lone failing child is just + // as opaque to errors.Unwrap as several. + name: "single member", + err: Group(fmt.Errorf("child a: %w", transient)), + wantRetryable: true, + }, + { + name: "retryable member last", + err: Group(fmt.Errorf("child a: %w", permanent), fmt.Errorf("child b: %w", transient)), + wantRetryable: true, + }, + { + // Same members reversed: the verdict must come from rank, not from + // the order the children happened to run in. + name: "retryable member first", + err: Group(fmt.Errorf("child a: %w", transient), fmt.Errorf("child b: %w", permanent)), + wantRetryable: true, + }, + { + name: "retryable outranks user", + err: Group(badInput, transient), + wantRetryable: true, + }, + { + name: "group nested below a wrap", + err: fmt.Errorf("dispatch: %w", Group(permanent, fmt.Errorf("child b: %w", transient))), + wantRetryable: true, + }, + { + name: "group nested inside another group", + err: Group(permanent, Group(badInput, transient)), + wantRetryable: true, + }, + { + // Both members are retryable, so only attribution is in question: + // a failure that is partly local is not blamed on the dependency. + name: "local retryable outranks dependency retryable", + err: Group(upstreamBlip, transient), + wantRetryable: true, + }, + { + name: "dependency retryable alone keeps its provenance", + err: Group(permanent, upstreamBlip), + wantRetryable: true, + wantDependency: true, + }, + { + name: "user outranks non-retryable dependency", + err: Group(upstreamGone, badInput), + wantUser: true, + }, + { + name: "no member recognized", + err: Group(errors.New("who knows"), errors.New("nor this")), + }, + { + // "nobody recognized this" is weaker evidence than any verdict, so + // an unrecognized member must not drown out a classified sibling. + name: "unrecognized member does not outrank a classified user sibling", + err: Group(errors.New("who knows"), badInput), + wantUser: true, + }, + { + name: "unrecognized member does not outrank a classified dependency sibling", + err: Group(errors.New("who knows"), upstreamGone), + wantDependency: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := p.Process(tt.err) + require.Error(t, out) + assert.Equal(t, tt.wantRetryable, IsRetryable(out)) + assert.Equal(t, tt.wantUser, IsUserError(out)) + assert.Equal(t, tt.wantDependency, IsDependencyError(out)) + }) + } +} + +// TestNewClassifierProcessor_MultiCauseErrorsOutsideGroupAreOpaque pins the +// opt-in half of the rule. errors.Join and a multi-%w fmt.Errorf also expose +// Unwrap() []error, but their causes are incidental rather than independent +// failures offered for ranking, so their members are never weighed and the +// error is classified by its outermost recognisable node like any other chain. +func TestNewClassifierProcessor_MultiCauseErrorsOutsideGroupAreOpaque(t *testing.T) { + transient := errors.New("deadlock") + badInput := errors.New("malformed payload") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable, badInput: User}) + + tests := []struct { + name string + err error + }{ + { + name: "errors.Join", + err: errors.Join(badInput, transient), + }, + { + name: "fmt.Errorf with two %w", + err: fmt.Errorf("validate: %w, cleanup: %w", badInput, transient), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := p.Process(tt.err) + require.Error(t, out) + assert.False(t, IsRetryable(out)) + assert.False(t, IsUserError(out)) + assert.True(t, errors.Is(out, transient), "the causes stay reachable, they are just not ranked") + }) + } +} + +func TestNewClassifierProcessor_GroupedMembersKeepEveryCause(t *testing.T) { + transient := errors.New("deadlock") + permanent := errors.New("schema mismatch") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + out := p.Process(Group(fmt.Errorf("child a: %w", permanent), fmt.Errorf("child b: %w", transient))) + + require.True(t, IsRetryable(out)) + assert.True(t, errors.Is(out, transient)) + assert.True(t, errors.Is(out, permanent), "the member that lost the rank must stay in the chain for diagnostics") +} + +// TestNewClassifierProcessor_WrappedMembersAreWeighed covers members that +// arrive already classified. A wrap speaks for the subtree beneath it, so it +// contributes a verdict to the group like any other member rather than deciding +// for its siblings — which is what makes the outcome independent of the order +// the members were reported in. +func TestNewClassifierProcessor_WrappedMembersAreWeighed(t *testing.T) { + transient := errors.New("deadlock") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + tests := []struct { + name string + err error + wantRetryable bool + wantUser bool + }{ + { + // IsUserError stays true alongside it: the losing member keeps its + // wrap in the chain, and only the outer one drives the retry. + name: "wrapped user error does not suppress a classifiable sibling", + err: Group(NewUserError(errors.New("malformed payload")), transient), + wantRetryable: true, + wantUser: true, + }, + { + name: "retryable wrap ranked ahead of a non-retryable one", + err: Group(NewDependencyError(errors.New("upstream 503")), NewRetryableError(errors.New("blip"))), + wantRetryable: true, + }, + { + // The same two wraps reversed. Before wraps were weighed, this pair + // resolved by whichever member errors.As reached first. + name: "retryable wrap ranked ahead of a non-retryable one, reversed", + err: Group(NewRetryableError(errors.New("blip")), NewDependencyError(errors.New("upstream 503"))), + wantRetryable: true, + }, + { + name: "sole wrapped member still classifies the group", + err: Group(NewUserError(errors.New("malformed payload")), errors.New("unrecognized")), + wantUser: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := p.Process(tt.err) + require.Error(t, out) + assert.Equal(t, tt.wantRetryable, IsRetryable(out)) + assert.Equal(t, tt.wantUser, IsUserError(out)) + }) + } +} + +// TestNewClassifierProcessor_SpineWrapStillShortCircuits pins the other half of +// the rule: a wrap above a group covers the whole group, so it is returned +// verbatim and no member is consulted. +func TestNewClassifierProcessor_SpineWrapStillShortCircuits(t *testing.T) { + transient := errors.New("deadlock") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + wrapped := NewUserError(Group(errors.New("child a"), transient)) + out := p.Process(wrapped) + + assert.Same(t, wrapped, out) + assert.True(t, IsUserError(out)) + assert.False(t, IsRetryable(out)) +} + +// TestNewClassifierProcessor_WrapChainKeepsOutermostVerdict guards the +// asymmetry between the two walks: rank decides between the members of a group, +// but down a wrap chain the outer node still wins outright, because it saw its +// cause and classified anyway. Without this the group rule would leak into +// ordinary chains and let a retryable cause override the verdict a caller +// deliberately put on top of it. +func TestNewClassifierProcessor_WrapChainKeepsOutermostVerdict(t *testing.T) { + inner := errors.New("deadlock") + outer := singleWrap{cause: inner} + p := NewClassifierProcessor(verdictByError{outer: User, inner: InfraRetryable}) + + out := p.Process(outer) + + assert.True(t, IsUserError(out)) + assert.False(t, IsRetryable(out)) +} + func TestAlwaysRetryableProcessor_NilIn(t *testing.T) { assert.NoError(t, AlwaysRetryableProcessor.Process(nil)) } diff --git a/submitqueue/extension/validator/composite/BUILD.bazel b/submitqueue/extension/validator/composite/BUILD.bazel index 55a9cf716..a1b45bf02 100644 --- a/submitqueue/extension/validator/composite/BUILD.bazel +++ b/submitqueue/extension/validator/composite/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/uber/submitqueue/submitqueue/extension/validator/composite", visibility = ["//visibility:public"], deps = [ + "//platform/errs:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/validator:go_default_library", ], diff --git a/submitqueue/extension/validator/composite/validator.go b/submitqueue/extension/validator/composite/validator.go index b6f38eff9..5788e9d65 100644 --- a/submitqueue/extension/validator/composite/validator.go +++ b/submitqueue/extension/validator/composite/validator.go @@ -16,13 +16,13 @@ package composite import ( "context" - "errors" + "github.com/uber/submitqueue/platform/errs" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/validator" ) -// compositeValidator runs all validators and joins their errors. +// compositeValidator runs all validators and groups their errors. type compositeValidator struct { // cfg is the per-queue identity this validator was built for. cfg validator.Config @@ -31,17 +31,19 @@ type compositeValidator struct { } // New creates a Validator bound to the queue named in cfg that evaluates all -// child validators and joins their errors. +// child validators and groups their errors. func New(cfg validator.Config, validators []validator.Validator) validator.Validator { return &compositeValidator{cfg: cfg, validators: validators} } func (c *compositeValidator) Validate(ctx context.Context, request entity.Request) error { - var errs []error + var failures []error for _, v := range c.validators { if err := v.Validate(ctx, request); err != nil { - errs = append(errs, err) + failures = append(failures, err) } } - return errors.Join(errs...) + // Group, not errors.Join: the children ran independently, so each failure + // must be classified on its own rather than the first one speaking for all. + return errs.Group(failures...) }