fix(errs): Retry transient grouped failures - #621
Conversation
33e11c5 to
31ce24d
Compare
Pull request was converted to draft
31ce24d to
f3b2d0a
Compare
f3b2d0a to
09f0cd9
Compare
| return 4 | ||
| case Infra: | ||
| return 3 | ||
| case User: |
There was a problem hiding this comment.
why user before infraDep?
| return Unknown | ||
| } | ||
|
|
||
| // verdictRank orders verdicts for combining the independent branches of a |
There was a problem hiding this comment.
too verbose? please ask Claude to make it crisp... it looses the clarity
| return Unknown | ||
| } | ||
|
|
||
| // wrapVerdict returns the verdict the framework wrap err carries, or Unknown if |
There was a problem hiding this comment.
do we need the doc..i think ideally code is clear enough to not explain, unless you think there is need for one, may shorten it
| // 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 |
There was a problem hiding this comment.
This says both passes traverse joins, but Pass 1 doesn't — it walks only via errors.Unwrap() error, which yields nothing for a join. The inline comment 26 lines below (L97-99) says exactly that: "along the single-cause spine only… The walk stops at a join."
Worth correcting precisely because the design depends on it: Pass 1 not descending is what lets a wrapped branch be ranked against its siblings instead of short-circuiting the whole join. TestNewClassifierProcessor_WrappedBranchesAreWeighed only passes because of it — if Pass 1 did descend, Process would return the branch's *userError verbatim rather than wrapping it in NewRetryableError.
Suggested scoping:
Pass 2 traverses joined errors (errors.Join, or anything else exposing
Unwrap() []error) as well as ordinary single-cause wraps; Pass 1 walks only the single-cause spine.
|
|
||
| ### 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. |
There was a problem hiding this comment.
Same issue as processor.go L71 — Pass 1 doesn't descend into joins, only Pass 2 does. This one also contradicts L47 nine lines up, which correctly says Pass 1 looks "on the error's single-cause spine."
s/Both passes descend/Pass 2 descends/ and the rest of the paragraph reads fine as-is.
|
|
||
| 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`. |
There was a problem hiding this comment.
Design question rather than a defect — this is clearly deliberate, and documented.
I checked every current caller and they all test IsRetryable first, so nothing is broken today:
platform/consumer/consumer.go:442—if !errs.IsRetryable(err), the actual retry decisionplatform/consumer/consumer.go:644—if IsRetryable {} else if IsUserError {}, metrics tagging onlystovepipe/controller/process/process.go:144—if !errs.IsRetryable(err)
The concern is reachability. Dual-true went from "a controller had to deliberately double-wrap" to "join any two children," but nothing in the API enforces the precedence. A future caller writing the equally natural if IsUserError(err) { ack } else if IsRetryable(err) { retry } would silently drop a transient failure — this PR's own bug arriving through a different door.
Might be worth either a contract note on IsUserError saying it must not be consulted before IsRetryable, or a single RetryDecision(err) helper that makes the precedence unskippable. Not blocking.
3 participants
Summary
What:
Why:
Test Plan
Revert Plan
Issues