Fix some cases of database download in GeolocationDbUpdater

module/CLI/src/Command/Visit/DownloadGeoLiteDbCommand.php

@@ -51,6 +51,11 @@ class DownloadGeoLiteDbCommand extends Command implements GeolocationDownloadPro
                 return ExitCode::EXIT_WARNING;
             }
 
+            if ($result === GeolocationResult::MAX_ERRORS_REACHED) {
+                $this->io->warning('Max consecutive errors reached. Cannot retry for a couple of days.');
+                return ExitCode::EXIT_WARNING;
+            }
+
             if ($this->progressBar === null) {
                 $this->io->info('GeoLite2 db file is up to date.');
             } else {

module/CLI/test/Command/Visit/DownloadGeoLiteDbCommandTest.php

@@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\CLI\Command\Visit;
 
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\Attributes\TestWith;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\CLI\Command\Visit\DownloadGeoLiteDbCommand;
@@ -74,17 +75,17 @@ class DownloadGeoLiteDbCommandTest extends TestCase
     }
 
     #[Test]
-    public function warningIsPrintedWhenLicenseIsMissing(): void
+    #[TestWith([GeolocationResult::LICENSE_MISSING, 'It was not possible to download GeoLite2 db'])]
+    #[TestWith([GeolocationResult::MAX_ERRORS_REACHED, 'Max consecutive errors reached'])]
+    public function warningIsPrintedForSomeResults(GeolocationResult $result, string $expectedWarningMessage): void
     {
-        $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturn(
-            GeolocationResult::LICENSE_MISSING,
-        );
+        $this->dbUpdater->expects($this->once())->method('checkDbUpdate')->withAnyParameters()->willReturn($result);
 
         $this->commandTester->execute([]);
         $output = $this->commandTester->getDisplay();
         $exitCode = $this->commandTester->getStatusCode();
 
-        self::assertStringContainsString('[WARNING] It was not possible to download GeoLite2 db', $output);
+        self::assertStringContainsString('[WARNING] ' . $expectedWarningMessage, $output);
         self::assertSame(ExitCode::EXIT_WARNING, $exitCode);
     }
 

module/Core/src/Geolocation/Entity/GeolocationDbUpdate.php

@@ -49,17 +49,11 @@ class GeolocationDbUpdate extends AbstractEntity
     }
 
     /**
-     * This update would require a new download if:
-     * - It is successful and older than 30 days
-     * - It is error and older than 2 days
+     * @param positive-int $days
      */
-    public function needsUpdate(): bool
+    public function isOlderThan(int $days): bool
     {
-        return match ($this->status) {
-            GeolocationDbUpdateStatus::SUCCESS => Chronos::now()->greaterThan($this->dateUpdated->addDays(30)),
-            GeolocationDbUpdateStatus::ERROR => Chronos::now()->greaterThan($this->dateUpdated->addDays(2)),
-            default => false,
-        };
+        return Chronos::now()->greaterThan($this->dateUpdated->addDays($days));
     }
 
     public function isInProgress(): bool
@@ -71,4 +65,9 @@ class GeolocationDbUpdate extends AbstractEntity
     {
         return $this->status === GeolocationDbUpdateStatus::ERROR;
     }
+
+    public function isSuccess(): bool
+    {
+        return $this->status === GeolocationDbUpdateStatus::SUCCESS;
+    }
 }

module/Core/src/Geolocation/GeolocationDbUpdater.php

@@ -13,8 +13,6 @@ use Shlinkio\Shlink\IpGeolocation\Exception\MissingLicenseException;
 use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface;
 use Symfony\Component\Lock\LockFactory;
 
-use function count;
-use function Shlinkio\Shlink\Core\ArrayUtils\every;
 use function sprintf;
 
 readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface
@@ -26,6 +24,7 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface
         private LockFactory $locker,
         private TrackingOptions $trackingOptions,
         private EntityManagerInterface $em,
+        private int $maxRecentAttemptsToCheck = 15, // TODO Make this configurable
     ) {
     }
 
