From 202f87552fa11343e7142edba84b1abb95ec37e6 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Wed, 21 Jun 2023 17:17:56 +0545 Subject: [PATCH 1/4] Display and delete invalid background jobs --- core/Command/Background/Queue/Delete.php | 3 +- core/Command/Background/Queue/Status.php | 41 ++++++++++++++++------- lib/private/BackgroundJob/JobList.php | 42 ++++++++++++++++++++++++ lib/public/BackgroundJob/IJobList.php | 17 +++++++++- 4 files changed, 88 insertions(+), 15 deletions(-) diff --git a/core/Command/Background/Queue/Delete.php b/core/Command/Background/Queue/Delete.php index 988c9b36b5c1..e0c35bdb2769 100644 --- a/core/Command/Background/Queue/Delete.php +++ b/core/Command/Background/Queue/Delete.php @@ -51,8 +51,7 @@ protected function configure() { protected function execute(InputInterface $input, OutputInterface $output): int { $id = $input->getArgument('Job ID'); - $job = $this->jobList->getById($id); - if ($job === null) { + if (!$this->jobList->jobIdExists($id)) { $output->writeln("Job with ID <$id> is not known."); return 1; } diff --git a/core/Command/Background/Queue/Status.php b/core/Command/Background/Queue/Status.php index 6ca17177cc5d..86faf1239a57 100644 --- a/core/Command/Background/Queue/Status.php +++ b/core/Command/Background/Queue/Status.php @@ -26,6 +26,7 @@ use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Helper\Table; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; class Status extends Command { @@ -40,7 +41,8 @@ public function __construct(IJobList $jobList) { protected function configure() { $this ->setName('background:queue:status') - ->setDescription('List queue status'); + ->setDescription('List queue status') + ->addOption('display-invalid-jobs', null, InputOption::VALUE_NONE, 'Display jobs that are no longer valid'); } private function getJobArgumentAsString($argument) { @@ -58,17 +60,32 @@ private function getJobArgumentAsString($argument) { protected function execute(InputInterface $input, OutputInterface $output): int { $t = new Table($output); $t->setHeaders(['Job ID', 'Job', 'Job Arguments', 'Last Run', 'Last Checked', 'Reserved At', 'Execution Duration (s)']); - $this->jobList->listJobs(function (IJob $job) use ($t) { - $t->addRow([ - $job->getId(), - \get_class($job), - $this->getJobArgumentAsString($job->getArgument()), - $job->getLastRun() == 0 ? 'N/A' : \date('c', $job->getLastRun()), - \date('c', $job->getLastChecked()), - $job->getReservedAt() == 0 ? 'N/A' : \date('c', $job->getReservedAt()), - $job->getExecutionDuration() == -1 ? 'N/A' : $job->getExecutionDuration(), - ]); - }); + if ($input->getOption('display-invalid-jobs')) { + $invalidJobs = $this->jobList->listInvalidJobs(); + foreach ($invalidJobs as $invalidJob) { + $t->addRow([ + $invalidJob['id'], + $invalidJob['class'], + $invalidJob['argument'], + $invalidJob['last_run'], + $invalidJob['last_checked'], + $invalidJob['reserved_at'], + $invalidJob['execution_duration'], + ]); + } + } else { + $this->jobList->listJobs(function (IJob $job) use ($t) { + $t->addRow([ + $job->getId(), + \get_class($job), + $this->getJobArgumentAsString($job->getArgument()), + $job->getLastRun() == 0 ? 'N/A' : \date('c', $job->getLastRun()), + \date('c', $job->getLastChecked()), + $job->getReservedAt() == 0 ? 'N/A' : \date('c', $job->getReservedAt()), + $job->getExecutionDuration() == -1 ? 'N/A' : $job->getExecutionDuration(), + ]); + }); + } $t->render(); return 0; } diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 28fa229b1b41..5dbfde3dfb74 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -240,6 +240,26 @@ public function getById($id) { } } + /** + * @param int $id + * @return bool + */ + public function jobIdExists($id) { + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs') + ->where($query->expr()->eq('id', $query->createNamedParameter($id, IQueryBuilder::PARAM_INT))); + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + if ($row) { + return true; + } else { + return false; + } + } + /** * get the job object from a row in the db * @@ -350,4 +370,26 @@ public function listJobs(\Closure $callback) { } $result->closeCursor(); } + + public function listInvalidJobs(): array { + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs'); + $result = $query->execute(); + $jobData = []; + + while ($row = $result->fetch()) { + try { + // Try to load the job as a service + \OC::$server->query($row['class']); + } catch (QueryException $e) { + if (!\class_exists($row['class'])) { + // job is from a disabled app or old version of an app + // so return the data about it + $jobData[] = $row; + } + } + } + return $jobData; + } } diff --git a/lib/public/BackgroundJob/IJobList.php b/lib/public/BackgroundJob/IJobList.php index 5b5175a7312c..307badcdb6b9 100644 --- a/lib/public/BackgroundJob/IJobList.php +++ b/lib/public/BackgroundJob/IJobList.php @@ -84,6 +84,13 @@ public function getNext(); */ public function getById($id); + /** + * @param int $id + * @return bool + * @since 10.13.0 + */ + public function jobIdExists($id); + /** * set the job that was last ran to the current time * @@ -129,13 +136,21 @@ public function setLastRun($job); public function setExecutionTime($job, $timeTaken); /** - * iterate over all jobs in the queue + * iterate over all valid jobs in the queue * * @return void * @since 10.2.0 */ public function listJobs(\Closure $callback); + /** + * get the details of all invalid jobs in the queue + * + * @return array + * @since 10.13.0 + */ + public function listInvalidJobs(); + /** * remove a specific job by id * @return void From 420acd41e8e10551252bea5b23c7ee9a48ff8852 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Wed, 21 Jun 2023 19:18:17 +0545 Subject: [PATCH 2/4] Unit tests for background queue command changes --- .../Command/Background/Queue/DeleteTest.php | 4 +-- .../Command/Background/Queue/StatusTest.php | 28 +++++++++++++++++++ tests/lib/BackgroundJob/JobListTest.php | 16 +++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/tests/Core/Command/Background/Queue/DeleteTest.php b/tests/Core/Command/Background/Queue/DeleteTest.php index c37d9578a4e2..9e616db64c0d 100644 --- a/tests/Core/Command/Background/Queue/DeleteTest.php +++ b/tests/Core/Command/Background/Queue/DeleteTest.php @@ -41,9 +41,9 @@ public function setUp(): void { parent::setUp(); $this->jobList = $this->createMock(IJobList::class); - $this->jobList->expects($this->any())->method('getById') + $this->jobList->expects($this->any())->method('jobIdExists') ->willReturnCallback(function ($id) { - return ($id !== '666') ? true : null; + return ($id !== '666') ? true : false; }); $command = new Delete($this->jobList); diff --git a/tests/Core/Command/Background/Queue/StatusTest.php b/tests/Core/Command/Background/Queue/StatusTest.php index 92a0607d94d3..7033269fa766 100644 --- a/tests/Core/Command/Background/Queue/StatusTest.php +++ b/tests/Core/Command/Background/Queue/StatusTest.php @@ -116,6 +116,34 @@ public function testJobWithSchedulingInfo() { +--------+------------------------------------+-----------------------+---------------------------+---------------------------+---------------------------+------------------------+ | 666 | OC\BackgroundJob\Legacy\RegularJob | {"k":"v","0":"test2"} | 1970-01-01T00:00:10+00:00 | 1970-01-01T00:00:10+00:00 | 1970-01-01T00:00:10+00:00 | 1 | +--------+------------------------------------+-----------------------+---------------------------+---------------------------+---------------------------+------------------------+ +EOS; + + $this->assertStringContainsString($expected, $output); + } + + public function testListingInvalidJob() { + $this->jobList->expects($this->any())->method('listInvalidJobs') + ->willReturn([ + [ + 'id' => '42', + 'class' => 'OC\BackgroundJob\Legacy\RegularJob', + 'argument' => '{"k":"v"}', + 'last_run' => '2023-01-01T00:00:10+00:00', + 'last_checked' => '2023-06-01T00:00:40+00:00', + 'reserved_at' => '2023-06-02T00:00:40+00:00', + 'execution_duration' => 7, + ] + ]); + $this->commandTester->execute( + ['--display-invalid-jobs' => null] + ); + $output = $this->commandTester->getDisplay(); + $expected = <<assertStringContainsString($expected, $output); diff --git a/tests/lib/BackgroundJob/JobListTest.php b/tests/lib/BackgroundJob/JobListTest.php index 74e07014cdf0..8ab6b24a23ba 100644 --- a/tests/lib/BackgroundJob/JobListTest.php +++ b/tests/lib/BackgroundJob/JobListTest.php @@ -125,6 +125,22 @@ public function testRemoveDifferentArgument($argument) { $this->assertEquals($existingJobs, $jobs); } + /** + * @dataProvider argumentProvider + * @param $argument + */ + public function testJobIdExists($argument) { + $this->timeFactory->method('getTime')->willReturn(164419800); + $job = new TestJob(); + $this->instance->add($job, $argument); + $jobs = $this->getAllSorted(); + $addedJob = $jobs[\count($jobs) - 1]; + $addedJobId = $addedJob->getId(); + $this->assertTrue($this->instance->jobIdExists($addedJobId)); + $this->instance->remove($job, $argument); + $this->assertFalse($this->instance->jobIdExists($addedJobId)); + } + /** * @dataProvider argumentProvider * @param $argument From dc1cbec226de22d53f966ce425cb6f5a324f3baf Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Wed, 21 Jun 2023 21:22:27 +0545 Subject: [PATCH 3/4] changelog for display and delete invalid background jobs --- changelog/unreleased/40846 | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 changelog/unreleased/40846 diff --git a/changelog/unreleased/40846 b/changelog/unreleased/40846 new file mode 100644 index 000000000000..b934bfdeabe3 --- /dev/null +++ b/changelog/unreleased/40846 @@ -0,0 +1,13 @@ +Change: display and delete invalid background jobs + +Background jobs can be no longer valid because they are from an old version of +an app, or from an app that has been disabled. These jobs can now be listed +with the command: + +occ background:queue:status --display-invalid-jobs + +And can be deleted with the command: + +occ background:queue:delete + +https://github.com/owncloud/core/pull/40846 From d08dffb2b5ed0a7b6c932cd77bed964bac3b6a78 Mon Sep 17 00:00:00 2001 From: Phil Davis Date: Thu, 22 Jun 2023 16:25:32 +0545 Subject: [PATCH 4/4] Refactor background:queue:status --display-only-invalid-jobs --- core/Command/Background/Queue/Status.php | 26 +++------- lib/private/BackgroundJob/JobList.php | 50 ++++++++----------- lib/public/BackgroundJob/IJobList.php | 12 +---- .../Command/Background/Queue/StatusTest.php | 43 ++++++++-------- 4 files changed, 54 insertions(+), 77 deletions(-) diff --git a/core/Command/Background/Queue/Status.php b/core/Command/Background/Queue/Status.php index 86faf1239a57..17b0aa89ef23 100644 --- a/core/Command/Background/Queue/Status.php +++ b/core/Command/Background/Queue/Status.php @@ -42,7 +42,7 @@ protected function configure() { $this ->setName('background:queue:status') ->setDescription('List queue status') - ->addOption('display-invalid-jobs', null, InputOption::VALUE_NONE, 'Display jobs that are no longer valid'); + ->addOption('display-only-invalid-jobs', null, InputOption::VALUE_NONE, 'Only display jobs that are no longer valid'); } private function getJobArgumentAsString($argument) { @@ -60,32 +60,20 @@ private function getJobArgumentAsString($argument) { protected function execute(InputInterface $input, OutputInterface $output): int { $t = new Table($output); $t->setHeaders(['Job ID', 'Job', 'Job Arguments', 'Last Run', 'Last Checked', 'Reserved At', 'Execution Duration (s)']); - if ($input->getOption('display-invalid-jobs')) { - $invalidJobs = $this->jobList->listInvalidJobs(); - foreach ($invalidJobs as $invalidJob) { - $t->addRow([ - $invalidJob['id'], - $invalidJob['class'], - $invalidJob['argument'], - $invalidJob['last_run'], - $invalidJob['last_checked'], - $invalidJob['reserved_at'], - $invalidJob['execution_duration'], - ]); - } - } else { - $this->jobList->listJobs(function (IJob $job) use ($t) { + $this->jobList->listJobs( + function (IJob $job, string $classAsString) use ($t) { $t->addRow([ $job->getId(), - \get_class($job), + $classAsString, $this->getJobArgumentAsString($job->getArgument()), $job->getLastRun() == 0 ? 'N/A' : \date('c', $job->getLastRun()), \date('c', $job->getLastChecked()), $job->getReservedAt() == 0 ? 'N/A' : \date('c', $job->getReservedAt()), $job->getExecutionDuration() == -1 ? 'N/A' : $job->getExecutionDuration(), ]); - }); - } + }, + $input->getOption('display-only-invalid-jobs') + ); $t->render(); return 0; } diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 5dbfde3dfb74..119e21d2fb3d 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -24,6 +24,7 @@ namespace OC\BackgroundJob; +use OC\Command\CommandJob; use OCP\AppFramework\QueryException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJob; @@ -264,10 +265,13 @@ public function jobIdExists($id) { * get the job object from a row in the db * * @param array $row + * @param bool $includeInvalidJobs + * @param bool $onlyInvalidJobs * @return IJob|null */ - private function buildJob($row) { + private function buildJob($row, $includeInvalidJobs = false, $onlyInvalidJobs = false) { try { + $jobIsValid = true; try { // Try to load the job as a service /** @var IJob $job */ @@ -278,11 +282,21 @@ private function buildJob($row) { $class = $row['class']; $job = new $class(); } else { - // job from disabled app or old version of an app, no need to do anything - return null; + // job is from disabled app or old version of an app + $jobIsValid = false; + if ($includeInvalidJobs) { + // Use CommandJob as a "template" for this job that has an unknown class + $job = new CommandJob(); + } else { + return null; + } } } + if ($jobIsValid && $onlyInvalidJobs) { + return null; + } + $job->setId($row['id']); $job->setLastRun($row['last_run']); $job->setArgument(\json_decode($row['argument'], true)); @@ -352,44 +366,24 @@ public function setExecutionTime($job, $timeTaken) { } /** + * @param \Closure $callback + * @param bool $onlyInvalidJobs * @inheritdoc */ - public function listJobs(\Closure $callback) { + public function listJobs(\Closure $callback, bool $onlyInvalidJobs = false) { $query = $this->connection->getQueryBuilder(); $query->select('*') ->from('jobs'); $result = $query->execute(); while ($row = $result->fetch()) { - $job = $this->buildJob($row); + $job = $this->buildJob($row, true, $onlyInvalidJobs); if ($job) { - if ($callback($job) === false) { + if ($callback($job, $row['class']) === false) { break; } } } $result->closeCursor(); } - - public function listInvalidJobs(): array { - $query = $this->connection->getQueryBuilder(); - $query->select('*') - ->from('jobs'); - $result = $query->execute(); - $jobData = []; - - while ($row = $result->fetch()) { - try { - // Try to load the job as a service - \OC::$server->query($row['class']); - } catch (QueryException $e) { - if (!\class_exists($row['class'])) { - // job is from a disabled app or old version of an app - // so return the data about it - $jobData[] = $row; - } - } - } - return $jobData; - } } diff --git a/lib/public/BackgroundJob/IJobList.php b/lib/public/BackgroundJob/IJobList.php index 307badcdb6b9..cebec0ff24c5 100644 --- a/lib/public/BackgroundJob/IJobList.php +++ b/lib/public/BackgroundJob/IJobList.php @@ -136,20 +136,12 @@ public function setLastRun($job); public function setExecutionTime($job, $timeTaken); /** - * iterate over all valid jobs in the queue + * iterate over jobs in the queue * * @return void * @since 10.2.0 */ - public function listJobs(\Closure $callback); - - /** - * get the details of all invalid jobs in the queue - * - * @return array - * @since 10.13.0 - */ - public function listInvalidJobs(); + public function listJobs(\Closure $callback, bool $onlyInvalidJobs = false); /** * remove a specific job by id diff --git a/tests/Core/Command/Background/Queue/StatusTest.php b/tests/Core/Command/Background/Queue/StatusTest.php index 7033269fa766..2273fe9a0f87 100644 --- a/tests/Core/Command/Background/Queue/StatusTest.php +++ b/tests/Core/Command/Background/Queue/StatusTest.php @@ -49,14 +49,15 @@ public function setUp(): void { } public function testCommandInput() { - $this->jobList->expects($this->any())->method('listJobs') + $this->jobList->expects($this->once())->method('listJobs') + ->with($this->isType('callable'), false) ->willReturnCallback(function (\Closure $callBack) { $job = new RegularJob(); $job->setId(666); $job->setLastChecked(10); $job->setReservedAt(0); $job->setExecutionDuration(-1); - $callBack($job); + $callBack($job, \get_class($job)); }); $this->commandTester->execute([]); @@ -73,7 +74,8 @@ public function testCommandInput() { } public function testJobWithArray() { - $this->jobList->expects($this->any())->method('listJobs') + $this->jobList->expects($this->once())->method('listJobs') + ->with($this->isType('callable'), false) ->willReturnCallback(function (\Closure $callBack) { $job = new RegularJob(); $job->setId(666); @@ -81,7 +83,7 @@ public function testJobWithArray() { $job->setLastChecked(10); $job->setReservedAt(0); $job->setExecutionDuration(-1); - $callBack($job); + $callBack($job, \get_class($job)); }); $this->commandTester->execute([]); $output = $this->commandTester->getDisplay(); @@ -97,7 +99,8 @@ public function testJobWithArray() { } public function testJobWithSchedulingInfo() { - $this->jobList->expects($this->any())->method('listJobs') + $this->jobList->expects($this->once())->method('listJobs') + ->with($this->isType('callable'), false) ->willReturnCallback(function (\Closure $callBack) { $job = new RegularJob(); $job->setId(666); @@ -106,7 +109,7 @@ public function testJobWithSchedulingInfo() { $job->setLastChecked(10); $job->setReservedAt(10); $job->setExecutionDuration(1); - $callBack($job); + $callBack($job, \get_class($job)); }); $this->commandTester->execute([]); $output = $this->commandTester->getDisplay(); @@ -122,27 +125,27 @@ public function testJobWithSchedulingInfo() { } public function testListingInvalidJob() { - $this->jobList->expects($this->any())->method('listInvalidJobs') - ->willReturn([ - [ - 'id' => '42', - 'class' => 'OC\BackgroundJob\Legacy\RegularJob', - 'argument' => '{"k":"v"}', - 'last_run' => '2023-01-01T00:00:10+00:00', - 'last_checked' => '2023-06-01T00:00:40+00:00', - 'reserved_at' => '2023-06-02T00:00:40+00:00', - 'execution_duration' => 7, - ] - ]); + $this->jobList->expects($this->once())->method('listJobs') + ->with($this->isType('callable'), true) + ->willReturnCallback(function (\Closure $callBack) { + $job = new RegularJob(); + $job->setId(42); + $job->setArgument(['k'=> 'v']); + $job->setLastRun(20); + $job->setLastChecked(40); + $job->setReservedAt(15); + $job->setExecutionDuration(7); + $callBack($job, \get_class($job)); + }); $this->commandTester->execute( - ['--display-invalid-jobs' => null] + ['--display-only-invalid-jobs' => true] ); $output = $this->commandTester->getDisplay(); $expected = <<