Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions lib/optimizely/cmab/cmab_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,35 @@ class DefaultCmabService
#
# @raise [ArgumentError] If cmab_cache is not an instance of LRUCache.
# @raise [ArgumentError] If cmab_client is not an instance of DefaultCmabClient.

NUM_LOCK_STRIPES = 1000

def initialize(cmab_cache, cmab_client, logger = nil)
@cmab_cache = cmab_cache
@cmab_client = cmab_client
@logger = logger
@locks = Array.new(NUM_LOCK_STRIPES) { Mutex.new }
end

def get_decision(project_config, user_context, rule_id, options)
lock_index = get_lock_index(user_context.user_id, rule_id)

@locks[lock_index].synchronize do
get_decision_impl(project_config, user_context, rule_id, options)
end
end

private

def get_lock_index(user_id, rule_id)
# Create a hash of user_id + rule_id for consistent lock selection
hash_input = "#{user_id}#{rule_id}"
# Use Ruby's built-in hash method and ensure positive result
hash_value = hash_input.hash.abs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String#hash is not deterministic across Ruby processes. It uses a random seed that changes per process/restart to prevent hash collision attacks. This means:

  • Same input → Different hash values in different Ruby processes
  • Cache keys will map to different locks in different processes
  • If using multi-process deployment (Unicorn, Puma workers), this defeats the purpose

Hmm, we shld use a use a deterministic hash function.

is there ruby gem for murmurhash?
If so the you could use:
require 'murmurhash3'

hash_value = MurmurHash3::V32.str_hash(hash_input)
hash_value % NUM_LOCK_STRIPES

if not, then could use:
require 'digest'

hash_value = Digest::MD5.hexdigest(hash_input).hex % (2**32)
hash_value % NUM_LOCK_STRIPES

hash_value % NUM_LOCK_STRIPES
end

def get_decision_impl(project_config, user_context, rule_id, options)
# Retrieves a decision for a given user and rule, utilizing a cache for efficiency.
#
# This method filters user attributes, checks for various cache-related options,
Expand Down Expand Up @@ -85,8 +107,6 @@ def get_decision(project_config, user_context, rule_id, options)
cmab_decision
end

private

def fetch_decision(rule_id, user_id, attributes)
# Fetches a decision for a given rule and user, along with user attributes.
#
Expand Down
42 changes: 42 additions & 0 deletions spec/cmab/cmab_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,46 @@
)
end
end

describe 'lock striping behavior' do
describe '#get_lock_index' do
it 'returns consistent lock index for same user/rule combination' do
user_id = 'test_user'
rule_id = 'test_rule'

# Get lock index multiple times
index1 = cmab_service.send(:get_lock_index, user_id, rule_id)
index2 = cmab_service.send(:get_lock_index, user_id, rule_id)
index3 = cmab_service.send(:get_lock_index, user_id, rule_id)

# All should be the same
expect(index1).to eq(index2), 'Same user/rule should always use same lock'
expect(index2).to eq(index3), 'Same user/rule should always use same lock'
end

it 'distributes different user/rule combinations across multiple locks' do
test_cases = [
%w[user1 rule1],
%w[user2 rule1],
%w[user1 rule2],
%w[user3 rule3],
%w[user4 rule4]
]

lock_indices = Set.new
test_cases.each do |user_id, rule_id|
index = cmab_service.send(:get_lock_index, user_id, rule_id)

# Verify index is within expected range
expect(index).to be >= 0, 'Lock index should be non-negative'
expect(index).to be < Optimizely::DefaultCmabService::NUM_LOCK_STRIPES, 'Lock index should be less than NUM_LOCK_STRIPES'

lock_indices.add(index)
end

# We should have multiple different lock indices (though not necessarily all unique due to hash collisions)
expect(lock_indices.size).to be > 1, 'Different user/rule combinations should generally use different locks'
end
end
end
end