fix(database): fail expired queued starts instead of running them late

A queued start or restart whose reservation expired no longer runs. Failed
queued starts are recorded as failed activities and release the reservation.

app/Actions/Database/RestartDatabase.php

@@ -26,9 +26,14 @@ class RestartDatabase
      */
     public function handle(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, ?string $reservation = null)
     {
-        $reservation ??= DatabaseOperationReservation::acquire($database->uuid);
         if ($reservation === null) {
-            return ResourceStartActivity::DATABASE_OPERATION_IN_PROGRESS_MESSAGE;
+            $reservation = DatabaseOperationReservation::acquire($database->uuid);
+            if ($reservation === null) {
+                return ResourceStartActivity::DATABASE_OPERATION_IN_PROGRESS_MESSAGE;
+            }
+        } elseif (! DatabaseOperationReservation::renew($database->uuid, $reservation)) {
+            // Do not stop the database for a request that expired in the queue.
+            return StartDatabase::operationInProgressError($database) ?? DatabaseOperationReservation::EXPIRED_MESSAGE;
         }
 
         try {
@@ -45,10 +50,17 @@ class RestartDatabase
                 return $prerequisiteError;
             }
             StopDatabase::run($database, dockerCleanup: false);
+            // The stop can take a while; keep the reservation for the start that follows.
+            DatabaseOperationReservation::renew($database->uuid, $reservation);
 
             return StartDatabase::run($database, $reservation);
         } finally {
             DatabaseOperationReservation::release($database->uuid, $reservation);
         }
     }
+
+    public function asJob(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, ?string $reservation = null): mixed
+    {
+        return StartDatabase::runQueued($database, 'Database restart failed.', fn (): mixed => $this->handle($database, $reservation));
+    }
 }

app/Actions/Database/StartDatabase.php

@@ -4,6 +4,7 @@ namespace App\Actions\Database;
 
 use App\Enums\ActivityTypes;
 use App\Enums\ProcessStatus;
+use App\Events\DatabaseStatusChanged;
 use App\Exceptions\DatabaseStartException;
 use App\Jobs\DatabaseStartJob;
 use App\Models\StandaloneClickhouse;
@@ -37,9 +38,13 @@ class StartDatabase
      */
     public function handle(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, ?string $reservation = null): Activity|string
     {
-        $reservation ??= DatabaseOperationReservation::acquire($database->uuid);
         if ($reservation === null) {
-            return ResourceStartActivity::DATABASE_OPERATION_IN_PROGRESS_MESSAGE;
+            $reservation = DatabaseOperationReservation::acquire($database->uuid);
+            if ($reservation === null) {
+                return ResourceStartActivity::DATABASE_OPERATION_IN_PROGRESS_MESSAGE;
+            }
+        } elseif (! DatabaseOperationReservation::renew($database->uuid, $reservation)) {
+            return self::expiredReservationError($database);
         }
 
         try {
@@ -49,6 +54,11 @@ class StartDatabase
         }
     }
 
+    public function asJob(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, ?string $reservation = null): Activity|string
+    {
+        return self::runQueued($database, 'Database start failed.', fn (): Activity|string => $this->handle($database, $reservation));
+    }
+
     private function queueStart(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, string $reservation): Activity|string
     {
         $server = $database->destination->server;
@@ -69,18 +79,7 @@ class StartDatabase
             'last_restart_type' => null,
         ]);
 
-        $activity = activity()
-            ->withProperties([
-                'server_uuid' => $server->uuid,
-                'type' => ActivityTypes::INLINE->value,
-                'type_uuid' => $database->uuid,
-                'status' => ProcessStatus::QUEUED->value,
-                'team_id' => $server->team_id,
-                'operation' => ResourceStartActivity::DATABASE_START_OPERATION,
-            ])
-            ->performedOn($database)
-            ->event(ActivityTypes::INLINE->value)
-            ->log('[]');
+        $activity = self::logQueuedStartActivity($database);
 
         if ($activity === null) {
             return 'Database start could not be queued because activity logging is disabled.';
@@ -107,6 +106,91 @@ class StartDatabase
         return $activity;
     }
 
