-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Implement background job to remove unused models #1098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| namespace OCA\SuspiciousLogin\BackgroundJob; | ||
|
|
||
| use OCA\SuspiciousLogin\Service\ModelStore; | ||
| use OCP\AppFramework\Utility\ITimeFactory; | ||
| use OCP\BackgroundJob\TimedJob; | ||
| use Psr\Log\LoggerInterface; | ||
| use Throwable; | ||
|
|
||
| class CleanupModelsJob extends TimedJob { | ||
|
|
||
| /** | ||
| * @var int Number of most recent models to keep per address type. | ||
| * Should match the number of models consumed by the statistics | ||
| * API. | ||
| */ | ||
| private const MODELS_TO_KEEP = 14; | ||
|
|
||
|
DerDreschner marked this conversation as resolved.
|
||
| public function __construct( | ||
| private readonly ModelStore $modelStore, | ||
| private readonly LoggerInterface $logger, | ||
| ITimeFactory $time, | ||
| ) { | ||
|
DerDreschner marked this conversation as resolved.
|
||
| parent::__construct($time); | ||
|
|
||
| // Run once per day | ||
| $this->setInterval(24 * 60 * 60); | ||
| $this->setTimeSensitivity(self::TIME_INSENSITIVE); | ||
| $this->allowParallelRuns = false; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. best use setAllowParallelRuns(false) here |
||
| } | ||
|
|
||
| #[\Override] | ||
| protected function run($argument): void { | ||
| try { | ||
| $this->modelStore->cleanup(self::MODELS_TO_KEEP); | ||
| } catch (Throwable $e) { | ||
| $this->logger->error('Error during model cleanup: ' . $e->getMessage(), [ | ||
| 'exception' => $e, | ||
| ]); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,4 +66,20 @@ public function findMostRecent(int $max, string $addressType): array { | |
|
|
||
| return $this->findEntities($query); | ||
| } | ||
|
|
||
| /** | ||
| * Find all models older than `$numberOfModelsToKeep` per `$addressType` | ||
| * | ||
| * @return Model[] | ||
| */ | ||
| public function findOld(int $numberOfModelsToKeep, string $addressType): array { | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $query = $qb->select('*') | ||
| ->from($this->getTableName()) | ||
| ->where($qb->expr()->eq('address_type', $qb->createNamedParameter($addressType))) | ||
| ->setFirstResult($numberOfModelsToKeep) | ||
| ->orderBy('created_at', 'desc'); | ||
|
|
||
|
Comment on lines
+77
to
+82
|
||
| return $this->findEntities($query); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -165,4 +165,59 @@ public function persist(Learner $estimator, Model $model) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw $e; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Remove old models, keeping only the `$numberOfModelsToKeep` ones | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * for each used address type (IPv4 / IPv6). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @throws \OCP\DB\Exception | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @throws \OCP\Files\NotPermittedException | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public function cleanup(int $numberOfModelsToKeep): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $oldModels = array_merge( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->modelMapper->findOld($numberOfModelsToKeep, Ipv4Strategy::getTypeName()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->modelMapper->findOld($numberOfModelsToKeep, IpV6Strategy::getTypeName()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+176
to
+180
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (empty($oldModels)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->logger->debug('No old models to clean up'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $modelsFolder = $this->appData->getFolder(self::APPDATA_MODELS_FOLDER); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (NotFoundException) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->logger->debug('Models folder does not exist, skipping file deletion'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return missing?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noo, in this case, the database has models we'll clean up to make both them consistentent to each other. The other way around (clean database, but model files in
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice: Shouldn't be handled right now as it's an error case anyway. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ($this->cacheFactory->isLocalCacheAvailable()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $cache = $this->cacheFactory->createLocal(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($oldModels as $oldModel) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $id = $oldModel->getId(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->logger->debug("Cleaning up old model $id"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Remove the model file from app data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isset($modelsFolder)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $modelFile = $modelsFolder->getFile((string)$id); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $modelFile->delete(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (NotFoundException) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->logger->debug("Model file $id not found in app data, skipping file deletion"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Evict from cache | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isset($cache)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $cache->remove($this->getCacheKey($id)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Remove the DB record | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->modelMapper->delete($oldModel); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $numberOfOldModels = count($oldModels); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->logger->info("Cleaned up {$numberOfOldModels} old model(s)"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+197
to
+221
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($oldModels as $oldModel) { | |
| $id = $oldModel->getId(); | |
| $this->logger->debug("Cleaning up old model $id"); | |
| // Remove the model file from app data | |
| if (isset($modelsFolder)) { | |
| try { | |
| $modelFile = $modelsFolder->getFile((string)$id); | |
| $modelFile->delete(); | |
| } catch (NotFoundException) { | |
| $this->logger->debug("Model file $id not found in app data, skipping file deletion"); | |
| } | |
| } | |
| // Evict from cache | |
| if (isset($cache)) { | |
| $cache->remove($this->getCacheKey($id)); | |
| } | |
| // Remove the DB record | |
| $this->modelMapper->delete($oldModel); | |
| } | |
| $numberOfOldModels = count($oldModels); | |
| $this->logger->info("Cleaned up {$numberOfOldModels} old model(s)"); | |
| $numberOfCleanedModels = 0; | |
| foreach ($oldModels as $oldModel) { | |
| $id = $oldModel->getId(); | |
| try { | |
| $this->logger->debug("Cleaning up old model $id"); | |
| // Remove the model file from app data | |
| if (isset($modelsFolder)) { | |
| try { | |
| $modelFile = $modelsFolder->getFile((string)$id); | |
| $modelFile->delete(); | |
| } catch (NotFoundException) { | |
| $this->logger->debug("Model file $id not found in app data, skipping file deletion"); | |
| } catch (\Throwable $e) { | |
| $this->logger->warning("Failed to delete model file $id from app data: " . $e->getMessage(), [ | |
| 'exception' => $e, | |
| 'modelId' => $id, | |
| ]); | |
| } | |
| } | |
| // Evict from cache | |
| if (isset($cache)) { | |
| $cache->remove($this->getCacheKey($id)); | |
| } | |
| // Remove the DB record | |
| $this->modelMapper->delete($oldModel); | |
| $numberOfCleanedModels++; | |
| } catch (\Throwable $e) { | |
| $this->logger->warning("Failed to clean up old model $id: " . $e->getMessage(), [ | |
| 'exception' => $e, | |
| 'modelId' => $id, | |
| ]); | |
| } | |
| } | |
| $this->logger->info("Cleaned up {$numberOfCleanedModels} old model(s)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the stats API and this job use the same constant to avoid them getting out of sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Wasn't sure if a separation would be preferred 😄