Fix schedules running twice, closes pterodactyl#1288

Author: DaneEveritt

CHANGELOG.md

@@ -3,6 +3,10 @@ This file is a running track of new features and fixes to each version of the pa
 
 This project follows [Semantic Versioning](http://semver.org) guidelines.
 
+## v0.7.10 (Derelict Dermodactylus)
+### Fixed
+* Scheduled tasks triggered manually no longer improperly change the `next_run_at` time and do not run twice in a row anymore.
+
 ## v0.7.9 (Derelict Dermodactylus)
 ### Fixed
 * Fixes a two-factor authentication bypass present in the password reset process for an account.

app/Console/Commands/Schedule/ProcessRunnableCommand.php

@@ -57,6 +57,11 @@ public function __construct(ProcessScheduleService $processScheduleService, Sche
     public function handle()
     {
         $schedules = $this->repository->getSchedulesToProcess(Chronos::now()->toAtomString());
+        if ($schedules->count() < 1) {
+            $this->line('There are no scheduled tasks for servers that need to be run.');
+
+            return;
+        }
 
         $bar = $this->output->createProgressBar(count($schedules));
         $schedules->each(function ($schedule) use ($bar) {

app/Http/Controllers/Server/Tasks/ActionController.php

@@ -2,7 +2,6 @@
 
 namespace Pterodactyl\Http\Controllers\Server\Tasks;
 
-use Cake\Chronos\Chronos;
 use Illuminate\Http\Request;
 use Illuminate\Http\Response;
 use Pterodactyl\Http\Controllers\Controller;
@@ -71,7 +70,7 @@ public function trigger(Request $request): Response
         $server = $request->attributes->get('server');
         $this->authorize('toggle-schedule', $server);
 
-        $this->processScheduleService->setRunTimeOverride(Chronos::now())->handle(
+        $this->processScheduleService->handle(
             $request->attributes->get('schedule')
         );
 

app/Repositories/Eloquent/ScheduleRepository.php

@@ -75,6 +75,7 @@ public function getSchedulesToProcess(string $timestamp): Collection
     {
         return $this->getBuilder()->with('tasks')
             ->where('is_active', true)
+            ->where('is_processing', false)
             ->where('next_run_at', '<=', $timestamp)
             ->get($this->getColumns());
     }

app/Services/Schedules/ProcessScheduleService.php

@@ -2,11 +2,8 @@
 
 namespace Pterodactyl\Services\Schedules;
 
-use DateTimeInterface;
 use Cron\CronExpression;
-use Cake\Chronos\Chronos;
 use Pterodactyl\Models\Schedule;
-use Cake\Chronos\ChronosInterface;
 use Illuminate\Contracts\Bus\Dispatcher;
 use Pterodactyl\Jobs\Schedule\RunTaskJob;
 use Pterodactyl\Contracts\Repository\TaskRepositoryInterface;
@@ -19,11 +16,6 @@ class ProcessScheduleService
      */
     private $dispatcher;
 
-    /**
-     * @var \DateTimeInterface|null
-     */
-    private $runTimeOverride;
-
     /**
      * @var \Pterodactyl\Contracts\Repository\ScheduleRepositoryInterface
      */
@@ -51,20 +43,6 @@ public function __construct(
         $this->taskRepository = $taskRepository;
     }
 
-    /**
-     * Set the time that this schedule should be run at. This will override the time
-     * defined on the schedule itself. Useful for triggering one-off task runs.
-     *
-     * @param \DateTimeInterface $time
-     * @return $this
-     */
-    public function setRunTimeOverride(DateTimeInterface $time)
-    {
-        $this->runTimeOverride = $time;
-
-        return $this;
-    }
-
     /**
      * Process a schedule and push the first task onto the queue worker.
      *
@@ -89,7 +67,7 @@ public function handle(Schedule $schedule)
 
         $this->scheduleRepository->update($schedule->id, [
             'is_processing' => true,
-            'next_run_at' => $this->getRunAtTime($formattedCron),
+            'next_run_at' => CronExpression::factory($formattedCron)->getNextRunDate(),
         ]);
 
         $this->taskRepository->update($task->id, ['is_queued' => true]);
@@ -98,19 +76,4 @@ public function handle(Schedule $schedule)
             (new RunTaskJob($task->id, $schedule->id))->delay($task->time_offset)
         );
     }
-
-    /**
-     * Get the timestamp to store in the database as the next_run time for a schedule.
-     *
-     * @param string $formatted
-     * @return \Cake\Chronos\ChronosInterface
-     */
-    private function getRunAtTime(string $formatted): ChronosInterface
-    {
-        if (! is_null($this->runTimeOverride)) {
-            return $this->runTimeOverride instanceof ChronosInterface ? $this->runTimeOverride : Chronos::instance($this->runTimeOverride);
-        }
-
-        return Chronos::instance(CronExpression::factory($formatted)->getNextRunDate());
-    }
 }

tests/Unit/Services/Schedules/ProcessScheduleServiceTest.php

@@ -5,7 +5,6 @@
 use Mockery as m;
 use Tests\TestCase;
 use Cron\CronExpression;
-use Cake\Chronos\Chronos;
 use Pterodactyl\Models\Task;
 use Pterodactyl\Models\Schedule;
 use Illuminate\Contracts\Bus\Dispatcher;
@@ -38,7 +37,6 @@ public function setUp()
     {
         parent::setUp();
 
-        Chronos::setTestNow(Chronos::now());
         $this->dispatcher = m::mock(Dispatcher::class);
         $this->scheduleRepository = m::mock(ScheduleRepositoryInterface::class);
         $this->taskRepository = m::mock(TaskRepositoryInterface::class);
@@ -59,7 +57,7 @@ public function testScheduleIsUpdatedAndRun()
         $formatted = sprintf('%s %s %s * %s', $model->cron_minute, $model->cron_hour, $model->cron_day_of_month, $model->cron_day_of_week);
         $this->scheduleRepository->shouldReceive('update')->with($model->id, [
             'is_processing' => true,
-            'next_run_at' => Chronos::parse(CronExpression::factory($formatted)->getNextRunDate()->format(Chronos::ATOM)),
+            'next_run_at' => CronExpression::factory($formatted)->getNextRunDate(),
         ]);
 
         $this->taskRepository->shouldReceive('update')->with($task->id, ['is_queued' => true])->once();
@@ -77,35 +75,11 @@ public function testScheduleIsUpdatedAndRun()
         $this->assertTrue(true);
     }
 
-    public function testScheduleRunTimeCanBeOverridden()
-    {
-        $model = factory(Schedule::class)->make();
-        $model->setRelation('tasks', collect([$task = factory(Task::class)->make([
-            'sequence_id' => 1,
-        ])]));
-
-        $this->scheduleRepository->shouldReceive('loadTasks')->with($model)->once()->andReturn($model);
-
-        $this->scheduleRepository->shouldReceive('update')->with($model->id, [
-            'is_processing' => true,
-            'next_run_at' => Chronos::now()->addSeconds(15),
-        ]);
-
-        $this->taskRepository->shouldReceive('update')->with($task->id, ['is_queued' => true])->once();
-
-        $this->dispatcher->shouldReceive('dispatch')->with(m::on(function ($class) use ($model, $task) {
-            $this->assertInstanceOf(RunTaskJob::class, $class);
-            $this->assertSame($task->time_offset, $class->delay);
-            $this->assertSame($task->id, $class->task);
-            $this->assertSame($model->id, $class->schedule);
-
-            return true;
-        }))->once();
-
-        $this->getService()->setRunTimeOverride(Chronos::now()->addSeconds(15))->handle($model);
-        $this->assertTrue(true);
-    }
-
+    /**
+     * Return an instance of the service for testing purposes.
+     *
+     * @return \Pterodactyl\Services\Schedules\ProcessScheduleService
+     */
     private function getService(): ProcessScheduleService
     {
         return new ProcessScheduleService($this->dispatcher, $this->scheduleRepository, $this->taskRepository);