Skip to content

feat: locking and retry in FB message parsing#7701

Merged
scmmishra merged 26 commits intodevelopfrom
feature/cw-2335
Aug 23, 2023
Merged

feat: locking and retry in FB message parsing#7701
scmmishra merged 26 commits intodevelopfrom
feature/cw-2335

Conversation

@scmmishra
Copy link
Member

@scmmishra scmmishra commented Aug 10, 2023

This PR adds a Redis based lock for Facebook message processing to avoid race conditions/concurrent processing of messages from the same sender.

This PR adds the Redis::LockManager, it provides a simple mechanism to handle distributed locks using Redis. example usage

lock_manager = Redis::LockManager.new

if lock_manager.lock("some_key")
  # Critical code that should not be run concurrently
  lock_manager.unlock("some_key")
end

This also adds a new namespace pool called $sherlock, our key solver. This is added to ensure that locks don't collide anywhere.

The LockManager is used in Webhooks::FacebookEventsJob, which uses the [retry_on](# https://edgeapi.rubyonrails.org/classes/ActiveJob/Exceptions/ClassMethods.html#method-i-retry_on) method to retry the message creation at least 5 times, with a 10 second backoff.

TODO

  • Tests for LockMangager
  • Tests for Webhooks::FacebookEventsJob retry mechanism
  • Figure out better timeouts for retry and lock duration

@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for chatwoot-storybook canceled.

Name Link
🔨 Latest commit 94113df
🔍 Latest deploy log https://app.netlify.com/sites/chatwoot-storybook/deploys/64e2ffc6de62c50008ea1f46

@scmmishra scmmishra changed the title feat: add naive retry in conversation fetching feat: add redis based locking and retry in FB message parsing Aug 14, 2023
@scmmishra scmmishra changed the title feat: add redis based locking and retry in FB message parsing feat: locking and retry in FB message parsing Aug 14, 2023
Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

LGTM overall. some minor comments on code organisation.

@scmmishra scmmishra marked this pull request as ready for review August 15, 2023 08:26
@scmmishra
Copy link
Member Author

@sojan-official LMK your thoughts on the key used to acquire the lock? Also about the lock timings, I was thinking we can increase the lock timeout to about 30 seconds

Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

LGTM overall .
Some minor comments. let's have this added for whatsapp messages as well as a follow up

ref: #5433

@scmmishra
Copy link
Member Author

@sojan-official tests are all done, just need to take a look at the key used for locking and we're good to go

@sojan-official
Copy link
Member

The code looks good to me. Let's test this on staging and then get it merged.

@scmmishra scmmishra temporarily deployed to staging-chatwoot August 18, 2023 10:29 Inactive
@scmmishra
Copy link
Member Author

@sojan-official tested this on staging, working fine. We're good to merge this

@scmmishra
Copy link
Member Author

@sojan-official the breaking test is because of a security audit. This PR solves it #7765

@scmmishra scmmishra merged commit 26ef21a into develop Aug 23, 2023
@scmmishra scmmishra deleted the feature/cw-2335 branch August 23, 2023 02:48
@sentry
Copy link

sentry bot commented Aug 23, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Webhooks::FacebookEventsJob::LockAcquisitionError: Failed to acquire lock for key: FB_MESSAGE_CREATE_LOCK::182015432606460::9739988746043421 (Webhoo... Sidekiq/Webhooks::FacebookEventsJob View Issue

Did you find this useful? React with a 👍 or 👎

rutvijmehta-harness pushed a commit to rutvijmehta-harness/chatwoot that referenced this pull request Aug 30, 2023
rutvijmehta-harness added a commit to rutvijmehta-harness/chatwoot that referenced this pull request Aug 30, 2023
Co-authored-by: Shivam Mishra <scm.mymail@gmail.com>
rutvijmehta-harness pushed a commit to rutvijmehta-harness/chatwoot that referenced this pull request Aug 31, 2023
rutvijmehta-harness added a commit to rutvijmehta-harness/chatwoot that referenced this pull request Aug 31, 2023
Co-authored-by: Shivam Mishra <scm.mymail@gmail.com>
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments