fix: prevent panic classifying errors with a target-inspecting Is method

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() }

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)
+	}
+}

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 (

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)
+	}
+}