Fix duplicate cronjob during sync (#955)

saeedvaziry · web-flow · commit 3d3e93e05624 · 2025-12-18T20:44:33.000+01:00
* Fix duplicate cronjob during sync

* fix commented cronjobs

* fix normalize
diff --git a/app/Actions/CronJob/SyncCronJobs.php b/app/Actions/CronJob/SyncCronJobs.php
@@ -29,15 +29,17 @@ private function syncUserCronJobs(Server $server, string $user): void
         // Get existing cronjobs from server for this user
         $crontabOutput = $this->getUserCrontab($server, $user);
 
-        // Get all Vito-managed cronjobs for this user
+        // Get all Vito-managed cronjobs for this user (including both server-level and site-level)
         $vitoCronJobs = $server->cronJobs()
             ->where('user', $user)
-            ->where('site_id', null) // Only server-level cronjobs
             ->get();
 
+        // Filter only server-level cronjobs (site_id = null) for status updates
+        $serverLevelCronJobs = $vitoCronJobs->where('site_id', null);
+
         if (empty($crontabOutput)) {
-            // If crontab is empty, mark all Vito cronjobs as disabled
-            foreach ($vitoCronJobs as $cronJob) {
+            // If crontab is empty, mark all server-level Vito cronjobs as disabled
+            foreach ($serverLevelCronJobs as $cronJob) {
                 if ($cronJob->status === CronjobStatus::READY) {
                     $cronJob->update(['status' => CronjobStatus::DISABLED]);
                 }
@@ -72,34 +74,50 @@ private function syncUserCronJobs(Server $server, string $user): void
                 continue;
             }
 
-            $frequency = implode(' ', array_slice($parts, 0, 5));
-            $command = $parts[5];
+            // Validate that the first 5 parts look like cron time fields
+            // Valid cron fields contain: numbers, *, -, /, and ,
+            $isValidCronFormat = true;
+            for ($i = 0; $i < 5; $i++) {
+                if (! preg_match('/^[\d\*\-\/,]+$/', $parts[$i])) {
+                    $isValidCronFormat = false;
+                    break;
+                }
+            }
+
+            if (! $isValidCronFormat) {
+                continue;
+            }
+
+            $frequency = $this->normalizeFrequency(implode(' ', array_slice($parts, 0, 5)));
+            $command = $this->normalizeCommand($parts[5]);
 
             $serverCronJobs[] = [
                 'frequency' => $frequency,
                 'command' => $command,
                 'commented' => $isCommented,
             ];
 
-            // Check if this matches a Vito-managed cronjob
+            // Check if this matches any Vito-managed cronjob (including site-level ones)
             $matchingCronJob = $vitoCronJobs->first(function ($cronJob) use ($frequency, $command) {
-                return $cronJob->frequency === $frequency && $cronJob->command === $command;
+                return $this->normalizeFrequency($cronJob->frequency) === $frequency && $this->normalizeCommand($cronJob->command) === $command;
             });
 
             if ($matchingCronJob) {
                 $foundCronJobs[] = $matchingCronJob->id;
 
-                // Update status based on comment state
-                if ($isCommented && $matchingCronJob->status === CronjobStatus::READY) {
-                    $matchingCronJob->update(['status' => CronjobStatus::DISABLED]);
-                } elseif (! $isCommented && $matchingCronJob->status === CronjobStatus::DISABLED) {
-                    $matchingCronJob->update(['status' => CronjobStatus::READY]);
+                // Update status based on comment state (only for server-level cronjobs)
+                if ($matchingCronJob->site_id === null) {
+                    if ($isCommented && $matchingCronJob->status === CronjobStatus::READY) {
+                        $matchingCronJob->update(['status' => CronjobStatus::DISABLED]);
+                    } elseif (! $isCommented && $matchingCronJob->status === CronjobStatus::DISABLED) {
+                        $matchingCronJob->update(['status' => CronjobStatus::READY]);
+                    }
                 }
             }
         }
 
-        // Mark Vito cronjobs that are no longer on the server as disabled
-        foreach ($vitoCronJobs as $cronJob) {
+        // Mark server-level Vito cronjobs that are no longer on the server as disabled
+        foreach ($serverLevelCronJobs as $cronJob) {
             if (! in_array($cronJob->id, $foundCronJobs) && $cronJob->status === CronjobStatus::READY) {
                 $cronJob->update(['status' => CronjobStatus::DISABLED]);
             }
@@ -108,7 +126,7 @@ private function syncUserCronJobs(Server $server, string $user): void
         // Create new cronjobs for manually created ones (not in Vito)
         foreach ($serverCronJobs as $cronJobData) {
             $isVitoManaged = $vitoCronJobs->contains(function ($cronJob) use ($cronJobData) {
-                return $cronJob->frequency === $cronJobData['frequency'] && $cronJob->command === $cronJobData['command'];
+                return $this->normalizeFrequency($cronJob->frequency) === $cronJobData['frequency'] && $this->normalizeCommand($cronJob->command) === $cronJobData['command'];
             });
 
             if (! $isVitoManaged) {
@@ -124,6 +142,18 @@ private function syncUserCronJobs(Server $server, string $user): void
         }
     }
 
+    private function normalizeFrequency(string $frequency): string
+    {
+        // Normalize frequency by ensuring single spaces between parts
+        return preg_replace('/\s+/', ' ', trim($frequency));
+    }
+
+    private function normalizeCommand(string $command): string
+    {
+        // Normalize command by ensuring single spaces between parts
+        return preg_replace('/\s+/', ' ', trim($command));
+    }
+
     /**
      * @throws SSHError
      */
diff --git a/app/Models/CronJob.php b/app/Models/CronJob.php
@@ -9,7 +9,7 @@
 
 /**
  * @property int $server_id
- * @property int $site_id
+ * @property ?int $site_id
  * @property string $command
  * @property string $user
  * @property string $frequency
diff --git a/tests/Feature/SyncCronjobTest.php b/tests/Feature/SyncCronjobTest.php
@@ -353,4 +353,172 @@ public function test_sync_handles_mixed_scenarios_with_deletions(): void
             'site_id' => null,
         ]);
     }
+
+    public function test_sync_normalizes_frequency_with_extra_spaces(): void
+    {
+        // Create a cronjob with normal spacing
+        $existingCronJob = CronJob::factory()->create([
+            'server_id' => $this->server->id,
+            'user' => 'root',
+            'command' => '/usr/bin/backup.sh',
+            'frequency' => '5 15 * * *',
+            'status' => CronjobStatus::READY,
+            'site_id' => null,
+        ]);
+
+        // Mock SSH to return the same cronjob with extra spaces
+        SSH::fake('5  15   *    *  * /usr/bin/backup.sh');
+
+        $this->actingAs($this->user)
+            ->post(route('cronjobs.sync', $this->server))
+            ->assertRedirect()
+            ->assertSessionHas('success', 'Cron jobs synced successfully.');
+
+        // Should not create duplicate, existing cronjob should remain
+        $cronJobs = CronJob::where('server_id', $this->server->id)
+            ->where('command', '/usr/bin/backup.sh')
+            ->where('site_id', null)
+            ->get();
+
+        // Should only have the one existing cronjob for each user (root + vito = 2 total)
+        $this->assertCount(2, $cronJobs);
+
+        // The original cronjob should still be ready
+        $existingCronJob->refresh();
+        $this->assertEquals(CronjobStatus::READY, $existingCronJob->status);
+    }
+
+    public function test_sync_recognizes_site_level_cronjobs(): void
+    {
+        // Create a site-level cronjob with the same command as what will be on the server
+        $siteCronJob = CronJob::factory()->create([
+            'server_id' => $this->server->id,
+            'user' => 'root',
+            'command' => '/usr/bin/backup.sh',
+            'frequency' => '5 15 * * *',
+            'status' => CronjobStatus::READY,
+            'site_id' => $this->site->id,
+        ]);
+
+        // Mock SSH to return a cronjob with the same frequency and command
+        SSH::fake('5 15 * * * /usr/bin/backup.sh');
+
+        $countBefore = CronJob::where('server_id', $this->server->id)
+            ->where('command', '/usr/bin/backup.sh')
+            ->count();
+
+        $this->actingAs($this->user)
+            ->post(route('cronjobs.sync', $this->server))
+            ->assertRedirect()
+            ->assertSessionHas('success', 'Cron jobs synced successfully.');
+
+        $countAfter = CronJob::where('server_id', $this->server->id)
+            ->where('command', '/usr/bin/backup.sh')
+            ->count();
+
+        // Before fix: would create duplicate with site_id = null
+        // After fix: recognizes site-level cronjob and doesn't duplicate it, only creates for vito user
+        // countBefore = 1 (site-level), countAfter should be 2 (site-level + vito user)
+        $this->assertEquals($countBefore + 1, $countAfter);
+
+        // The site-level cronjob should remain unchanged
+        $siteCronJob->refresh();
+        $this->assertEquals($this->site->id, $siteCronJob->site_id);
+        $this->assertEquals(CronjobStatus::READY, $siteCronJob->status);
+    }
+
+    public function test_sync_handles_frequency_with_mixed_spacing_in_db(): void
+    {
+        // Create a cronjob with extra spaces in the frequency (simulating old data)
+        $existingCronJob = CronJob::factory()->create([
+            'server_id' => $this->server->id,
+            'user' => 'root',
+            'command' => '/usr/bin/backup.sh',
+            'frequency' => '5  15  *  *  *', // Double spaces
+            'status' => CronjobStatus::READY,
+            'site_id' => null,
+        ]);
+
+        // Mock SSH to return the same cronjob with normalized spacing
+        SSH::fake('5 15 * * * /usr/bin/backup.sh');
+
+        $this->actingAs($this->user)
+            ->post(route('cronjobs.sync', $this->server))
+            ->assertRedirect()
+            ->assertSessionHas('success', 'Cron jobs synced successfully.');
+
+        // Should not create duplicate
+        $cronJobs = CronJob::where('server_id', $this->server->id)
+            ->where('command', '/usr/bin/backup.sh')
+            ->where('site_id', null)
+            ->get();
+
+        // Should only have the one existing cronjob for each user (root + vito = 2 total)
+        $this->assertCount(2, $cronJobs);
+
+        // The original cronjob should still be ready
+        $existingCronJob->refresh();
+        $this->assertEquals(CronjobStatus::READY, $existingCronJob->status);
+    }
+
+    public function test_sync_ignores_crontab_documentation_comments(): void
+    {
+        // Mock SSH to return crontab with documentation comments (like the default crontab header)
+        $crontabWithComments = '# Edit this file to introduce tasks to be run by cron.
+#
+# Each task to run has to be defined through a single line
+# m h  dom mon dow   command
+#
+0 2 * * * /usr/bin/backup.sh';
+
+        SSH::fake($crontabWithComments);
+
+        $this->actingAs($this->user)
+            ->post(route('cronjobs.sync', $this->server))
+            ->assertRedirect()
+            ->assertSessionHas('success', 'Cron jobs synced successfully.');
+
+        // Should only create cronjobs for the actual cron line, not the documentation comments
+        $cronJobs = CronJob::where('server_id', $this->server->id)->get();
+
+        // Should have 2 cronjobs (1 for root, 1 for vito), not 6 (which would include the comment lines)
+        $this->assertCount(2, $cronJobs);
+
+        // Both should have the actual backup command
+        $this->assertTrue($cronJobs->every(fn ($cronJob) => $cronJob->command === '/usr/bin/backup.sh'));
+    }
+
+    public function test_sync_normalizes_command_with_extra_spaces(): void
+    {
+        // Create a cronjob with normal spacing in command
+        $existingCronJob = CronJob::factory()->create([
+            'server_id' => $this->server->id,
+            'user' => 'root',
+            'command' => 'ls -la',
+            'frequency' => '* * * * *',
+            'status' => CronjobStatus::READY,
+            'site_id' => null,
+        ]);
+
+        // Mock SSH to return the same cronjob with extra spaces in command
+        SSH::fake('* * *  * * ls  -la');
+
+        $this->actingAs($this->user)
+            ->post(route('cronjobs.sync', $this->server))
+            ->assertRedirect()
+            ->assertSessionHas('success', 'Cron jobs synced successfully.');
+
+        // Should not create duplicate, existing cronjob should remain
+        $cronJobs = CronJob::where('server_id', $this->server->id)
+            ->where('site_id', null)
+            ->get();
+
+        // Should only have the one existing cronjob for each user (root + vito = 2 total)
+        $this->assertCount(2, $cronJobs);
+
+        // The original cronjob should still be ready
+        $existingCronJob->refresh();
+        $this->assertEquals(CronjobStatus::READY, $existingCronJob->status);
+        $this->assertEquals('ls -la', $existingCronJob->command);
+    }
 }
