From 62d861d23a3e1fcd4fb726d7918574f4cd922c9c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:10:10 +0200 Subject: [PATCH] fix(jobs): resume S3 revalidation and remove orphaned notification throttles - The S3 revalidation job stops within its time budget and continues from a cursor on the next run. - An hourly sweep deletes throttle rows whose subject no longer exists. --- app/Jobs/CleanupInstanceStuffsJob.php | 16 +++++++ app/Jobs/RevalidateUnusableS3StoragesJob.php | 31 ++++++++++++- app/Models/NotificationThrottle.php | 33 +++++++++++++ tests/Feature/NotificationThrottleTest.php | 35 ++++++++++++++ .../RevalidateUnusableS3StoragesJobTest.php | 46 +++++++++++++++++++ 5 files changed, 159 insertions(+), 2 deletions(-) diff --git a/app/Jobs/CleanupInstanceStuffsJob.php b/app/Jobs/CleanupInstanceStuffsJob.php index e37a39c3dc..85c39a8e08 100644 --- a/app/Jobs/CleanupInstanceStuffsJob.php +++ b/app/Jobs/CleanupInstanceStuffsJob.php @@ -2,6 +2,7 @@ namespace App\Jobs; +use App\Models\NotificationThrottle; use App\Models\ScheduledDatabaseBackup; use App\Models\TeamInvitation; use App\Models\User; @@ -35,6 +36,7 @@ class CleanupInstanceStuffsJob implements ShouldBeEncrypted, ShouldBeUnique, Sho $this->cleanupInvitationLink(); $this->cleanupExpiredEmailChangeRequests(); $this->enforceBackupRetention(); + $this->deleteOrphanedNotificationThrottles(); } catch (\Throwable $e) { Log::error('CleanupInstanceStuffsJob failed with error: '.$e->getMessage()); } @@ -59,6 +61,20 @@ class CleanupInstanceStuffsJob implements ShouldBeEncrypted, ShouldBeUnique, Sho ]); } + private function deleteOrphanedNotificationThrottles(): void + { + if (! Cache::add('notification-throttle-orphan-cleanup', true, 3600)) { + return; + } + + try { + NotificationThrottle::deleteOrphans(); + } catch (\Throwable $e) { + Log::error('Failed to delete orphaned notification throttles: '.$e->getMessage()); + Cache::forget('notification-throttle-orphan-cleanup'); + } + } + private function enforceBackupRetention(): void { if (! Cache::add('backup-retention-enforcement', true, 1800)) { diff --git a/app/Jobs/RevalidateUnusableS3StoragesJob.php b/app/Jobs/RevalidateUnusableS3StoragesJob.php index fe9593b6fc..2240b457fb 100644 --- a/app/Jobs/RevalidateUnusableS3StoragesJob.php +++ b/app/Jobs/RevalidateUnusableS3StoragesJob.php @@ -10,9 +10,13 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Cache; /** * Re-tests S3 storages that failed validation, so a storage that becomes reachable again is usable without manual action. + * + * Each run stops after a time budget and saves the last tested id, so the next run continues from there + * instead of re-testing the same slow storages from the start. */ class RevalidateUnusableS3StoragesJob implements ShouldBeEncrypted, ShouldBeUnique, ShouldQueue { @@ -22,18 +26,41 @@ class RevalidateUnusableS3StoragesJob implements ShouldBeEncrypted, ShouldBeUniq public $timeout = 1800; + /** + * Stay well below $timeout, because a single unreachable endpoint can take a while to fail. + */ + public const TIME_BUDGET_SECONDS = 1200; + + public const CURSOR_CACHE_KEY = 'revalidate-unusable-s3-storages:last-id'; + public function handle(): void { - S3Storage::query() + $deadline = now()->addSeconds(self::TIME_BUDGET_SECONDS); + $lastId = Cache::get(self::CURSOR_CACHE_KEY); + + $completed = S3Storage::query() ->where('is_usable', false) - ->chunkById(100, function ($storages): void { + ->when($lastId !== null, fn ($query) => $query->where('id', '>', $lastId)) + ->chunkById(100, function ($storages) use ($deadline): bool { foreach ($storages as $storage) { + if (now()->greaterThanOrEqualTo($deadline)) { + return false; + } + try { $storage->testConnection(shouldSave: true); } catch (\Throwable) { // testConnection() keeps the storage unusable and notifies the team once. } + + Cache::forever(self::CURSOR_CACHE_KEY, $storage->id); } + + return true; }); + + if ($completed) { + Cache::forget(self::CURSOR_CACHE_KEY); + } } } diff --git a/app/Models/NotificationThrottle.php b/app/Models/NotificationThrottle.php index b33a6ac5f7..a43e24c433 100644 --- a/app/Models/NotificationThrottle.php +++ b/app/Models/NotificationThrottle.php @@ -7,6 +7,7 @@ use Carbon\CarbonInterface; use Closure; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\MorphTo; +use Illuminate\Database\Eloquent\Relations\Relation; use Throwable; /** @@ -133,4 +134,36 @@ class NotificationThrottle extends Model ->where('notification', $notification) ->exists(); } + + /** + * Delete rows whose subject no longer exists. Subjects are often removed with bulk or cascading + * deletes that skip model events, so this sweep is the reliable cleanup. Soft-deleted subjects are kept. + * + * Returns the number of deleted rows. + */ + public static function deleteOrphans(): int + { + $deleted = 0; + + foreach (static::query()->distinct()->pluck('notifiable_type') as $type) { + $class = Relation::getMorphedModel($type) ?? $type; + $throttles = static::query()->where('notifiable_type', $type); + + if (! is_a($class, Model::class, true)) { + $deleted += $throttles->delete(); + + continue; + } + + $subject = new $class; + $deleted += $throttles->whereNotExists( + $class::query() + ->withoutGlobalScopes() + ->selectRaw('1') + ->whereColumn($subject->getQualifiedKeyName(), (new static)->qualifyColumn('notifiable_id')) + )->delete(); + } + + return $deleted; + } } diff --git a/tests/Feature/NotificationThrottleTest.php b/tests/Feature/NotificationThrottleTest.php index 77ad73cc80..2f1a2d2be6 100644 --- a/tests/Feature/NotificationThrottleTest.php +++ b/tests/Feature/NotificationThrottleTest.php @@ -1,7 +1,9 @@ $this->server->sendUnreachableNotification())->toThrow(RuntimeException::class) ->and(NotificationThrottle::wasSent($this->server, Unreachable::class))->toBeFalse(); }); + +it('deletes throttle rows whose subject no longer exists', function () { + $trashedServer = Server::factory()->create(['team_id' => $this->team->id]); + $trashedServer->delete(); + $rows = [ + 'kept' => ['notifiable_type' => $this->server->getMorphClass(), 'notifiable_id' => $this->server->id], + 'kept-soft-deleted' => ['notifiable_type' => $trashedServer->getMorphClass(), 'notifiable_id' => $trashedServer->id], + 'missing-server' => ['notifiable_type' => $this->server->getMorphClass(), 'notifiable_id' => 999999], + 'missing-backup' => ['notifiable_type' => ScheduledDatabaseBackup::class, 'notifiable_id' => 999999], + 'missing-class' => ['notifiable_type' => 'App\Models\RemovedModel', 'notifiable_id' => 1], + ]; + foreach ($rows as $notification => $row) { + NotificationThrottle::query()->insert($row + ['notification' => $notification, 'sent_at' => now()]); + } + + expect(NotificationThrottle::deleteOrphans())->toBe(3) + ->and(NotificationThrottle::query()->orderBy('notification')->pluck('notification')->all())->toBe(['kept', 'kept-soft-deleted']); +}); + +it('deletes orphaned throttle rows during the instance cleanup', function () { + InstanceSettings::query()->firstOrCreate(['id' => 0]); + NotificationThrottle::record($this->server, Unreachable::class); + NotificationThrottle::query()->insert([ + 'notifiable_type' => ScheduledDatabaseBackup::class, + 'notifiable_id' => 999999, + 'notification' => 'App\Notifications\Database\BackupMissing', + 'sent_at' => now(), + ]); + + (new CleanupInstanceStuffsJob)->handle(); + + expect(NotificationThrottle::query()->pluck('notifiable_type')->all())->toBe([$this->server->getMorphClass()]); +}); diff --git a/tests/Feature/RevalidateUnusableS3StoragesJobTest.php b/tests/Feature/RevalidateUnusableS3StoragesJobTest.php index 94b3667841..7fec8808a5 100644 --- a/tests/Feature/RevalidateUnusableS3StoragesJobTest.php +++ b/tests/Feature/RevalidateUnusableS3StoragesJobTest.php @@ -3,6 +3,7 @@ use App\Jobs\RevalidateUnusableS3StoragesJob; use App\Models\S3Storage; use App\Models\Team; +use Carbon\Carbon; use Illuminate\Filesystem\FilesystemAdapter; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Storage; @@ -61,3 +62,48 @@ it('does not test storages that are already usable', function () { (new RevalidateUnusableS3StoragesJob)->handle(); }); + +it('includes the storage with id 0 on the first run', function () { + $storage = revalidationStorage(Team::factory()->create()); + S3Storage::query()->whereKey($storage->id)->update(['id' => 0]); + $disk = Mockery::mock(FilesystemAdapter::class); + $disk->shouldReceive('files')->once()->andReturn([]); + Storage::shouldReceive('build')->once()->andReturn($disk); + + (new RevalidateUnusableS3StoragesJob)->handle(); + + expect(S3Storage::find(0)->is_usable)->toBeTrue(); +}); + +it('stops when the time budget is used and continues with the next storage on the next run', function () { + $team = Team::factory()->create(); + $first = revalidationStorage($team, ['bucket' => 'first-bucket']); + $second = revalidationStorage($team, ['bucket' => 'second-bucket']); + $tested = []; + Storage::shouldReceive('build')->andReturnUsing(function (array $config) use (&$tested) { + $tested[] = $config['bucket']; + $disk = Mockery::mock(FilesystemAdapter::class); + $disk->shouldReceive('files')->andReturnUsing(function () { + // A slow endpoint uses the whole time budget of the run. + Carbon::setTestNow(now()->addSeconds(RevalidateUnusableS3StoragesJob::TIME_BUDGET_SECONDS + 1)); + + throw new RuntimeException('Connection timed out'); + }); + + return $disk; + }); + + (new RevalidateUnusableS3StoragesJob)->handle(); + expect($tested)->toBe(['first-bucket']); + + (new RevalidateUnusableS3StoragesJob)->handle(); + expect($tested)->toBe(['first-bucket', 'second-bucket']); + + // After the last storage, the next run starts from the beginning again. + (new RevalidateUnusableS3StoragesJob)->handle(); + expect($tested)->toBe(['first-bucket', 'second-bucket', 'first-bucket']) + ->and($first->fresh()->is_usable)->toBeFalse() + ->and($second->fresh()->is_usable)->toBeFalse(); + + Carbon::setTestNow(); +});