From 6dc332c2035f922dd2c9c4dbf5fadf041531f6a6 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:10:10 +0200 Subject: [PATCH] perf(sentinel): cache traffic per minute, skip unsafe keys, batch owner lookups - Align the traffic window to the minute so the 60 s cache is used. - Skip only an unsafe key instead of hiding the whole server. - Look up unknown container owners in one query per push. --- app/Jobs/PushServerUpdateJob.php | 10 +++- app/Livewire/Analytics.php | 9 +-- app/Livewire/Dashboard/TrafficAnalytics.php | 9 +-- app/Services/SentinelTrafficClient.php | 48 +++++++++++----- bootstrap/helpers/docker.php | 43 +++++++++++++- .../Feature/ContainerOwnerUuidLabelsTest.php | 48 ++++++++++++++++ .../TrafficAnalytics/GlobalAnalyticsTest.php | 57 +++++++++++++++++++ .../SentinelTrafficCacheTest.php | 27 +++++++++ .../SentinelTrafficClientTest.php | 51 +++++++++++++++++ 9 files changed, 270 insertions(+), 32 deletions(-) diff --git a/app/Jobs/PushServerUpdateJob.php b/app/Jobs/PushServerUpdateJob.php index 3224d3a5e1..8bd3420859 100644 --- a/app/Jobs/PushServerUpdateJob.php +++ b/app/Jobs/PushServerUpdateJob.php @@ -235,6 +235,14 @@ class PushServerUpdateJob implements ShouldBeEncrypted, ShouldQueue, Silenced $this->allServiceApplicationIds = $this->serviceApplicationsById->keys(); $this->allServiceDatabaseIds = $this->serviceDatabasesById->keys(); + // Owners of application containers outside this server's applications, in one query. + $foreignApplicationIdsByUuid = containerApplicationIdsByUuid( + $this->applications, + $this->containers + ->map(fn ($container) => collect(data_get($container, 'labels'))) + ->filter(fn (Collection $labels) => $labels->has('coolify.managed') && isContainerOfType($labels, 'application')) + ); + foreach ($this->containers as $container) { $containerStatus = data_get($container, 'state', 'exited'); $rawHealthStatus = data_get($container, 'health_status'); @@ -259,7 +267,7 @@ class PushServerUpdateJob implements ShouldBeEncrypted, ShouldQueue, Silenced } // Containers are matched by owner UUID; numeric ids differ between instances. if (isContainerOfType($labels, 'application')) { - $applicationId = resolveContainerApplicationId($this->applications, $labels); + $applicationId = resolveContainerApplicationId($this->applications, $labels, $foreignApplicationIdsByUuid); if ($applicationId === null) { continue; } diff --git a/app/Livewire/Analytics.php b/app/Livewire/Analytics.php index 9135147876..961bd545d6 100644 --- a/app/Livewire/Analytics.php +++ b/app/Livewire/Analytics.php @@ -560,14 +560,7 @@ class Analytics extends Component */ private function window(): array { - $to = now(); - $from = match ($this->range) { - '7d' => now()->subDays(7), - '30d' => now()->subDays(30), - default => now()->subDay(), - }; - - return [$from->toIso8601ZuluString(), $to->toIso8601ZuluString()]; + return SentinelTrafficClient::rangeWindow($this->range); } public function placeholder(array $params = []): View diff --git a/app/Livewire/Dashboard/TrafficAnalytics.php b/app/Livewire/Dashboard/TrafficAnalytics.php index c24d7dac79..2f67705ca8 100644 --- a/app/Livewire/Dashboard/TrafficAnalytics.php +++ b/app/Livewire/Dashboard/TrafficAnalytics.php @@ -141,14 +141,7 @@ class TrafficAnalytics extends Component */ private function window(): array { - $to = now(); - $from = match ($this->range) { - '7d' => now()->subDays(7), - '30d' => now()->subDays(30), - default => now()->subDay(), - }; - - return [$from->toIso8601ZuluString(), $to->toIso8601ZuluString()]; + return SentinelTrafficClient::rangeWindow($this->range); } public function placeholder(): View diff --git a/app/Services/SentinelTrafficClient.php b/app/Services/SentinelTrafficClient.php index 89c24bbbbe..259a54d00d 100644 --- a/app/Services/SentinelTrafficClient.php +++ b/app/Services/SentinelTrafficClient.php @@ -9,6 +9,7 @@ use App\Data\Traffic\TrafficSeriesBucketData; use App\Models\Server; use Illuminate\Support\Collection; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Log; class SentinelTrafficClient { @@ -57,15 +58,20 @@ class SentinelTrafficClient /** * Convert a UI range key (24h/7d/30d) into ISO-8601 Zulu from/to bounds. * + * The window ends at the next full minute instead of the current second: the bounds are + * part of the 60s response cache key, so every load within that minute reuses the cached + * response. The window still covers "now" (Sentinel rolls traffic up per minute, so the + * end in the future adds nothing) and spans exactly the requested range. + * * @return array{0: string, 1: string} */ public static function rangeWindow(string $range): array { - $to = now(); + $to = now()->utc()->ceilMinute(); $from = match ($range) { - '7d' => now()->subDays(7), - '30d' => now()->subDays(30), - default => now()->subDay(), + '7d' => $to->copy()->subDays(7), + '30d' => $to->copy()->subDays(30), + default => $to->copy()->subDay(), }; return [$from->toIso8601ZuluString(), $to->toIso8601ZuluString()]; @@ -352,10 +358,7 @@ class SentinelTrafficClient return []; } - return array_values(array_filter( - array_map(fn ($app) => is_array($app) ? ($app['uuid'] ?? null) : null, $bundle['apps'] ?? []), - fn ($uuid) => is_string($uuid) && $uuid !== '' - )); + return $this->safeReportedKeys(array_map(fn ($app) => is_array($app) ? ($app['uuid'] ?? null) : null, $bundle['apps'] ?? [])); } // Fallback for older Sentinel without /traffic/dashboard: batch the individual endpoints. @@ -374,10 +377,29 @@ class SentinelTrafficClient return []; } - return array_values(array_filter( - $this->apps(), - fn ($uuid) => is_string($uuid) && $uuid !== '' - )); + return $this->safeReportedKeys($this->apps()); + } + + /** + * The usable keys of a Sentinel-reported key list. A key that would be unsafe in a request + * url is dropped (and logged) on its own, so it doesn't fail the whole server's analytics. + * + * @param array $keys + * @return array + */ + private function safeReportedKeys(array $keys): array + { + $keys = array_values(array_filter($keys, fn ($key) => is_string($key) && $key !== '')); + $safe = array_values(array_filter($keys, fn (string $key) => $this->isSafeKey($key))); + + if (count($safe) !== count($keys)) { + Log::warning('Traffic analytics skipped unsafe app keys reported by Sentinel', [ + 'server' => $this->server->uuid, + 'skipped' => count($keys) - count($safe), + ]); + } + + return $safe; } /** @@ -509,7 +531,7 @@ class SentinelTrafficClient foreach ($bundle['apps'] ?? [] as $app) { $uuid = is_array($app) ? ($app['uuid'] ?? null) : null; - if (is_string($uuid) && $uuid !== '' && isset($app['overview'])) { + if (is_string($uuid) && $this->isSafeKey($uuid) && isset($app['overview'])) { $put($this->overviewUrl($uuid, $from, $to), $app['overview']); } } diff --git a/bootstrap/helpers/docker.php b/bootstrap/helpers/docker.php index e2f5947dbe..8883cd0cab 100644 --- a/bootstrap/helpers/docker.php +++ b/bootstrap/helpers/docker.php @@ -118,8 +118,11 @@ function resolveContainerOwner(Collection $resources, Collection|array|string $l /** * Id of the application that owns a container, also for applications not in $applications * (for example the parent of a preview). Same order as resolveContainerOwner(). + * + * @param array|null $applicationIdsByUuid Owners outside $applications, preloaded with + * containerApplicationIdsByUuid(); null queries per call. */ -function resolveContainerApplicationId(Collection $applications, Collection|array|string $labels): ?int +function resolveContainerApplicationId(Collection $applications, Collection|array|string $labels, ?array $applicationIdsByUuid = null): ?int { $labels = is_string($labels) ? format_docker_labels_to_json($labels) : collect($labels); @@ -129,11 +132,47 @@ function resolveContainerApplicationId(Collection $applications, Collection|arra } $uuid = containerOwnerUuid($labels, 'application'); - $id = $uuid ? Application::withTrashed()->where('uuid', $uuid)->value('id') : null; + if (! $uuid) { + return null; + } + if ($applicationIdsByUuid !== null) { + return $applicationIdsByUuid[$uuid] ?? null; + } + + $id = Application::withTrashed()->where('uuid', $uuid)->value('id'); return $id !== null ? (int) $id : null; } +/** + * Ids of the applications (including trashed ones) that own the given containers but are not in + * $applications, keyed by owner UUID, in one query. Pass the result to resolveContainerApplicationId() + * so a batch of containers does not query once per container. + * + * @param iterable $containerLabels + * @return array + */ +function containerApplicationIdsByUuid(Collection $applications, iterable $containerLabels): array +{ + $knownUuids = $applications->pluck('uuid')->flip(); + $uuids = []; + foreach ($containerLabels as $labels) { + $uuid = containerOwnerUuid($labels, 'application'); + if ($uuid && ! $knownUuids->has($uuid)) { + $uuids[$uuid] = true; + } + } + if ($uuids === []) { + return []; + } + + return Application::withTrashed() + ->whereIn('uuid', array_keys($uuids)) + ->pluck('id', 'uuid') + ->map(fn ($id) => (int) $id) + ->all(); +} + /** * Whether a container belongs to a resource of the given type (by UUID or older numeric id label). * diff --git a/tests/Feature/ContainerOwnerUuidLabelsTest.php b/tests/Feature/ContainerOwnerUuidLabelsTest.php index 18155c2a63..67957a181d 100644 --- a/tests/Feature/ContainerOwnerUuidLabelsTest.php +++ b/tests/Feature/ContainerOwnerUuidLabelsTest.php @@ -16,6 +16,7 @@ use App\Models\StandaloneDocker; use App\Models\StandalonePostgresql; use App\Models\Team; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Process; uses(RefreshDatabase::class); @@ -198,6 +199,53 @@ describe('status updates after a server transfer', function () { ->and($this->application->fresh()->status)->toStartWith('running'); }); + test('Sentinel push looks up foreign application owners with one query for all containers', function () { + $deleted = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'destination_id' => $this->destination->id, + 'destination_type' => $this->destination->getMorphClass(), + ]); + $deleted->delete(); + + $containers = collect(range(1, 5))->map(fn (int $i) => [ + 'name' => "foreign-{$i}", + 'state' => 'running', + 'health_status' => 'healthy', + 'labels' => [ + 'coolify.managed' => 'true', + 'coolify.applicationUuid' => "foreign-app-{$i}", + 'coolify.pullRequestId' => '0', + 'com.docker.compose.service' => "foreign-app-{$i}", + ], + ])->push([ + 'name' => $deleted->uuid.'-pr-3', + 'state' => 'running', + 'health_status' => 'healthy', + 'labels' => [ + 'coolify.managed' => 'true', + 'coolify.applicationUuid' => $deleted->uuid, + 'coolify.pullRequestId' => '3', + 'com.docker.compose.service' => $deleted->uuid.'-pr-3', + ], + ])->all(); + + $job = new PushServerUpdateJob($this->server, ['containers' => $containers]); + + DB::enableQueryLog(); + $job->handle(); + $ownerLookups = collect(DB::getQueryLog()) + ->filter(fn (array $query) => str_contains($query['query'], 'from "applications"') && preg_match('/"uuid" (=|in) /', $query['query']) === 1) + ->count(); + DB::disableQueryLog(); + + $previewLabels = end($containers)['labels']; + expect($ownerLookups)->toBe(1) + // A trashed owner still resolves, with and without the preloaded ids. + ->and(resolveContainerApplicationId(collect(), $previewLabels))->toBe($deleted->id) + ->and(resolveContainerApplicationId(collect(), $previewLabels, containerApplicationIdsByUuid(collect(), [$previewLabels])))->toBe($deleted->id) + ->and(containerApplicationIdsByUuid(collect([$this->application]), [['coolify.applicationUuid' => $this->application->uuid]]))->toBe([]); + }); + test('Sentinel push matches a new application container by its UUID label', function () { $job = new PushServerUpdateJob($this->server, ['containers' => [[ 'name' => $this->application->uuid, diff --git a/tests/Feature/TrafficAnalytics/GlobalAnalyticsTest.php b/tests/Feature/TrafficAnalytics/GlobalAnalyticsTest.php index 420af1ae97..b5cd80f6b5 100644 --- a/tests/Feature/TrafficAnalytics/GlobalAnalyticsTest.php +++ b/tests/Feature/TrafficAnalytics/GlobalAnalyticsTest.php @@ -12,6 +12,7 @@ use App\Models\Team; use App\Models\User; use App\Services\SentinelTrafficClient; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Carbon; use Livewire\Livewire; uses(RefreshDatabase::class); @@ -425,3 +426,59 @@ it('shows the not-enabled empty state when no server has traffic analytics on', ->assertSee('Traffic analytics is not enabled') ->assertDontSee('Unique visitors'); }); + +it('keeps a server visible when Sentinel reports one unsafe app key', function () { + $server = bootEnabledGlobalServer(); + + $project = Project::factory()->create(['team_id' => $this->team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + $destination = StandaloneDocker::where('server_id', $server->id)->first() + ?? StandaloneDocker::factory()->create(['server_id' => $server->id, 'network' => 'coolify-test']); + $application = Application::factory()->create([ + 'name' => 'Safe Leaderboard App', + 'environment_id' => $environment->id, + 'destination_id' => $destination->id, + 'destination_type' => StandaloneDocker::class, + ]); + + $fake = new FakeGlobalAnalyticsTrafficClient($server); + $fake->responses = fakeGlobalAnalyticsResponses([$application->uuid, 'bad key$(reboot)']); + app()->bind(SentinelTrafficClient::class, fn () => $fake); + + loadLazy(Livewire::test(Analytics::class)) + ->assertOk() + ->assertSee('1,000') + ->assertSee('Safe Leaderboard App') + ->assertDontSee('bad key$(reboot)'); +}); + +it('queries Sentinel with the same window for loads within one cache interval', function () { + $server = bootEnabledGlobalServer(); + + $fake = new class($server) extends FakeGlobalAnalyticsTrafficClient + { + public array $urls = []; + + protected function raw(string $url): string + { + $this->urls[] = $url; + + return parent::raw($url); + } + }; + $fake->responses = fakeGlobalAnalyticsResponses(); + app()->bind(SentinelTrafficClient::class, fn () => $fake); + + $this->travelTo(Carbon::parse('2026-09-30 10:00:05', 'UTC')); + $component = loadLazy(Livewire::test(Analytics::class)); + $firstUrls = $fake->urls; + $fake->urls = []; + + $this->travelTo(Carbon::parse('2026-09-30 10:00:50', 'UTC')); + $component->call('loadData'); + + // The second load asks for exactly the windows of the first one (the 60s cache keys). + expect($firstUrls)->not->toBeEmpty() + ->and($fake->urls)->not->toBeEmpty() + ->and(array_values(array_diff($fake->urls, $firstUrls)))->toBe([]); +}); diff --git a/tests/Feature/TrafficAnalytics/SentinelTrafficCacheTest.php b/tests/Feature/TrafficAnalytics/SentinelTrafficCacheTest.php index 408c53ee26..233ac6c7d7 100644 --- a/tests/Feature/TrafficAnalytics/SentinelTrafficCacheTest.php +++ b/tests/Feature/TrafficAnalytics/SentinelTrafficCacheTest.php @@ -5,6 +5,7 @@ use App\Models\Server; use App\Models\Team; use App\Services\SentinelTrafficClient; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Carbon; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Process; @@ -48,3 +49,29 @@ it('surfaces the real remote error instead of a TypeError when the fetch fails', 'failed docker exec' => [1, '', 'Error response from daemon: No such container: coolify-sentinel', 'No such container: coolify-sentinel'], 'literal null body' => [0, 'null', '', 'Traffic analytics returned an invalid response.'], ]); + +it('keeps the range window stable within the 60s cache interval so repeated loads hit the cache', function () { + Cache::flush(); + $team = Team::factory()->create(); + $server = Server::factory()->create(['team_id' => $team->id]); + $client = new CountingTrafficClient($server); + + $this->travelTo(Carbon::parse('2026-09-30 10:00:05', 'UTC')); + [$from, $to] = SentinelTrafficClient::rangeWindow('24h'); + $client->overview('app', $from, $to); + + $this->travelTo(Carbon::parse('2026-09-30 10:00:47', 'UTC')); + [$laterFrom, $laterTo] = SentinelTrafficClient::rangeWindow('24h'); + $client->overview('app', $laterFrom, $laterTo); + + expect($client->calls)->toBe(1) + ->and([$laterFrom, $laterTo])->toBe([$from, $to]) + // The window still reaches "now" and spans exactly the requested range. + ->and($to)->toBe('2026-09-30T10:01:00Z') + ->and($from)->toBe('2026-09-29T10:01:00Z'); + + $this->travelTo(Carbon::parse('2026-09-30 10:01:03', 'UTC')); + [, $nextTo] = SentinelTrafficClient::rangeWindow('7d'); + expect($nextTo)->toBe('2026-09-30T10:02:00Z') + ->and(SentinelTrafficClient::rangeWindow('7d')[0])->toBe('2026-09-23T10:02:00Z'); +}); diff --git a/tests/Feature/TrafficAnalytics/SentinelTrafficClientTest.php b/tests/Feature/TrafficAnalytics/SentinelTrafficClientTest.php index 05bfbcddf2..0ea6f05d2e 100644 --- a/tests/Feature/TrafficAnalytics/SentinelTrafficClientTest.php +++ b/tests/Feature/TrafficAnalytics/SentinelTrafficClientTest.php @@ -5,6 +5,7 @@ use App\Models\Server; use App\Models\Team; use App\Services\SentinelTrafficClient; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Log; uses(RefreshDatabase::class); @@ -302,3 +303,53 @@ it('double-quotes the url in the remote curl command so & is not a shell backgro // container shell backgrounds curl at the `&` and only `from=...` reaches Sentinel. expect($command)->toContain('"'.$url.'"'); }); + +it('skips an unsafe app key in the dashboard bundle and keeps the other apps', function () { + $server = Server::factory()->create(['team_id' => $this->team->id]); + Log::spy(); + + $bundle = json_encode([ + 'overview' => ['requests' => 7], + 'paths' => [], + 'breakdowns' => [], + 'series' => [], + 'apps' => [ + ['uuid' => 'app-a', 'overview' => ['requests' => 4]], + ['uuid' => 'bad key;rm -rf /', 'overview' => ['requests' => 2]], + ['uuid' => 'app-b', 'overview' => ['requests' => 1]], + ], + ]); + + $client = new class($server, $bundle) extends SentinelTrafficClient + { + public function __construct($server, private string $bundle) + { + parent::__construct($server); + } + + protected function remoteFetch(string $url): string + { + if (str_contains($url, '/traffic/dashboard')) { + return $this->bundle; + } + throw new RuntimeException("unexpected individual fetch: {$url}"); + } + }; + + $apps = $client->prefetchServerWide(null, 'F', 'T', [], '24h'); + + expect($apps)->toBe(['app-a', 'app-b']) + ->and($client->overview('app-b', 'F', 'T')->requests)->toBe(1); + Log::shouldHaveReceived('warning')->withArgs(fn ($message) => str_contains($message, 'unsafe'))->once(); +}); + +it('skips an unsafe app key reported by an older Sentinel without the dashboard route', function () { + $server = Server::factory()->create(['team_id' => $this->team->id]); + Log::spy(); + + $client = new FakeTrafficClient($server); + $client->response = json_encode(['app-a', 'bad"key', 'app-b']); + + expect($client->prefetchServerWide(null, 'F', 'T', [], '24h'))->toBe(['app-a', 'app-b']); + Log::shouldHaveReceived('warning')->withArgs(fn ($message) => str_contains($message, 'unsafe'))->once(); +});