diff --git a/app/Console/Commands/CleanupRedis.php b/app/Console/Commands/CleanupRedis.php index a13cda0b8..f6a2de75b 100644 --- a/app/Console/Commands/CleanupRedis.php +++ b/app/Console/Commands/CleanupRedis.php @@ -7,9 +7,9 @@ class CleanupRedis extends Command { - protected $signature = 'cleanup:redis {--dry-run : Show what would be deleted without actually deleting} {--skip-overlapping : Skip overlapping queue cleanup}'; + protected $signature = 'cleanup:redis {--dry-run : Show what would be deleted without actually deleting} {--skip-overlapping : Skip overlapping queue cleanup} {--clear-locks : Clear stale WithoutOverlapping locks}'; - protected $description = 'Cleanup Redis (Horizon jobs, metrics, overlapping queues, and related data)'; + protected $description = 'Cleanup Redis (Horizon jobs, metrics, overlapping queues, cache locks, and related data)'; public function handle() { @@ -56,6 +56,13 @@ public function handle() $deletedCount += $overlappingCleaned; } + // Clean up stale cache locks (WithoutOverlapping middleware) + if ($this->option('clear-locks')) { + $this->info('Cleaning up stale cache locks...'); + $locksCleaned = $this->cleanupCacheLocks($dryRun); + $deletedCount += $locksCleaned; + } + if ($dryRun) { $this->info("DRY RUN: Would delete {$deletedCount} out of {$totalKeys} keys"); } else { @@ -273,4 +280,56 @@ private function deduplicateQueueContents($redis, $queueKey, $dryRun) return $cleanedCount; } + + private function cleanupCacheLocks(bool $dryRun): int + { + $cleanedCount = 0; + + // Use the default Redis connection (database 0) where cache locks are stored + $redis = Redis::connection('default'); + + // Get all keys matching WithoutOverlapping lock pattern + $allKeys = $redis->keys('*'); + $lockKeys = []; + + foreach ($allKeys as $key) { + // Match cache lock keys: they contain 'laravel-queue-overlap' + if (preg_match('/overlap/i', $key)) { + $lockKeys[] = $key; + } + } + if (empty($lockKeys)) { + $this->info(' No cache locks found.'); + + return 0; + } + + $this->info(' Found '.count($lockKeys).' cache lock(s)'); + + foreach ($lockKeys as $lockKey) { + // Check TTL to identify stale locks + $ttl = $redis->ttl($lockKey); + + // TTL = -1 means no expiration (stale lock!) + // TTL = -2 means key doesn't exist + // TTL > 0 means lock is valid and will expire + if ($ttl === -1) { + if ($dryRun) { + $this->warn(" Would delete STALE lock (no expiration): {$lockKey}"); + } else { + $redis->del($lockKey); + $this->info(" ✓ Deleted STALE lock: {$lockKey}"); + } + $cleanedCount++; + } elseif ($ttl > 0) { + $this->line(" Skipping active lock (expires in {$ttl}s): {$lockKey}"); + } + } + + if ($cleanedCount === 0) { + $this->info(' No stale locks found (all locks have expiration set)'); + } + + return $cleanedCount; + } } diff --git a/app/Console/Commands/Init.php b/app/Console/Commands/Init.php index 6e8d18f61..4bc818f0a 100644 --- a/app/Console/Commands/Init.php +++ b/app/Console/Commands/Init.php @@ -73,7 +73,7 @@ public function handle() $this->cleanupUnusedNetworkFromCoolifyProxy(); try { - $this->call('cleanup:redis'); + $this->call('cleanup:redis', ['--clear-locks' => true]); } catch (\Throwable $e) { echo "Error in cleanup:redis command: {$e->getMessage()}\n"; } diff --git a/app/Jobs/ScheduledJobManager.php b/app/Jobs/ScheduledJobManager.php index 18ca0008c..9937444b8 100644 --- a/app/Jobs/ScheduledJobManager.php +++ b/app/Jobs/ScheduledJobManager.php @@ -52,7 +52,8 @@ public function middleware(): array { return [ (new WithoutOverlapping('scheduled-job-manager')) - ->releaseAfter(60), // Release the lock after 60 seconds if job fails + ->expireAfter(60) // Lock expires after 1 minute to prevent stale locks + ->dontRelease(), // Don't re-queue on lock conflict ]; } diff --git a/tests/Unit/ScheduledJobManagerLockTest.php b/tests/Unit/ScheduledJobManagerLockTest.php new file mode 100644 index 000000000..3f3ae593a --- /dev/null +++ b/tests/Unit/ScheduledJobManagerLockTest.php @@ -0,0 +1,60 @@ +middleware(); + + // Assert middleware exists + expect($middleware)->toBeArray() + ->and($middleware)->toHaveCount(1); + + $overlappingMiddleware = $middleware[0]; + + // Assert it's a WithoutOverlapping instance + expect($overlappingMiddleware)->toBeInstanceOf(WithoutOverlapping::class); + + // Use reflection to check private properties + $reflection = new ReflectionClass($overlappingMiddleware); + + // Check expireAfter is set (should be 60 seconds - matches job frequency) + $expiresAfterProperty = $reflection->getProperty('expiresAfter'); + $expiresAfterProperty->setAccessible(true); + $expiresAfter = $expiresAfterProperty->getValue($overlappingMiddleware); + + expect($expiresAfter)->toBe(60) + ->and($expiresAfter)->toBeGreaterThan(0, 'expireAfter must be set to prevent stale locks'); + + // Check releaseAfter is NOT set (we use dontRelease) + $releaseAfterProperty = $reflection->getProperty('releaseAfter'); + $releaseAfterProperty->setAccessible(true); + $releaseAfter = $releaseAfterProperty->getValue($overlappingMiddleware); + + expect($releaseAfter)->toBeNull('releaseAfter should be null when using dontRelease()'); + + // Check the lock key + $keyProperty = $reflection->getProperty('key'); + $keyProperty->setAccessible(true); + $key = $keyProperty->getValue($overlappingMiddleware); + + expect($key)->toBe('scheduled-job-manager'); +}); + +it('prevents stale locks by ensuring expireAfter is always set', function () { + $job = new ScheduledJobManager; + $middleware = $job->middleware(); + + $overlappingMiddleware = $middleware[0]; + $reflection = new ReflectionClass($overlappingMiddleware); + + $expiresAfterProperty = $reflection->getProperty('expiresAfter'); + $expiresAfterProperty->setAccessible(true); + $expiresAfter = $expiresAfterProperty->getValue($overlappingMiddleware); + + // Critical check: expireAfter MUST be set to prevent GitHub issue #4539 + expect($expiresAfter)->not->toBeNull( + 'expireAfter() is required to prevent stale locks (see GitHub #4539)' + ); +});