From 98475ff3d11c65e9d0d1dcbffc2b7eec2f509b80 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/README.md | 21 +++- platform/errs/processor.go | 160 ++++++++++++++++++++----- platform/errs/processor_test.go | 206 ++++++++++++++++++++++++++++++++ 3 files changed, 356 insertions(+), 31 deletions(-) diff --git a/platform/errs/README.md b/platform/errs/README.md index 391054c4c..940a741c3 100644 --- a/platform/errs/README.md +++ b/platform/errs/README.md @@ -44,13 +44,30 @@ 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. +### Joined errors + +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. + +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 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. + +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 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. + +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`. + +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. + ### 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 +148,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 branches of a join, which are classified and ranked independently. ### When *not* to classify in a controller diff --git a/platform/errs/processor.go b/platform/errs/processor.go index 6fd806f02..0de4b423d 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,20 @@ 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 +// Both passes traverse joined errors (errors.Join, or anything else exposing +// Unwrap() []error) as well as ordinary single-cause wraps. A wrap classifies +// the subtree beneath it and no more, so a wrapped branch of a join is weighed +// against its siblings instead of answering for them; classify documents how. +// +// 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 +94,20 @@ 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. The walk stops at a join because + // errors.Unwrap yields nothing for one, which is the behavior we want: a + // wrap inside one branch speaks only for that branch, and classify weighs + // it against its siblings rather than letting it silently answer for them. 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,113 @@ 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 branches of a join (Unwrap() []error) nothing shadows anything. +// The branches 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 branch failed first. +// +// The second rule is why a framework wrap does not get the short-circuit it +// gets on the spine: within a join it is one branch'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 joined, ok := cur.(interface{ Unwrap() []error }); ok { + verdict := Unknown + for _, branch := range joined.Unwrap() { + if v := p.classify(branch); verdictRank(v) > verdictRank(verdict) { + verdict = v + } + } + return verdict + } + + cur = errors.Unwrap(cur) + } + return Unknown +} + +// wrapVerdict returns the verdict the framework wrap err carries, or Unknown if +// err is not a wrap. It inspects the single node it is given, never the chain. +// Every wrap maps to a real verdict, which is what makes Unknown usable as the +// "not a wrap" answer. +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 verdicts for combining the independent branches of a +// joined error. The highest-ranked branch verdict becomes the verdict for the +// join as a whole. +// +// It must be an explicit table rather than a comparison on Verdict itself, +// whose constants are declaration-ordered and not severity-ordered: +// InfraDependency numerically exceeds InfraRetryable, so ranking by value would +// let a non-retryable branch discard a retryable sibling. +// +// Two principles set the order. Retryable outranks 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 and leaves it for someone +// to find and replay by hand. Within a retryability tier the verdict that +// implicates this service outranks the one that points elsewhere, so a failure +// that is partly ours is not reported as a pure dependency or user problem and +// routed away from the people who can fix it. That second ordering only moves +// attribution: every verdict in the non-retryable tier yields the same transport +// outcome. +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..61fe23076 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 join 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,197 @@ func TestNewClassifierProcessor_NoClassifiersReturnsUnchanged(t *testing.T) { assert.False(t, IsUserError(out)) } +// TestNewClassifierProcessor_JoinedBranches covers the reason the walk knows +// about Unwrap() []error at all: a caller that fans work out to several +// children reports their failures with errors.Join, and errors.Unwrap cannot +// see into one, so classifiers used to never be offered any branch. +func TestNewClassifierProcessor_JoinedBranches(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 + }{ + { + // errors.Join builds a join node even for one error, so a lone + // failing child is just as opaque to errors.Unwrap as several. + name: "single branch", + err: errors.Join(fmt.Errorf("child a: %w", transient)), + wantRetryable: true, + }, + { + name: "retryable branch last", + err: errors.Join(fmt.Errorf("child a: %w", permanent), fmt.Errorf("child b: %w", transient)), + wantRetryable: true, + }, + { + // Same branches reversed: the verdict must come from rank, not from + // the order the children happened to run in. + name: "retryable branch first", + err: errors.Join(fmt.Errorf("child a: %w", transient), fmt.Errorf("child b: %w", permanent)), + wantRetryable: true, + }, + { + name: "retryable outranks user", + err: errors.Join(badInput, transient), + wantRetryable: true, + }, + { + name: "join nested below a wrap", + err: fmt.Errorf("dispatch: %w", errors.Join(permanent, fmt.Errorf("child b: %w", transient))), + wantRetryable: true, + }, + { + name: "join nested inside another join", + err: errors.Join(permanent, errors.Join(badInput, transient)), + wantRetryable: true, + }, + { + // Both branches 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: errors.Join(upstreamBlip, transient), + wantRetryable: true, + }, + { + name: "dependency retryable alone keeps its provenance", + err: errors.Join(permanent, upstreamBlip), + wantRetryable: true, + wantDependency: true, + }, + { + name: "user outranks non-retryable dependency", + err: errors.Join(upstreamGone, badInput), + wantUser: true, + }, + { + name: "no branch recognized", + err: errors.Join(errors.New("who knows"), errors.New("nor this")), + }, + } + + 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)) + }) + } +} + +func TestNewClassifierProcessor_JoinedBranchesKeepEveryCause(t *testing.T) { + transient := errors.New("deadlock") + permanent := errors.New("schema mismatch") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + out := p.Process(errors.Join(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 branch that lost the rank must stay in the chain for diagnostics") +} + +// TestNewClassifierProcessor_WrappedBranchesAreWeighed covers branches that +// arrive already classified. A wrap speaks for the subtree beneath it, so it +// contributes a verdict to the join like any other branch rather than deciding +// for its siblings — which is what makes the outcome independent of the order +// the branches were reported in. +func TestNewClassifierProcessor_WrappedBranchesAreWeighed(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 branch 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: errors.Join(NewUserError(errors.New("malformed payload")), transient), + wantRetryable: true, + wantUser: true, + }, + { + name: "retryable wrap ranked ahead of a non-retryable one", + err: errors.Join(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 branch errors.As reached first. + name: "retryable wrap ranked ahead of a non-retryable one, reversed", + err: errors.Join(NewRetryableError(errors.New("blip")), NewDependencyError(errors.New("upstream 503"))), + wantRetryable: true, + }, + { + name: "sole wrapped branch still classifies the join", + err: errors.Join(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 join covers the whole join, so it is returned +// verbatim and no branch is consulted. +func TestNewClassifierProcessor_SpineWrapStillShortCircuits(t *testing.T) { + transient := errors.New("deadlock") + p := NewClassifierProcessor(verdictByError{transient: InfraRetryable}) + + wrapped := NewUserError(errors.Join(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 branches of a join, +// but down a wrap chain the outer node still wins outright, because it saw its +// cause and classified anyway. Without this the join 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)) }