-
Notifications
You must be signed in to change notification settings - Fork 5.1k
fix: Prevent Duplicate Cron Jobs for Chatwoot Message Sync #1906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Prevent Duplicate Cron Jobs for Chatwoot Message Sync #1906
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnhances the Chatwoot message sync cron job by assigning a unique identifier via cuid(), caching it in Redis, and validating this identifier at execution time to ensure only the most recent job runs. Sequence diagram for Chatwoot message sync cron job execution with duplicate preventionsequenceDiagram
participant BaileysStartupService
participant RedisCache
participant CronJob
participant ChatwootService
BaileysStartupService->>RedisCache: hSet(cronKey, instanceName, cronId)
BaileysStartupService->>CronJob: schedule sync task
loop On cron job trigger
CronJob->>RedisCache: hGet(cronKey, instanceName)
alt If storedId == cronId
CronJob->>ChatwootService: syncLostMessages()
else If storedId != cronId
CronJob-->>CronJob: Skip execution
end
end
Class diagram for BaileysStartupService and ChatwootService changesclassDiagram
class BaileysStartupService {
+instance: Instance
+chatwootService: ChatwootService
+startChatwootSyncCron()
}
class ChatwootService {
+getCache()
+syncLostMessages(options, config, prepare)
}
class RedisCache {
+hSet(key, field, value)
+hGet(key, field)
}
BaileysStartupService --> ChatwootService
ChatwootService --> RedisCache
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider setting an expiration/TTL on the Redis hash entries to avoid stale cronIds accumulating if instances are removed or renamed.
- It may be more robust to explicitly stop or destroy existing cron tasks on startup rather than only relying on the Redis ID check to prevent overlapping jobs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider setting an expiration/TTL on the Redis hash entries to avoid stale cronIds accumulating if instances are removed or renamed.
- It may be more robust to explicitly stop or destroy existing cron tasks on startup rather than only relying on the Redis ID check to prevent overlapping jobs.
## Individual Comments
### Comment 1
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:4368` </location>
<code_context>
const task = cron.schedule('0,30 * * * *', async () => {
+ // Check ID before executing (only if cache is available)
+ const cache = this.chatwootService.getCache();
+ if (cache) {
+ const storedId = await cache.hGet(cronKey, this.instance.name);
+ if (storedId && storedId !== cronId) return;
+ }
this.chatwootService.syncLostMessages({ instanceName: this.instance.name }, chatwootConfig, prepare);
</code_context>
<issue_to_address>
Returning early in the cron task may mask underlying issues.
Adding a log statement when the task is skipped due to ID mismatch will help with future debugging and monitoring.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (cache) {
const storedId = await cache.hGet(cronKey, this.instance.name);
if (storedId && storedId !== cronId) return;
}
=======
if (cache) {
const storedId = await cache.hGet(cronKey, this.instance.name);
if (storedId && storedId !== cronId) {
this.logger?.info?.(
`[ChatwootSyncLostMessages] Skipping cron task for instance "${this.instance.name}" due to ID mismatch (storedId: ${storedId}, cronId: ${cronId})`
);
return;
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:4362` </location>
<code_context>
+ // Generate ID for this cron task and store in cache
+ const cronId = cuid();
+ const cronKey = `chatwoot:syncLostMessages`;
+ await this.chatwootService.getCache()?.hSet(cronKey, this.instance.name, cronId);
+
</code_context>
<issue_to_address>
Cron key does not include instance name, which may cause collisions.
Since cronKey is a static value, using the same cache across multiple environments or services could result in key collisions. To prevent this, add environment or service-specific identifiers to cronKey.
Suggested implementation:
```typescript
const env = process.env.NODE_ENV || 'development';
const cronKey = `chatwoot:syncLostMessages:${env}:${this.instance.name}`;
await this.chatwootService.getCache()?.hSet(cronKey, this.instance.name, cronId);
```
```typescript
if (cache) {
const env = process.env.NODE_ENV || 'development';
const cronKey = `chatwoot:syncLostMessages:${env}:${this.instance.name}`;
const storedId = await cache.hGet(cronKey, this.instance.name);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (cache) { | ||
| const storedId = await cache.hGet(cronKey, this.instance.name); | ||
| if (storedId && storedId !== cronId) return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Returning early in the cron task may mask underlying issues.
Adding a log statement when the task is skipped due to ID mismatch will help with future debugging and monitoring.
| if (cache) { | |
| const storedId = await cache.hGet(cronKey, this.instance.name); | |
| if (storedId && storedId !== cronId) return; | |
| } | |
| if (cache) { | |
| const storedId = await cache.hGet(cronKey, this.instance.name); | |
| if (storedId && storedId !== cronId) { | |
| this.logger?.info?.( | |
| `[ChatwootSyncLostMessages] Skipping cron task for instance "${this.instance.name}" due to ID mismatch (storedId: ${storedId}, cronId: ${cronId})` | |
| ); | |
| return; | |
| } | |
| } |
7c9f4f9 to
613d486
Compare
Bug Fix: Prevent Duplicate Cron Jobs for Chatwoot Message Sync
Problem: Multiple instances of the same cron job were being created when the syncChatwootLostMessages() method was called repeatedly, leading to duplicate message syncing operations.
Solution: Implemented a unique identifier system using cuid() to ensure only one active cron job per instance:
This ensures that even if multiple cron jobs are accidentally created, only the most recent one will actually execute the sync operation.
Summary by Sourcery
Prevent duplicate cron jobs for Chatwoot message syncing by assigning each job a unique ID stored in Redis and validating it before execution.
Bug Fixes: