Skip to content
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

Flows cron triggers multiple times #15052

Closed
stoberbryan opened this issue Aug 12, 2022 · 16 comments · Fixed by #18584
Closed

Flows cron triggers multiple times #15052

stoberbryan opened this issue Aug 12, 2022 · 16 comments · Fixed by #18584
Assignees
Labels

Comments

@stoberbryan
Copy link

Describe the Bug

Flows Trigger 3 times no matter what.
Created New Flow to validate:
Only the Create Data functionality causes it.
Emits Events On and Off Does Not Affect it

To Reproduce

  1. Create Flow
  2. Create Data Step
  3. Email Yourself

You will get 3 emails and items

Errors Shown

No response

What version of Directus are you using?

9.15.1

What version of Node.js are you using?

16.16.0

What database are you using?

cloud pg

What browser are you using?

chrome

How are you deploying Directus?

cloud

@stoberbryan
Copy link
Author

I forgot to add that it starts with a cron step
if i skip create data, i get 1 email

@rijkvanzanten
Copy link
Member

This is a known limitation of the cron setup currently, as multiple instances of directus aren't aware of each other, so each instance of directus will execute the cron job simultaneously.

I'll leave this open as a bug, as it's already a thing we're actively working on 👍🏻

@licitdev licitdev changed the title Flows Trigger 3 Times Flows cron triggers multiple times Aug 12, 2022
@stoberbryan
Copy link
Author

Thank you!

@azrikahar
Copy link
Contributor

Ref #13381 which is the primary discussion on solutions for this known limitation.

@rijkvanzanten rijkvanzanten added this to the v10.0.0 milestone Aug 19, 2022
@rijkvanzanten
Copy link
Member

We should generate a redis key for flow ID + the timestamp the cron is supposed to run (not exact current timestamp) with a TTL of like 3 seconds. We can run a redis increment call on that key, if it returns 1, the current instance is the one to run the flow, otherwise, ignore it.

@rijkvanzanten
Copy link
Member

Linear: ENG-209

@colemahatmccreary
Copy link

Is there any update here?

I'm running into race conditions in my app that I can't seem to get around without solving this.

Thanks!

@adrs0u
Copy link

adrs0u commented Mar 13, 2023

Hello! directus support refers us here! The problem also (still) occurs in the directus Cloud version.
The cron is triggered twice.

1

2

It would be good if this works, otherwise you can not use cron scheduling
Thank you!

@macntech
Copy link

We are also highly interested into an update of the fix. We running into the same issue on a scaled system. Thanks!

@metalmarco
Copy link

We have the same issue on a single instance installation (no Directus Cloud)

Screenshot 2023-03-17 alle 09 39 24

@metaaifactory
Copy link

Same here :/
Capture

Do you have any news on a fix ?

@rijkvanzanten rijkvanzanten removed this from the v10.0.0 milestone Apr 5, 2023
@nickrum nickrum self-assigned this Apr 11, 2023
@BrianJM
Copy link

BrianJM commented May 5, 2023

Is there a workaround?

Is it possible to isolate cron tasks to a single instance? For example,

  • Single instance for cron tasks and flows
  • HA cluster allocated for UI

@nickrum
Copy link
Member

nickrum commented May 7, 2023

We had a little discussion about the best fix for this issue and the solution we came up with is to use a "Synchronized Clock" mechanism via redis.

The way this mechanism should work is as follows:

The supposed next execution time of each flow with a schedule trigger is stored in redis with the UUID (and some prefix like directus:clock:) as the key and the execution time as a timestamp with seconds-precision as the value. We only need seconds-precision because cron only supports intervals down to seconds. We can compute the supposed next execution time from the cron instruction and the current time because each cron instruction clearly specifies at which exact second it should be executed next.
Whenever a schedule flow is triggered on one Directus instance, it checks if the next supposed execution time based on the current time is greater than the value stored in redis. If it is, then the value in redis is updated with the value computed by the Directus instance and the instance executes the flow. If it is not, then the value in redis is neither updated, nor is the flow executed by the instance. It is important that this "check and update if greater than"-operation is performed atomically by redis. This ensures that there are no race conditions where one instance determines that its execution time is greater than the value in redis and another instance also determines that its execution time is greater than the value in redis before the first instance could update the value.

This mechanism ensures that, provided the internal clocks of all instances do not deviate by more than one second, for each supposed execution time of a schedule flow, the flow is executed only by the instance that accesses the value in redis first.
This mechanism doesn't take the case into consideration that a Directus instance updates the execution time in redis and crashes before it can finish executing the flow. This is a tradeof we accept because Directus can not know whether it is save to re-execute a flow that has been executed up until a certain point and there is currently no way for the user to specify this.

@paescuj paescuj self-assigned this May 8, 2023
@u12206050
Copy link
Contributor

@rijkvanzanten the current cache (Keyv) doesn't support atomic updates correct? So are you switching it out or communicating with redis more directly.

Also, I wonder if having to need redis for this to work is viable, vs just having a directus_queue table in the database like laravel has?

@rijkvanzanten
Copy link
Member

@rijkvanzanten the current cache (Keyv) doesn't support atomic updates correct? So are you switching it out or communicating with redis more directly.

Keyv just uses Redis under the hood. Redis is single-threaded, so every update is atomic by default 🙂

Also, I wonder if having to need redis for this to work is viable, vs just having a directus_queue table in the database like laravel has?

Writes to a database aren't reliably atomic or timing-error-free like Redis would be for this use case, so I'm hesitant to go that route. Besides, we're already requiring Redis when you're in a horizontally scaling environment as we're relying on it's pubsub for websockets among other things 👍🏻

@BrianJM
Copy link

BrianJM commented May 11, 2023

Also, I wonder if having to need redis for this to work is viable, vs just having a directus_queue table in the database like laravel has?

Writes to a database aren't reliably atomic or timing-error-free like Redis would be for this use case, so I'm hesitant to go that route. Besides, we're already requiring Redis when you're in a horizontally scaling environment as we're relying on it's pubsub for websockets among other things 👍🏻

👍 for redis.

Using relational tables will eventually result in table bloat, especially under heavy use, which will need to be monitored and managed; even so, tables may need to be rotated occasionally.

@rijkvanzanten rijkvanzanten assigned nickrum and unassigned paescuj May 24, 2023
rijkvanzanten pushed a commit that referenced this issue May 24, 2023
…flows and hooks (#18584)

* Add synchronization to schedule flows and hooks

Fixes #15052

* Add changeset

* Add test

* Add to sequential list

* Fix spelling in changeset

---------

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Co-authored-by: ian <licitdev@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project