chore(feature): improve error message and logs when variation change error occurs

Deleting a variation that is still referenced returned a generic error that
did not explain the cause. References inside the flag surfaced as
"InvalidArgumentNotMatchFormatError" with field "variation", and references
from other flags surfaced as a bare "FailedPreconditionError". Neither
carried the variation or the flag holding the reference, and no server-side
log was written at all.

Split the single ErrVariationInUse sentinel into six errors, one per cause,
each with its own message key so the console can explain what to fix:

  - VariationInUseByOffVariationError
  - VariationInUseByDefaultStrategyError
  - VariationInUseByTargetingRuleError
  - VariationInUseByIndividualTargetingError
  - VariationInUseByPrerequisiteError
  - VariationInUseByFeatureFlagRuleError

The cross-flag errors embed the referencing flag id and name so the console
can name it, and their message spells out the whole relationship for the
logs ("variation X of feature A is used as a prerequisite by feature B").

Also add logVariationInUseError so the rejected update is recorded with the
environment, the flag, the variations being deleted and the reference.
Validation failures were previously not logged anywhere.

Note that the gRPC code for same-flag references changes from
InvalidArgument to FailedPrecondition, matching the code already used for
cross-flag references.

Closes #1794

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: s35560

pkg/api/api/grpc_status.go

@@ -123,6 +123,18 @@ func convertErrorReason(errorType pkgErr.ErrorType) string {
 		return "EXCEEDED_MAX"
 	case pkgErr.ErrorTypeOutOfRange:
 		return "OUT_OF_RANGE"
+	case pkgErr.ErrorTypeVariationInUseByOffVariation:
+		return "VARIATION_IN_USE_BY_OFF_VARIATION"
+	case pkgErr.ErrorTypeVariationInUseByDefaultStrategy:
+		return "VARIATION_IN_USE_BY_DEFAULT_STRATEGY"
+	case pkgErr.ErrorTypeVariationInUseByTargetingRule:
+		return "VARIATION_IN_USE_BY_TARGETING_RULE"
+	case pkgErr.ErrorTypeVariationInUseByIndividualTarget:
+		return "VARIATION_IN_USE_BY_INDIVIDUAL_TARGETING"
+	case pkgErr.ErrorTypeVariationInUseByPrerequisite:
+		return "VARIATION_IN_USE_BY_PREREQUISITE"
+	case pkgErr.ErrorTypeVariationInUseByFeatureFlagRule:
+		return "VARIATION_IN_USE_BY_FEATURE_FLAG_RULE"
 	default:
 		return "UNKNOWN"
 	}
@@ -150,7 +162,13 @@ func convertStatusCode(errorType pkgErr.ErrorType) codes.Code {
 		return codes.Internal
 	case pkgErr.ErrorTypeInternal:
 		return codes.Internal
-	case pkgErr.ErrorTypeFailedPrecondition:
+	case pkgErr.ErrorTypeFailedPrecondition,
+		pkgErr.ErrorTypeVariationInUseByOffVariation,
+		pkgErr.ErrorTypeVariationInUseByDefaultStrategy,
+		pkgErr.ErrorTypeVariationInUseByTargetingRule,
+		pkgErr.ErrorTypeVariationInUseByIndividualTarget,
+		pkgErr.ErrorTypeVariationInUseByPrerequisite,
+		pkgErr.ErrorTypeVariationInUseByFeatureFlagRule:
 		return codes.FailedPrecondition
 	case pkgErr.ErrorTypeUnavailable:
 		return codes.Unavailable

pkg/api/api/grpc_status_test.go

@@ -230,3 +230,71 @@ func TestNewGRPCStatus_EdgeCases(t *testing.T) {
 		})
 	}
 }
