fix: prevent lost custom_attributes updates from concurrent jsonb writes (#14040)
## Linear ticket https://linear.app/chatwoot/issue/CW-6834/billing-upgrade-didnt-work ## Description A `customer.subscription.updated` Stripe webhook for account 76162 returned 200 OK but did not persist the new `subscribed_quantity`. Root cause: a race condition between the webhook handler and `increment_response_usage` (Captain usage counter), both doing read-modify-write on the `custom_attributes` JSONB column. The webhook wrote `quantity: 6`, then a concurrent `save` from `increment_response_usage` overwrote the entire hash with stale data — restoring `quantity: 5`. Fix: use atomic `jsonb_set` so usage counter updates only touch the single key they care about, instead of rewriting the whole `custom_attributes` hash. `increment_custom_attribute` also performs the increment in SQL, making concurrent increments correct as well. ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? - New regression spec in `handle_stripe_event_service_spec.rb` that simulates concurrent webhook + `increment_response_usage` and asserts both `subscribed_quantity` and `captain_responses_usage` survive - Existing account, billing, captain, and topup specs all pass locally ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules
This commit is contained in:
committed by
GitHub
parent
45b6ea6b3f
commit
0592cccca9
@@ -16,20 +16,15 @@ module Enterprise::Account::PlanUsageAndLimits # rubocop:disable Metrics/ModuleL
|
||||
end
|
||||
|
||||
def increment_response_usage
|
||||
current_usage = custom_attributes[CAPTAIN_RESPONSES_USAGE].to_i || 0
|
||||
custom_attributes[CAPTAIN_RESPONSES_USAGE] = current_usage + 1
|
||||
save
|
||||
increment_custom_attribute(CAPTAIN_RESPONSES_USAGE)
|
||||
end
|
||||
|
||||
def reset_response_usage
|
||||
custom_attributes[CAPTAIN_RESPONSES_USAGE] = 0
|
||||
save
|
||||
update_custom_attribute(CAPTAIN_RESPONSES_USAGE, 0)
|
||||
end
|
||||
|
||||
def update_document_usage
|
||||
# this will ensure that the document count is always accurate
|
||||
custom_attributes[CAPTAIN_DOCUMENTS_USAGE] = captain_documents.count
|
||||
save
|
||||
update_custom_attribute(CAPTAIN_DOCUMENTS_USAGE, captain_documents.count)
|
||||
end
|
||||
|
||||
def email_transcript_enabled?
|
||||
@@ -130,6 +125,27 @@ module Enterprise::Account::PlanUsageAndLimits # rubocop:disable Metrics/ModuleL
|
||||
ChatwootApp.max_limit
|
||||
end
|
||||
|
||||
# Atomic jsonb_set to avoid clobbering concurrent writes to other custom_attributes keys.
|
||||
# Goes through Account relation (rather than raw connection) so shard routing is respected.
|
||||
# rubocop:disable Rails/SkipsModelValidations
|
||||
def update_custom_attribute(key, value)
|
||||
Account.where(id: id).update_all([
|
||||
"custom_attributes = jsonb_set(COALESCE(custom_attributes, '{}'), ARRAY[:key], :value::jsonb)",
|
||||
{ key: key, value: value.to_json }
|
||||
])
|
||||
custom_attributes[key] = value
|
||||
end
|
||||
|
||||
def increment_custom_attribute(key)
|
||||
Account.where(id: id).update_all([
|
||||
"custom_attributes = jsonb_set(COALESCE(custom_attributes, '{}'), ARRAY[:key], " \
|
||||
'(COALESCE((custom_attributes ->> :key)::int, 0) + 1)::text::jsonb)',
|
||||
{ key: key }
|
||||
])
|
||||
custom_attributes[key] = custom_attributes[key].to_i + 1
|
||||
end
|
||||
# rubocop:enable Rails/SkipsModelValidations
|
||||
|
||||
def validate_limit_keys
|
||||
errors.add(:limits, ': Invalid data') unless self[:limits].is_a? Hash
|
||||
self[:limits] = {} if self[:limits].blank?
|
||||
|
||||
@@ -83,6 +83,37 @@ describe Enterprise::Billing::HandleStripeEventService do
|
||||
end
|
||||
end
|
||||
|
||||
describe 'subscription quantity update' do
|
||||
before do
|
||||
allow(subscription).to receive(:[]).with('plan')
|
||||
.and_return({ 'id' => 'price_startups', 'product' => 'plan_id_startups', 'name' => 'Startups' })
|
||||
end
|
||||
|
||||
it 'updates subscribed_quantity' do
|
||||
allow(subscription).to receive(:[]).with('quantity').and_return(6)
|
||||
|
||||
stripe_event_service.new.perform(event: event)
|
||||
|
||||
expect(account.reload.custom_attributes['subscribed_quantity']).to eq(6)
|
||||
end
|
||||
|
||||
it 'persists quantity even when increment_response_usage runs concurrently' do
|
||||
allow(subscription).to receive(:[]).with('quantity').and_return(6)
|
||||
account.update!(custom_attributes: account.custom_attributes.merge('captain_responses_usage' => 100))
|
||||
|
||||
# Simulate: webhook updates quantity, then a concurrent increment_response_usage writes usage
|
||||
stripe_event_service.new.perform(event: event)
|
||||
account.reload
|
||||
|
||||
# Simulate concurrent increment_response_usage (atomic jsonb_set, not full hash overwrite)
|
||||
account.increment_response_usage
|
||||
|
||||
# Quantity must survive the concurrent usage update
|
||||
expect(account.reload.custom_attributes['subscribed_quantity']).to eq(6)
|
||||
expect(account.reload.custom_attributes['captain_responses_usage']).to eq(101)
|
||||
end
|
||||
end
|
||||
|
||||
describe 'subscription deletion handling' do
|
||||
it 'calls CreateStripeCustomerService on subscription deletion' do
|
||||
allow(event).to receive(:type).and_return('customer.subscription.deleted')
|
||||
|
||||
Reference in New Issue
Block a user