diff --git a/app/Jobs/DockerCleanupJob.php b/app/Jobs/DockerCleanupJob.php index 6730e7a976..1a338f093f 100644 --- a/app/Jobs/DockerCleanupJob.php +++ b/app/Jobs/DockerCleanupJob.php @@ -16,6 +16,7 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Cache; class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue { @@ -95,6 +96,7 @@ class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue $this->execution_log = DockerCleanupExecution::create([ 'server_id' => $this->server->id, ]); + $this->rememberExecution(); if (! $this->server->isFunctional()) { $this->execution_log->update([ @@ -204,21 +206,41 @@ class DockerCleanupJob implements ShouldBeEncrypted, ShouldQueue 'finished_at' => Carbon::now()->toImmutable(), ]); } + + if (! $failed && $this->executionCacheKey()) { + Cache::forget($this->executionCacheKey()); + } } } + /** + * failed() runs on a fresh job copy built from the queue payload (for example after a worker timeout), + * so the execution this run created is kept under the queue job uuid instead of on the job. + */ + private function rememberExecution(): void + { + if ($this->executionCacheKey()) { + Cache::put($this->executionCacheKey(), $this->execution_log->id, now()->addSeconds((int) config('queue.connections.redis.retry_after', 86400) + 3600)); + } + } + + private function executionCacheKey(): ?string + { + $jobUuid = $this->job?->uuid(); + + return $jobUuid ? 'docker-cleanup-execution:'.$jobUuid : null; + } + public function failed(?\Throwable $exception): void { if ($this->occurrenceUuid) { app(ScheduledJobDeliveryService::class)->fail($this->occurrenceUuid, $this->job?->uuid() ?? $this->occurrenceUuid); } - $execution = DockerCleanupExecution::query() - ->where('server_id', $this->server->id) - ->where('status', 'running') - ->whereNull('finished_at') - ->latest('id') - ->first(); + // Only this run's own execution. A run that never started (e.g. a manual run whose wait for the + // server lock ran out) created none and must not fail another running cleanup. + $executionId = $this->executionCacheKey() ? Cache::pull($this->executionCacheKey()) : null; + $execution = $executionId ? DockerCleanupExecution::query()->find($executionId) : null; if (! $execution) { return; diff --git a/tests/Feature/DockerCleanupJobTest.php b/tests/Feature/DockerCleanupJobTest.php index 37cafbd54d..31b12bb7ac 100644 --- a/tests/Feature/DockerCleanupJobTest.php +++ b/tests/Feature/DockerCleanupJobTest.php @@ -19,6 +19,8 @@ use App\Services\ScheduledJobDeliveryService; use App\Support\Actions\UniqueUntilProcessingJobDecorator; use Illuminate\Database\Schema\Blueprint; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Queue\Jobs\FakeJob; +use Illuminate\Queue\MaxAttemptsExceededException; use Illuminate\Support\Carbon; use Illuminate\Support\Collection; use Illuminate\Support\Facades\Artisan; @@ -134,26 +136,63 @@ it('creates a failed execution record when server is force disabled', function ( ->and($execution->message)->toContain('not functional'); }); -it('finishes the latest running execution when the job fails after a timeout', function () { - $user = User::factory()->create(); - $team = $user->teams()->first(); - $server = Server::factory()->create(['team_id' => $team->id]); +function dockerCleanupQueueJob(string $uuid): FakeJob +{ + return new class($uuid) extends FakeJob + { + public function __construct(private string $jobUuid) {} - $olderExecution = DockerCleanupExecution::create([ - 'server_id' => $server->id, - ]); - $timedOutExecution = DockerCleanupExecution::create([ - 'server_id' => $server->id, - ]); + public function getRawBody() + { + return json_encode(['uuid' => $this->jobUuid]); + } + }; +} - $job = new DockerCleanupJob($server); - $job->failed(new RuntimeException('Docker cleanup job has timed out.')); +it('finishes its own execution when the job fails after a timeout, not a newer running cleanup', function () { + Notification::fake(); + Event::fake([DockerCleanupDone::class]); + $server = dockerCleanupReachableServer(); + $queueJob = dockerCleanupQueueJob('prr-fix-timed-out-cleanup'); + $statesWhenTimedOut = null; + dockerCleanupFakeProcesses(function () use ($server, $queueJob, &$statesWhenTimedOut) { + if ($statesWhenTimedOut !== null) { + return; + } + $ownExecution = DockerCleanupExecution::query()->sole(); + $scheduledExecution = DockerCleanupExecution::create(['server_id' => $server->id]); - expect($timedOutExecution->refresh()->status)->toBe('failed') - ->and($timedOutExecution->message)->toBe('Docker cleanup job has timed out.') - ->and($timedOutExecution->finished_at)->not->toBeNull() - ->and($olderExecution->refresh()->status)->toBe('running') - ->and($olderExecution->finished_at)->toBeNull(); + // The worker timeout calls failed() on a fresh job copy built from the same queue payload. + (new DockerCleanupJob($server, true))->setJob($queueJob)->failed(new RuntimeException('Docker cleanup job has timed out.')); + + $statesWhenTimedOut = [ + 'own' => $ownExecution->fresh()->only(['status', 'message']), + 'ownFinished' => $ownExecution->fresh()->finished_at !== null, + 'scheduled' => $scheduledExecution->fresh()->only(['status', 'finished_at']), + ]; + }); + + (new DockerCleanupJob($server, true))->setJob($queueJob)->handle(); + + expect($statesWhenTimedOut['own'])->toBe(['status' => 'failed', 'message' => 'Docker cleanup job has timed out.']) + ->and($statesWhenTimedOut['ownFinished'])->toBeTrue() + ->and($statesWhenTimedOut['scheduled'])->toBe(['status' => 'running', 'finished_at' => null]); +}); + +it('leaves a running scheduled cleanup untouched when a manual cleanup fails before it started', function () { + Notification::fake(); + Event::fake([DockerCleanupDone::class]); + $server = dockerCleanupReachableServer(); + $scheduledExecution = DockerCleanupExecution::create(['server_id' => $server->id]); + + (new DockerCleanupJob($server, true)) + ->setJob(dockerCleanupQueueJob('prr-fix-waiting-manual-cleanup')) + ->failed(new MaxAttemptsExceededException('App\Jobs\DockerCleanupJob has been attempted too many times.')); + + expect($scheduledExecution->fresh()->status)->toBe('running') + ->and($scheduledExecution->fresh()->finished_at)->toBeNull(); + Notification::assertNothingSent(); + Event::assertNotDispatched(DockerCleanupDone::class); }); it('skips a scheduled cleanup occurrence while another cleanup holds the server lock, so it is not reported missed later', function () {