-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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] Apps-Engine's scheduler failing to update run tasks #22882
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sampaiodiego
previously requested changes
Aug 9, 2021
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.
this change is breaking some endpoints, can you please take a look?
This reverts commit d471479.
d-gubert
changed the title
[FIX] Apps Engine's scheduler failing to update run tasks
[FIX] Apps-Engine's scheduler failing to update run tasks
Aug 9, 2021
d-gubert
dismissed
sampaiodiego’s stale review
August 11, 2021 20:23
Approach has changed since review
sampaiodiego
approved these changes
Aug 13, 2021
sampaiodiego
pushed a commit
that referenced
this pull request
Aug 16, 2021
Co-authored-by: Douglas Gubert <douglas.gubert@gmail.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Agenda, the library that manages scheduling, depended on setting a job property named
nextRunAt
asundefined
to signal whether it should be run on schedule or not. Rocket.Chat's current Mongo driver ignoresundefined
values when updating documents and this was causing jobs to never stop running as Agenda couldn't clear that property (set them asundefined
).This updates Rocket.Chat's dependency on Agenda.js to point to a fork that fixes the problem.
Issue(s)
Steps to test or reproduce
Starting from Rocket.Chat@3.17.0:
Install this example app. At startup, it creates a job that should run only once 10 seconds after it is registered.
schedule-once_0.0.1.zip
It runs every 5 seconds (minimum amount of time allowed by Agenda) indefinitely.
Further comments
First iteration:
Make mongo driver's configuration about bson types explicit so
undefined
fields are recognized and not dropped by the query parser. The issue started happening after Meteor's 2.2 update.Agenda, the Apps Engine's dependency that manages apps scheduling, needs this feature as it depends on setting certain values as
undefined
when updating documents. For the case of tasks that run only once, the propertynextRunAt
, that determines if a job must be run in the future, is never set to undefined, thus making the scheduler run it indefinitely.Somewhere down the dependency tree some function is ignoring or changing this value. As of right now we couldn't identify where, but it prevented updates with fields having
undefined
as their values. The problem is solved when we set this mongodb property explicitly.Related documentation:
Node.js MongoDB Driver API: https://mongodb.github.io/node-mongodb-native/3.6/api/Db.html
Second iteration (current):
Fix agenda.js itself and update Rocket.Chat's dependency to the fixed version.