diff --git a/posthog/settings/feature_flags.py b/posthog/settings/feature_flags.py index ec84c07a93e6..d44604077bc3 100644 --- a/posthog/settings/feature_flags.py +++ b/posthog/settings/feature_flags.py @@ -105,14 +105,15 @@ "TEAM_METADATA_CACHE_VERIFICATION_GRACE_PERIOD_MINUTES", 5, type_cast=int ) -# Feature flag limits to prevent memory issues during flag evaluation/caching. -# These limits are configurable via environment variables and can be overridden -# in Helm charts per environment. -# -# Defaults are set well above observed production maximums to avoid impacting -# normal usage while protecting against extreme outliers. - -# Maximum number of feature flags allowed per team +# Feature flag limits, configurable via environment variables and overridable in Helm +# charts per environment. Defaults are set well above observed production maximums to +# avoid impacting normal usage while protecting against extreme outliers. + +# Maximum number of feature flags allowed per team. Counts non-archived flags only, so it +# bounds a team's live roster. It does not bound the evaluation payload: archived flags are +# still serialized into the flags hypercache and the local-evaluation response, so a team +# that keeps archiving and recreating flags keeps growing them. The per-flag filter size +# cap below is what limits the cost of any single flag. MAX_FEATURE_FLAGS_PER_TEAM: int = get_from_env("MAX_FEATURE_FLAGS_PER_TEAM", 2000, type_cast=int) # Maximum size in bytes for a single flag's filters JSON diff --git a/products/experiments/backend/experiment_service.py b/products/experiments/backend/experiment_service.py index 14d56b81eca1..63e5e87ed7f3 100644 --- a/products/experiments/backend/experiment_service.py +++ b/products/experiments/backend/experiment_service.py @@ -2145,7 +2145,10 @@ def _unarchive_linked_feature_flag( # archived-only payload matches no approval action (enable/disable detect on # `active`, update on `filters`), so the gate can't raise ApprovalRequired here # and roll back a just-created change request. - unarchive_flag(feature_flag, team=self.team, user=self.user, request=request) + # + # Waive the flag cap: archiving this flag freed a slot the team may have since + # filled, and blocking the undo would strand the experiment without its flag. + unarchive_flag(feature_flag, team=self.team, user=self.user, request=request, allow_exceeding_flag_limit=True) experiment.feature_flag_auto_archived = False experiment.save(update_fields=["feature_flag_auto_archived"]) diff --git a/products/experiments/backend/test/test_experiment_service.py b/products/experiments/backend/test/test_experiment_service.py index d1166fb48361..b65a5164839f 100644 --- a/products/experiments/backend/test/test_experiment_service.py +++ b/products/experiments/backend/test/test_experiment_service.py @@ -3001,6 +3001,28 @@ def test_unarchive_experiment_skips_flag_without_feature_flag_write_scope(self): assert flag.archived is True assert experiment.feature_flag_auto_archived is True + def test_unarchive_experiment_restores_flag_over_team_flag_limit(self): + # Archiving the experiment freed a slot the team then filled. Restoring the flag is + # an undo, so it is waived from the cap rather than stranding the experiment. + experiment = self._create_ended_experiment(name="Unarchive At Limit", feature_flag_key="unarchive-at-limit") + experiment.feature_flag.active = False + experiment.feature_flag.save() + service = self._service() + service.archive_experiment(experiment) + experiment.refresh_from_db() + assert experiment.feature_flag_auto_archived is True + # The experiment's own flag is archived, so this is the team's only counted flag. + self._create_flag(key="fills-the-cap") + + with self.settings(MAX_FEATURE_FLAGS_PER_TEAM=1): + service.unarchive_experiment(experiment) + + experiment.refresh_from_db() + assert experiment.archived is False + flag = FeatureFlag.objects.get(pk=experiment.feature_flag_id) + assert flag.archived is False + assert experiment.feature_flag_auto_archived is False + def test_archive_experiment_denies_disabling_flag_when_approval_required(self): experiment = self._create_ended_experiment(name="Approval Gated", feature_flag_key="approval-gated-flag") service = self._service() diff --git a/products/feature_flags/backend/api/feature_flag.py b/products/feature_flags/backend/api/feature_flag.py index 5930c1b1fa58..f8965ea20327 100644 --- a/products/feature_flags/backend/api/feature_flag.py +++ b/products/feature_flags/backend/api/feature_flag.py @@ -578,25 +578,34 @@ def _filter_person_properties_for_flag( return {key: value for key, value in person_properties.items() if key in referenced_keys} -def check_flag_limits_for_team( - team_id: int, - is_create: bool = True, -) -> None: +def check_flag_limits_for_team(team_id: int, *, adds_to_count: bool) -> None: """ - Check if creating a flag would exceed the team's flag count limit. + Check if a write would push the team past its flag count limit. + + ``adds_to_count`` says this write moves a flag into the counted set: creating and unarchiving + both do. Unarchiving has to clear the cap or a team mints slots for free by archiving flags, + creating replacements, then unarchiving the originals. - Only enforced on create -- updates to existing flags don't change the count. + Undoing a soft delete is deliberately exempt, even though it grows the counted set the same + way. A team can sit above the cap (it shipped without a backfill, and lowering it puts teams + over at once), and charging the undo would make deleting a flag one-way for them: the delete + succeeds and the undo fails, with nothing in the product to recover it. That leaves a narrow + delete-create-restore path past the cap, accepted as the lesser cost. + + Archived flags are excluded so archiving frees a slot without destroying the experiment + and survey data linked to the flag; soft-deleted flags are already excluded by the default + manager. Disabled flags count, since a team can re-enable them. """ - if not is_create: + if not adds_to_count: return count_limit = settings.MAX_FEATURE_FLAGS_PER_TEAM - flag_count = FeatureFlag.objects.filter(team_id=team_id).count() + flag_count = FeatureFlag.objects.filter(team_id=team_id, archived=False).count() if flag_count >= count_limit: raise serializers.ValidationError( f"Maximum of {count_limit:,} feature flags allowed per team. " - f"Please delete unused flags or contact support to increase this limit." + f"Archive or delete unused flags, or contact support to increase this limit." ) @@ -1060,7 +1069,7 @@ def validate(self, attrs): self._validate_device_bucketing_with_persist_auth(attrs) self._validate_encrypted_payloads_require_remote_config(attrs) self._validate_archived_flags_are_disabled(attrs) - self._validate_flag_limits() + self._validate_flag_limits(attrs) # Materialize the remote-config 100% rollout default here, before the approval gate runs in # create(), so a remote-config create trips the rollout policy instead of slipping past it. @@ -1231,11 +1240,22 @@ def validate_key(self, value): return value - def _validate_flag_limits(self) -> None: - """Validate that the team has not exceeded its flag count limit.""" + def _validate_flag_limits(self, attrs: dict) -> None: + """Enforce the team's flag cap on writes that move this flag into the counted set.""" + if self.context.get("allow_exceeding_flag_limit"): + return + # Compare counted-set membership before and after rather than testing `archived` and + # `deleted` separately: a flag sits outside the set if either one is set, so a write + # clearing only one of them (unarchiving a flag that stays deleted) adds nothing. + counted_before = self.instance is not None and not self.instance.archived and not self.instance.deleted + archived_after = attrs.get("archived", self.instance.archived if self.instance else False) + deleted_after = attrs.get("deleted", self.instance.deleted if self.instance else False) + counted_after = not archived_after and not deleted_after + # A soft-deleted flag can only re-enter the counted set by being restored, so this one + # term exempts every undo-delete shape. See check_flag_limits_for_team for why. + was_deleted = self.instance is not None and self.instance.deleted check_flag_limits_for_team( - team_id=self.context["team_id"], - is_create=self.instance is None, + self.context["team_id"], adds_to_count=counted_after and not counted_before and not was_deleted ) @functools.cached_property diff --git a/products/feature_flags/backend/api/test/__snapshots__/test_organization_feature_flag.ambr b/products/feature_flags/backend/api/test/__snapshots__/test_organization_feature_flag.ambr index db9984d6581e..5af80cf2bec7 100644 --- a/products/feature_flags/backend/api/test/__snapshots__/test_organization_feature_flag.ambr +++ b/products/feature_flags/backend/api/test/__snapshots__/test_organization_feature_flag.ambr @@ -248,7 +248,8 @@ WHERE U0."id" = 99999 LIMIT 1) OR ("posthog_team"."parent_team_id" IS NULL - AND "posthog_featureflag"."team_id" = 99999))) + AND "posthog_featureflag"."team_id" = 99999)) + AND NOT "posthog_featureflag"."archived") ''' # --- # name: TestOrganizationFeatureFlagCopy.test_copy_feature_flag_create_new.14 diff --git a/products/feature_flags/backend/api/test/test_feature_flag.py b/products/feature_flags/backend/api/test/test_feature_flag.py index 53aee37cfaf4..1ad7bec7fe38 100644 --- a/products/feature_flags/backend/api/test/test_feature_flag.py +++ b/products/feature_flags/backend/api/test/test_feature_flag.py @@ -12999,7 +12999,7 @@ def test_bulk_delete_handles_mixed_key_rename_scenarios(self): class TestFeatureFlagLimits(APIBaseTest): """Tests for feature flag creation and update limits.""" - def _create_flag(self, key: str, filters: Optional[dict] = None) -> FeatureFlag: + def _create_flag(self, key: str, filters: Optional[dict] = None, **kwargs: bool) -> FeatureFlag: """Helper to create a flag directly in the database.""" if filters is None: filters = {"groups": [{"rollout_percentage": 100, "properties": []}]} @@ -13008,6 +13008,7 @@ def _create_flag(self, key: str, filters: Optional[dict] = None) -> FeatureFlag: created_by=self.user, key=key, filters=filters, + **kwargs, ) def test_cannot_create_flag_when_team_exceeds_count_limit(self): @@ -13044,31 +13045,39 @@ def test_cannot_create_flag_without_filters_when_team_exceeds_count_limit(self): assert response.status_code == status.HTTP_400_BAD_REQUEST assert "Maximum of 3 feature flags allowed per team" in response.json()["detail"] - def test_can_update_existing_flag_when_team_at_count_limit(self): - # Create flags up to the limit + @parameterized.expand( + [ + ("rename", {"name": "Updated description"}), + ("archive", {"archived": True, "active": False}), + ] + ) + def test_can_update_existing_flag_when_team_at_count_limit(self, _name: str, payload: dict) -> None: + # Neither update grows the counted set, so both work at the cap. Archiving especially: + # it is the remedy the limit error recommends. flag1 = self._create_flag("flag-1") self._create_flag("flag-2") - # Updating an existing flag should succeed even at the limit with self.settings(MAX_FEATURE_FLAGS_PER_TEAM=2): response = self.client.patch( f"/api/projects/{self.team.id}/feature_flags/{flag1.id}", - {"name": "Updated description"}, + payload, ) - assert response.status_code == status.HTTP_200_OK - assert response.json()["name"] == "Updated description" + assert response.status_code == status.HTTP_200_OK, response.json() - def test_deleted_flags_do_not_count_toward_limit(self): - # Create two flags - flag1 = self._create_flag("flag-1") + @parameterized.expand( + [ + ("deleted", {"deleted": True}, status.HTTP_201_CREATED), + ("archived", {"archived": True, "active": False}, status.HTTP_201_CREATED), + ("disabled", {"active": False}, status.HTTP_400_BAD_REQUEST), + ] + ) + def test_flag_state_effect_on_count_limit(self, _name: str, flag_state: dict, expected_status: int) -> None: + # Only deleted and archived flags free up a slot. A disabled flag is an ordinary + # flag the team can re-enable, so it still counts. + self._create_flag("flag-1", **flag_state) self._create_flag("flag-2") - # Soft-delete one - flag1.deleted = True - flag1.save() - - # Now we should be able to create a new flag with self.settings(MAX_FEATURE_FLAGS_PER_TEAM=2): response = self.client.post( f"/api/projects/{self.team.id}/feature_flags", @@ -13078,7 +13087,55 @@ def test_deleted_flags_do_not_count_toward_limit(self): }, ) - assert response.status_code == status.HTTP_201_CREATED + assert response.status_code == expected_status + + @parameterized.expand( + [ + ( + "unarchive_at_limit", + {"archived": True, "active": False}, + {"archived": False}, + 0, + status.HTTP_400_BAD_REQUEST, + ), + ( + "unarchive_under_limit", + {"archived": True, "active": False}, + {"archived": False}, + 1, + status.HTTP_200_OK, + ), + # Undoing a soft delete is exempt, so a team over the cap can still recover a flag + # it deleted by mistake. + ("restore_at_limit", {"deleted": True}, {"deleted": False}, 0, status.HTTP_200_OK), + # The flag stays soft-deleted, so it does not re-enter the counted set and must + # not be charged for it. + ( + "unarchive_at_limit_but_stays_deleted", + {"deleted": True, "archived": True, "active": False}, + {"archived": False}, + 0, + status.HTTP_200_OK, + ), + ] + ) + def test_unarchiving_respects_count_limit_but_restoring_is_exempt( + self, _name: str, excluded_state: dict, payload: dict, spare_slots: int, expected_status: int + ) -> None: + # Unarchiving puts the flag back in the counted set, so it clears the same bar as a + # create. Otherwise archive-create-unarchive mints slots for free. + excluded_flag = self._create_flag("excluded-flag", **excluded_state) + counted_keys = ["flag-1", "flag-2"] + for key in counted_keys: + self._create_flag(key) + + with self.settings(MAX_FEATURE_FLAGS_PER_TEAM=len(counted_keys) + spare_slots): + response = self.client.patch( + f"/api/projects/{self.team.id}/feature_flags/{excluded_flag.id}", + payload, + ) + + assert response.status_code == expected_status def test_per_flag_filter_size_limit_on_create(self): # Create a filter with many properties that exceeds 1KB diff --git a/products/feature_flags/backend/facade/api.py b/products/feature_flags/backend/facade/api.py index 1c06dc961496..9e4f63f39cce 100644 --- a/products/feature_flags/backend/facade/api.py +++ b/products/feature_flags/backend/facade/api.py @@ -101,7 +101,15 @@ def _redact_unchanged_encrypted_payloads(flag: FeatureFlag, data: dict) -> dict: return {**data, "filters": {**filters, "payloads": redacted}} -def update_flag(flag: FeatureFlag, data: dict, *, team: Team, user: Any, request: Any | None = None) -> FeatureFlag: +def update_flag( + flag: FeatureFlag, + data: dict, + *, + team: Team, + user: Any, + request: Any | None = None, + allow_exceeding_flag_limit: bool = False, +) -> FeatureFlag: """Gated partial update: routes through FeatureFlagSerializer so @approval_gate, validation, and activity logging apply. ``data`` is a partial flag write payload (fields it omits are untouched) applied as-is — nothing is silently dropped. @@ -109,12 +117,16 @@ def update_flag(flag: FeatureFlag, data: dict, *, team: Team, user: Any, request ``user=None`` is a system write (see module docstring): ``last_modified_by`` is cleared, activity is logged as system, the approval gate is skipped. + ``allow_exceeding_flag_limit`` waives MAX_FEATURE_FLAGS_PER_TEAM for this write. Only + unarchiving (``archived: False``) consults the cap, since that is the one update that grows + the counted set, so the waiver has no effect on any other update. + Encrypted payload values carried over unchanged from ``flag.get_filters()`` are preserved as-is, never re-validated or re-encrypted.""" data = _redact_unchanged_encrypted_payloads(flag, data) - serializer = FeatureFlagSerializer( - flag, data=data, partial=True, context=_serializer_context(team, user, request, method="PATCH") - ) + context = _serializer_context(team, user, request, method="PATCH") + context["allow_exceeding_flag_limit"] = allow_exceeding_flag_limit + serializer = FeatureFlagSerializer(flag, data=data, partial=True, context=context) serializer.is_valid(raise_exception=True) saved = serializer.save() if saved.has_encrypted_payloads: @@ -165,13 +177,33 @@ def archive_flag( return update_flag(flag, data, team=team, user=user, request=request) -def unarchive_flag(flag: FeatureFlag, *, team: Team, user: Any, request: Any | None = None) -> FeatureFlag: +def unarchive_flag( + flag: FeatureFlag, + *, + team: Team, + user: Any, + request: Any | None = None, + allow_exceeding_flag_limit: bool = False, +) -> FeatureFlag: """Unarchive a flag through the gated serializer path. The flag stays disabled; re-enabling it is a separate, explicit write (``set_flag_active``). + + Archived flags don't count against the team's flag limit, so unarchiving grows the + counted set and has to clear the cap the way a create does — this raises when the team + is already there. Pass ``allow_exceeding_flag_limit`` to waive it when the write undoes + an archive the calling product performed itself, so the undo can't be blocked by a cap + the original archive helped free up. """ - return update_flag(flag, {"archived": False}, team=team, user=user, request=request) + return update_flag( + flag, + {"archived": False}, + team=team, + user=user, + request=request, + allow_exceeding_flag_limit=allow_exceeding_flag_limit, + ) def _roll_out_variant( diff --git a/products/feature_flags/backend/test/test_facade.py b/products/feature_flags/backend/test/test_facade.py index f430843fcb5b..2ef0e05c8100 100644 --- a/products/feature_flags/backend/test/test_facade.py +++ b/products/feature_flags/backend/test/test_facade.py @@ -24,6 +24,7 @@ flag_disable_requires_approval, set_flag_active, ship_variant, + unarchive_flag, update_flag, ) from products.feature_flags.backend.facade.filters import ( @@ -83,6 +84,21 @@ def test_archive_active_flag_without_disable_is_rejected(self): assert flag.archived is False assert flag.active is True + def test_unarchive_flag_enforces_the_count_limit_by_default(self): + # The waiver is opt-in. Without it, unarchiving through the facade clears the cap + # like a create, so a caller can't skip it just by going through the facade. + archived_flag = FeatureFlag.objects.create( + team=self.team, created_by=self.user, key="facade-archived-flag", active=False, archived=True + ) + self._create_flag() + + with self.settings(MAX_FEATURE_FLAGS_PER_TEAM=1): + with self.assertRaises(ValidationError): + unarchive_flag(archived_flag, team=self.team, user=self.user) + + archived_flag.refresh_from_db() + assert archived_flag.archived is True + def test_ship_variant_without_base_filters_uses_flag_filters(self): flag = self._create_flag( filters={