+
+func TestNewGRPCStatus_VariationInUse(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name           string
+		errorType      pkgErr.ErrorType
+		expectedReason string
+	}{
+		{
+			name:           "off variation",
+			errorType:      pkgErr.ErrorTypeVariationInUseByOffVariation,
+			expectedReason: "VARIATION_IN_USE_BY_OFF_VARIATION",
+		},
+		{
+			name:           "default strategy",
+			errorType:      pkgErr.ErrorTypeVariationInUseByDefaultStrategy,
+			expectedReason: "VARIATION_IN_USE_BY_DEFAULT_STRATEGY",
+		},
+		{
+			name:           "targeting rule",
+			errorType:      pkgErr.ErrorTypeVariationInUseByTargetingRule,
+			expectedReason: "VARIATION_IN_USE_BY_TARGETING_RULE",
+		},
+		{
+			name:           "individual targeting",
+			errorType:      pkgErr.ErrorTypeVariationInUseByIndividualTarget,
+			expectedReason: "VARIATION_IN_USE_BY_INDIVIDUAL_TARGETING",
+		},
+		{
+			name:           "prerequisite",
+			errorType:      pkgErr.ErrorTypeVariationInUseByPrerequisite,
+			expectedReason: "VARIATION_IN_USE_BY_PREREQUISITE",
+		},
+		{
+			name:           "feature flag rule",
+			errorType:      pkgErr.ErrorTypeVariationInUseByFeatureFlagRule,
+			expectedReason: "VARIATION_IN_USE_BY_FEATURE_FLAG_RULE",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			st := NewGRPCStatus(pkgErr.NewErrorVariationInUse(
+				pkgErr.FeaturePackageName,
+				tt.errorType,
+				"variation in use",
+				map[string]string{"featureId": "feature-2", "featureName": "Flag B"},
+			))
+
+			assert.Equal(t, codes.FailedPrecondition, st.Code())
+			assert.Equal(t, "feature:variation in use", st.Message())
+
+			for _, detail := range st.Details() {
+				errorInfo, ok := detail.(*errdetails.ErrorInfo)
+				if !ok {
+					continue
+				}
+				assert.Equal(t, tt.expectedReason, errorInfo.Reason)
+				assert.Equal(t, string(tt.errorType), errorInfo.Metadata["messageKey"])
+				assert.Equal(t, "feature-2", errorInfo.Metadata["featureId"])
+				assert.Equal(t, "Flag B", errorInfo.Metadata["featureName"])
+			}
+		})
+	}
+}

pkg/error/error.go

@@ -17,6 +17,7 @@ package error
 import (
 	"errors"
 	"fmt"
+	"maps"
 	"strconv"
 	"strings"
 )
