Fix for missed cronjobs history cleanup

[LukeOl]

This is Magento 1 legacy code, so the same issue exists in Magento 1.

Magento clean up mechanism for scheduled cronjobs has two options:

1. Clean success history after x seconds
2. Clean failure history after y seconds

Success job is the one with status Success.
Failure job is the one with statuses Missed or Error.

Unfortunately, when Magento cleans up job's history, the script will clean jobs with status Missed, ignoring option "Clean success/failure history after some seconds".

Scheduled cronjobs with status Missed do not have executed time field filled. So Missed cronjob will be erased besides of Clean success/failure history after some seconds option.

I propose to use "ScheduledAt" time (because it is always filled when schedule is generated) if job was not executed ("ExecutedAt" field is empty).

Testing scenario for this case:

1. Set cronjob configuration:
   • "Generate Schedules Every" to '15'
   • "Schedule Ahead for" to '20'
   • "Missed if Not Run Within" to '3'
   • "History Cleanup Every" to '10'
   • "Success History Lifetime" to '60'
   • "Failure History Lifetime" to '600'
2. Add job to cron job with interval of * * * * * (every minute)
3. Run ./pub/cron.php in Magento 2 directory (scheduler should generate jobs for new task in every minute)
4. Run ./pub/cron.php after 5 minutes (scheduler will mark some of new jobs as missed)
5. Run ./pub/cron.php after 15 - history will be cleaned up, jobs marked as missed will be erased (while they should not be erased, in my opinion)

[vrann]

Hi, @LukeOl
Thanks for your contribution! It is an interesting case.

The scheduler in Magento 2 marks jobs as "Missed" in case if the time in "ScheduledAt" field had already passed and it happened more than "Missed if Not Run Within" minutes ago. If job marked as "Missed" it can only mean that execution of the job failed. All the jobs marked as "Missed" will never run again and should be deleted after waiting for "Failure History Lifetime" minutes. Looking into Observer class I see that it marks job as “Missed” but never put proper value to executed_at field, so that later job will be deleted immediately, instead of waiting for the time configured for keeping the history. Am I correctly understand the issue?

The problem which I see with the fix is that Observer will wait needed amount of minutes since job was created, not since job was failed. If "Missed if Not Run Within” is long enough in comparison with "Failure History Lifetime” then admin will never get a chance to see the jobs marked as “Missed”. It will be better to update the executed_at field with the current timestamp whenever job status is changed to “missed” or “error”. Does this makes sense? Are you willing to do this change?

Also, I would suggest to add tests to highlight the issue. It is something we ask contributors to do as a part of https://github.com/magento/magento2/wiki/Contribution-Guide

Tracking number MAGETWO-34526

[LukeOl]

Hello, @vrann

Yes, you understand issue correctly.

But I don't think filling execution_at field with current timestamp is correct solution.
Please consider this scenario:

1. Cron was tried to run job, but this job meets "Missed" status criteria ("ScheduledAt" field had already passed and it happened more than "Missed if Not Run Within" minutes ago)
2. Cron set status "Missed" to job, and fills execution_at field with current timestamp
3. Sometime later, Someone is checking cron history and he/she spots jobs marked as "Missed" but it "execution_at" field is filled. So that person might be confused, he/she will not be sure if this job has been executed or missed (and as a example this person is not a developer, who investigate this in code).

That I proposed my way to solve this issue.
To clean proper history we have to rely on time close enough to potential job execution time, and I think this time is saved in scheduled_at field.

About tests: what exactly do you need (unit, other, ?)? Only for this case or overall for Cron? (because I haven't found any tests related to cron yet - if it exists please point my where it are).
Tests might be more difficult part of my pull request, so it might take a bit longer, so I would be grateful for your patience.
I need to consult that with my team.

Thx for your answer & greetings.
Luke

[vrann]

Hi, @LukeOl,

you are right, it is valid concern. However I think it is responsibility of the UI to display this data in the way that is not confusing. For example it may call it “date when it was marked as missed” and it will be clear what it means.

Regardless of that, solution should take into account the time that passed before job was marked as missed. Otherwise this “history lifetime” configuration will be confusing (and this is what I was hoping to fix). Just as an example, it may be changed to something like $checkTime = $record->getScheduledAt()) + $timeWaitingForMissed;

Regarding tests, please take a look at Magento\Cron\Model\ObserverTest::testDispatchCleanup() in the dev/tests/unit/testsuite, it looks like you would need to add another case to test there and design it in the same way as other tests in this class.

Definitely let me know if you cannot make changes now and I will take care of that. This PR is already valuable regardless of will you do any further work on it.

Thanks!

[vrann]

@LukeOl this change was covered with the tests in the internal codebase. We will merge this PR soon. Thanks!

[LukeOl]

Hi @vrann

Sorry for not giving sign of life. I was workloaded and I was unable to answer you, sorry for this.

I am glad of your message about merging my change :)

If there is something about the case, which might be help to you guys, fell free to ask me! I will try give faster feedback since last attempt :)

Thanks!

[sshrewz]

@LukeOl, this has now been published in 0.74.0-beta1. Thank you again for your valuable contribution and for your continued support in Magento! We greatly appreciate it!