Skip to content

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Feb 12, 2025

  • Resolves: #

Summary

Right now timed jobs are always loaded from the db and instantiated. Jobs with intervals higher than the cron interval are continuously loaded and checked, \OCP\BackgroundJob\TimedJob::start prevents the actual execution.
As an optimization we can preemptively check if a job is ready. Otherwise we park it for a later check.

How to test

  1. Set breakpoints in timed jobs constructors
  2. Observe how the jobs are instantiated once for the initial check and only every instantiated again when they are ready or two days have passed.

TODO

  • Make the changes
  • Manually test the changes
    • Jobs that are ready are executed like before
    • Jobs are parked as expected
    • Once the parked timestamp elapsed, jobs run again like before

Checklist

@marcelklehr
Copy link
Member

Tests might be nice for this, and it's not hard, the test infra for bg jobs is quite nice, imho: https://github.com/nextcloud/server/blob/master/tests/lib/BackgroundJob/JobListTest.php

// re-checking with every cron run.
// To avoid bugs that lead to jobs never executing again, the future timestamp is
// capped at two days.
$nextCheck = min($nextPossibleRun, $now + 48 * 3600);
Copy link
Member

@marcelklehr marcelklehr Feb 13, 2025

Choose a reason for hiding this comment

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

Do you have more details on why the 48 hours?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that someone might set a (dynamic) interval that is too high so that a job is never checked again. This gives the job a chance to recover.
For any jobs that run rarely (like every two weeks) and disabled apps, that would also help clear up the row earlier.

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Is it not possible to add a condition on lastrun+interval as part of the SQL query instead?

@ChristophWurst
Copy link
Member Author

Is it not possible to add a condition on lastrun+interval as part of the SQL query instead?

interval only exists in memory of the PHP process and is not materialized to the oc_jobs table.

image

^ I can't tell which rows represent a timed job, neither do I know the interval.

@susnux susnux added this to the Nextcloud 32 milestone Mar 2, 2025
@ChristophWurst ChristophWurst requested a review from a team as a code owner March 18, 2025 13:11
@ChristophWurst ChristophWurst requested review from artonge, nfebe and skjnldsv and removed request for a team March 18, 2025 13:11
@ChristophWurst ChristophWurst force-pushed the perf/cron/delay-timedjob-checking branch from 3fa3611 to 8bc9498 Compare March 31, 2025 09:55
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the perf/cron/delay-timedjob-checking branch from 8bc9498 to 2395526 Compare March 31, 2025 11:21
@ChristophWurst ChristophWurst merged commit e655ffe into master Mar 31, 2025
199 of 201 checks passed
@ChristophWurst ChristophWurst deleted the perf/cron/delay-timedjob-checking branch March 31, 2025 15:31
@ChristophWurst
Copy link
Member Author

/backport to stable31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants