fix(features): inherit mv hashing salt in change requests and scheduled commits

api/core/workflows_services.py

@@ -53,6 +53,13 @@ def _publish_feature_states(self) -> None:
             if not fs.live_from or fs.live_from < now:
                 fs.live_from = now
 
+            # The draft is a brand new row about to replace the live feature
+            # state, so carry over the multivariate bucketing seed to keep
+            # variant assignment stable for enrolled identities.
+            if fs.mv_hashing_salt is None and fs.identity_id is None:
+                if superseded := fs.get_superseded_live_feature_state():
+                    fs.mv_hashing_salt = superseded.get_mv_hashing_salt_for_successor()
+
             fs.version = fs.get_next_version_number(
                 environment_id=fs.environment_id,  # type: ignore[arg-type]
                 feature_id=fs.feature_id,
@@ -61,7 +68,9 @@ def _publish_feature_states(self) -> None:
             )
 
         if feature_states:
-            type(fs).objects.bulk_update(feature_states, ["live_from", "version"])
+            type(fs).objects.bulk_update(
+                feature_states, ["live_from", "version", "mv_hashing_salt"]
+            )
 
     def _publish_environment_feature_versions(
         self, published_by: "FFAdminUser"

api/features/models.py

@@ -6,6 +6,7 @@
 import uuid
 from copy import deepcopy
 
+import structlog
 from django.conf import settings
 from django.contrib.contenttypes.fields import GenericRelation
 from django.core.exceptions import (
@@ -83,6 +84,7 @@
 from . import audit_helpers
 
 logger = logging.getLogger(__name__)
+event_logger = structlog.get_logger("features")
 
 if typing.TYPE_CHECKING:
     from environments.identities.models import Identity
@@ -463,7 +465,7 @@ class FeatureState(
     LifecycleModelMixin,  # type: ignore[misc]
     abstract_base_auditable_model_factory(  # type: ignore[misc]
         historical_records_excluded_fields=["uuid"],
-        change_details_excluded_fields=["live_from", "version"],
+        change_details_excluded_fields=["live_from", "version", "mv_hashing_salt"],
         show_change_details_for_create=True,
     ),
 ):
@@ -677,9 +679,8 @@ def clone(
         clone.id = None
         clone.uuid = uuid.uuid4()
         # Preserve the multivariate bucketing seed so that recreating this feature
-        # state does not re-bucket already-enrolled identities. If the source never
-        # had an explicit salt, capture its id (the seed used until now).
-        clone.mv_hashing_salt = self.mv_hashing_salt or self.id
+        # state does not re-bucket already-enrolled identities.
+        clone.mv_hashing_salt = self.get_mv_hashing_salt_for_successor()
 
         if self.feature_segment:
             # We can only create a new feature segment if we are cloning to another environment,
@@ -706,9 +707,11 @@ def clone(
         self.feature_state_value.clone(clone)
 
         if self.feature.type == MULTIVARIATE:
+            # Clone in id order so the new rows keep the same relative id order,
+            # which variant bucketing iterates over.
             mv_values = [
                 mv_value.clone(feature_state=clone, persist=False)
-                for mv_value in self.multivariate_feature_state_values.all()
+                for mv_value in self.multivariate_feature_state_values.order_by("id")
             ]
             MultivariateFeatureStateValue.objects.bulk_create(mv_values)
 
@@ -771,6 +774,45 @@ def get_feature_state_value_defaults(self) -> dict[str, typing.Any]:
 
         return {"type": type_, key_name: parse_func(value)}
 
+    def get_mv_hashing_salt_for_successor(self) -> int:
+        """Return the multivariate bucketing seed a feature state superseding
+        this one must carry to keep variant assignment stable. If this state
+        never had an explicit salt, its id is the seed used until now.
+        See https://github.com/Flagsmith/flagsmith/issues/7913.
+        """
+        return self.mv_hashing_salt or self.id
+
+    def get_superseded_live_feature_state(self) -> typing.Optional["FeatureState"]:
+        """Return the live feature state of this state's lineage (same
+        environment, feature and segment) that this state supersedes when it
+        goes live. Only meaningful for v1 versioning; identity overrides have
+        no versioned lineage.
+        """
+        # A segment override's FeatureSegment row can be recreated with it, so
+        # match the lineage by its segment, not by the FeatureSegment row.
+        if self.feature_segment_id is None:
+            lineage_filter = Q(feature_segment__isnull=True)
+        else:
+            lineage_filter = Q(
+                feature_segment__segment_id=self.feature_segment.segment_id  # type: ignore[union-attr]
+            )
+
+        superseded: FeatureState | None = (
+            FeatureState.objects.filter(
+                lineage_filter,
+                environment_id=self.environment_id,
+                feature_id=self.feature_id,
+                identity__isnull=True,
+                version__isnull=False,
+                live_from__isnull=False,
+                live_from__lte=timezone.now(),
+            )
+            .exclude(id=self.id)
+            .order_by("-version")
+            .first()
+        )
+        return superseded
+
     def get_multivariate_feature_state_value(
         self, identity_hash_key: str
     ) -> AbstractBaseFeatureValueModel:
@@ -831,22 +873,37 @@ def check_for_duplicate_feature_state(self):  # type: ignore[no-untyped-def]
 
     @hook(BEFORE_CREATE)
     def check_mv_hashing_salt_preserved(self):  # type: ignore[no-untyped-def]
-        """Fail if a feature state is recreated without keeping its bucketing salt.
-
-        Under v2 versioning a feature state must be recreated via clone(), which
-        copies mv_hashing_salt so multivariate variant assignment stays stable for
-        enrolled identities. clone() always sets a non-null salt, so a freshly
-        created state with no salt that the previous version already had was made
-        some other way and would re-bucket those identities. Raise so the
-        offending path is caught in tests. Identity overrides are skipped: they
-        are not versioned or cloned. See
+        """Complain if a feature state is recreated without keeping its bucketing salt.
+
+        A feature state must be recreated via clone(), which copies
+        mv_hashing_salt so multivariate variant assignment stays stable for
+        enrolled identities, or set the salt explicitly (see
+        get_mv_hashing_salt_for_successor call sites). A freshly created state
+        with no salt whose lineage already existed was made some other way and
+        re-buckets those identities: raise under v2 versioning so the offending
+        path is caught in tests; warn under v1. Identity overrides are skipped:
+        they are not versioned or cloned. See
         https://github.com/Flagsmith/flagsmith/issues/7913.
         """
-        if (
-            self.mv_hashing_salt is not None
-            or self.identity_id is not None
-            or not self.environment.use_v2_feature_versioning  # type: ignore[union-attr]
-        ):
+        if self.mv_hashing_salt is not None or self.identity_id is not None:
+            return
+
+        if not self.environment.use_v2_feature_versioning:  # type: ignore[union-attr]
+            # Under v1 versioning, change request drafts inherit the salt when
+            # committed (see ChangeRequestCommitService); any other path
+            # recreating a live multivariate lineage re-buckets its identities,
+            # so leave a trace for operators instead of failing the write.
+            if (
+                self.change_request_id is None
+                and self.feature.type == MULTIVARIATE
+                and (superseded := self.get_superseded_live_feature_state()) is not None
+            ):
+                event_logger.warning(
+                    "feature_state.mv_variants_rebucketed",
+                    environment__id=self.environment_id,
+                    feature__id=self.feature_id,
+                    superseded_feature_state__id=superseded.id,
+                )
             return
 
         previous_version = (

api/features/versioning/serializers.py

@@ -37,6 +37,39 @@ class Meta(CustomCreateSegmentOverrideFeatureStateSerializer.Meta):
             + ("feature",)  # type: ignore[assignment]
         )
 
+    def create(self, validated_data: dict) -> "FeatureState":  # type: ignore[type-arg]
+        # A state created directly in a version may supersede an override the
+        # previous version already had (e.g. one deleted from this draft and
+        # recreated), so carry over the multivariate bucketing seed to keep
+        # variant assignment stable for enrolled identities.
+        if validated_data.get("mv_hashing_salt") is None and (
+            inherited_salt := self._get_inherited_mv_hashing_salt(validated_data)
+        ):
+            validated_data["mv_hashing_salt"] = inherited_salt
+        return super().create(validated_data)
+
+    def _get_inherited_mv_hashing_salt(
+        self,
+        validated_data: dict,  # type: ignore[type-arg]
+    ) -> int | None:
+        version: EnvironmentFeatureVersion | None = validated_data.get(
+            "environment_feature_version"
+        )
+        feature_segment_data = validated_data.get("feature_segment")
+        if version is None or feature_segment_data is None:
+            return None
+
+        previous_version = version.get_previous_version()
+        if previous_version is None:
+            return None
+
+        superseded: "FeatureState | None" = previous_version.feature_states.filter(
+            feature_segment__segment=feature_segment_data["segment"]
+        ).first()
+        if superseded is None:
+            return None
+        return superseded.get_mv_hashing_salt_for_successor()
+
     def save(self, **kwargs):  # type: ignore[no-untyped-def]
         response = super().save(**kwargs)  # type: ignore[no-untyped-call]
 

api/tests/unit/features/test_unit_features_models.py

@@ -8,6 +8,7 @@
 from django.utils import timezone
 from pytest_lazyfixture import lazy_fixture  # type: ignore[import-untyped]
 from pytest_mock import MockerFixture
+from pytest_structlog import StructuredLogCapture
 
 from environments.identities.models import Identity
 from environments.models import Environment
@@ -828,6 +829,110 @@ def test_feature_state_create__new_segment_override_under_v2__is_allowed(
     assert feature_state.mv_hashing_salt is None
 
 
+def test_feature_state_create__v1_recreates_live_multivariate_state_directly__logs_warning(
+    environment: Environment,
+    multivariate_feature: Feature,
+    log: StructuredLogCapture,
+) -> None:
+    # Given the live feature state of a multivariate feature in a v1
+    # versioning environment
+    live_feature_state = FeatureState.objects.get(
+        environment=environment,
+        feature=multivariate_feature,
+        identity=None,
+        feature_segment=None,
+    )
+
+    # When a new version of the feature state is created directly, outside any
+    # sanctioned recreation flow
+    feature_state = FeatureState.objects.create(
+        feature=multivariate_feature,
+        environment=environment,
+        version=2,
+    )
+
+    # Then no salt is inherited, and the re-bucketing is logged for operators
+    assert feature_state.mv_hashing_salt is None
+    assert log.has(
+        "feature_state.mv_variants_rebucketed",
+        level="warning",
+        environment__id=environment.id,
+        feature__id=multivariate_feature.id,
+        superseded_feature_state__id=live_feature_state.id,
+    )
+
+
+def test_feature_state_create__v1_change_request_draft__does_not_log_warning(
+    environment: Environment,
+    multivariate_feature: Feature,
+    admin_user: FFAdminUser,
+    log: StructuredLogCapture,
+) -> None:
+    # Given a change request
+    change_request = ChangeRequest.objects.create(
+        title="Test CR", environment=environment, user=admin_user
+    )
+
+    # When a draft feature state is created for it
+    FeatureState.objects.create(
+        feature=multivariate_feature,
+        environment=environment,
+        change_request=change_request,
+        version=None,
+    )
+
+    # Then no warning is logged: the draft inherits its salt at commit time
+    assert not log.has("feature_state.mv_variants_rebucketed")
+
+
+def test_feature_state_create__v1_recreates_live_standard_state_directly__does_not_log_warning(
+    environment: Environment,
+    feature: Feature,
+    log: StructuredLogCapture,
+) -> None:
+    # Given
+
+    # When
+    feature_state = FeatureState.objects.create(
+        feature=feature,
+        environment=environment,
+        version=2,
+    )
+
+    # Then
+    assert feature_state.mv_hashing_salt is None
+    assert not log.has("feature_state.mv_variants_rebucketed")
+
+
+def test_get_superseded_live_feature_state__segment_override_draft__returns_live_override(
+    environment: Environment,
+    multivariate_feature: Feature,
+    segment: Segment,
+) -> None:
+    # Given a live segment override, and an (uncommitted) draft feature state
+    # recreating it
+    feature_segment = FeatureSegment.objects.create(
+        feature=multivariate_feature, segment=segment, environment=environment
+    )
+    live_override = FeatureState.objects.create(
+        feature=multivariate_feature,
+        environment=environment,
+        feature_segment=feature_segment,
+    )
+    draft = FeatureState.objects.create(
+        feature=multivariate_feature,
+        environment=environment,
+        feature_segment=feature_segment,
+        version=None,
+    )
+
+    # When
+    superseded = draft.get_superseded_live_feature_state()
+
+    # Then
+    assert superseded == live_override
+
+
 @mock.patch.object(FeatureState, "get_multivariate_feature_state_value")
 def test_get_feature_state_value__multivariate_feature__returns_mv_value(  # type: ignore[no-untyped-def]
     mock_get_mv_feature_state_value, environment, multivariate_feature, identity

api/tests/unit/features/versioning/test_unit_versioning_serializers.py

@@ -0,0 +1,49 @@
+from environments.models import Environment
+from features.models import Feature
+from features.versioning.models import EnvironmentFeatureVersion
+from features.versioning.serializers import (
+    CustomEnvironmentFeatureVersionFeatureStateSerializer,
+)
+from segments.models import Segment
+
+
+def test_get_inherited_mv_hashing_salt__no_feature_segment__returns_none(
+    environment_v2_versioning: Environment,
+    feature: Feature,
+) -> None:
+    # Given
+    version = EnvironmentFeatureVersion.objects.get(
+        environment=environment_v2_versioning, feature=feature
+    )
+    serializer = CustomEnvironmentFeatureVersionFeatureStateSerializer()
+
+    # When
+    inherited_salt = serializer._get_inherited_mv_hashing_salt(
+        {"environment_feature_version": version}
+    )
+
+    # Then
+    assert inherited_salt is None
+
+
+def test_get_inherited_mv_hashing_salt__no_previous_version__returns_none(
+    environment_v2_versioning: Environment,
+    feature: Feature,
+    segment: Segment,
+) -> None:
+    # Given
+    initial_version = EnvironmentFeatureVersion.objects.get(
+        environment=environment_v2_versioning, feature=feature
+    )
+    serializer = CustomEnvironmentFeatureVersionFeatureStateSerializer()
+
+    # When
+    inherited_salt = serializer._get_inherited_mv_hashing_salt(
+        {
+            "environment_feature_version": initial_version,
+            "feature_segment": {"segment": segment},
+        }
+    )
+
+    # Then
+    assert inherited_salt is None