Skip to content

Unlock failed cron jobs and set a high "last_checked" value to avoid continous re-check - #10010

Merged
MorrisJobke merged 4 commits into
masterfrom
bugfix/9992/fix-blocking-cron-job
Jul 9, 2018
Merged

Unlock failed cron jobs and set a high "last_checked" value to avoid continous re-check#10010
MorrisJobke merged 4 commits into
masterfrom
bugfix/9992/fix-blocking-cron-job

Conversation

@MorrisJobke

@MorrisJobkeMorrisJobke commented Jun 26, 2018

Copy link
Copy Markdown
Member
  • fixes issue where cronjobs of a not-loaded app are marked as "still running" because there is a "reserved_at" value stored
  • fixesCronjob blocking update (forever) #9992
  • add a repair step that sets the value of all jobs to 0 during upgrade to not run into this issue in the future for fast upgrades

@MorrisJobke

Copy link
Copy Markdown
MemberAuthor

I also added another check in the updater to only wait for cron if the upgrade is from a version after this commit to have the fixed cron jobs already included. It could still cause trouble if there is this cronjob entry, then an upgrade to 14.0.0.7 happens and then directly the next upgrade happens without cron.php to execute to clean up the reserved jobs. Maybe I will add another repair step to reset the "reserved_at" entries.

$job = $this->buildJob($row);

if ($job === null) {
// set the last_checked to 12h in the future to not check failing jobs all over again

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically a non-exiting job or one of a disabled app, not a "failing job"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If execute() fails, it is still blocked for ever

@jancborchardtjancborchardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the update issue #9992 for me

@MorrisJobkeMorrisJobke self-assigned this Jun 28, 2018
@tobiasKaminsky

Copy link
Copy Markdown
Member

I ran into this, and with this PR, I had to manually set maintenance to false and reload the upgrade process.
Now it works fine 👍
(thanks @rullzer for pointing me here)

@MorrisJobkeMorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 2, 2018
@MorrisJobke
MorrisJobkeforce-pushed the bugfix/9992/fix-blocking-cron-job branch from 63dd136 to 009621dCompareJuly 5, 2018 10:14
@codecov

codecovBot commented Jul 5, 2018

Copy link
Copy Markdown

Codecov Report

Merging #10010 into master will increase coverage by 0.55%.
The diff coverage is 70%.

@@ Coverage Diff @@## master #10010 +/- ##
============================================
+ Coverage 51.42% 51.97% +0.55% + Complexity 26413 26019 -394 
============================================
Files 1690 1661 -29 Lines 97716 96181 -1535 Branches 1292 1290 -2 ============================================
- Hits 50248 49990 -258 + Misses 47468 46191 -1277
Impacted FilesCoverage ΔComplexity Δ
version.php0% <0%> (ø)0 <0> (ø)⬇️
lib/private/Updater.php5.29% <0%> (-0.02%)121 <0> (+1)
lib/private/BackgroundJob/JobList.php74.05% <100%> (+1.2%)31 <0> (ø)⬇️
core/Command/Maintenance/Mode.php0% <0%> (-83.79%)6% <0%> (-2%)
apps/dav/lib/CalDAV/CalendarRoot.php0% <0%> (-75%)1% <0%> (-3%)
apps/user_ldap/lib/User/User.php75.25% <0%> (-2.75%)123% <0%> (-6%)
apps/user_ldap/lib/User_LDAP.php77.61% <0%> (-1.95%)84% <0%> (ø)
apps/files/lib/Command/Scan.php22.04% <0%> (-1.8%)51% <0%> (-2%)
apps/files_trashbin/lib/Expiration.php90.32% <0%> (-1.62%)29% <0%> (ø)
...ederatedfilesharing/lib/FederatedShareProvider.php67.28% <0%> (-1.12%)84% <0%> (+2%)
... and 99 more

@MorrisJobke
MorrisJobkeforce-pushed the bugfix/9992/fix-blocking-cron-job branch from 009621d to b7ce899CompareJuly 5, 2018 10:28
@MorrisJobkeMorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 5, 2018
@MorrisJobkeMorrisJobke removed their assignment Jul 5, 2018
@MorrisJobke

Copy link
Copy Markdown
MemberAuthor

Ready for review. I added a repair step and it worked over here.

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the trick for me.
I vote to merge as it at least improves the situation.

…continous re-check
* fixes issue where cronjobs of a not-loaded app are marked as "still running" because there is a "reserved_at" value stored
* fixed#9992
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
* see #9992
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
MorrisJobkeforce-pushed the bugfix/9992/fix-blocking-cron-job branch from b7ce899 to 79801adCompareJuly 9, 2018 12:55
@MorrisJobke

Copy link
Copy Markdown
MemberAuthor

I just updated the version again as master had it already and added two times:

* @suppress SqlInjectionChecker

because there were two false positives.

@MorrisJobkeMorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 9, 2018
@MorrisJobke
MorrisJobke merged commit ed6352a into masterJul 9, 2018
@MorrisJobke
MorrisJobke deleted the bugfix/9992/fix-blocking-cron-job branch July 9, 2018 13:49
@MorrisJobkeMorrisJobke mentioned this pull request Aug 31, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: install and update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cronjob blocking update (forever)

5 participants

@MorrisJobke@tobiasKaminsky@rullzer@nickvergessen@jancborchardt