diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 2547681e56a2..0a7ceb20f729 100644 --- a/api/core/workflows_services.py +++ b/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" diff --git a/api/features/models.py b/api/features/models.py index 2733d358e9ab..ebcd1878916f 100644 --- a/api/features/models.py +++ b/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 = ( diff --git a/api/features/versioning/serializers.py b/api/features/versioning/serializers.py index 0d75f8c5087c..6d848acf71c7 100644 --- a/api/features/versioning/serializers.py +++ b/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] diff --git a/api/tests/unit/features/test_unit_features_models.py b/api/tests/unit/features/test_unit_features_models.py index 5002b3498aa4..afa88f7ba62d 100644 --- a/api/tests/unit/features/test_unit_features_models.py +++ b/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 diff --git a/api/tests/unit/features/versioning/test_unit_versioning_serializers.py b/api/tests/unit/features/versioning/test_unit_versioning_serializers.py new file mode 100644 index 000000000000..df564bfd043e --- /dev/null +++ b/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 diff --git a/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py b/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py index 67e9d2e5dbc3..da33bae15663 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py @@ -927,3 +927,72 @@ def test_update_flag_v2__retained_plus_passed_exceeds_100__raises( multivariate_values=[MultivariateValueChangeSet(option_b.id, 100.0)], ), ) + + +def test_update_flag__v2_versioning_multivariate_weight_increase__keeps_enrolled_identities_in_variant( + environment_v2_versioning: Environment, + multivariate_feature: Feature, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given a multivariate feature split 50/50 between two variants, and the + # variant each of a range of identities is bucketed into + author = AuthorData(user=admin_user) + option_a, option_b, option_c = multivariate_options + feature_state = update_flag( + environment_v2_versioning, + multivariate_feature, + FlagChangeSet( + author=author, + enabled=True, + feature_state_value="control", + type_="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 50.0), + MultivariateValueChangeSet(option_b.id, 50.0), + MultivariateValueChangeSet(option_c.id, 0.0), + ], + ), + ) + identity_hash_keys = [f"identity-{i}" for i in range(100)] + original_assignment = { + key: feature_state.get_multivariate_feature_state_value(key).pk + for key in identity_hash_keys + } + + # When the first variant's allocation is increased to 60/40 + new_feature_state = update_flag( + environment_v2_versioning, + multivariate_feature, + FlagChangeSet( + author=author, + enabled=True, + feature_state_value="control", + type_="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 60.0), + MultivariateValueChangeSet(option_b.id, 40.0), + MultivariateValueChangeSet(option_c.id, 0.0), + ], + ), + ) + + # Then identities already in the grown variant stay in it, and the only + # movement is from the shrunk variant into the grown one + new_assignment = { + key: new_feature_state.get_multivariate_feature_state_value(key).pk + for key in identity_hash_keys + } + movers = { + key + for key in identity_hash_keys + if new_assignment[key] != original_assignment[key] + } + assert movers + assert all(original_assignment[key] == option_b.id for key in movers) + assert all(new_assignment[key] == option_a.id for key in movers) + assert all( + new_assignment[key] == original_assignment[key] + for key in identity_hash_keys + if key not in movers + ) diff --git a/api/tests/unit/features/versioning/test_unit_versioning_views.py b/api/tests/unit/features/versioning/test_unit_versioning_views.py index b55b062003d9..3b633ad52032 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_views.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_views.py @@ -1797,3 +1797,62 @@ def test_create_version__feature_state_save__dispatches_gitlab_state_change( # Then assert response.status_code == status.HTTP_201_CREATED assert mock_dispatch.call_count == 1 + + +def test_create_feature_version_feature_state__override_recreated_in_draft__inherits_mv_hashing_salt( + admin_client: APIClient, + admin_user: FFAdminUser, + environment_v2_versioning: Environment, + multivariate_feature: Feature, + segment: Segment, +) -> None: + # Given a published version containing a segment override for a + # multivariate feature + overridden_version = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=multivariate_feature + ) + original_override = FeatureState.objects.create( + feature=multivariate_feature, + environment=environment_v2_versioning, + environment_feature_version=overridden_version, + feature_segment=FeatureSegment.objects.create( + feature=multivariate_feature, + segment=segment, + environment=environment_v2_versioning, + environment_feature_version=overridden_version, + ), + ) + overridden_version.publish(published_by=admin_user) + + # and a draft version from which the (cloned) override has been removed + draft_version = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=multivariate_feature + ) + cloned_override = draft_version.feature_states.get(feature_segment__segment=segment) + cloned_override.feature_segment.delete() + + # When the override is recreated in the draft version via the API + url = reverse( + "api-v1:versioning:environment-feature-version-featurestates-list", + args=[ + environment_v2_versioning.id, + multivariate_feature.id, + draft_version.uuid, + ], + ) + data = { + "feature_segment": {"segment": segment.id}, + "enabled": True, + "feature_state_value": { + "string_value": "recreated!", + }, + } + response = admin_client.post( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then the recreated override carries the original override's id as its + # bucketing salt, keeping multivariate variant assignment stable + assert response.status_code == status.HTTP_201_CREATED + recreated_override = FeatureState.objects.get(id=response.json()["id"]) + assert recreated_override.mv_hashing_salt == original_override.id diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index 4068d34021d4..dce1054a8169 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -22,7 +22,7 @@ from audit.related_object_type import RelatedObjectType from core.helpers import get_current_site_url from environments.models import Environment -from features.models import Feature, FeatureState +from features.models import Feature, FeatureSegment, FeatureState from features.versioning.models import ( EnvironmentFeatureVersion, VersionChangeSet, @@ -1167,3 +1167,93 @@ def test_project_delete__v2_versioning_with_change_requests__does_not_trigger_au assert project.deleted_at is not None mock_went_live_task.delay.assert_not_called() mock_updated_task.delay.assert_not_called() + + +def test_change_request_commit__v1_multivariate_feature__keeps_variant_bucketing_stable( + environment: Environment, + multivariate_feature: Feature, + admin_user: FFAdminUser, +) -> None: + # Given the current live environment-default feature state of a multivariate + # feature, and the variant each of a range of identities is bucketed into + live_feature_state = FeatureState.objects.get( + environment=environment, + feature=multivariate_feature, + identity=None, + feature_segment=None, + ) + identity_hash_keys = [f"identity-{i}" for i in range(50)] + original_assignment = { + key: live_feature_state.get_multivariate_feature_state_value(key).pk + for key in identity_hash_keys + } + + # and a change request carrying a draft feature state for the same feature, + # as created by the API when a change request is raised under v1 versioning + change_request = ChangeRequest.objects.create( + title="Test CR", environment=environment, user=admin_user + ) + draft_feature_state = FeatureState.objects.create( + feature=multivariate_feature, + environment=environment, + change_request=change_request, + version=None, + ) + + # When the change request is committed + change_request.commit(committed_by=admin_user) + + # Then the draft feature state is now the live one, carrying the superseded + # feature state's id as its bucketing salt + new_live_feature_state = next( + fs + for fs in get_environment_flags_list( + environment, feature_name=multivariate_feature.name + ) + if fs.feature_segment_id is None and fs.identity_id is None + ) + assert new_live_feature_state.id == draft_feature_state.id + assert new_live_feature_state.mv_hashing_salt == live_feature_state.id + + # and every identity stays in the same variant as before the commit + new_assignment = { + key: new_live_feature_state.get_multivariate_feature_state_value(key).pk + for key in identity_hash_keys + } + assert new_assignment == original_assignment + + +def test_change_request_commit__v1_segment_override_draft__inherits_mv_hashing_salt( + environment: Environment, + multivariate_feature: Feature, + segment: Segment, + admin_user: FFAdminUser, +) -> None: + # Given a live segment override for a multivariate feature + 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, + ) + + # and a change request carrying a draft feature state recreating the override + change_request = ChangeRequest.objects.create( + title="Test CR", environment=environment, user=admin_user + ) + draft_feature_state = FeatureState.objects.create( + feature=multivariate_feature, + environment=environment, + feature_segment=feature_segment, + change_request=change_request, + version=None, + ) + + # When the change request is committed + change_request.commit(committed_by=admin_user) + + # Then the draft carries the superseded override's id as its bucketing salt + draft_feature_state.refresh_from_db() + assert draft_feature_state.mv_hashing_salt == live_override.id diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 83315507a230..eb0f148bcbdc 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -139,6 +139,16 @@ Attributes: - `environment.id` - `organisation.id` +### `features.feature_state.mv_variants_rebucketed` + +Logged at `warning` from: + - `api/features/models.py:901` + +Attributes: + - `environment.id` + - `feature.id` + - `superseded_feature_state.id` + ### `gitlab.api_call.failed` Logged at `error` from: @@ -542,7 +552,7 @@ Attributes: ### `workflows.missing_live_segment` Logged at `warning` from: - - `api/core/workflows_services.py:114` + - `api/core/workflows_services.py:123` Attributes: - `draft_segment` @@ -550,7 +560,7 @@ Attributes: ### `workflows.segment_revision_created` Logged at `info` from: - - `api/core/workflows_services.py:119` + - `api/core/workflows_services.py:128` Attributes: - `revision_id`