Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions lib/private/BackgroundJob/JobList.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Psr\Log\LoggerInterface;
use function get_class;
use function json_encode;
use function min;
use function strlen;

class JobList implements IJobList {
Expand Down Expand Up @@ -209,6 +210,26 @@ public function getNext(bool $onlyTimeSensitive = false, ?array $jobClasses = nu
return $this->getNext($onlyTimeSensitive, $jobClasses);
}

if ($job instanceof \OCP\BackgroundJob\TimedJob) {
$now = $this->timeFactory->getTime();
$nextPossibleRun = $job->getLastRun() + $job->getInterval();
if ($now < $nextPossibleRun) {
// This job is not ready for execution yet. Set timestamps to the future to avoid
// 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.

$updateTimedJob = $this->connection->getQueryBuilder();
$updateTimedJob->update('jobs')
->set('last_checked', $updateTimedJob->createNamedParameter($nextCheck, IQueryBuilder::PARAM_INT))
->where($updateTimedJob->expr()->eq('id', $updateTimedJob->createParameter('jobid')));
$updateTimedJob->setParameter('jobid', $row['id']);
$updateTimedJob->executeStatement();

return $this->getNext($onlyTimeSensitive, $jobClasses);
}
}

$update = $this->connection->getQueryBuilder();
$update->update('jobs')
->set('reserved_at', $update->createNamedParameter($this->timeFactory->getTime()))
Expand Down
9 changes: 9 additions & 0 deletions lib/public/BackgroundJob/TimedJob.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ public function setInterval(int $seconds) {
$this->interval = $seconds;
}

/**
* Get the interval [seconds] for the job
*
* @since 32.0.0
*/
public function getInterval(): int {
return $this->interval;
}

/**
* Whether the background job is time sensitive and needs to run soon after
* the scheduled interval, of if it is okay to be delayed until a later time.
Expand Down
29 changes: 26 additions & 3 deletions tests/lib/BackgroundJob/JobListTest.php
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
Expand Down Expand Up @@ -153,7 +156,11 @@ public function testHasDifferentArgument($argument): void {
$this->assertFalse($this->instance->has($job, 10));
}

protected function createTempJob($class, $argument, $reservedTime = 0, $lastChecked = 0) {
protected function createTempJob($class,
$argument,
int $reservedTime = 0,
int $lastChecked = 0,
int $lastRun = 0): int {
if ($lastChecked === 0) {
$lastChecked = time();
}
Expand All @@ -163,11 +170,12 @@ protected function createTempJob($class, $argument, $reservedTime = 0, $lastChec
->values([
'class' => $query->createNamedParameter($class),
'argument' => $query->createNamedParameter($argument),
'last_run' => $query->createNamedParameter(0, IQueryBuilder::PARAM_INT),
'last_run' => $query->createNamedParameter($lastRun, IQueryBuilder::PARAM_INT),
'last_checked' => $query->createNamedParameter($lastChecked, IQueryBuilder::PARAM_INT),
'reserved_at' => $query->createNamedParameter($reservedTime, IQueryBuilder::PARAM_INT),
]);
$query->execute();
$query->executeStatement();
return $query->getLastInsertId();
}

public function testGetNext(): void {
Expand Down Expand Up @@ -200,6 +208,21 @@ public function testGetNextSkipReserved(): void {
$this->assertEquals(2, $nextJob->getArgument());
}

public function testGetNextSkipTimed(): void {
$job = new TestTimedJobNew($this->timeFactory);
$jobId = $this->createTempJob(get_class($job), 1, 123456789, 12345, 123456789 - 5);
$this->timeFactory->expects(self::atLeastOnce())
->method('getTime')
->willReturn(123456789);

$nextJob = $this->instance->getNext();

self::assertNull($nextJob);
$job = $this->instance->getById($jobId);
self::assertInstanceOf(TestTimedJobNew::class, $job);
self::assertEquals(123456789 - 5, $job->getLastRun());
}

public function testGetNextSkipNonExisting(): void {
$job = new TestJob();
$this->createTempJob('\OC\Non\Existing\Class', 1, 0, 12345);
Expand Down
26 changes: 26 additions & 0 deletions tests/lib/BackgroundJob/TestTimedJobNew.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-only
*/

namespace Test\BackgroundJob;

use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\TimedJob;

class TestTimedJobNew extends TimedJob {
public bool $ran = false;

public function __construct(ITimeFactory $timeFactory) {
parent::__construct($timeFactory);
$this->setInterval(10);
}

public function run($argument) {
$this->ran = true;
}
}
14 changes: 0 additions & 14 deletions tests/lib/BackgroundJob/TimedJobTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,6 @@
namespace Test\BackgroundJob;

use OCP\AppFramework\Utility\ITimeFactory;
use OCP\BackgroundJob\TimedJob;

class TestTimedJobNew extends TimedJob {
public bool $ran = false;

public function __construct(ITimeFactory $timeFactory) {
parent::__construct($timeFactory);
$this->setInterval(10);
}

public function run($argument) {
$this->ran = true;
}
}

class TimedJobTest extends \Test\TestCase {
private DummyJobList $jobList;
Expand Down
Loading