From 5bd3f59455425458ceaf5bb625ba50dd8c707079 Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Mon, 3 Sep 2018 14:32:33 -0700 Subject: [PATCH] Fix schedules running twice, closes #1288 --- CHANGELOG.md | 4 ++ .../Schedule/ProcessRunnableCommand.php | 5 +++ .../Server/Tasks/ActionController.php | 3 +- .../Eloquent/ScheduleRepository.php | 1 + .../Schedules/ProcessScheduleService.php | 39 +------------------ .../Schedules/ProcessScheduleServiceTest.php | 38 +++--------------- 6 files changed, 18 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab0365a17..d6f9a3a1d 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/app/Console/Commands/Schedule/ProcessRunnableCommand.php b/app/Console/Commands/Schedule/ProcessRunnableCommand.php index 89e8fac8e..e5a40fb43 100644 --- a/app/Console/Commands/Schedule/ProcessRunnableCommand.php +++ b/app/Console/Commands/Schedule/ProcessRunnableCommand.php @@ -57,6 +57,11 @@ class ProcessRunnableCommand extends Command 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) { diff --git a/app/Http/Controllers/Server/Tasks/ActionController.php b/app/Http/Controllers/Server/Tasks/ActionController.php index 8d5a83bd7..498db8537 100644 --- a/app/Http/Controllers/Server/Tasks/ActionController.php +++ b/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 @@ class ActionController extends Controller $server = $request->attributes->get('server'); $this->authorize('toggle-schedule', $server); - $this->processScheduleService->setRunTimeOverride(Chronos::now())->handle( + $this->processScheduleService->handle( $request->attributes->get('schedule') ); diff --git a/app/Repositories/Eloquent/ScheduleRepository.php b/app/Repositories/Eloquent/ScheduleRepository.php index a7b53602b..1d2dda244 100644 --- a/app/Repositories/Eloquent/ScheduleRepository.php +++ b/app/Repositories/Eloquent/ScheduleRepository.php @@ -75,6 +75,7 @@ class ScheduleRepository extends EloquentRepository implements ScheduleRepositor { return $this->getBuilder()->with('tasks') ->where('is_active', true) + ->where('is_processing', false) ->where('next_run_at', '<=', $timestamp) ->get($this->getColumns()); } diff --git a/app/Services/Schedules/ProcessScheduleService.php b/app/Services/Schedules/ProcessScheduleService.php index 07275950c..4ada2aee8 100644 --- a/app/Services/Schedules/ProcessScheduleService.php +++ b/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 @@ class ProcessScheduleService $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 @@ class ProcessScheduleService $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 @@ class ProcessScheduleService (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()); - } } diff --git a/tests/Unit/Services/Schedules/ProcessScheduleServiceTest.php b/tests/Unit/Services/Schedules/ProcessScheduleServiceTest.php index febde488e..40c087c1c 100644 --- a/tests/Unit/Services/Schedules/ProcessScheduleServiceTest.php +++ b/tests/Unit/Services/Schedules/ProcessScheduleServiceTest.php @@ -5,7 +5,6 @@ namespace Tests\Unit\Services\Schedules; 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 @@ class ProcessScheduleServiceTest extends TestCase { 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 @@ class ProcessScheduleServiceTest extends TestCase $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 @@ class ProcessScheduleServiceTest extends TestCase $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);