@@ -56,16 +55,12 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface
     private function downloadIfNeeded(
         GeolocationDownloadProgressHandlerInterface|null $downloadProgressHandler,
     ): GeolocationResult {
-        $maxRecentAttemptsToCheck = 15; // TODO Make this configurable
-
-        // Get last 15 download attempts
         $recentDownloads = $this->em->getRepository(GeolocationDbUpdate::class)->findBy(
             criteria: ['filesystemId' => GeolocationDbUpdate::currentFilesystemId()],
             orderBy: ['dateUpdated' => 'DESC'],
-            limit: $maxRecentAttemptsToCheck,
+            limit: $this->maxRecentAttemptsToCheck,
         );
         $mostRecentDownload = $recentDownloads[0] ?? null;
-        $amountOfRecentAttempts = count($recentDownloads);
 
         // If most recent attempt is in progress, skip check.
         // This is a safety check in case the lock is released before the previous download has finished.
@@ -73,19 +68,31 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface
             return GeolocationResult::CHECK_SKIPPED;
         }
 
-        // If all recent attempts are errors, and the most recent one is not old enough, skip download
-        if (
-            $amountOfRecentAttempts === $maxRecentAttemptsToCheck
-            && every($recentDownloads, static fn (GeolocationDbUpdate $update) => $update->isError())
-            && ! $mostRecentDownload->needsUpdate()
-        ) {
-            return GeolocationResult::CHECK_SKIPPED;
+        $amountOfErrorsSinceLastSuccess = 0;
+        foreach ($recentDownloads as $recentDownload) {
+            // Count attempts until a success is found
+            if ($recentDownload->isSuccess()) {
+                break;
+            }
+            $amountOfErrorsSinceLastSuccess++;
         }
 
-        // Try to download if there are no attempts, the database file does not exist or most recent attempt was
-        // successful and is old enough
-        $olderDbExists = $amountOfRecentAttempts > 0 && $this->dbUpdater->databaseFileExists();
-        if (! $olderDbExists || $mostRecentDownload->needsUpdate()) {
+        // If max amount of consecutive errors has been reached and the most recent one is not old enough, skip download
+        // for 2 days to avoid hitting potential API limits in geolocation services
+        $lastAttemptIsError = $mostRecentDownload !== null && $mostRecentDownload->isError();
+        // FIXME Once max errors are reached there will be one attempt every 2 days, but it should be 15 attempts every
+        //       2 days. Leaving like this for simplicity for now.
+        $maxConsecutiveErrorsReached = $amountOfErrorsSinceLastSuccess === $this->maxRecentAttemptsToCheck;
+        if ($lastAttemptIsError && $maxConsecutiveErrorsReached && ! $mostRecentDownload->isOlderThan(days: 2)) {
+            return GeolocationResult::MAX_ERRORS_REACHED;
+        }
+
+        // Try to download if:
+        // - There are no attempts or the database file does not exist
+        // - Last update errored (and implicitly, the max amount of consecutive errors has not been reached)
+        // - Most recent attempt is older than 30 days (and implicitly, successful)
+        $olderDbExists = $mostRecentDownload !== null && $this->dbUpdater->databaseFileExists();
+        if (! $olderDbExists || $lastAttemptIsError || $mostRecentDownload->isOlderThan(days: 30)) {
             return $this->downloadAndTrackUpdate($downloadProgressHandler, $olderDbExists);
         }
 
@@ -112,7 +119,7 @@ readonly class GeolocationDbUpdater implements GeolocationDbUpdaterInterface
             return GeolocationResult::LICENSE_MISSING;
         } catch (GeolocationDbUpdateFailedException $e) {
             $dbUpdate->finishWithError(
-                sprintf('%s. Prev: %s', $e->getMessage(), $e->getPrevious()?->getMessage() ?? '-'),
+                sprintf('%s Prev: %s', $e->getMessage(), $e->getPrevious()?->getMessage() ?? '-'),
             );
             throw $e;
         } finally {

module/Core/src/Geolocation/GeolocationResult.php

@@ -5,6 +5,7 @@ namespace Shlinkio\Shlink\Core\Geolocation;
 enum GeolocationResult
 {
     case CHECK_SKIPPED;
+    case MAX_ERRORS_REACHED;
     case LICENSE_MISSING;
     case DB_CREATED;
     case DB_UPDATED;