@@ -62,6 +63,14 @@ const (
 	ErrorTypeExceededMax              ErrorType = "ExceededMaxError"
 	ErrorTypeOutOfRange               ErrorType = "OutOfRangeError"
 	ErrorTypeDifferentVariationsSize  ErrorType = "DifferentVariationsSizeError"
+
+	// A variation cannot be deleted while something still references it.
+	ErrorTypeVariationInUseByOffVariation     ErrorType = "VariationInUseByOffVariationError"
+	ErrorTypeVariationInUseByDefaultStrategy  ErrorType = "VariationInUseByDefaultStrategyError"
+	ErrorTypeVariationInUseByTargetingRule    ErrorType = "VariationInUseByTargetingRuleError"
+	ErrorTypeVariationInUseByIndividualTarget ErrorType = "VariationInUseByIndividualTargetingError"
+	ErrorTypeVariationInUseByPrerequisite     ErrorType = "VariationInUseByPrerequisiteError"
+	ErrorTypeVariationInUseByFeatureFlagRule  ErrorType = "VariationInUseByFeatureFlagRuleError"
 )
 
 type BktError struct {
@@ -100,6 +109,24 @@ func (e *BktError) Wrap(err error) {
 	e.wrappedError = errors.Join(e.wrappedError, err)
 }
 
+// AsVariationInUseError reports whether err says a variation is still referenced.
+func AsVariationInUseError(err error) (*BktError, bool) {
+	var bktErr *BktError
+	if !errors.As(err, &bktErr) {
+		return nil, false
+	}
+	switch bktErr.ErrorType() {
+	case ErrorTypeVariationInUseByOffVariation,
+		ErrorTypeVariationInUseByDefaultStrategy,
+		ErrorTypeVariationInUseByTargetingRule,
+		ErrorTypeVariationInUseByIndividualTarget,
+		ErrorTypeVariationInUseByPrerequisite,
+		ErrorTypeVariationInUseByFeatureFlagRule:
+		return bktErr, true
+	}
+	return nil, false
+}
+
 func newBktError(pkg string, errorType ErrorType, message string) *BktError {
 	return &BktError{
 		packageName: pkg,
@@ -206,3 +233,21 @@ func NewErrorOutOfRange(pkg string, message string, field string, min int, max i
 func NewErrorDifferentVariationsSize(pkg string, message string) *BktError {
 	return newBktError(pkg, ErrorTypeDifferentVariationsSize, message)
 }
+
+// NewErrorVariationInUse creates an error naming the reference that keeps a
+// variation from being deleted. keyValues may be nil.
+func NewErrorVariationInUse(
+	pkg string,
+	errorType ErrorType,
+	message string,
+	keyValues map[string]string,
+) *BktError {
+	embedded := make(map[string]string, len(keyValues))
+	maps.Copy(embedded, keyValues)
+	return &BktError{
+		packageName:       pkg,
+		errorType:         errorType,
+		message:           message,
+		embeddedKeyValues: embedded,
+	}
+}

pkg/error/error_test.go

@@ -323,3 +323,100 @@ func TestErrorAs(t *testing.T) {
 		t.Error("Expected fieldErr to wrap originalErr")
 	}
 }
+
+func TestNewErrorVariationInUse(t *testing.T) {
+	t.Parallel()
+
+	err := NewErrorVariationInUse(
+		FeaturePackageName,
+		ErrorTypeVariationInUseByPrerequisite,
+		"feature: variation variation-1 is used as a prerequisite by feature feature-2",
+		map[string]string{"featureId": "feature-2", "featureName": "Flag B"},
+	)
+
+	assert.Equal(t, FeaturePackageName, err.PackageName())
+	assert.Equal(t, ErrorTypeVariationInUseByPrerequisite, err.ErrorType())
+	assert.Equal(t, "VariationInUseByPrerequisiteError", err.MessageKey())
+	assert.Equal(
+		t,
+		"feature:feature: variation variation-1 is used as a prerequisite by feature feature-2",
+		err.Error(),
+	)
+	assert.Equal(
+		t,
+		map[string]string{"featureId": "feature-2", "featureName": "Flag B"},
+		err.EmbeddedKeyValues(),
+	)
+}
+
+func TestNewErrorVariationInUse_CopiesKeyValues(t *testing.T) {
+	t.Parallel()
+
+	keyValues := map[string]string{"featureId": "feature-2"}
+	err := NewErrorVariationInUse(
+		FeaturePackageName,
+		ErrorTypeVariationInUseByPrerequisite,
+		"feature: variation in use",
+		keyValues,
+	)
+	keyValues["featureId"] = "mutated"
+
+	assert.Equal(t, "feature-2", err.EmbeddedKeyValues()["featureId"])
+}
+
+func TestAsVariationInUseError(t *testing.T) {
+	t.Parallel()
+
+	patterns := []struct {
+		desc     string
+		err      error
+		expected bool
+	}{
+		{
+			desc:     "false: nil",
+			err:      nil,
+			expected: false,
+		},
+		{
+			desc:     "false: not a BktError",
+			err:      errors.New("something else"),
+			expected: false,
+		},
+		{
+			desc:     "false: another BktError type",
+			err:      NewErrorFailedPrecondition(FeaturePackageName, "failed precondition"),
+			expected: false,
+		},
+		{
+			desc: "true: off variation",
+			err: NewErrorVariationInUse(
+				FeaturePackageName, ErrorTypeVariationInUseByOffVariation, "in use", nil,
+			),
+			expected: true,
+		},
+		{
+			desc: "true: individual targeting",
+			err: NewErrorVariationInUse(
+				FeaturePackageName, ErrorTypeVariationInUseByIndividualTarget, "in use", nil,
+			),
+			expected: true,
+		},
+		{
+			desc: "true: feature flag rule",
+			err: NewErrorVariationInUse(
+				FeaturePackageName, ErrorTypeVariationInUseByFeatureFlagRule, "in use", nil,
+			),
+			expected: true,
+		},
+	}
+	for _, p := range patterns {
+		t.Run(p.desc, func(t *testing.T) {
+			t.Parallel()
+			bktErr, ok := AsVariationInUseError(p.err)
+			assert.Equal(t, p.expected, ok)
+			if !p.expected {
+				assert.Nil(t, bktErr)
+			}
+		})
+	}
+}

pkg/feature/api/error.go

@@ -207,11 +207,6 @@ var (
 		pkgErr.NewErrorFailedPrecondition(
 			pkgErr.FeaturePackageName,
 			"can't archive because this feature is used as a prerequsite"))
-	statusVariationInUseByOtherFeatures = api.NewGRPCStatus(
-		pkgErr.NewErrorFailedPrecondition(
-			pkgErr.FeaturePackageName,
-			"can't remove this variation because it is used as a prerequisite or rule in other features",
-		))
 	// flag trigger
 	statusMissingTriggerFeatureID = api.NewGRPCStatus(
 		pkgErr.NewErrorInvalidArgEmpty(pkgErr.FeaturePackageName, "missing trigger feature id", "FeatureFlagID"))

pkg/feature/api/feature.go

@@ -31,6 +31,7 @@ import (
 	evaluation "github.com/bucketeer-io/bucketeer/v2/evaluation/go"
 	"github.com/bucketeer-io/bucketeer/v2/pkg/api/api"
 	domainevent "github.com/bucketeer-io/bucketeer/v2/pkg/domainevent/domain"
+	pkgErr "github.com/bucketeer-io/bucketeer/v2/pkg/error"
 	experimentdomain "github.com/bucketeer-io/bucketeer/v2/pkg/experiment/domain"
 	"github.com/bucketeer-io/bucketeer/v2/pkg/feature/domain"
 	"github.com/bucketeer-io/bucketeer/v2/pkg/feature/scheduled"
@@ -802,6 +803,7 @@ func (s *FeatureService) updateFeatureWithinTransaction(
 		},
 	)
 	if err != nil {
+		s.logVariationInUseError(ctx, err, req)
 		return nil, nil, err
 	}
 	if err := s.upsertTags(ctx, updated.Tags, req.EnvironmentId); err != nil {
@@ -824,6 +826,7 @@ func (s *FeatureService) updateFeatureWithinTransaction(
 	}
 	// Validate that variations being deleted are not used in other features
 	if err := validateVariationDeletion(req.VariationChanges, features, req.Id); err != nil {
+		s.logVariationInUseError(ctx, err, req)
 		return nil, nil, err
 	}
 	event, err := domainevent.NewEvent(
@@ -1014,6 +1017,36 @@ func (s *FeatureService) DeleteFeature(
 	return &featureproto.DeleteFeatureResponse{}, nil
 }
 
+// logVariationInUseError records which variation blocked the update and what
+// still references it. Validation failures are otherwise not logged at all.
+func (s *FeatureService) logVariationInUseError(
+	ctx context.Context,
+	err error,
+	req *featureproto.UpdateFeatureRequest,
+) {
+	bktErr, ok := pkgErr.AsVariationInUseError(err)
+	if !ok {
+		return
+	}
+	deleted := make([]string, 0, len(req.VariationChanges))
+	for _, change := range req.VariationChanges {
+		if change.ChangeType == featureproto.ChangeType_DELETE {
+			deleted = append(deleted, change.Variation.GetId())
+		}
+	}
+	s.logger.Error(
+		"Failed to update feature because a variation is still in use",
+		log.FieldsFromIncomingContext(ctx).AddFields(
+			zap.Error(err),
+			zap.String("environmentId", req.EnvironmentId),
+			zap.String("featureId", req.Id),
+			zap.Strings("deletedVariationIds", deleted),
+			// Empty when the reference is in the flag being updated.
+			zap.Any("reference", bktErr.EmbeddedKeyValues()),
+		)...,
+	)
+}
+
 func (s *FeatureService) convUpdateFeatureError(err error) error {
 	switch err {
 	case v2fs.ErrFeatureNotFound,

pkg/feature/api/validation.go

@@ -16,7 +16,6 @@ package api
 
 import (
 	"context"
-	"errors"
 	"regexp"
 
 	"github.com/bucketeer-io/bucketeer/v2/pkg/api/api"
@@ -428,17 +427,11 @@ func validateVariationDeletion(
 		dependentFeaturesSlice = append(dependentFeaturesSlice, f)
 	}
 
-	// Check if the deleted variation is used as a prerequisite or rule in other features
-	if err := featuredomain.ValidateVariationUsage(
+	// Check if the deleted variation is used as a prerequisite or rule in other features.
+	// The error already names the reference, so it is returned as-is.
+	return featuredomain.ValidateVariationUsage(
 		dependentFeaturesSlice,
 		targetFeatureID,
 		deletedVariations,
-	); err != nil {
-		if errors.Is(err, featuredomain.ErrVariationInUse) {
-			return statusVariationInUseByOtherFeatures.Err()
-		}
-		return err
-	}
-
-	return nil
+	)
 }