Skip to content

Commit fb5930d

Browse files
authored
[FSSDK-11917] Fix for: A specific user does not get bucketed correctly in the GO SDK (#423)
* Fix float32 precision bug causing bucket values to exceed 9999 for users with hash values near max uint32 * Fix float32 precision bug causing bucket values to exceed 9999 for users with hash values near max uint32 * Trigger CI
1 parent 92b83d2 commit fb5930d

File tree

2 files changed

+84
-2
lines changed

2 files changed

+84
-2
lines changed

pkg/decision/bucketer/murmurhashbucketer.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import (
2727
"github.com/twmb/murmur3"
2828
)
2929

30-
var maxHashValue = float32(math.Pow(2, 32))
30+
var maxHashValue = math.Pow(2, 32)
3131

3232
// DefaultHashSeed is the hash seed to use for murmurhash
3333
const DefaultHashSeed = 1
@@ -60,7 +60,7 @@ func (b MurmurhashBucketer) Generate(bucketingKey string) int {
6060
b.logger.Error(fmt.Sprintf("Unable to generate a hash for the bucketing key=%s", bucketingKey), err)
6161
}
6262
hashCode := hasher.Sum32()
63-
ratio := float32(hashCode) / maxHashValue
63+
ratio := float64(hashCode) / maxHashValue
6464
return int(ratio * maxTrafficValue)
6565
}
6666

pkg/decision/bucketer/murmurhashbucketer_test.go

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,3 +68,85 @@ func TestGenerateBucketValue(t *testing.T) {
6868
assert.Equal(t, 2434, bucketer.Generate(bucketingKey3))
6969
assert.Equal(t, 5439, bucketer.Generate(bucketingKey4))
7070
}
71+
72+
// TestFloat64PrecisionEdgeCase tests the fix for FSSDK-11917
73+
// This test ensures that users with hash values near the maximum uint32 value
74+
// are bucketed correctly (bucket value 9999) instead of getting bucket value 10000
75+
// which would exclude them from the "Everyone Else" rollout rule.
76+
func TestFloat64PrecisionEdgeCase(t *testing.T) {
77+
bucketer := NewMurmurhashBucketer(logging.GetLogger("", "TestFloat64PrecisionEdgeCase"), DefaultHashSeed)
78+
79+
// The main test: ensure all bucket values are in valid range [0, 9999]
80+
// This catches any case where float32 precision would cause bucket value 10000
81+
for i := 0; i < 10000; i++ {
82+
bucketingKey := fmt.Sprintf("user_%d_rule_12345", i)
83+
bucket := bucketer.Generate(bucketingKey)
84+
assert.GreaterOrEqual(t, bucket, 0, "Bucket value should be >= 0 for key %s", bucketingKey)
85+
assert.LessOrEqual(t, bucket, 9999, "Bucket value should be <= 9999 for key %s (float32 bug would produce 10000)", bucketingKey)
86+
}
87+
88+
// Test with a bucketing key that Mark Biesheuvel identified
89+
// The issue occurs with specific user ID + rule ID combinations
90+
// Test a range of user IDs to find edge cases
91+
for userID := 15580841; userID <= 15580851; userID++ {
92+
t.Run(fmt.Sprintf("UserID_%d", userID), func(t *testing.T) {
93+
// Test with different rule IDs that might trigger the edge case
94+
ruleIDs := []string{"default-rollout-386456-20914452332", "rule_123", "everyone_else"}
95+
for _, ruleID := range ruleIDs {
96+
bucketingKey := fmt.Sprintf("%d%s", userID, ruleID)
97+
bucket := bucketer.Generate(bucketingKey)
98+
assert.GreaterOrEqual(t, bucket, 0, "Bucket should be >= 0")
99+
assert.LessOrEqual(t, bucket, 9999, "Bucket should be <= 9999 (float32 would produce 10000 for some edge cases)")
100+
}
101+
})
102+
}
103+
}
104+
105+
// TestBucketingWithHighHashValues tests that users with hash values close to max uint32
106+
// are correctly bucketed into rollout rules
107+
func TestBucketingWithHighHashValues(t *testing.T) {
108+
bucketer := NewMurmurhashBucketer(logging.GetLogger("", "TestBucketingWithHighHashValues"), DefaultHashSeed)
109+
110+
// Create a traffic allocation for "Everyone Else" (0-10000 range)
111+
everyoneElseAllocation := []entities.Range{
112+
{
113+
EntityID: "variation_1",
114+
EndOfRange: 10000,
115+
},
116+
}
117+
118+
// Test various users to ensure they are properly bucketed
119+
// The bucketing key needs to match the format used in actual bucketing (userID + experimentID or ruleID)
120+
testBucketingKey := "15580846default-rollout-386456-20914452332"
121+
bucketValue := bucketer.Generate(testBucketingKey)
122+
123+
// The important test: bucket value should be in valid range [0, 9999]
124+
assert.GreaterOrEqual(t, bucketValue, 0, "Bucket value should be >= 0")
125+
assert.LessOrEqual(t, bucketValue, 9999, "Bucket value should be <= 9999")
126+
127+
// Test that this user is bucketed into the "Everyone Else" variation
128+
entityID := bucketer.BucketToEntity(testBucketingKey, everyoneElseAllocation)
129+
assert.Equal(t, "variation_1", entityID, "User should be bucketed into 'Everyone Else' variation")
130+
131+
// Test that the fix doesn't break normal bucketing behavior
132+
// Create a more complex traffic allocation
133+
complexAllocation := []entities.Range{
134+
{
135+
EntityID: "",
136+
EndOfRange: 5000, // 50% get no variation
137+
},
138+
{
139+
EntityID: "variation_a",
140+
EndOfRange: 10000, // 50% get variation_a
141+
},
142+
}
143+
144+
// Test with the specific bucketing key
145+
entityID = bucketer.BucketToEntity(testBucketingKey, complexAllocation)
146+
// The bucket value will determine which variation is returned
147+
if bucketValue < 5000 {
148+
assert.Equal(t, "", entityID, "User with bucket < 5000 should get no variation")
149+
} else {
150+
assert.Equal(t, "variation_a", entityID, "User with bucket >= 5000 should get variation_a")
151+
}
152+
}

0 commit comments

Comments
 (0)