From 610f00ef44a105036c5f04d397ae7a9703dc4527 Mon Sep 17 00:00:00 2001 From: basgys Date: Thu, 18 Jun 2026 13:48:14 +0200 Subject: [PATCH] fix: prevent panic classifying errors with target-inspecting Is AdaptiveThrottle classified the throttled function's error with a value-based guard, errors.Is(err, errRejected{}). The errRejected{} target has a nil inner, and errRejected.Error() dereferences inner. errors.Is walks the returned error's chain and invokes each link's Is(error) bool method with that target. Any error type whose Is inspects the target via Error() (e.g. message-comparing error libraries) thus triggers errRejected{}.Error() -> nil deref -> panic. This fires on the first call for ordinary non-rejection errors (bad request, auth, validation) that happen to be such a type; stdlib errors.New errors have no Is method, which is why it went unnoticed. Detect the wrapper by type instead (errors.As / errors.AsType), which never calls Error() on the target. Also make RejectedError(nil) return nil: wrapping a nil error as a rejection is meaningless and was the only way to construct an errRejected with a nil inner, so this restores the invariant that inner is never nil and a successful call is never accounted as a rejection. Applied to both v1 (root module) and v2. --- adaptive.go | 55 ++++++++++++++++++++++++++++++++------------- adaptive_test.go | 47 ++++++++++++++++++++++++++++++++++++++ v2/adaptive.go | 31 ++++++++++++++++--------- v2/adaptive_test.go | 53 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 26 deletions(-) diff --git a/adaptive.go b/adaptive.go index 7124e07..d578b4d 100644 --- a/adaptive.go +++ b/adaptive.go @@ -152,14 +152,18 @@ func (t *AdaptiveThrottle) Throttle( err = fn(ctx) now = t.nowTime() + var re errRejected switch { case err == nil: t.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller - err = err.(errRejected).inner - - fallthrough + case errors.As(err, &re): + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + t.reject(priority, now) case t.checkIsRejected(err): t.reject(priority, now) default: @@ -395,14 +399,18 @@ func Throttle[T any]( res, err = throttledFn(ctx) now = at.nowTime() + var re errRejected switch { case err == nil: at.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller - err = err.(errRejected).inner - - fallthrough + case errors.As(err, &re): + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + at.reject(priority, now) case at.checkIsRejected(err): at.reject(priority, now) default: @@ -453,14 +461,18 @@ func WithAdaptiveThrottle[T any]( res, err = throttledFn() now = at.nowTime() + var re errRejected switch { case err == nil: at.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller - err = err.(errRejected).inner - - fallthrough + case errors.As(err, &re): + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + at.reject(priority, now) case at.checkIsRejected(err): at.reject(priority, now) default: @@ -476,8 +488,19 @@ func WithAdaptiveThrottle[T any]( // Any error that indicates that the backend is unhealthy should be wrapped with // `RejectedError`. But other errors, such as bad requests, authentication failures, // pre-condition failures, etc., should not be wrapped with `RejectedError`. -func RejectedError(err error) error { return errRejected{inner: err} } +// +// A nil error is returned unchanged: wrapping "no error" as a rejection is +// meaningless. This also keeps errRejected's invariant that inner is never nil. +func RejectedError(err error) error { + if err == nil { + return nil + } + + return errRejected{inner: err} +} +// errRejected marks an error as a rejection. inner is never nil: it is only +// constructed by RejectedError, which returns nil for a nil error. type errRejected struct{ inner error } func (err errRejected) Error() string { return err.inner.Error() } diff --git a/adaptive_test.go b/adaptive_test.go index 1852650..d69ed7d 100644 --- a/adaptive_test.go +++ b/adaptive_test.go @@ -564,3 +564,50 @@ func (alwaysShed) Uint64() uint64 { return 0 } type neverShed struct{} func (neverShed) Uint64() uint64 { return math.MaxUint64 } + +// msgComparingErr mimics error types (common in third-party libraries) whose Is +// method inspects the target via Error(). Such a type, in the returned error's +// chain, used to detonate the value-based errors.Is(err, errRejected{}) guard by +// calling errRejected{}.Error() on a nil inner. +type msgComparingErr struct{ message string } + +func (e *msgComparingErr) Error() string { return e.message } +func (e *msgComparingErr) Is(target error) bool { return e.message == target.Error() } + +// TestWithAdaptiveThrottleMsgComparingErrorDoesNotPanic verifies that an error +// whose Is method inspects the target is classified without panicking, and is +// returned to the caller unchanged (a bare error is not a rejection). +func TestWithAdaptiveThrottleMsgComparingErrorDoesNotPanic(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "boom"} + _, err := WithAdaptiveThrottle(at, High, func() (int, error) { return 0, in }) + if !errors.Is(err, in) { + t.Fatalf("got %v; want original error returned unchanged", err) + } +} + +// TestWithAdaptiveThrottleRejectedMsgComparingErrorUnwraps verifies that such an +// error wrapped in RejectedError is classified as a rejection and unwrapped back +// to the original error for the caller. +func TestWithAdaptiveThrottleRejectedMsgComparingErrorUnwraps(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "backend down"} + _, err := WithAdaptiveThrottle(at, High, func() (int, error) { return 0, RejectedError(in) }) + if err != in { + t.Fatalf("got %v; want unwrapped original error", err) + } +} + +// TestRejectedErrorNil verifies that RejectedError(nil) returns nil, so a +// successful call is never accounted as a rejection. +func TestRejectedErrorNil(t *testing.T) { + if err := RejectedError(nil); err != nil { + t.Fatalf("RejectedError(nil) = %v; want nil", err) + } + + at := NewAdaptiveThrottle(4) + res, err := WithAdaptiveThrottle(at, High, func() (int, error) { return 42, RejectedError(nil) }) + if err != nil || res != 42 { + t.Fatalf("got (res=%d, err=%v); want (42, nil)", res, err) + } +} diff --git a/v2/adaptive.go b/v2/adaptive.go index 53218f8..a34de57 100644 --- a/v2/adaptive.go +++ b/v2/adaptive.go @@ -349,16 +349,17 @@ func Throttle[T any]( res, err = throttledFn(ctx) now = at.now() - switch { + switch re, rejected := errors.AsType[errRejected](err); { + case rejected: + // Explicitly marked as a rejection via RejectedError. Detect the + // wrapper by type rather than value: errors.Is(err, errRejected{}) + // would pass a nil-inner target whose Error() panics when a chain + // link's Is method inspects it. Unwrap to return the original error + // to the caller. + err = re.inner + at.reject(priority, now) case err == nil: at.accept(priority, now) - case errors.Is(err, errRejected{}): - // Unwrap error to return the original error to the caller. - if re, ok := errors.AsType[errRejected](err); ok { - err = re.inner - } - - fallthrough case at.isRejectedError(err): at.reject(priority, now) default: @@ -378,8 +379,19 @@ func Throttle[T any]( // Any error that indicates that the backend is unhealthy should be wrapped with // `RejectedError`. But other errors, such as bad requests, authentication failures, // pre-condition failures, etc., should not be wrapped with `RejectedError`. -func RejectedError(err error) error { return errRejected{inner: err} } +// +// A nil error is returned unchanged: wrapping "no error" as a rejection is +// meaningless. This also keeps errRejected's invariant that inner is never nil. +func RejectedError(err error) error { + if err == nil { + return nil + } + + return errRejected{inner: err} +} +// errRejected marks an error as a rejection. inner is never nil: it is only +// constructed by RejectedError, which returns nil for a nil error. type errRejected struct{ inner error } func (err errRejected) Error() string { return err.inner.Error() } @@ -390,7 +402,6 @@ func (err errRejected) Is(target error) bool { return ok } - func clamp(lo, x, hi float64) float64 { return max(lo, min(x, hi)) } type ( diff --git a/v2/adaptive_test.go b/v2/adaptive_test.go index 9fa41bd..09622f5 100644 --- a/v2/adaptive_test.go +++ b/v2/adaptive_test.go @@ -564,3 +564,56 @@ func (alwaysShed) Uint64() uint64 { return 0 } type neverShed struct{} func (neverShed) Uint64() uint64 { return math.MaxUint64 } + +// msgComparingErr mimics error types (common in third-party libraries) whose Is +// method inspects the target via Error(). Such a type, in the returned error's +// chain, used to detonate the value-based errors.Is(err, errRejected{}) guard by +// calling errRejected{}.Error() on a nil inner. +type msgComparingErr struct{ message string } + +func (e *msgComparingErr) Error() string { return e.message } +func (e *msgComparingErr) Is(target error) bool { return e.message == target.Error() } + +// TestThrottleMsgComparingErrorDoesNotPanic verifies that an error whose Is +// method inspects the target is classified without panicking, and is returned to +// the caller unchanged (a bare error is not a rejection). +func TestThrottleMsgComparingErrorDoesNotPanic(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "boom"} + _, err := Throttle(context.Background(), at, High, + func(ctx context.Context) (int, error) { return 0, in }, + ) + if !errors.Is(err, in) { + t.Fatalf("got %v; want original error returned unchanged", err) + } +} + +// TestThrottleRejectedMsgComparingErrorUnwraps verifies that such an error +// wrapped in RejectedError is classified as a rejection and unwrapped back to the +// original error for the caller. +func TestThrottleRejectedMsgComparingErrorUnwraps(t *testing.T) { + at := NewAdaptiveThrottle(4) + in := &msgComparingErr{message: "backend down"} + _, err := Throttle(context.Background(), at, High, + func(ctx context.Context) (int, error) { return 0, RejectedError(in) }, + ) + if err != in { + t.Fatalf("got %v; want unwrapped original error", err) + } +} + +// TestRejectedErrorNil verifies that RejectedError(nil) returns nil, so a +// successful call is never accounted as a rejection. +func TestRejectedErrorNil(t *testing.T) { + if err := RejectedError(nil); err != nil { + t.Fatalf("RejectedError(nil) = %v; want nil", err) + } + + at := NewAdaptiveThrottle(4) + res, err := Throttle(context.Background(), at, High, + func(ctx context.Context) (int, error) { return 42, RejectedError(nil) }, + ) + if err != nil || res != 42 { + t.Fatalf("got (res=%d, err=%v); want (42, nil)", res, err) + } +}