Repository navigation
fix(flags): respect variant order in has_hash_dependent_variants - #93647
Conversation
The check returned false whenever any variant sat at 100, but get_matching_variant walks the variants in order, so an earlier smaller variant still takes the low hashes. For [40, 100] the first variant serves every hash below 0.40, meaning assignment is hash dependent while the optimizer said it was not. The consequence is that needs_hash_key_override skips the experience continuity lookup for those flags, so a user whose distinct_id changes can be moved to a different variant, which is the thing continuity exists to prevent. It now counts reachable variants instead: those whose running total is still under 100 and whose own share is non-zero.
|
😎 Merged successfully - details. |
github-actions
Bot
added
feature/feature-flags
The rewrite also changes all-zero variants from hash dependent to not, since no variant is ever selected there. Pinning both that and a 100 sitting mid-list.
Prompt To Fix All With AI### Issue 1
rust/feature-flags/src/flags/flag_operations.rs:1375-1376
The comment describes how the previous rule behaved rather than only documenting the enduring assignment contract, leaving transient implementation history in source that can become stale.
```suggestion
// Hashes below 0.40 select "control", so assignment depends on the hash and
// continuity lookups must not be skipped.
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(flags): respect variant order in has..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The old rule saw the 100 and returned false, but hashes below 0.40 select "control", | ||
| // so assignment does depend on the hash and continuity lookups must not be skipped. |
There was a problem hiding this comment.
| // The old rule saw the 100 and returned false, but hashes below 0.40 select "control", | |
| // so assignment does depend on the hash and continuity lookups must not be skipped. | |
| // Hashes below 0.40 select "control", so assignment depends on the hash and | |
| // continuity lookups must not be skipped. |
File Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: rust/feature-flags/src/flags/flag_operations.rs
Line: 1375-1376
Comment:
The comment describes how the previous rule behaved rather than only documenting the enduring assignment contract, leaving transient implementation history in source that can become stale.
```suggestion
// Hashes below 0.40 select "control", so assignment depends on the hash and
// continuity lookups must not be skipped.
```
**File Used:** `AGENTS.md` ([source](https://github.com/posthog/posthog/blob/ab1a925ea41bc88fbdba9df2673b322f15e693b4/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.
Fair, and it applies to a second comment you did not flag. Fixed in 0ecd1fc.
Took your suggestion as written on this one. Also rewrote the all-zero test's comment, which had the same problem: it said the old rule called that shape hash dependent. Both now describe only what the variant shape means for assignment, which is what stays true.
CLAUDE.md in this repo says the same thing, that change history belongs in the commit message rather than the source, so this was mine to catch.
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
Two test comments explained what the rule used to return instead of what the shape means. The repo convention keeps change history in the commit, not the source.
has_hash_dependent_variantsreturned false whenever any variant sat at 100, butget_matching_variantwalks the variants in order, so an earlier smaller variant still takes the low hashes. On a[40, 100]flag the first variant serves every hash below 0.40, which means assignment does depend on the hash while the optimizer said it did not.What that breaks today
needs_hash_key_overrideuses this to decide whether to look up the experience continuity override. For those flags it skips the lookup, so bucketing falls back to the currentdistinct_idinstead of the stored one. A user whosedistinct_idchanges, which is exactly what continuity exists to handle, can be moved to a different variant.It only bites when
optimize_experience_continuity_lookupsis on, since the legacy path always does the lookup.The fix
Count reachable variants rather than looking for a 100 anywhere: a variant is reachable while the running total before it is under 100 and its own share is non-zero, and hashing decides nothing when at most one is. Position is what matters, not presence:
[40, 100][30, 100, 50][0, 0][100, 40][0, 100][150, 10][50, 50]The
[0, 0]row is a second correction that came out of the rewrite: no variant is ever selected there, so the hash decides nothing and the lookup can be skipped. Harmless either way, but worth naming rather than leaving it to be discovered.Scope
Found while writing a migration that clamps rollout sums over 100. That migration guards these flags itself rather than depending on this fix, so the two are independent. In production 12 flags in prod-us and 2 in prod-eu have the shape; none currently have continuity enabled, so nothing is mis-bucketed right now, but any of them could be by turning it on.
Tests
Six new cases covering every row above, plus a
needs_hash_key_overridecase for[40, 100]with continuity on. The existing[100, 0]test still passes unchanged, since the corrected rule agrees with the old one there. Verified the new[40, 100]test fails against the old rule.