diff --git a/.ai/lessons.md b/.ai/lessons.md index 36a776d5a4..3452106a2e 100644 --- a/.ai/lessons.md +++ b/.ai/lessons.md @@ -154,3 +154,6 @@ ## Validate input safely - In PCRE, `$` also matches before a final `\n`. In validation regexes, use `\z` or reject control characters first, or a trailing newline passes. + +## Measure database reads end to end +- On PostgreSQL 15, `EXPLAIN (ANALYZE, BUFFERS)` does not detoast or send the selected columns, so a wide `jsonb`/`json`/`text` column does not show in it. Time the full fetch (`psql \timing` with `-o /dev/null`, or PDO `fetchAll`) as well before you blame or rule out an index. diff --git a/app/Jobs/ScheduledJobManager.php b/app/Jobs/ScheduledJobManager.php index 7e137dadaa..48b4b0dae1 100644 --- a/app/Jobs/ScheduledJobManager.php +++ b/app/Jobs/ScheduledJobManager.php @@ -17,6 +17,7 @@ use Cron\CronExpression; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\Relations\HasOne; use Illuminate\Database\Eloquent\Relations\MorphTo; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; @@ -741,7 +742,7 @@ class ScheduledJobManager implements ShouldQueue private function getServersForCleanupQuery(): Builder { - $query = Server::with('settings') + $query = Server::with(['settings' => fn (HasOne $query) => $query->withoutPatchCheckResult()]) ->whereNotNull('ip') ->where('ip', '!=', '') ->whereNotIn('ip', Server::PLACEHOLDER_IPS); diff --git a/app/Jobs/ServerManagerJob.php b/app/Jobs/ServerManagerJob.php index 815bb09361..f21f66a3e3 100644 --- a/app/Jobs/ServerManagerJob.php +++ b/app/Jobs/ServerManagerJob.php @@ -8,6 +8,7 @@ use App\Models\Team; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldBeEncrypted; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Database\Eloquent\Relations\HasOne; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\SerializesModels; @@ -78,11 +79,12 @@ class ServerManagerJob implements ShouldBeEncrypted, ShouldQueue private function getServers(): Collection { - $allServers = Server::with(['settings', 'cloudProviderToken']); + $relations = ['settings' => fn (HasOne $query) => $query->withoutPatchCheckResult(), 'cloudProviderToken']; + $allServers = Server::with($relations); if (isCloud()) { $servers = $allServers->whereRelation('team.subscription', 'stripe_invoice_paid', true)->get(); - $own = Team::find(0)->servers()->with(['settings', 'cloudProviderToken'])->get(); + $own = Team::find(0)->servers()->with($relations)->get(); return $servers->merge($own)->unique('id')->values(); } else { diff --git a/app/Models/ServerSetting.php b/app/Models/ServerSetting.php index f251b7fd5a..ae32835c10 100644 --- a/app/Models/ServerSetting.php +++ b/app/Models/ServerSetting.php @@ -5,10 +5,12 @@ namespace App\Models; use App\Enums\ServerRole; use App\Enums\TrafficIpMode; use Illuminate\Contracts\Encryption\DecryptException; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Schema; use OpenApi\Attributes as OA; #[OA\Schema( @@ -231,6 +233,9 @@ class ServerSetting extends Model 'geoip_maxmind_license_key', ]; + /** @var list|null */ + private static ?array $columnsWithoutPatchCheckResult = null; + protected static function booted() { static::creating(function ($setting) { @@ -465,6 +470,17 @@ class ServerSetting extends Model ); } + /** + * Selects every column except patch_check_result, which holds the full update list of the last patch + * check (often tens of kilobytes). Use it for instance-wide server loops that do not read the result. + */ + public function scopeWithoutPatchCheckResult(Builder $query): Builder + { + self::$columnsWithoutPatchCheckResult ??= array_values(array_diff(Schema::getColumnListing($this->getTable()), ['patch_check_result'])); + + return $query->select(self::$columnsWithoutPatchCheckResult); + } + /** * Summary of the saved patch check, or null when the server was never checked. * Results saved before security and reboot detection count as 0 and false. diff --git a/tests/Feature/ServerSettingsBulkLoadTest.php b/tests/Feature/ServerSettingsBulkLoadTest.php new file mode 100644 index 0000000000..befb493855 --- /dev/null +++ b/tests/Feature/ServerSettingsBulkLoadTest.php @@ -0,0 +1,118 @@ + true]); + InstanceSettings::forceCreate(['id' => 0, 'instance_timezone' => 'UTC']); + $this->team = Team::factory()->create(); + $this->patchResult = ['package_manager' => 'apt', 'total_updates' => 1, 'updates' => [['package' => 'openssl', 'new_version' => '3.0.13-2']]]; +}); + +afterEach(function () { + Carbon::setTestNow(); +}); + +function createBulkLoadServer(Team $team, array $settings): Server +{ + $server = Server::factory()->create(['team_id' => $team->id]); + $server->settings->forceFill([ + 'is_reachable' => true, + 'is_usable' => true, + 'server_timezone' => 'UTC', + ...$settings, + ])->saveQuietly(); + + return $server; +} + +/** + * @return list + */ +function serverSettingsQueriesDuring(Closure $callback): array +{ + $queries = []; + DB::listen(function ($query) use (&$queries) { + if (str_contains($query->sql, 'from "server_settings"')) { + $queries[] = $query->sql; + } + }); + $callback(); + + return $queries; +} + +it('loads every settings column except the patch result and matches each settings row to its server', function () { + $first = createBulkLoadServer($this->team, ['docker_cleanup_frequency' => '0 1 * * *', 'patch_check_result' => $this->patchResult]); + $second = createBulkLoadServer($this->team, ['docker_cleanup_frequency' => '0 2 * * *', 'patch_check_frequency' => 'daily']); + + $servers = Server::query() + ->whereKey([$first->id, $second->id]) + ->with(['settings' => fn (HasOne $query) => $query->withoutPatchCheckResult()]) + ->get() + ->keyBy('id'); + + foreach ([$first, $second] as $server) { + $full = ServerSetting::query()->where('server_id', $server->id)->firstOrFail()->getAttributes(); + unset($full['patch_check_result']); + + expect($servers[$server->id]->settings->getAttributes())->toBe($full); + } + expect($servers[$first->id]->settings->docker_cleanup_frequency)->toBe('0 1 * * *') + ->and($servers[$second->id]->settings->docker_cleanup_frequency)->toBe('0 2 * * *') + ->and($servers[$second->id]->settings->patch_check_frequency)->toBe('daily') + ->and($servers[$first->id]->settings->is_reachable)->toBeTrue(); +}); + +it('does not read the patch results of all servers in the server manager, and its jobs still get them', function () { + Queue::fake(); + Carbon::setTestNow(Carbon::create(2026, 10, 28, 12, 0, 0, 'UTC')); + $server = createBulkLoadServer($this->team, ['patch_check_frequency' => '* * * * *', 'patch_check_result' => $this->patchResult]); + + $queries = serverSettingsQueriesDuring(fn () => (new ServerManagerJob)->handle()); + + expect($queries)->not->toBeEmpty(); + foreach ($queries as $sql) { + expect($sql)->not->toContain('patch_check_result')->not->toContain('*'); + } + + Queue::assertPushed(ServerPatchCheckJob::class, function (ServerPatchCheckJob $job) use ($server) { + $restored = unserialize(serialize($job)); + + return $restored->server->id === $server->id + && $restored->server->settings->patch_check_result === $this->patchResult; + }); +}); + +it('does not read the patch results of all servers when it schedules Docker cleanups', function () { + Queue::fakeExcept(ScheduledJobManager::class); + Carbon::setTestNow(Carbon::create(2026, 10, 28, 12, 0, 0, 'UTC')); + $server = createBulkLoadServer($this->team, ['docker_cleanup_frequency' => '* * * * *', 'patch_check_result' => $this->patchResult]); + + $queries = serverSettingsQueriesDuring(fn () => (new ScheduledJobManager)->handle()); + + expect(collect($queries)->filter(fn (string $sql) => str_contains($sql, '"server_id" in')))->not->toBeEmpty(); + foreach ($queries as $sql) { + if (str_contains($sql, '"server_id" in')) { + expect($sql)->not->toContain('patch_check_result')->not->toContain('*'); + } + } + Queue::assertPushed(DockerCleanupJob::class, fn (DockerCleanupJob $job) => $job->server->id === $server->id); +});