Skip to content

Commit 31ce24d

Browse files
committed
Merge branch 'main' into prath.shenoy/classify-joined-errors
2 parents 98475ff + 574465f commit 31ce24d

18 files changed

Lines changed: 449 additions & 345 deletions

File tree

platform/errs/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ go_library(
55
srcs = [
66
"errs.go",
77
"failure.go",
8+
"group.go",
89
"processor.go",
910
],
1011
importpath = "github.com/uber/submitqueue/platform/errs",
@@ -17,6 +18,7 @@ go_test(
1718
srcs = [
1819
"errs_test.go",
1920
"failure_test.go",
21+
"group_test.go",
2022
"processor_test.go",
2123
],
2224
embed = [":go_default_library"],

platform/errs/README.md

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,22 +51,38 @@ Two implementations ship in this package:
5151

5252
- **`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.
5353

54-
### Joined errors
54+
### Grouped errors
5555

56-
Both passes descend into joined errors — `errors.Join`, `fmt.Errorf` with more than one `%w`, or any other error exposing `Unwrap() []error` — as well as ordinary single-cause wraps. This matters for anything that fans work out to several children and reports their failures together, `submitqueue/extension/validator/composite` being the current example: `errors.Unwrap` returns nil for a join, so a walk built on it alone sees the join node and nothing beneath it, and every branch goes unclassified.
56+
`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:
5757

58-
The two shapes combine differently, because they mean different things:
58+
```go
59+
var failures []error
60+
for _, h := range handlers {
61+
if err := h.Handle(ctx, event); err != nil {
62+
failures = append(failures, fmt.Errorf("%s: %w", h.Name(), err))
63+
}
64+
}
65+
return errs.Group(failures...)
66+
```
67+
68+
Pass 2 descends into the members of a group, as well as down ordinary single-cause wraps. Without that, `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.
69+
70+
**Grouping is opt-in, and `Group` is how you opt in.** The processor recognizes a group only by the type `Group` returns, not by the `Unwrap() []error` method. That method alone does not mean "independent failures": `fmt.Errorf("%w: %w", ErrNotFound, err)` uses it for two facets of a single failure, and `errors.Join` uses it for whatever a caller happened to bundle, often a real failure beside a cleanup error on a shutdown path. Ranking those would let an incidental sibling decide retryability for a failure nobody meant to group — a transient error joined to the failure that caused it would make the whole thing look retryable. So the deliberate spelling is required, and every other multi-cause error is an opaque node.
71+
72+
This costs less than it appears, because it only governs verdicts a **classifier** has to derive. `errors.Is` and `errors.As` traverse multiple causes on their own, so a member that already carries a framework wrap is honored wherever it sits: `IsRetryable(errors.Join(cleanupErr, NewRetryableError(cause)))` is true with no group involved.
73+
74+
A group and a wrap chain combine differently, because they mean different things:
5975

6076
- **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.
61-
- **Across the branches of a join**, nothing shadows anything. The branches 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 branch failed first.
77+
- **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.
6278

6379
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.
6480

65-
A framework wrap classifies the subtree beneath it and no further. Above a join it covers the whole join and Pass 1 returns the error verbatim, so no branch is consulted. *Inside* a branch it is one branch'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 branch. That is what keeps a sibling's transient failure from being discarded by a branch that happened to arrive pre-classified, and it also removes an ordering artifact: two wrapped branches of differing retryability used to resolve by whichever one `errors.As` reached first.
81+
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.
6682

67-
The losing branch keeps its wrap in the chain, so `IsUserError` and `IsRetryable` can both report true for the same joined error — one from a branch, one from the outer wrap. Only the outer wrap drives the retry decision, the same way it does under `AlwaysRetryableProcessor`.
83+
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`.
6884

69-
One operational consequence worth knowing before relying on any of this: **retrying a join 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.
85+
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.
7086

7187
### Choosing a processor
7288

@@ -148,7 +164,7 @@ if err != nil {
148164
Two practical rules fall out of the short-circuit semantics:
149165

150166
- **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.
151-
- **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 branches of a join, which are classified and ranked independently.
167+
- **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.
152168

153169
### When *not* to classify in a controller
154170

platform/errs/processor.go

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,11 @@ type ErrorProcessor interface {
6868
// relative to a classifier call, so this avoids running classifiers whenever
6969
// the chain is already classified deeper down.
7070
//
71-
// Both passes traverse joined errors (errors.Join, or anything else exposing
72-
// Unwrap() []error) as well as ordinary single-cause wraps. A wrap classifies
73-
// the subtree beneath it and no more, so a wrapped branch of a join is weighed
71+
// Grouping is opt-in. Pass 2 descends into the members of a Group, where a wrap
72+
// classifies the subtree beneath it and no more, so a wrapped member is weighed
7473
// against its siblings instead of answering for them; classify documents how.
74+
// Any other multi-cause error is an opaque node, so a caller that wants its
75+
// failures weighed together says so with Group.
7576
//
7677
// Passing no classifiers is valid — the processor will still honor any
7778
// framework wrap already in the chain and otherwise return err unchanged.
@@ -96,9 +97,9 @@ func (p classifierProcessor) Process(err error) error {
9697

9798
// Pass 1 — framework-wrap check, along the single-cause spine only. A wrap
9899
// found here sits above everything else in err, so it classifies the whole
99-
// error and is returned verbatim. The walk stops at a join because
100+
// error and is returned verbatim. The walk stops at a group because
100101
// errors.Unwrap yields nothing for one, which is the behavior we want: a
101-
// wrap inside one branch speaks only for that branch, and classify weighs
102+
// wrap inside one member speaks only for that member, and classify weighs
102103
// it against its siblings rather than letting it silently answer for them.
103104
for cur := err; cur != nil; cur = errors.Unwrap(cur) {
104105
if wrapVerdict(cur) != Unknown {
@@ -132,15 +133,23 @@ func (p classifierProcessor) Process(err error) error {
132133
// - Down a wrap chain (Unwrap() error) the outermost verdict wins. A wrapper
133134
// saw the error it wrapped and chose to add context on top of it, so it
134135
// speaks with more knowledge than its cause.
135-
// - Across the branches of a join (Unwrap() []error) nothing shadows anything.
136-
// The branches are independent failures that happened to be reported
137-
// together, and their order is the order the caller ran them in, not a
138-
// precedence. They combine by verdictRank so that the result cannot depend
139-
// on which branch failed first.
136+
// - Across the members of a Group nothing shadows anything. The members are
137+
// independent failures that happened to be reported together, and their
138+
// order is the order the caller ran them in, not a precedence. They combine
139+
// by verdictRank so that the result cannot depend on which member failed
140+
// first.
140141
//
141142
// The second rule is why a framework wrap does not get the short-circuit it
142-
// gets on the spine: within a join it is one branch's account of one failure,
143+
// gets on the spine: within a group it is one member's account of one failure,
143144
// with no standing to classify the failures beside it.
145+
//
146+
// Only the group Group builds is walked that way, because Unwrap() []error on
147+
// its own does not mean "independent failures". fmt.Errorf("%w: %w", ...)
148+
// produces one for two facets of a single failure, and errors.Join produces one
149+
// for whatever a caller happened to bundle. Ranking those would let an
150+
// incidental sibling decide retryability for a failure nobody meant to group,
151+
// so the walk asks for the deliberate spelling and treats every other
152+
// multi-cause error as an opaque node.
144153
func (p classifierProcessor) classify(err error) Verdict {
145154
for cur := err; cur != nil; {
146155
// A wrap is authoritative for everything it contains, so it answers for
@@ -155,10 +164,10 @@ func (p classifierProcessor) classify(err error) Verdict {
155164
}
156165
}
157166

158-
if joined, ok := cur.(interface{ Unwrap() []error }); ok {
167+
if group, ok := cur.(*groupedError); ok {
159168
verdict := Unknown
160-
for _, branch := range joined.Unwrap() {
161-
if v := p.classify(branch); verdictRank(v) > verdictRank(verdict) {
169+
for _, member := range group.Unwrap() {
170+
if v := p.classify(member); verdictRank(v) > verdictRank(verdict) {
162171
verdict = v
163172
}
164173
}
@@ -193,14 +202,14 @@ func wrapVerdict(err error) Verdict {
193202
return Unknown
194203
}
195204

196-
// verdictRank orders verdicts for combining the independent branches of a
197-
// joined error. The highest-ranked branch verdict becomes the verdict for the
198-
// join as a whole.
205+
// verdictRank orders verdicts for combining the independent members of a
206+
// grouped error. The highest-ranked member verdict becomes the verdict for the
207+
// group as a whole.
199208
//
200209
// It must be an explicit table rather than a comparison on Verdict itself,
201210
// whose constants are declaration-ordered and not severity-ordered:
202211
// InfraDependency numerically exceeds InfraRetryable, so ranking by value would
203-
// let a non-retryable branch discard a retryable sibling.
212+
// let a non-retryable member discard a retryable sibling.
204213
//
205214
// Two principles set the order. Retryable outranks non-retryable because the
206215
// two mistakes cost differently — a wrong "retryable" spends a bounded retry

0 commit comments

Comments
 (0)