From ed531992a8014ff321bc2bf21ac6a8331a43c912 Mon Sep 17 00:00:00 2001 From: Varun Patil Date: Sun, 10 Mar 2024 15:08:18 -0700 Subject: [PATCH] Do not retry failed indexing (#948) Signed-off-by: Varun Patil --- CHANGELOG.md | 1 + appinfo/routes.php | 1 + lib/Command/Index.php | 32 ++++++- lib/Controller/AdminController.php | 25 +++++ lib/Db/TimelineWrite.php | 13 ++- lib/Db/TimelineWriteFailures.php | 93 +++++++++++++++++++ .../Version602003Date20240310203729.php | 75 +++++++++++++++ lib/Service/Index.php | 12 ++- src/components/admin/AdminTypes.ts | 1 + src/components/admin/sections/Indexing.vue | 84 +++++++++-------- src/components/admin/sections/Places.vue | 4 +- src/services/API.ts | 4 + 12 files changed, 299 insertions(+), 46 deletions(-) create mode 100644 lib/Db/TimelineWriteFailures.php create mode 100644 lib/Migration/Version602003Date20240310203729.php diff --git a/CHANGELOG.md b/CHANGELOG.md index a3242d589..d7510d88d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. ## [Unreleased] - Hide files starting with `.` in the timeline +- Prevent automatically retrying failed indexing jobs ## [v6.2.2] - 2024-01-10 diff --git a/appinfo/routes.php b/appinfo/routes.php index 294dc958f..6da9a71ee 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -96,6 +96,7 @@ function w($base, $param) ['name' => 'Admin#getSystemStatus', 'url' => '/api/system-status', 'verb' => 'GET'], ['name' => 'Admin#getSystemConfig', 'url' => '/api/system-config', 'verb' => 'GET'], ['name' => 'Admin#setSystemConfig', 'url' => '/api/system-config/{key}', 'verb' => 'PUT'], + ['name' => 'Admin#getFailureLogs', 'url' => '/api/failure-logs', 'verb' => 'GET'], ['name' => 'Admin#placesSetup', 'url' => '/api/occ/places-setup', 'verb' => 'POST'], // Service worker and assets diff --git a/lib/Command/Index.php b/lib/Command/Index.php index 0b07455e6..39b884f1f 100644 --- a/lib/Command/Index.php +++ b/lib/Command/Index.php @@ -42,6 +42,7 @@ class IndexOpts public ?string $user = null; public ?string $folder = null; public ?string $group = null; + public bool $retry = false; public bool $skipCleanup = false; public function __construct(InputInterface $input) @@ -50,7 +51,8 @@ public function __construct(InputInterface $input) $this->clear = (bool) $input->getOption('clear'); $this->user = $input->getOption('user'); $this->folder = $input->getOption('folder'); - $this->skipCleanup = $input->getOption('skip-cleanup'); + $this->retry = (bool) $input->getOption('retry'); + $this->skipCleanup = (bool) $input->getOption('skip-cleanup'); $this->group = $input->getOption('group'); } } @@ -82,6 +84,7 @@ protected function configure(): void ->addOption('folder', null, InputOption::VALUE_REQUIRED, 'Index only the specified folder (relative to the user\'s root)') ->addOption('force', 'f', InputOption::VALUE_NONE, 'Force refresh of existing index entries') ->addOption('clear', null, InputOption::VALUE_NONE, 'Clear all existing index entries') + ->addOption('retry', 'r', InputOption::VALUE_NONE, 'Retry indexing of failed files') ->addOption('skip-cleanup', null, InputOption::VALUE_NONE, 'Skip cleanup step (removing index entries with missing files)') ; } @@ -109,6 +112,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int // Perform steps based on opts $this->checkClear(); $this->checkForce(); + $this->checkRetry(); // Run the indexer $this->runIndex(); @@ -118,6 +122,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->indexer->cleanupStale(); } + // Warn about skipped files + $this->warnRetry(); + return 0; } catch (\Exception $e) { $this->output->writeln("{$e->getMessage()}".PHP_EOL); @@ -161,10 +168,31 @@ protected function checkForce(): void } $this->output->writeln('Forcing refresh of existing index entries'); - $this->tw->orphanAll(); } + /** + * Check and act on the retry option if set. + */ + protected function checkRetry(): void + { + if (!$this->opts->retry) { + return; + } + + $this->output->writeln("Retrying indexing of failed files"); + $this->tw->clearAllFailures(); + } + + /** + * Warn about skipped files (called at the end of indexing). + */ + protected function warnRetry(): void { + if ($count = $this->tw->countFailures()) { + $this->output->writeln("Indexing skipped for ${count} failed files, use --retry to try again"); + } + } + /** * Run the indexer. */ diff --git a/lib/Controller/AdminController.php b/lib/Controller/AdminController.php index ccaab81bf..14ab67e0b 100644 --- a/lib/Controller/AdminController.php +++ b/lib/Controller/AdminController.php @@ -85,6 +85,7 @@ public function getSystemStatus(): Http\Response return Util::guardEx(function () { $config = \OC::$server->get(\OCP\IConfig::class); $index = \OC::$server->get(\OCA\Memories\Service\Index::class); + $tw = \OC::$server->get(\OCA\Memories\Db\TimelineWrite::class); // Build status array $status = []; @@ -107,6 +108,7 @@ public function getSystemStatus(): Http\Response // Check number of indexed files $status['indexed_count'] = $index->getIndexedCount(); + $status['failure_count'] = $tw->countFailures(); // Automatic indexing stats $jobStart = (int) $config->getAppValue(Application::APPNAME, 'last_index_job_start', (string) 0); @@ -178,6 +180,29 @@ public function getSystemStatus(): Http\Response }); } + /** + * @AdminRequired + * + * @NoCSRFRequired + */ + public function getFailureLogs(): Http\Response { + return Util::guardExDirect(static function (Http\IOutput $out) { + $tw = \OC::$server->get(\OCA\Memories\Db\TimelineWrite::class); + + $out->setHeader('Content-Type: text/plain'); + $out->setHeader('X-Accel-Buffering: no'); + $out->setHeader('Cache-Control: no-cache'); + + foreach ($tw->listFailures() as $log) { + $fileid = str_pad((string) $log['fileid'], 12, ' ', STR_PAD_RIGHT); // size + $mtime = $log['mtime']; + $reason = $log['reason']; + + $out->setOutput("{$fileid}[{$mtime}]\t{$reason}\n"); + } + }); + } + /** * @AdminRequired * diff --git a/lib/Db/TimelineWrite.php b/lib/Db/TimelineWrite.php index 666de699c..eb56dc19c 100644 --- a/lib/Db/TimelineWrite.php +++ b/lib/Db/TimelineWrite.php @@ -11,7 +11,7 @@ use OCP\IDBConnection; use OCP\Lock\ILockingProvider; -const DELETE_TABLES = ['memories', 'memories_livephoto', 'memories_places']; +const DELETE_TABLES = ['memories', 'memories_livephoto', 'memories_places', 'memories_failures']; const TRUNCATE_TABLES = ['memories_mapclusters']; class TimelineWrite @@ -19,6 +19,7 @@ class TimelineWrite use TimelineWriteMap; use TimelineWriteOrphans; use TimelineWritePlaces; + use TimelineWriteFailures; public function __construct( protected IDBConnection $connection, @@ -182,7 +183,15 @@ public function processFile( $query->insert('memories')->values($params); } - return $query->executeStatement() > 0; + // Execute query + $updated = $query->executeStatement() > 0; + + // Clear failures if successful + if ($updated) { + $this->clearFailures($file); + } + + return $updated; } /** diff --git a/lib/Db/TimelineWriteFailures.php b/lib/Db/TimelineWriteFailures.php new file mode 100644 index 000000000..4ad8d7c98 --- /dev/null +++ b/lib/Db/TimelineWriteFailures.php @@ -0,0 +1,93 @@ +getPath()})"; + + // Remove all previous failures for this file + $this->connection->beginTransaction(); + $this->clearFailures($file); + + // Add the failure to the database + $query = $this->connection->getQueryBuilder(); + $query->insert('memories_failures') + ->values([ + 'fileid' => $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT), + 'mtime' => $query->createNamedParameter($file->getMtime(), IQueryBuilder::PARAM_INT), + 'reason' => $query->createNamedParameter($reason, IQueryBuilder::PARAM_STR), + ]) + ->executeStatement() + ; + $this->connection->commit(); + } + + /** + * Mark a file as successfully indexed. + * The entry will be removed from the failures table. + * + * @param File $file The file that was successfully indexed + */ + public function clearFailures(File $file): void + { + $query = $this->connection->getQueryBuilder(); + $query->delete('memories_failures') + ->where($query->expr()->eq('fileid', $query->createNamedParameter($file->getId(), IQueryBuilder::PARAM_INT))) + ->executeStatement() + ; + } + + /** + * Get the count of failed files. + */ + public function countFailures(): int + { + $query = $this->connection->getQueryBuilder(); + $query->select($query->createFunction('COUNT(fileid)')) + ->from('memories_failures') + ; + return (int) $query->executeQuery()->fetchOne(); + } + + /** + * Get the list of failures. + */ + public function listFailures(): array + { + return $this->connection->getQueryBuilder() + ->select('*') + ->from('memories_failures') + ->executeQuery() + ->fetchAll(); + ; + } + + /** + * Clear all failures from the database. + */ + public function clearAllFailures(): void + { + // Delete all entries and reset autoincrement counter + $this->connection->executeStatement( + $this->connection->getDatabasePlatform()->getTruncateTableSQL('*PREFIX*memories_failures', false)); + } +} \ No newline at end of file diff --git a/lib/Migration/Version602003Date20240310203729.php b/lib/Migration/Version602003Date20240310203729.php new file mode 100644 index 000000000..300f61bca --- /dev/null +++ b/lib/Migration/Version602003Date20240310203729.php @@ -0,0 +1,75 @@ + + * @author Varun Patil + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OCA\Memories\Migration; + +use OCP\DB\ISchemaWrapper; +use OCP\DB\Types; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +class Version602003Date20240310203729 extends SimpleMigrationStep +{ + /** + * @param Closure(): ISchemaWrapper $schemaClosure + */ + public function preSchemaChange(IOutput $output, \Closure $schemaClosure, array $options): void {} + + /** + * @param \Closure(): ISchemaWrapper $schemaClosure + */ + public function changeSchema(IOutput $output, \Closure $schemaClosure, array $options): ?ISchemaWrapper + { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + if (!$schema->hasTable('memories_failures')) { + $table = $schema->createTable('memories_failures'); + $table->addColumn('id', 'integer', [ + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->addColumn('fileid', Types::BIGINT, [ + 'notnull' => true, + 'length' => 20, + ]); + $table->addColumn('mtime', Types::BIGINT, [ + 'notnull' => true, + 'length' => 20, + ]); + $table->addColumn('reason', 'text', [ + 'notnull' => false, + ]); + + $table->setPrimaryKey(['id']); + $table->addIndex(['fileid', 'mtime'], 'memories_fail_fid_mt_idx'); + } + + return $schema; + } + + /** + * @param Closure(): ISchemaWrapper $schemaClosure + */ + public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options): void {} +} diff --git a/lib/Service/Index.php b/lib/Service/Index.php index 6c337c6b1..58f8702f4 100644 --- a/lib/Service/Index.php +++ b/lib/Service/Index.php @@ -150,17 +150,24 @@ public function indexFolder(Folder $folder): void ; // Filter out files that are already indexed - $addFilter = static function (string $table, string $alias) use (&$query): void { + $addFilter = static function ( + string $table, + string $alias, + bool $orphan = true, + ) use (&$query): void { $query->leftJoin('f', $table, $alias, $query->expr()->andX( $query->expr()->eq('f.fileid', "{$alias}.fileid"), $query->expr()->eq('f.mtime', "{$alias}.mtime"), - $query->expr()->eq("{$alias}.orphan", $query->expr()->literal(0)), + $orphan + ? $query->expr()->eq("{$alias}.orphan", $query->expr()->literal(0)) + : $query->expr()->literal(1), )); $query->andWhere($query->expr()->isNull("{$alias}.fileid")); }; $addFilter('memories', 'm'); $addFilter('memories_livephoto', 'lp'); + $addFilter('memories_failures', 'fail', false); // Get file IDs to actually index $fileIds = $query->executeQuery()->fetchAll(\PDO::FETCH_COLUMN); @@ -201,6 +208,7 @@ public function indexFile(File $file): void $this->log("Skipping file {$path} due to lock", true); } catch (\Exception $e) { $this->error("Failed to index file {$path}: {$e->getMessage()}"); + $this->tw->markFailed($file, $e->getMessage()); } finally { $this->tempManager->clean(); } diff --git a/src/components/admin/AdminTypes.ts b/src/components/admin/AdminTypes.ts index 4f54aeb63..b7e213eb5 100644 --- a/src/components/admin/AdminTypes.ts +++ b/src/components/admin/AdminTypes.ts @@ -48,6 +48,7 @@ export type ISystemStatus = { bad_encryption: boolean; indexed_count: number; + failure_count: number; mimes: string[]; imagick: string | false; gis_type: number; diff --git a/src/components/admin/sections/Indexing.vue b/src/components/admin/sections/Indexing.vue index 8e6c31ce9..58bdb7e08 100644 --- a/src/components/admin/sections/Indexing.vue +++ b/src/components/admin/sections/Indexing.vue @@ -4,30 +4,16 @@ @@ -125,6 +128,7 @@ import { defineComponent } from 'vue'; import { translate as t } from '@services/l10n'; +import { API } from '@services/API'; import AdminMixin from '../AdminMixin'; @@ -132,5 +136,11 @@ export default defineComponent({ name: 'Indexing', title: t('memories', 'Media Indexing'), mixins: [AdminMixin], + + methods: { + openFailureLogs() { + return window.open(API.FAILURE_LOGS()); + }, + }, }); diff --git a/src/components/admin/sections/Places.vue b/src/components/admin/sections/Places.vue index ae071041c..d7ccb2a65 100644 --- a/src/components/admin/sections/Places.vue +++ b/src/components/admin/sections/Places.vue @@ -13,9 +13,7 @@ > {{ status.gis_count > 0 - ? t('memories', 'Database is populated with {n} geometries.', { - n: status.gis_count, - }) + ? t('memories', 'Database is populated with {n} geometries.', { n: status.gis_count }) : t('memories', 'Geometry table has not been created.') }} {{ diff --git a/src/services/API.ts b/src/services/API.ts index 0ce33cf00..07fcd6605 100644 --- a/src/services/API.ts +++ b/src/services/API.ts @@ -198,6 +198,10 @@ export class API { return gen(`${BASE}/system-status`); } + static FAILURE_LOGS() { + return gen(`${BASE}/failure-logs`); + } + static OCC_PLACES_SETUP() { return gen(`${BASE}/occ/places-setup`); }