Merge pull request #6975 from coollabsio/fix-cron-validation-errors

Fix stale lock issue causing scheduled tasks to stop (#4539)

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;
+    }
 }

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";
         }

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
         ];
     }
 

tests/Unit/ScheduledJobManagerLockTest.php

@@ -0,0 +1,60 @@
+<?php
+
+use App\Jobs\ScheduledJobManager;
+use Illuminate\Queue\Middleware\WithoutOverlapping;
+
+it('uses WithoutOverlapping middleware with expireAfter to prevent stale locks', function () {
+    $job = new ScheduledJobManager;
+    $middleware = $job->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)'
+    );
+});