Repository navigation
fix(feature-flags): clamp variant rollouts that sum over 100 - #92797
Conversation
Every evaluator walks the variants accumulating percentages and stops at the first whose running total passes the hash, which never exceeds 1.0, so 40/40/40 already serves 40/40/20. Rewriting it to what is actually served changes nothing anyone can observe. Checked against the Rust server and the Python, Go, .NET and JS SDKs; Java does no local evaluation. Sums under 100 are left alone: the shortfall is users who match the flag and get no variant, and handing that slice to someone is the customer's call.
|
😎 Merged successfully - details. |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/feature_flags/backend/migrations/0015_clamp_rollout_percentages_over_100.py |
98.6% | 57 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 33660700655 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
warehouse_sources_queue |
██████░░░░░░░░░░░░░░ 29.1% |
92 / 316 |
demo |
███████████░░░░░░░░░ 57.0% |
1,523 / 2,671 |
autoresearch |
████████████░░░░░░░░ 62.5% |
5 / 8 |
data_tools |
█████████████░░░░░░░ 66.2% |
90 / 136 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
batch_exports |
████████████████░░░░ 80.1% |
19,628 / 24,501 |
apm |
█████████████████░░░ 83.3% |
1,234 / 1,481 |
signals |
█████████████████░░░ 84.6% |
33,420 / 39,521 |
cdp |
█████████████████░░░ 85.3% |
4,332 / 5,077 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
data_modeling |
█████████████████░░░ 86.7% |
9,654 / 11,133 |
notebooks |
██████████████████░░ 87.8% |
11,261 / 12,829 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
data_warehouse |
██████████████████░░ 88.6% |
12,793 / 14,436 |
cohorts |
██████████████████░░ 89.1% |
7,369 / 8,275 |
business_knowledge |
██████████████████░░ 89.1% |
4,440 / 4,984 |
engineering_analytics |
██████████████████░░ 89.4% |
8,130 / 9,090 |
dashboards |
██████████████████░░ 89.6% |
6,811 / 7,602 |
tasks |
██████████████████░░ 90.0% |
63,289 / 70,319 |
streamlit_apps |
██████████████████░░ 90.2% |
2,500 / 2,773 |
links |
██████████████████░░ 90.6% |
183 / 202 |
exports |
██████████████████░░ 90.7% |
9,034 / 9,963 |
managed_warehouse |
██████████████████░░ 90.8% |
8,734 / 9,618 |
visual_review |
██████████████████░░ 90.9% |
6,880 / 7,567 |
alerts |
██████████████████░░ 91.0% |
5,208 / 5,722 |
canvas |
██████████████████░░ 91.0% |
4,919 / 5,403 |
conversations |
██████████████████░░ 91.1% |
21,293 / 23,367 |
mcp_analytics |
██████████████████░░ 91.2% |
4,375 / 4,796 |
error_tracking |
██████████████████░░ 91.6% |
12,929 / 14,116 |
notifications |
██████████████████░░ 92.2% |
1,101 / 1,194 |
early_access_features |
███████████████████░ 92.5% |
1,323 / 1,430 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
slack_app |
███████████████████░ 92.8% |
12,381 / 13,342 |
stamphog |
███████████████████░ 92.9% |
5,775 / 6,216 |
web_analytics |
███████████████████░ 93.2% |
18,634 / 20,004 |
surveys |
███████████████████░ 93.2% |
6,043 / 6,486 |
context_layer |
███████████████████░ 93.3% |
2,594 / 2,779 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
mcp_store |
███████████████████░ 93.5% |
7,639 / 8,174 |
posthog_ai |
███████████████████░ 93.5% |
1,372 / 1,468 |
marketing_analytics |
███████████████████░ 93.5% |
17,326 / 18,530 |
ai_observability |
███████████████████░ 93.7% |
18,098 / 19,306 |
approvals |
███████████████████░ 93.7% |
3,492 / 3,725 |
billing_alerts |
███████████████████░ 94.0% |
2,069 / 2,200 |
legal_documents |
███████████████████░ 94.1% |
1,970 / 2,093 |
endpoints |
███████████████████░ 94.3% |
8,865 / 9,405 |
review_hog |
███████████████████░ 94.3% |
9,982 / 10,583 |
tracing |
███████████████████░ 94.4% |
2,683 / 2,842 |
workflows |
███████████████████░ 94.6% |
11,249 / 11,894 |
access_control |
███████████████████░ 94.9% |
5,826 / 6,137 |
customer_analytics |
███████████████████░ 95.0% |
17,990 / 18,943 |
annotations |
███████████████████░ 95.2% |
795 / 835 |
product_analytics |
███████████████████░ 95.3% |
19,356 / 20,321 |
replay_vision |
███████████████████░ 95.4% |
26,087 / 27,356 |
experiments |
███████████████████░ 95.5% |
29,271 / 30,660 |
actions |
███████████████████░ 95.5% |
706 / 739 |
skills |
███████████████████░ 95.6% |
5,403 / 5,651 |
messaging |
███████████████████░ 95.8% |
3,646 / 3,805 |
logs |
███████████████████░ 95.9% |
13,525 / 14,107 |
growth |
███████████████████░ 95.9% |
7,644 / 7,969 |
feature_flags |
███████████████████░ 96.1% |
19,789 / 20,586 |
revenue_analytics |
███████████████████░ 96.3% |
1,841 / 1,911 |
user_interviews |
███████████████████░ 96.5% |
2,639 / 2,735 |
warehouse_sources |
███████████████████░ 97.2% |
407,990 / 419,844 |
data_quality |
███████████████████░ 97.5% |
4,103 / 4,209 |
analytics_platform |
████████████████████ 98.1% |
2,286 / 2,330 |
metrics |
████████████████████ 98.2% |
3,557 / 3,621 |
data_catalog |
████████████████████ 98.4% |
3,235 / 3,289 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
⚠️ Django migration SQL — 1 new migration to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/feature_flags/backend/migrations/0015_clamp_rollout_percentages_over_100.py
--
-- Raw Python operation
--
-- THIS OPERATION CANNOT BE WRITTEN AS SQLLast updated: 2026-09-02 17:27 UTC (fe1a3f9)
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 0 Safe | 1 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
feature_flags.0015_clamp_rollout_percentages_over_100
└─ #1 ⚠️ RunPython: RunPython data migration needs review for performance
📚 How to Deploy These Changes Safely
RunPython:
Use batching for large data migrations:
- Use
.iterator()to avoid loading all rows into memory - Use
.bulk_update()instead of saving individual objects - Batch size: 1,000-10,000 rows per batch
- Add pauses between batches
- Consider background jobs for very large updates (millions of rows)
See the migration safety guide
Last updated: 2026-09-02 17:28 UTC (fe1a3f9)
Prompt To Fix All With AI### Issue 1
products/feature_flags/backend/migrations/0015_clamp_rollout_percentages_over_100.py:72
**Migration signatures lack concrete types**
The new migration callback leaves both parameters unannotated, `_clamp_filters` uses bare container types, and the associated test helpers introduce several `Any` annotations. This reduces useful static coverage for a one-time data rewrite and does not satisfy the repository's strict-typing convention.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(feature-flags): clamp variant rollou..." | Re-trigger Greptile |
| all flags read-only and writes only the ones that change. Soft-deleted and inactive flags | ||
| included: their filters are blanked in the cached payload, but restoring or re-enabling a | ||
| flag would put the stored value straight back in play.""" | ||
| FeatureFlag = apps.get_model("feature_flags", "FeatureFlag") |
There was a problem hiding this comment.
Migration signatures lack concrete types
The new migration callback leaves both parameters unannotated, _clamp_filters uses bare container types, and the associated test helpers introduce several Any annotations. This reduces useful static coverage for a one-time data rewrite and does not satisfy the repository's strict-typing convention.
Context Used: AGENTS.MD (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/feature_flags/backend/migrations/0015_clamp_rollout_percentages_over_100.py
Line: 72
Comment:
**Migration signatures lack concrete types**
The new migration callback leaves both parameters unannotated, `_clamp_filters` uses bare container types, and the associated test helpers introduce several `Any` annotations. This reduces useful static coverage for a one-time data rewrite and does not satisfy the repository's strict-typing convention.
**Context Used:** AGENTS.MD ([source](https://github.com/posthog/posthog/blob/master/AGENTS.MD))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Taking you up on the offer to remember this one, since it will recur on every migration.
Migrations here deliberately don't follow the repo-wide typing convention. The three siblings this one is modelled on all use the same shape, including 0014 which merged yesterday with two human approvals:
0011 def clean_flag_filters_recoverable_violations(apps, schema_editor):
0013 def _narrow_rollout_percentages(filters: dict) -> tuple[dict, int]:
0014 def clean_flag_filters_inert_violations(apps, schema_editor):
Annotating the callback properly means importing StateApps and BaseDatabaseSchemaEditor into the migration, which cuts against the "migrations must stay self-contained while app code evolves" rule that 0011 states in its own comments. That self-containment is why these files also keep frozen copies of constants rather than importing them.
The test helpers are the harness's own signature rather than a choice: posthog/test/base.py:1687 declares def setUpBeforeMigration(self, apps) unannotated, and products/experiments/backend/test/test_migration_0035.py types it apps: Any, which is what this test follows.
mypy runs repo-wide in CI and passes on this branch (19,523 files), so nothing here is actually escaping the type checker.
There was a problem hiding this comment.
Understood. I’ll treat the established migration convention as intentional and won’t flag unannotated migration callbacks, bare container annotations, or Any in migration test harness helpers as typing violations.
haacked
left a comment
There was a problem hiding this comment.
A few blocking issues inline.
has_hash_dependent_variants returns false when any variant sits at 100, even where an earlier smaller one still takes the low hashes. Clamping removes the 100 and flips it true, starting the continuity override lookup, which can change which identifier is hashed. That only matters with continuity on, since get_hash picks the override branch on has_experience_continuity, so the guard is the intersection of both. No production flag currently has both. Also freezes the tolerance constant rather than importing a live one, and narrows the no-observable-change claim: a hash of exactly 1.0 stops matching the last variant, which is how a correctly summing flag has always behaved. Tests now assert a whole filters object, exercise a write landing mid-scan against the compare-and-swap, run past the first 500-row batch, and cover a non-dict filters value and a boolean rollout.
| while True: | ||
| # _base_manager documents that soft-deleted rows are in scope (a historical model's | ||
| # manager is plain anyway). Nothing here touches payloads, so encrypted flags are in scope. | ||
| rows = list(FeatureFlag._base_manager.filter(id__gt=last_id).order_by("id").only("id", "filters")[:BATCH_SIZE]) |
There was a problem hiding this comment.
blocking: The scan runs one extra SELECT per flag row. .only("id", "filters") defers ensure_experience_continuity, and line 119 reads it, so Django re-fetches that one column per instance. The read happens before _clamp_filters can bail out, and filters has a model default, so it fires for every row in the table. That turns rows/500 queries into rows + rows/500, each one a pgbouncer round trip while the deploy waits on migrate. 0014 avoids this only because its loop touches nothing but flag.filters.
| rows = list(FeatureFlag._base_manager.filter(id__gt=last_id).order_by("id").only("id", "filters")[:BATCH_SIZE]) | |
| rows = list( | |
| FeatureFlag._base_manager.filter(id__gt=last_id) | |
| .order_by("id") | |
| .only("id", "filters", "ensure_experience_continuity")[:BATCH_SIZE] | |
| ) |
There was a problem hiding this comment.
Confirmed and fixed in 0f244cc.
Embarrassing detail: I did write this exact .only(...) when adding the continuity guard, but ruff had collapsed the query onto one line and my edit matched the old three-line form, so the replacement silently did nothing. No assertion on the match, so I never noticed. The field is loaded now.
| assert rollouts(self._filters(self.first_takes_all_id)) == [100, 0] | ||
| assert rollouts(self._filters(self.unreachable_id)) == [50, 50, 0] | ||
|
|
||
| def test_keeps_whole_numbers_as_integers(self) -> None: |
There was a problem hiding this comment.
blocking: This test passes with the guard it names deleted. Every fixture in the file produces identical output with or without the float(value).is_integer() conversion. The straddling fixture is all ints, so the remainder is already the int 20, and the fractional remainder 33.34 is not whole. The case that needs the guard is a decimal flag whose remainder lands whole: 33.5/33.5/50 clamps to exactly 33.0, and only the conversion makes it 33. Without it, the migration writes the float shape #84957 fixed straight back into jsonb.
A fixture whose output actually differs would pin it down:
self.whole_remainder_id = make_flag(
"whole-remainder", {"groups": [], "multivariate": variants(33.5, 33.5, 50)}
).idassert rollouts(self._filters(self.whole_remainder_id)) == [33.5, 33.5, 33]
assert [type(r) for r in rollouts(self._filters(self.whole_remainder_id))] == [float, float, int]There was a problem hiding this comment.
You are right, and I checked it rather than taking it on faith. Running every fixture through the transform with and without the conversion:
[40, 40, 40] with=[40, 40, 20] without=[40, 40, 20] same
[60, 60] with=[60, 40] without=[60, 40] same
[150, 10] with=[100, 0] without=[100, 0] same
[50, 50, 50] with=[50, 50, 0] without=[50, 50, 0] same
[33.33, 33.33, 50] with=[33.33, 33.33, 33.34] without=[33.33, 33.33, 33.34] same
[33.5, 33.5, 50] with=[33.5, 33.5, 33] without=[33.5, 33.5, 33.0] differs
Added your fixture in 0f244cc and verified it fails with the conversion removed and passes with it restored.
| used: float = 0 | ||
| for rollout in rollouts: | ||
| headroom = 100 - used | ||
| value = rollout if rollout <= headroom else max(headroom, 0) |
There was a problem hiding this comment.
suggestion: The remainder can land as a float artifact. used accumulates in binary floating point, so [10.1, 20.2, 30.3, 50.5] clamps to [10.1, 20.2, 30.3, 39.400000000000006], and that stored value is what the flag editor shows and the local-evaluation payload carries. Rounding fixes it. The shift is at most 5e-11, inside the 1e-9 tolerance, so a rerun still no-ops, and no current expected output moves.
| value = rollout if rollout <= headroom else max(headroom, 0) | |
| value = rollout if rollout <= headroom else round(max(headroom, 0), 10) |
There was a problem hiding this comment.
Taken in 0f244cc, with a fixture asserting [10.1, 20.2, 30.3, 50.5] stores 39.4 rather than 39.400000000000006.
| continue | ||
| # Compare-and-swap on the value we read: a flag edited between the batch select and | ||
| # this write keeps the newer filters instead of being reverted to our snapshot. | ||
| written = FeatureFlag._base_manager.filter(id=flag.id, filters=flag.filters).update(filters=new_filters) |
There was a problem hiding this comment.
suggestion: Worth a docstring line on cache propagation. .update() fires no post_save, so the flag-definitions cache keeps the pre-clamp payload until the hypercache verification task repairs the drift. It will: a filters diff counts as real drift, and the grace period keys off updated_at, which this write also leaves untouched. Not observable, since both values pick the same variant. But the docstring already reasons about the cached payload for soft-deleted flags, so a reader will assume this was considered. Say which way it goes.
There was a problem hiding this comment.
Added in 0f244cc. The docstring now says the cache serves the pre-clamp payload until the verification task repairs the drift, and that both values pick the same variant meanwhile, so a reader does not have to work out whether it was considered.
| """One-time cleanup of multivariate flags whose variant percentages sum over 100 (#50084). | ||
|
|
||
| Only the over-100 case: the evaluators truncate it already, so rewriting it serves the same | ||
| variant for every hash below 1.0. The exception is a hash of exactly 1.0, which calculate_hash |
There was a problem hiding this comment.
suggestion: "the exception is a hash of exactly 1.0" is narrower than what actually moves. The experiment sample-ratio-mismatch check builds its expected counts from these stored percentages (products/experiments/backend/hogql_queries/experiment_exposures_query_runner.py:202-266), so a 40/40/40 flag shows a mismatch warning today that clears after this migration. A 150/10 flag clamped to 100/0 stops returning a sample-ratio result at all, and the uneven-split bias warning (products/experiments/backend/analysis_health.py:37) can change verdict too. All corrections, but visible on the experiment page. Worth a docstring line.
There was a problem hiding this comment.
Good catch, and confirmed: experiment_exposures_query_runner.py builds rollout_percentages straight from the stored variants, and analysis_health.py reads them for the even-distribution check, so both change on clamped flags.
Recorded in the docstring in 0f244cc, framed as corrections rather than regressions: a 40/40/40 flag stops reporting a sample-ratio mismatch it never really had.
| already 40/40/20. Verified against the Rust server, plus the Python, Go, .NET and JS SDKs. | ||
| Whole numbers stay ints because the .NET and Java SDKs deserialize rollout percentages as | ||
| integers (#84957).""" | ||
| if any(isinstance(r, bool) or not isinstance(r, int | float) for r in rollouts): |
There was a problem hiding this comment.
question: Does the over-100 population you sized in Metabase hold any negative percentages? A negative earlier variant pushes headroom above 100, so [-10, 150] becomes [-10, 110]. Evaluation is unchanged, but the flag then fails the API's 0-100 validation the next time someone saves it from the UI. If none exist, this is belt and braces. If any do, or r < 0 in the guard skips them like the other malformed shapes.
| if any(isinstance(r, bool) or not isinstance(r, int | float) for r in rollouts): | |
| if any(isinstance(r, bool) or not isinstance(r, int | float) or r < 0 for r in rollouts): |
There was a problem hiding this comment.
Added the guard in 0f244cc rather than waiting on the count, since skipping a negative is consistent with how the other malformed shapes are treated and costs nothing if none exist.
I have not sized negatives specifically. The earlier Metabase pass counted over-100 sums and the risky shape, not the sign of individual percentages, so I will run that separately and follow up here with the number.
There was a problem hiding this comment.
Sized it: zero in both regions, on both counts.
| any negative percentage | negative and sum over 100 | |
|---|---|---|
| prod-us | 0 | 0 |
| prod-eu | 0 | 0 |
So belt and braces, as you put it. Keeping the guard anyway since it costs nothing and matches how the other malformed shapes are handled.
…int guard The scan read ensure_experience_continuity while only() deferred it, so Django refetched that column once per row, turning rows/500 queries into rows + rows/500 while the deploy waits on migrate. Every existing fixture produced identical output with or without the int conversion, so the test named after it did not exercise it. A decimal set whose remainder lands whole does: 33.5/33.5/50 stores 33.0 without the guard. Also rounds the remainder, since accumulates in binary floating point, skips negative percentages that would lift headroom above 100, and records where the change is visible: the experiment sample-ratio and uneven-split checks read these values, and the cached payload lags until the verification task repairs it.
JamesPatrickGill
left a comment
There was a problem hiding this comment.
All six addressed in 0f244cc and verified against the diff. Nice turnaround, and thanks for showing your working on the fixture table.
gustavohstrassburger
left a comment
There was a problem hiding this comment.
One issue from the latest commit. The continuity guard added in 680e3e0 reads a field the batch query defers, so the scan now issues a query per row.
Everything haacked raised checks out against the code at head, and I mutation-tested the three test-coverage claims: weakening the compare-and-swap to filter(id=flag.id), dropping the isinstance(r, bool) guard, and breaking out after the first batch each make the matching test fail for the right reason.
|
|
||
| updated_rows = 0 | ||
| skipped_concurrent = 0 | ||
|
|
There was a problem hiding this comment.
blocking: ensure_experience_continuity is not in this .only(), but line 119 reads it, so each row triggers its own SELECT.
Captured the queries over 25 candidate flags:
TOTAL queries: 52 SELECTs: 27 rows: 25
SELECT "id", "filters" FROM posthog_featureflag WHERE id > 0 ORDER BY id LIMIT 500
SELECT "id", "ensure_experience_continuity" FROM posthog_featureflag WHERE id = 6466 LIMIT 21
SELECT "id", "ensure_experience_continuity" FROM posthog_featureflag WHERE id = 6467 LIMIT 21
...
One batched SELECT becomes one query per flag. The read sits before the _clamp_filters early return, so it fires for every row with dict filters, which is the whole table rather than the ~62 flags that get clamped. That turns a bounded scan into a per-row round trip against the table the flags API writes to.
| rows = list( | |
| FeatureFlag._base_manager.filter(id__gt=last_id) | |
| .order_by("id") | |
| .only("id", "filters", "ensure_experience_continuity")[:BATCH_SIZE] | |
| ) |
Worth a query-count assertion too, since nothing currently fails if the field drops back out of the .only().
There was a problem hiding this comment.
Same finding James raised, and it was fixed in 0f244cc shortly before your comment landed, so the field is in the .only() on the current head. Sorry for the crossed wires.
The second half was not done and is the more useful part, so thank you: added in fe1a3f9. It captures the queries the scan issues over 25 candidate flags and asserts they do not scale with row count, exactly 2 SELECTs, one batch plus the empty batch that ends the loop.
Verified it fails for the right reason: removing ensure_experience_continuity from the .only() makes it fail, restoring it makes it pass.
Worth saying that this is not hypothetical. That field dropped out once already: I wrote the .only() when adding the continuity guard, ruff had collapsed the query onto one line, my edit matched the old three-line form and silently did nothing. Nothing failed, which is precisely the gap you are pointing at.
The continuity field already dropped out of the only() once, silently, and nothing failed. Captures the queries the scan issues and asserts they do not scale with the number of rows.
Multivariate flags whose variant percentages add up to more than 100 now store what the evaluators already serve.
40/40/40becomes40/40/20,150/10becomes100/0.Why the assignment does not change
Every evaluator walks the variants in order adding up the percentages, and returns the first one whose running total passes the user's hash. That hash never goes above 1.0, so a variant starting at or beyond 100 can never be picked, and one that straddles it only serves the remainder. Checked against the Rust server and the Python, Go, .NET and JS SDKs. The Java SDK does not do local evaluation at all.
Where it is visible anyway
Three carve-outs, all of them corrections rather than regressions:
calculate_hashcan return it, and a clamped flag then returns no variant, which is what a correctly summing flag has always done. Odds are 1 in 2^60.40/40/40flag stops reporting a mismatch it never really had..update()fires nopost_save. Both values pick the same variant meanwhile.What it leaves alone
has_hash_dependent_variants, which starts the continuity override lookup and can change which identifier is hashed. Zero flags in either region currently have both, so nothing is skipped today.How it runs
Tests
Cover the straddling and unreachable cases, integers and decimals including a remainder that lands whole, the float artifact in the remainder, the continuity guard in all three of its states, rows past the first batch, a write landing mid scan against the compare and swap, whole
filterspreservation, and the malformed shapes it has to skip rather than raise on.