fix(server): dispatch each cron trigger exactly once (#22113)
## Problem
App/logic-function crons occasionally fire **twice, ~1 minute apart**.
The most visible symptom is a notification cron sending the same Discord
DM (or channel post) at e.g. `17:00` and again at `17:01`.
## Root cause
`CronTriggerCronJob` runs every minute (`* * * * *`) and re-dispatches
any logic function whose pattern is "due" according to `shouldRunNow`:
```ts
const diff = Math.abs(prevTriggerDate.getTime() - now.getTime());
return diff < rootCronIntervalMs; // 60_000
```
The detection window (`60_000ms`) is **equal to** the 60s tick interval.
So when a root tick drifts across a minute boundary (runs slightly
early/late, or BullMQ fires a catch-up), two adjacent ticks can both see
the *same* trigger as "within the last 60s" and each enqueue a
`LogicFunctionTriggerJob`. The dispatch isn't idempotent, so the
function runs twice.
## Fix
Make dispatch idempotent, keyed on the trigger itself:
- New `getMatchingTriggerTimestamp(pattern, now)` returns the epoch-ms
of the matched trigger (stable regardless of *when* within the window
the root job runs), or `null`. `shouldRunNow` now delegates to it —
behaviour unchanged.
- Before enqueuing, `CronTriggerCronJob` claims a
`logic-function-cron:{workspace}:{function}:{triggerTs}` key in the
`EngineLock` cache. A second tick that resolves to the same trigger
finds the key and skips.
Distinct triggers always have distinct timestamps (hence distinct keys),
so a later legitimate run is never suppressed. The TTL (2 min) only
needs to outlive the detection window.
## Notes
- `WorkflowCronTriggerCronJob` uses the same `shouldRunNow` pattern and
has the same latent double-dispatch; left out of this PR to keep it
focused, but the new helper makes the same guard a small follow-up.
- The cache `get`-then-`set` isn't atomic; for the observed failure mode
(ticks ~1 min apart, sequential) it's reliable. A Redis `SET NX` would
also close the rare concurrent-multi-instance race.
## Test plan
- [x] `should-run-now.utils.spec.ts` extended: two ticks within one
window resolve to the same timestamp; out-of-window and invalid patterns
return `null`. All 8 pass.
- [x] `oxlint --type-aware` + `oxfmt` clean on changed files.
<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/twentyhq/twenty/pull/22113?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
This commit is contained in:
@@ -0,0 +1,9 @@
|
||||
import { Module } from '@nestjs/common';
|
||||
|
||||
import { CronTriggerDeduplicationService } from 'src/engine/core-modules/cron/services/cron-trigger-deduplication.service';
|
||||
|
||||
@Module({
|
||||
providers: [CronTriggerDeduplicationService],
|
||||
exports: [CronTriggerDeduplicationService],
|
||||
})
|
||||
export class CronModule {}
|
||||
+50
@@ -0,0 +1,50 @@
|
||||
import { Injectable } from '@nestjs/common';
|
||||
|
||||
import { CronExpressionParser } from 'cron-parser';
|
||||
|
||||
import { InjectCacheStorage } from 'src/engine/core-modules/cache-storage/decorators/cache-storage.decorator';
|
||||
import { CacheStorageService } from 'src/engine/core-modules/cache-storage/services/cache-storage.service';
|
||||
import { CacheStorageNamespace } from 'src/engine/core-modules/cache-storage/types/cache-storage-namespace.enum';
|
||||
|
||||
const ROOT_CRON_INTERVAL_MS = 60_000;
|
||||
const CRON_DISPATCH_DEDUP_TTL_MS = 2 * 60_000;
|
||||
|
||||
@Injectable()
|
||||
export class CronTriggerDeduplicationService {
|
||||
constructor(
|
||||
@InjectCacheStorage(CacheStorageNamespace.EngineLock)
|
||||
private readonly cacheStorageService: CacheStorageService,
|
||||
) {}
|
||||
|
||||
async shouldDispatch(
|
||||
keyPrefix: string,
|
||||
pattern: string,
|
||||
now: Date,
|
||||
): Promise<boolean> {
|
||||
let lastTriggerTimestamp: number;
|
||||
|
||||
try {
|
||||
lastTriggerTimestamp = CronExpressionParser.parse(pattern, {
|
||||
currentDate: now,
|
||||
})
|
||||
.prev()
|
||||
.getTime();
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
|
||||
const isDueWithinThisTick =
|
||||
now.getTime() - lastTriggerTimestamp < ROOT_CRON_INTERVAL_MS;
|
||||
|
||||
if (!isDueWithinThisTick) {
|
||||
return false;
|
||||
}
|
||||
|
||||
const dedupKey = `${keyPrefix}:${lastTriggerTimestamp}`;
|
||||
|
||||
return this.cacheStorageService.acquireLock(
|
||||
dedupKey,
|
||||
CRON_DISPATCH_DEDUP_TTL_MS,
|
||||
);
|
||||
}
|
||||
}
|
||||
+2
@@ -2,6 +2,7 @@ import { Module } from '@nestjs/common';
|
||||
import { TypeOrmModule } from '@nestjs/typeorm';
|
||||
|
||||
import { TokenModule } from 'src/engine/core-modules/auth/token/token.module';
|
||||
import { CronModule } from 'src/engine/core-modules/cron/cron.module';
|
||||
import { WorkspaceDomainsModule } from 'src/engine/core-modules/domain/workspace-domains/workspace-domains.module';
|
||||
import { LogicFunctionTriggerJob } from 'src/engine/core-modules/logic-function/logic-function-trigger/jobs/logic-function-trigger.job';
|
||||
import { CronTriggerCronCommand } from 'src/engine/core-modules/logic-function/logic-function-trigger/triggers/cron/cron-trigger.cron.command';
|
||||
@@ -19,6 +20,7 @@ import { WorkspaceCacheModule } from 'src/engine/workspace-cache/workspace-cache
|
||||
TokenModule,
|
||||
WorkspaceDomainsModule,
|
||||
WorkspaceCacheModule,
|
||||
CronModule,
|
||||
],
|
||||
providers: [
|
||||
LogicFunctionTriggerJob,
|
||||
|
||||
+10
-2
@@ -5,6 +5,7 @@ import { isDefined } from 'twenty-shared/utils';
|
||||
import { WorkspaceActivationStatus } from 'twenty-shared/workspace';
|
||||
import { Repository } from 'typeorm';
|
||||
|
||||
import { CronTriggerDeduplicationService } from 'src/engine/core-modules/cron/services/cron-trigger-deduplication.service';
|
||||
import { SentryCronMonitor } from 'src/engine/core-modules/cron/sentry-cron-monitor.decorator';
|
||||
import { ExceptionHandlerService } from 'src/engine/core-modules/exception-handler/exception-handler.service';
|
||||
import { InjectMessageQueue } from 'src/engine/core-modules/message-queue/decorators/message-queue.decorator';
|
||||
@@ -18,7 +19,6 @@ import {
|
||||
LogicFunctionTriggerJobData,
|
||||
} from 'src/engine/core-modules/logic-function/logic-function-trigger/jobs/logic-function-trigger.job';
|
||||
import { WorkspaceCacheService } from 'src/engine/workspace-cache/services/workspace-cache.service';
|
||||
import { shouldRunNow } from 'src/utils/should-run-now.utils';
|
||||
|
||||
export const CRON_TRIGGER_CRON_PATTERN = '* * * * *';
|
||||
|
||||
@@ -33,6 +33,7 @@ export class CronTriggerCronJob {
|
||||
private readonly workspaceRepository: Repository<WorkspaceEntity>,
|
||||
private readonly workspaceCacheService: WorkspaceCacheService,
|
||||
private readonly exceptionHandlerService: ExceptionHandlerService,
|
||||
private readonly cronTriggerDeduplicationService: CronTriggerDeduplicationService,
|
||||
) {}
|
||||
|
||||
@Process(CronTriggerCronJob.name)
|
||||
@@ -73,7 +74,14 @@ export class CronTriggerCronJob {
|
||||
continue;
|
||||
}
|
||||
|
||||
if (!shouldRunNow(cronSettings.pattern, now)) {
|
||||
const shouldDispatch =
|
||||
await this.cronTriggerDeduplicationService.shouldDispatch(
|
||||
`logic-function-cron:${activeWorkspace.id}:${logicFunction.id}`,
|
||||
cronSettings.pattern,
|
||||
now,
|
||||
);
|
||||
|
||||
if (!shouldDispatch) {
|
||||
continue;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user