+    private static function logQueuedStartActivity(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database): ?Activity
+    {
+        $server = $database->destination->server;
+
+        return activity()
+            ->withProperties([
+                'server_uuid' => $server->uuid,
+                'type' => ActivityTypes::INLINE->value,
+                'type_uuid' => $database->uuid,
+                'status' => ProcessStatus::QUEUED->value,
+                'team_id' => $server->team_id,
+                'operation' => ResourceStartActivity::DATABASE_START_OPERATION,
+            ])
+            ->performedOn($database)
+            ->event(ActivityTypes::INLINE->value)
+            ->log('[]');
+    }
+
+    /**
+     * A queued action whose reservation expired or was taken over must not run: a newer start
+     * may have run in the meantime. If another operation is in progress, that one reports its
+     * own result; otherwise the caller must record the expired request as failed.
+     */
+    private static function expiredReservationError(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database): string
+    {
+        return self::operationInProgressError($database) ?? DatabaseOperationReservation::EXPIRED_MESSAGE;
+    }
+
+    /**
+     * Run a queued start or restart. The request that queued it has already answered, so an
+     * error would be lost. Record it as a failed start activity, the same as a failed start
+     * job, so that the UI shows the error. An error that already has its own start activity
+     * (created by this run) or that reports another operation in progress is not recorded again.
+     *
+     * @param  callable(): (Activity|string|null)  $operation
+     */
+    public static function runQueued(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, string $failureMessage, callable $operation): mixed
+    {
+        $lastStartActivityId = self::latestStartActivityId($database);
+
+        try {
+            $result = $operation();
+        } catch (Throwable $e) {
+            if (self::latestStartActivityId($database) === $lastStartActivityId) {
+                self::recordFailedStart($database, $e instanceof DatabaseStartException ? $e->getMessage() : $failureMessage);
+            }
+
+            throw $e;
+        }
+
+        if (is_string($result)
+            && $result !== ResourceStartActivity::DATABASE_OPERATION_IN_PROGRESS_MESSAGE
+            && self::latestStartActivityId($database) === $lastStartActivityId) {
+            self::recordFailedStart($database, $result);
+        }
+
+        return $result;
+    }
+
+    public static function recordFailedStart(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database, string $message): void
+    {
+        try {
+            $activity = self::logQueuedStartActivity($database);
+            if ($activity !== null) {
+                ResourceStartActivity::markFailed($activity, $message);
+            }
+        } finally {
+            event(new DatabaseStatusChanged);
+        }
+    }
+
+    private static function latestStartActivityId(StandaloneRedis|StandalonePostgresql|StandaloneMongodb|StandaloneMysql|StandaloneMariadb|StandaloneKeydb|StandaloneDragonfly|StandaloneClickhouse|StandaloneSqlite $database): ?int
+    {
+        if (blank($database->uuid)) {
+            return null;
+        }
+
+        $id = Activity::query()
+            ->where('properties->type_uuid', $database->uuid)
+            ->where('properties->operation', ResourceStartActivity::DATABASE_START_OPERATION)
+            ->max('id');
+
+        return $id === null ? null : (int) $id;
+    }
+
     /**
      * Reserve a start or restart before it is queued, so that a parallel request cannot also
      * queue one before the queued action has created its activity. Pass the token to the

app/Support/DatabaseOperationReservation.php

@@ -20,6 +20,8 @@ class DatabaseOperationReservation
 {
     public const TTL_SECONDS = ResourceStartActivity::QUEUED_STALE_AFTER_SECONDS;
 
+    public const EXPIRED_MESSAGE = 'The queued start or restart waited too long in the queue and was not run. Start the database again.';
+
     public static function key(string $databaseUuid): string
     {
         return "database-operation-pending:{$databaseUuid}";
@@ -55,6 +57,30 @@ class DatabaseOperationReservation
         return $holder !== null && $holder !== $token;
     }
 
+    /**
+     * Confirm that the token still holds the reservation and extend it, so it does not expire
+     * while the operation runs. False when the reservation expired (the queued action waited
+     * longer than the TTL) or a newer request took it over: the operation must not run then.
+     */
+    public static function renew(?string $databaseUuid, ?string $token): bool
+    {
+        if ($token === null) {
+            return false;
+        }
+
+        if (blank($databaseUuid)) {
+            return true;
+        }
+
+        if (Cache::get(self::key($databaseUuid)) !== $token) {
+            return false;
+        }
+
+        Cache::put(self::key($databaseUuid), $token, self::TTL_SECONDS);
+
+        return true;
+    }
+
     /**
      * Release the reservation only if the token still holds it, so that an expired
      * reservation does not remove the reservation of a newer request.

tests/Feature/DatabaseStartConcurrencyTest.php

@@ -35,6 +35,7 @@ uses(RefreshDatabase::class);
 beforeEach(function () {
     InstanceSettings::forceCreate(['id' => 0]);
     config()->set('cache.default', 'array');
+    Server::flushIdentityMap();
 
     $this->team = Team::factory()->create();
     $this->user = User::factory()->create();
@@ -378,6 +379,139 @@ describe('pending operation reservation', function () {
             ->and(databaseStartActivityCount($this->database->uuid))->toBe(0);
     });
 
+    it('fails an expired queued start instead of running it late', function () {
+        Queue::fake();
+        Event::fake([DatabaseStatusChanged::class]);
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/start")
+            ->assertOk();
+
+        $this->travel(DatabaseOperationReservation::TTL_SECONDS + 1)->seconds();
+
+        expect(runPushedDatabaseAction(StartDatabase::class))->toBe(DatabaseOperationReservation::EXPIRED_MESSAGE)
+            ->and(databaseOperationReserved($this->database->uuid))->toBeFalse();
+        Queue::assertNotPushed(DatabaseStartJob::class);
+        Event::assertDispatched(DatabaseStatusChanged::class);
+
+        $failed = Activity::query()->where('properties->type_uuid', $this->database->uuid)->sole();
+        expect(data_get($failed, 'properties.status'))->toBe(ProcessStatus::ERROR->value)
+            ->and(data_get($failed, 'properties.operation'))->toBe(ResourceStartActivity::DATABASE_START_OPERATION)
+            ->and(data_get($failed, 'properties.error'))->toBe(DatabaseOperationReservation::EXPIRED_MESSAGE)
+            ->and(ResourceStartActivity::latestRunning($this->database->uuid))->toBeNull();
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/start")
+            ->assertOk();
+    });
+
+    it('does not start the database a second time when an expired queued start runs after a newer start finished', function () {
+        Queue::fake();
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/start")
+            ->assertOk();
+
+        $this->travel(DatabaseOperationReservation::TTL_SECONDS + 1)->seconds();
+        $uiActivity = StartDatabase::run($this->database);
+        expect($uiActivity)->toBeInstanceOf(Activity::class);
+        $uiActivity->properties = $uiActivity->properties->merge(['status' => ProcessStatus::FINISHED->value]);
+        $uiActivity->save();
+
+        expect(runPushedDatabaseAction(StartDatabase::class))->toBe(DatabaseOperationReservation::EXPIRED_MESSAGE);
+        Queue::assertPushed(DatabaseStartJob::class, 1);
+    });
+
+    it('fails an expired queued restart without stopping the database', function () {
+        Queue::fake();
+        StopDatabase::shouldRun()->never();
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/restart")
+            ->assertOk();
+
+        $this->travel(DatabaseOperationReservation::TTL_SECONDS + 1)->seconds();
+
+        expect(runPushedDatabaseAction(RestartDatabase::class))->toBe(DatabaseOperationReservation::EXPIRED_MESSAGE);
+        Queue::assertNotPushed(DatabaseStartJob::class);
+        $failed = Activity::query()->where('properties->type_uuid', $this->database->uuid)->sole();
+        expect(data_get($failed, 'properties.error'))->toBe(DatabaseOperationReservation::EXPIRED_MESSAGE);
+    });
+
+    it('keeps the reservation of a queued restart while the database stops', function () {
+        Queue::fake();
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/restart")
+            ->assertOk();
+
+        $this->travel(DatabaseOperationReservation::TTL_SECONDS - 10)->seconds();
+        StopDatabase::shouldRun()->once()->andReturnUsing(function () {
+            $this->travel(20)->seconds();
+
+            return 'Database stopped successfully';
+        });
+
+        expect(runPushedDatabaseAction(RestartDatabase::class))->toBeInstanceOf(Activity::class);
+        Queue::assertPushed(DatabaseStartJob::class, 1);
+    });
+
+    it('records a failed start when the queued action cannot start the database', function () {
+        Queue::fake();
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/start")
+            ->assertOk();
+
+        $job = Queue::pushed(JobDecorator::class, fn (JobDecorator $job) => $job->getAction() instanceof StartDatabase)->last();
+        $job->getParameters()[0]->destination->server->settings->is_reachable = false;
+
+        expect($job->handle())->toBe('Server is not functional')
+            ->and(databaseOperationReserved($this->database->uuid))->toBeFalse();
+        $failed = Activity::query()->where('properties->type_uuid', $this->database->uuid)->sole();
+        expect(data_get($failed, 'properties.status'))->toBe(ProcessStatus::ERROR->value)
+            ->and(data_get($failed, 'properties.error'))->toBe('Server is not functional');
+    });
+
+    it('records a failed start when the queued restart action throws', function () {
+        Queue::fake();
+        StopDatabase::shouldRun()->once()->andThrow(new RuntimeException('ssh: connection refused'));
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/restart")
+            ->assertOk();
+
+        expect(fn () => runPushedDatabaseAction(RestartDatabase::class))->toThrow(RuntimeException::class);
+
+        expect(databaseOperationReserved($this->database->uuid))->toBeFalse();
+        $failed = Activity::query()->where('properties->type_uuid', $this->database->uuid)->sole();
+        expect(data_get($failed, 'properties.status'))->toBe(ProcessStatus::ERROR->value)
+            ->and(data_get($failed, 'properties.error'))->toBe('Database restart failed.');
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/start")
+            ->assertOk();
+    });
+
+    it('does not record a second failed start when the queued start already failed its own activity', function () {
+        Queue::fake();
+
+        $this->withHeaders(databaseStartApiHeaders($this->bearerToken))
+            ->postJson("/api/v1/databases/{$this->database->uuid}/start")
+            ->assertOk();
+
+        $job = Queue::pushed(JobDecorator::class, fn (JobDecorator $job) => $job->getAction() instanceof StartDatabase)->last();
+        $queue = Queue::partialMock();
+        $queue->shouldReceive('connection')->andReturnSelf();
+        $queue->shouldReceive('push', 'pushOn')->andThrow(new RuntimeException('redis down'));
+
+        expect(fn () => $job->handle())->toThrow(RuntimeException::class, 'redis down');
+
+        $activities = Activity::query()->where('properties->type_uuid', $this->database->uuid)->get();
+        expect($activities)->toHaveCount(1)
+            ->and(data_get($activities->first(), 'properties.error'))->toBe('Database start could not be queued.');
+    });
+
     it('does not start from the UI while an API start is still in the queue', function () {
         Queue::fake();