Skip to content
Open
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
1 change: 1 addition & 0 deletions appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
<job>OCA\SuspiciousLogin\BackgroundJob\ETLJob</job>
<job>OCA\SuspiciousLogin\BackgroundJob\TrainJobIpV4</job>
<job>OCA\SuspiciousLogin\BackgroundJob\TrainJobIpV6</job>
<job>OCA\SuspiciousLogin\BackgroundJob\CleanupModelsJob</job>
</background-jobs>

<commands>
Expand Down
50 changes: 50 additions & 0 deletions lib/BackgroundJob/CleanupModelsJob.php
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;

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Collaborator Author

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 😄


Comment thread
DerDreschner marked this conversation as resolved.
public function __construct(
private readonly ModelStore $modelStore,
private readonly LoggerInterface $logger,
ITimeFactory $time,
) {
Comment thread
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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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,
]);
}
}
}
16 changes: 16 additions & 0 deletions lib/Db/ModelMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

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

findOld() orders only by created_at (an integer timestamp). If multiple models share the same created_at value (same second), the ordering becomes non-deterministic and the cleanup may delete a model that should have been kept. Add a secondary, deterministic sort key (e.g., id descending) to ensure the “most recent N” models are consistently preserved.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As the model generation takes longer then a second, this shouldn't happen at all. But I'll add it anyway just to be on the safe side.

return $this->findEntities($query);
}
}
55 changes: 55 additions & 0 deletions lib/Service/ModelStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

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

cleanup() loads all old models for both address types into memory at once (findOld() without a limit, then array_merge). On long-running instances this can produce a very large array and make the daily background job slow or memory-heavy. Consider deleting in batches (e.g., fetch a limited page of old models/ids, delete, repeat) so runtime and memory stay bounded.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

True, but I assume that the number of entries is manageable? It's usually 2 models (IPv4/IPv6) per day... To get more then 10.000 entries in the database, you would need to have the app activated for over 13 years.


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');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

return missing?

@DerDreschner DerDreschner Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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 appdata) isn't supported right now. Not sure if that should be covered as well... 🤔 What do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

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

cleanup() will abort the entire cleanup run if deleting a single model fails (e.g., file deletion throwing NotPermittedException or DB deletion throwing). For a maintenance/background task this makes cleanup brittle: one problematic record can prevent all subsequent old models from being removed. Consider wrapping the per-model cleanup body in a try/catch, logging the failure (with model id), and continuing with the next model (optionally still attempting DB deletion even if file deletion fails).

Suggested change
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)");

Copilot uses AI. Check for mistakes.
}
}
Loading
Loading