fix: Avoid double updating on the non-recurring jobs vs recurring jobs search in resumeOnRestart

[b0dea]

This is related to my PR created and merged here: #62. See also the question below.

I have realised that in resumeOnRestart, the first updateMany might update both recurring and non-recurring jobs, and then the second one will update non-recurring jobs again. Now, I have split the search completely by adding a strong check when searching non-recurring jobs.

As a separate question @code-xhyun

• do you think we need to add { $expr: { $eq: ['$runCount', '$finishedCount'] } } in the second part of the non-recurrent query search (where we search for not locked and not finished non-recurrent jobs)? As in, to check if runCount is same as finishedCount but there is nolastFinishedAt set for example.

Please take an overall look, I might have missed some other cases.

Thank you.

[b0dea]

@code-xhyun see if my question is relevant. Thank you

[b0dea]

@code-xhyun I think this should be merged in the same 1.6.4 if possible (if not in .5) so we avoid redundant double updates. But let me know what you think. Thanks a lot.

[code-xhyun]

@b0dea Hmm... Regarding your question, I think it would be good to add that part. However, since I haven't been paying much attention to this lately, there might be some cases I missed.

Currently, version 1.6.4 has already been released. This should be included in the 1.6.5 release.

[code-xhyun]

@b0dea And make sure to standardize the commit conventions!

[b0dea]

@b0dea Hmm... Regarding your question, I think it would be good to add that part. However, since I haven't been paying much attention to this lately, there might be some cases I missed.

Currently, version 1.6.4 has already been released. This should be included in the 1.6.5 release.

On this note @code-xhyun - is Pulse going to keep being maintained in the long run? I have been using this in one of my projects since moving to MongoDB's latest drive and I just want to make sure it's ok to keep using it as a long-term solution. Thank you

Also, let's leave for now the check of runCount and finishedCount to avoid more edge-cases. I think the proposed change to avoid double update on non and recurring jobs is enough for now.

[b0dea]

I added some more checks on resumeOnRestart + comment on each case so we know what we do there. From my POV, this PR is ready to be merged whenever you have time also to get a good look on all changes and see if anything else to be added. Thank you @code-xhyun.

[code-xhyun]

@b0dea
I have recently joined a new team as a founder and am working on a new company.
Until this project is completed, I may not be able to contribute directly to coding tasks.
However, I will continue to review PRs and handle tasks like today.
Thank you for your understanding and support!

[code-xhyun]

@b0dea
If you'd like, I can add you as a direct admin to this repository.

[b0dea]

@code-xhyun just discovered into my project, after updating to 1.6.4 that there is an issue (I guess it's out of the changes I made - even if unit tests are passing (added more now) with nextRunAt on recurrent jobs.

I have a recurrent job that's planned to be run each 10 minutes, nextRunAt is set correctly at the start and then, after the first run, nextRunAt gets defaulted to null instead of being computed. I expect the issue is under the computeNextRun function, but I can't find its root.

Can you quickly look also, please? Otherise 1.6.4 might be buggy, even if the unit tests I added are passing.

Thank you

[b0dea]

@b0dea If you'd like, I can add you as a direct admin to this repository.

Thanks for the trust. I am happy to help where I can. However, I am also the founder of a startup - so my coding time is limited.

[code-xhyun]

@b0dea For now, I have invited you as a maintainer. Currently, I’m out of the office for company work and unable to review the code

[b0dea]

@b0dea For now, I have invited you as a maintainer. Currently, I’m out of the office for company work and unable to review the code

Ok. Thank you. Do take a look also when you have time. I am going to look also and let you know (will push in this PR) if I find the root of it.

[b0dea]

@b0dea For now, I have invited you as a maintainer. Currently, I’m out of the office for company work and unable to review the code

Ok. Thank you. Do take a look also when you have time. I am going to look also and let you know (will push in this PR) if I find the root of it.

@code-xhyun ok so the issue is surely under ther default value on nextRunAt in the job model. I have reverted that in this PR. The issue of re-running successful non-recurring jobs will still be there then (at server restart), but I don't have the time to keep looking into it. Please take a look also and we probably have to release it asap in 1.6.5 if this is ok - and keep the original issue (non-recurring jobs gets re-run), but at least we can successfully run the recurring jobs?

[code-xhyun]

🎉 This PR is included in version 1.6.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[b0dea]

🎉 This PR is included in version 1.6.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

Unfortunately, as many unit tests I am doing (and all are passing), in my system when I use the new plugin - due to some reason (and now if we look at changes, only resumeOnRestart is changed basically), nextRunAt is still null. Please look also, no need to code, just let me know what you think we should change.
@code-xhyun